-----Original Message-----
From: Darrell Ball <db...@vmware.com>
Date: Wednesday, August 9, 2017 at 1:38 PM
To: "Chandran, Sugesh" <sugesh.chand...@intel.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.

    
    
    -----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.

I did some background investigation around this Sugesh.
I originally expected that an init function ptr for OVS only portion of 
dp-packet by passed and saved with the dpdk library to be
used to opaquely init when rte calls priv memory, which is ovs common memory, 
in this case.
The func ptr is used but only for init of the dpdk memory pools (which confirms 
what you mentioned) and not later when mbuf
memory is reused for subsequent packets. Some OVS common memory (i.e. non-mbuf 
portion) of dp-packet must still be initialized
for every packet; the transient fields and seem to be otherwise done in other 
code paths during packet processing. So, it made me
wonder why we bother doing the non-mbuf (OVS specific) init at pool creation 
time for the OVS specific transient fields. I did some
initial testing and will follow up separately on this. 
        
        > 
        >     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