-----Original Message-----
From: "Chandran, Sugesh" <sugesh.chand...@intel.com>
Date: Wednesday, August 9, 2017 at 12:55 PM
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.

    
    
    Regards
    _Sugesh
    
    <snip>
    >     >     >
    >     >     > 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.
    > 
    [Sugesh] No, we don’t have to initialize all the fields. But we can have a 
generic function
    to init all mbuf fields that are relevant in OVS. 
    For now its only ol_flags. In the future this function must be updated when 
we use more
    fields from rte_mbuf.

We are on the same page then.
I plan for the function to have a generic name, so this fine.

    
    Sorry, I really didn’t get the case you mentioned above, where the init get 
called twice.
    What I can see in the code is , during the dpdk mp init, the 
    the dp_packet_init is called for rte_mbuf initialization. This happens at 
port initiation.
    
    On packet reception, there wont any call to dp_packet_init.

I meant for the memory related to packets that are received, not during the 
reception time itself.
However, the memory will be reused for subsequent packets, right. You can’t use 
a piece of memory
once and never use it again – this is called a memory leak. This means each 
mbuf will need to be initialized again and again.
    
    
    > 
    >     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=05T6d
    > 75pGHINP78-6gwS87E3IhAXzHojsFzyNSDdaTY&e=
    > 
    
    



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

Reply via email to