[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