[ovs-dev] [patch_v5] dp-packet: Reset DPDK HWOL checksum flags on init.
Darrell Ball
dball at vmware.com
Thu Aug 10 16:14:36 UTC 2017
-----Original Message-----
From: Darrell Ball <dball at vmware.com>
Date: Wednesday, August 9, 2017 at 1:38 PM
To: "Chandran, Sugesh" <sugesh.chandran at intel.com>, Ben Pfaff <blp at ovn.org>
Cc: "dev at openvswitch.org" <dev at openvswitch.org>
Subject: Re: [ovs-dev] [patch_v5] dp-packet: Reset DPDK HWOL checksum flags on init.
-----Original Message-----
From: "Chandran, Sugesh" <sugesh.chandran at intel.com>
Date: Wednesday, August 9, 2017 at 12:55 PM
To: Darrell Ball <dball at vmware.com>, Ben Pfaff <blp at ovn.org>
Cc: "dev at openvswitch.org" <dev at 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
> > dev at 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=
>
More information about the dev
mailing list