[ovs-dev] [patch_v5] dp-packet: Reset DPDK HWOL checksum flags on init.
Darrell Ball
dball at vmware.com
Tue Aug 15 16:49:37 UTC 2017
-----Original Message-----
From: "Chandran, Sugesh" <sugesh.chandran at intel.com>
Date: Sunday, August 13, 2017 at 8:06 AM
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
> -----Original Message-----
> From: Darrell Ball [mailto:dball at vmware.com]
> Sent: Thursday, August 10, 2017 5:15 PM
> To: Chandran, Sugesh <sugesh.chandran at intel.com>; 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: 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.
[Sugesh] Sure. If the OVS specific fields(non mbuf) are initialed afterwards in packet processing
(In all the cases) we may remove it from the mempool init.
[Darrell] Based on your last response Sugesh, I am not sure if there is some slight disconnect about what I
fully meant/intended. Anyways, I sent a patch here
https://mail.openvswitch.org/pipermail/ovs-dev/2017-August/337432.html
>
> >
> > 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