[ovs-dev] [patch_v5] dp-packet: Reset DPDK HWOL checksum flags on init.

[Chandran, Sugesh]

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