Regards _Sugesh
> -----Original Message----- > From: Darrell Ball [mailto:[email protected]] > Sent: Wednesday, August 16, 2017 6:58 PM > To: Chandran, Sugesh <[email protected]>; Darrell Ball > <[email protected]>; [email protected] > Subject: Re: [ovs-dev] [RFC patch v1] dp-packet: Refactor DPDK packet > initialization. > > > > -----Original Message----- > From: <[email protected]> on behalf of "Chandran, > Sugesh" <[email protected]> > Date: Wednesday, August 16, 2017 at 6:55 AM > To: Darrell Ball <[email protected]>, "[email protected]" > <[email protected]> > 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: [email protected] [mailto:ovs-dev- > > [email protected]] On Behalf Of Darrell Ball > > Sent: Tuesday, August 15, 2017 5:14 PM > > To: [email protected]; [email protected] > > 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 <[email protected]> > > --- > > 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 > > [email protected] > > 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 > [email protected] > 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
