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

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



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