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

Chandran, Sugesh sugesh.chandran at intel.com
Fri Aug 18 09:32:59 UTC 2017


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