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

Darrell Ball dball at vmware.com
Fri Aug 25 08:36:29 UTC 2017


Thanks for the review Sugesh
I added your Acked-by

I applied it to https://github.com/darball/ovs/commits/dpdk_merge



On 8/18/17, 2:32 AM, "Chandran, Sugesh" <sugesh.chandran at intel.com> wrote:

    Can you release the patch for this? Looks to me it’s a good to have.
    
    
    Regards
    _Sugesh
    
    
    > -----Original Message-----
    > From: Chandran, Sugesh
    > Sent: Friday, August 18, 2017 10:31 AM
    > To: 'Darrell Ball' <dball at vmware.com>; Darrell Ball <dlu998 at gmail.com>;
    > dev at openvswitch.org
    > Subject: RE: [ovs-dev] [RFC patch v1] dp-packet: Refactor DPDK packet
    > initialization.
    > 
    > 
    > 
    > Regards
    > _Sugesh
    > 
    > 
    > > -----Original Message-----
    > > From: Darrell Ball [mailto:dball at vmware.com]
    > > Sent: Wednesday, August 16, 2017 6:58 PM
    > > To: Chandran, Sugesh <sugesh.chandran at intel.com>; Darrell Ball
    > > <dlu998 at gmail.com>; dev at openvswitch.org
    > > Subject: Re: [ovs-dev] [RFC patch v1] dp-packet: Refactor DPDK packet
    > > initialization.
    > >
    > >
    > >
    > > -----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.
    > [Sugesh] Ok, That’s fine.
    > 
    > >
    > >
    > >
    > >     > 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_xEhDXQ0qZ
    > > TE&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_xEhDXQ0qZ
    > > TE&e=
    > >
    
    













More information about the dev mailing list