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: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev-
> boun...@openvswitch.org] On Behalf Of Darrell Ball
> Sent: Wednesday, August 9, 2017 6:58 PM
> To: Ben Pfaff <b...@ovn.org>
> Cc: d...@openvswitch.org
> Subject: Re: [ovs-dev] [patch_v5] dp-packet: Reset DPDK HWOL checksum
> flags on init.
> 
> 
> 
> -----Original Message-----
> From: Ben Pfaff <b...@ovn.org>
> Date: Wednesday, August 9, 2017 at 10:40 AM
> To: Darrell Ball <db...@vmware.com>
> Cc: Darrell Ball <dlu...@gmail.com>, "d...@openvswitch.org"
> <d...@openvswitch.org>
> 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: <ovs-dev-boun...@openvswitch.org> on behalf of Ben Pfaff
> <b...@ovn.org>
>     > Date: Wednesday, August 9, 2017 at 10:15 AM
>     > To: Darrell Ball <dlu...@gmail.com>
>     > Cc: "d...@openvswitch.org" <d...@openvswitch.org>
>     > 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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to