[ovs-dev] [RFC patch v1] dp-packet: Refactor DPDK packet initialization.

Darrell Ball dball at vmware.com
Wed Aug 16 17:57:40 UTC 2017



-----Original Message-----
From: <ovs-dev-bounces at openvswitch.org> on behalf of "Chandran, Sugesh" <sugesh.chandran at intel.com>
Date: Wednesday, August 16, 2017 at 6:55 AM
To: Darrell Ball <dlu998 at gmail.com>, "dev at openvswitch.org" <dev at openvswitch.org>
Subject: Re: [ovs-dev] [RFC patch v1] dp-packet: Refactor DPDK	packet	initialization.

    Hi Darrel,
    Thank you for the patch.
    Please find my comments below.
    
    Regards
    _Sugesh
    
    
    > -----Original Message-----
    > From: ovs-dev-bounces at openvswitch.org [mailto:ovs-dev-
    > bounces at openvswitch.org] On Behalf Of Darrell Ball
    > Sent: Tuesday, August 15, 2017 5:14 PM
    > To: dlu998 at gmail.com; dev at openvswitch.org
    > Subject: [ovs-dev] [RFC patch v1] dp-packet: Refactor DPDK packet
    > initialization.
    > 
    > DPDK uses dp-packet pools and manages the mbuf portion of each packet.
    > When a pool is created, partial initialization is also done on the OVS portion
    > (i.e. non-mbuf).  Since packet memory is reused, this is not very useful for
    > transient fields and is also misleading.  Furthermore, some of these transient
    > fields are properly initialized for DPDK packets entering OVS anyways, which
    > is the only reasonable way to do this.
    > Another field, cutlen, is initialized in this manner in the pool and intended to
    > be reset when cutlen is applied on sending the packet out. However, if
    > cutlen context is set but the packet is not sent out for some reason, then the
    > packet header would be corrupted in the memory pool.  It is better to just
    > reset the cutlen in the packets when received.  I did not detect a degradation
    > in performance, however, I would be willing to have some degradation, since
    [Sugesh] I also did some testing in our lab with your patch and didn't find any degradation as such.
    The approach looks fine to me. Only one concern is about init the cutlen for all the packet structure.
    It looks to me bit of overhead even I don't see any performance degradation as such in my testing.
    Similarly the md strcture of dp_packet doesn't init completely due to the performance reason.
    My understanding is the dp_packet can have corrupted uninitialized data and it is not necessary to init
    all the fields.

[Darrell] In general, it goes without saying that not everything needs init; it is case by case.

 So should that be ok to leave out this init ?

[Darrell]:  cutlen corrupts packets (although there is a legitimate use case) so we need to extra careful here.
                  I think cutlen needs init because we cannot track all future error corner cases where it is not reset in
                 the dp-packet header.
                 Since both of us don’t see any degradation by initing it, I would like to keep the initialization. I would even keep it
                 with some small degradation, since it is that important.

    
    
    > this is a proper way to handle this.  In addition to initializing cutlen in received
    > packets, the other OVS transient fields are removed from the DPDK pool
    > initialization.
    > 
    > Signed-off-by: Darrell Ball <dlu998 at gmail.com>
    > ---
    >  lib/dp-packet.c   | 16 +++++++++-------
    >  lib/dp-packet.h   | 10 ++++++++++
    >  lib/netdev-dpdk.c |  2 ++
    >  3 files changed, 21 insertions(+), 7 deletions(-)
    > 
    > diff --git a/lib/dp-packet.c b/lib/dp-packet.c index c1f43f3..312f63a 100644
    > --- a/lib/dp-packet.c
    > +++ b/lib/dp-packet.c
    > @@ -92,16 +92,18 @@ dp_packet_use_const(struct dp_packet *b, const
    > void *data, size_t size)
    >      dp_packet_set_size(b, size);
    >  }
    > 
    > -/* Initializes 'b' as an empty dp_packet that contains the 'allocated' bytes of
    > - * memory starting at 'base'.  DPDK allocated dp_packet and *data is
    > allocated
    > - * from one continous memory region, so in memory data start right after
    > - * dp_packet.  Therefore there is special method to free this type of
    > - * buffer.  dp_packet base, data and size are initialized by dpdk rcv() so no
    > - * need to initialize those fields. */
    > +/* Initializes 'b' as an empty dp_packet that contains the 'allocated' bytes.
    > + * DPDK allocated dp_packet and *data is allocated from one continous
    > +memory
    > + * region as part of memory pool, so in memory data start right after
    > + * dp_packet.  Therefore, there is a special method to free this type
    > +of
    > + * buffer.  Here, non-transient ovs dp-packet fields are initialized
    > +for
    > + * packets that are part of a DPDK memory pool. */
    >  void
    >  dp_packet_init_dpdk(struct dp_packet *b, size_t allocated)  {
    > -    dp_packet_init__(b, allocated, DPBUF_DPDK);
    > +    dp_packet_set_allocated(b, allocated);
    > +    b->source = DPBUF_DPDK;
    > +    b->packet_type = htonl(PT_ETH);
    >  }
    > 
    >  /* Initializes 'b' as an empty dp_packet with an initial capacity of 'size'
    > diff --git a/lib/dp-packet.h b/lib/dp-packet.h index bb3f9db..cbeb831 100644
    > --- a/lib/dp-packet.h
    > +++ b/lib/dp-packet.h
    > @@ -801,6 +801,16 @@ dp_packet_delete_batch(struct dp_packet_batch
    > *batch, bool may_steal)  }
    > 
    >  static inline void
    > +dp_packet_batch_init_cutlen(struct dp_packet_batch *batch) {
    > +    struct dp_packet *packet;
    > +
    > +    DP_PACKET_BATCH_FOR_EACH (packet, batch) {
    > +        dp_packet_reset_cutlen(packet);
    > +    }
    > +}
    > +
    > +static inline void
    >  dp_packet_batch_apply_cutlen(struct dp_packet_batch *batch)  {
    >      if (batch->trunc) {
    > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 1aaf6f7..92453b2
    > 100644
    > --- a/lib/netdev-dpdk.c
    > +++ b/lib/netdev-dpdk.c
    > @@ -1644,6 +1644,7 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq
    > *rxq,
    >                                           nb_rx, dropped);
    >      rte_spinlock_unlock(&dev->stats_lock);
    > 
    > +    dp_packet_batch_init_cutlen(batch);
    >      batch->count = (int) nb_rx;
    >      return 0;
    >  }
    > @@ -1683,6 +1684,7 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq,
    > struct dp_packet_batch *batch)
    >          rte_spinlock_unlock(&dev->stats_lock);
    >      }
    > 
    > +    dp_packet_batch_init_cutlen(batch);
    >      batch->count = nb_rx;
    > 
    >      return 0;
    > --
    > 1.9.1
    > 
    > _______________________________________________
    > dev mailing list
    > dev at openvswitch.org
    > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=8F2TyXQLRSV1ShhsQ4-5RM9Y-bLOTVfMzlTnkPlQ90Y&s=mXjrw5j6ezo9KBACYhQb5jqIk3IM61O_xEhDXQ0qZTE&e= 
    _______________________________________________
    dev mailing list
    dev at openvswitch.org
    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=8F2TyXQLRSV1ShhsQ4-5RM9Y-bLOTVfMzlTnkPlQ90Y&s=mXjrw5j6ezo9KBACYhQb5jqIk3IM61O_xEhDXQ0qZTE&e= 
    



More information about the dev mailing list