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. So should that be ok to leave out this init ? > 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://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
