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' <[email protected]>; Darrell Ball <[email protected]>; > [email protected] > Subject: RE: [ovs-dev] [RFC patch v1] dp-packet: Refactor DPDK packet > initialization. > > > > 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
