-----Original Message-----
From: "Chandran, Sugesh" <sugesh.chand...@intel.com>
Date: Wednesday, August 9, 2017 at 11:17 AM
To: Darrell Ball <db...@vmware.com>, Ben Pfaff <b...@ovn.org>
Cc: "d...@openvswitch.org" <d...@openvswitch.org>
Subject: RE: [ovs-dev] [patch_v5] dp-packet: Reset DPDK HWOL checksum flags on 
init.

    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.

There is no higher cost associated with initializing all the ol_flags vs some 
flags, so that is fine.
It will be done twice in the case of a packet received from a dpdk device, but 
it is a small cost.
I was more concerned about the dual responsibility conflict when packets are 
received from a DPDK
device and this is why I wanted to limit the scope of the flag management in 
OVS; it should be ok, though.
Hence, I mentioned that I will initialize all the ol_flags.

JTBC, are you suggesting to initialize all the fields below ?
This would mean when packets are received from a DPDK dev, both the rte library 
and OVS
would separately initialize all the fields below – meaning, it would be done 
twice for each packet.
    
    
    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://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwIFAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=wM40bZgtG4Ul1nb53ufLGymPOplaXttP5W6_wL2w_XA&s=05T6d75pGHINP78-6gwS87E3IhAXzHojsFzyNSDdaTY&e=
 
    

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to