Regards _Sugesh
> -----Original Message----- > From: Darrell Ball [mailto:db...@vmware.com] > Sent: Thursday, August 10, 2017 5:15 PM > To: Chandran, Sugesh <sugesh.chand...@intel.com>; 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: 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. [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. > > > > > 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
