Hi Darrel, I reviewed and tested the patch. It does fixed the UT failures in --with-dpdk case.
One comment below. Regards _Sugesh > -----Original Message----- > From: [email protected] [mailto:ovs-dev- > [email protected]] On Behalf Of Darrell Ball > Sent: Wednesday, August 9, 2017 6:58 PM > To: Ben Pfaff <[email protected]> > Cc: [email protected] > Subject: Re: [ovs-dev] [patch_v5] dp-packet: Reset DPDK HWOL checksum > flags on init. > > > > -----Original Message----- > From: Ben Pfaff <[email protected]> > Date: Wednesday, August 9, 2017 at 10:40 AM > To: Darrell Ball <[email protected]> > Cc: Darrell Ball <[email protected]>, "[email protected]" > <[email protected]> > Subject: Re: [ovs-dev] [patch_v5] dp-packet: Reset DPDK HWOL checksum > flags on init. > > On Wed, Aug 09, 2017 at 05:32:02PM +0000, Darrell Ball wrote: > > > > > > -----Original Message----- > > From: <[email protected]> on behalf of Ben Pfaff > <[email protected]> > > Date: Wednesday, August 9, 2017 at 10:15 AM > > To: Darrell Ball <[email protected]> > > Cc: "[email protected]" <[email protected]> > > Subject: Re: [ovs-dev] [patch_v5] dp-packet: Reset DPDK HWOL > checksum flags on init. > > > > On Tue, Aug 08, 2017 at 06:54:46PM -0700, Darrell Ball wrote: > > > Reset the DPDK HWOL checksum flags in dp_packet_init_. > > > The new HWOL bad checksum flag is uninitialized for non-dpdk ports > and > > > this is noticed as test failures using netdev-dummy ports, when > built > with > > > the --with-dpdk flag set. Hence, in this case, packets may be > marked > as > > > having a bad checksum. > > > > > diff --git a/lib/dp-packet.c b/lib/dp-packet.c > > > index 67aa406..4926993 100644 > > > --- a/lib/dp-packet.c > > > +++ b/lib/dp-packet.c > > > @@ -31,6 +31,7 @@ dp_packet_init__(struct dp_packet *b, size_t > allocated, enum dp_packet_source so > > > dp_packet_reset_offsets(b); > > > pkt_metadata_init(&b->md, 0); > > > dp_packet_rss_invalidate(b); > > > + reset_dp_packet_checksum_ol_flags(b); > > > > This function un-sets some bits in p->mbuf.ol_flags. The need for > this > > implies that nothing initializes p->mbuf.ol_flags. > > > > Correct, I reused reset_dp_packet_checksum_ol_flags() to do the > initialization as well > > I could also have created a separate function. > > > > In case a DPDK dev is used, those flags will be managed by DPDK. > > > > That sounds like a > > bug in itself--is there a missing call to initialize the mbuf > somewhere? > > > > Are you suggesting to initialize the whole mbuf for each packet ? > > The issue that I'm raising is that it's unusual to take an > uninitialized, indeterminate field, and then initialize it by clearing a > few bits. It's much more conventional to initialize it by setting it to > zero, like this: > > p->mbuf.ol_flags = 0; > > > That is better; I will create a separate function then. > I will resend > Thanks [Sugesh] I also agree with Ben here. Currently OVS uses only checksum offload flags from mbuf(As I am aware of). But there are other flag bits that may get used in future like TSO. So its better to initialize the mbuf properly before using. Here is the mbuf reset function in DPDK that get called when an existing memory is mapped to Mbuf. I believe only the ol_flags are relevant for now in OVS. static inline void rte_pktmbuf_reset(struct rte_mbuf *m) { m->next = NULL; m->pkt_len = 0; m->tx_offload = 0; m->vlan_tci = 0; m->vlan_tci_outer = 0; m->nb_segs = 1; m->port = 0xff; m->ol_flags = 0; m->packet_type = 0; rte_pktmbuf_reset_headroom(m); m->data_len = 0; __rte_mbuf_sanity_check(m, 1); } > > > That made me wonder whether there's a larger problem of a failure to > initialize the mbuf more generally, although of course that does not > necessarily follow. > > > _______________________________________________ > 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
