[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