Hi Jerin From: Jerin Jacob, Sent: Monday, February 12, 2018 12:21 PM > -----Original Message----- > > Date: Mon, 12 Feb 2018 09:49:55 +0000 > > From: Matan Azrad <ma...@mellanox.com> > > To: Jerin Jacob <jerin.ja...@caviumnetworks.com> > > CC: "email@example.com" <firstname.lastname@example.org>, "ferruh.yi...@intel.com" > > <ferruh.yi...@intel.com>, Thomas Monjalon <tho...@monjalon.net>, > > Konstantin Ananyev <konstantin.anan...@intel.com>, Pavan Nikhilesh > > <pbhagavat...@caviumnetworks.com> > > Subject: RE: [dpdk-dev] [PATCH v2] ethdev: make ethdev data cache > > aligned > > > > Hi Jerin > > > > From: Jerin Jacob, Sent: Monday, February 12, 2018 11:26 AM > > > -----Original Message----- > > > > Date: Mon, 12 Feb 2018 09:04:07 +0000 > > > > From: Matan Azrad <ma...@mellanox.com> > > > > To: Jerin Jacob <jerin.ja...@caviumnetworks.com>, "email@example.com" > > > > <firstname.lastname@example.org> > > > > CC: "ferruh.yi...@intel.com" <ferruh.yi...@intel.com>, Thomas > > > > Monjalon <tho...@monjalon.net>, Konstantin Ananyev > > > > <konstantin.anan...@intel.com>, Pavan Nikhilesh > > > > <pbhagavat...@caviumnetworks.com> > > > > Subject: RE: [dpdk-dev] [PATCH v2] ethdev: make ethdev data cache > > > > aligned > > > > > > > > Hi Jerin > > > > > > > > From: Jerin Jacob [mailto:jerin.ja...@caviumnetworks.com] > > > > > Since struct rte_eth_dev_data used in the fast path, making it > > > > > as cache aligned. > > > > > > > > > > Fixes: af75078fece3 ("first public release") > > > > > Fixes: 5b7ba31148a8 ("ethdev: add port ownership") > > > > > > > > Looks like it is just improvement. > > > > No need the above "fixes" lines (also fix title is not needed as you > > > > did). > > > > > > I think, It varies the way we look at it. I don't think, either way > > > it matters in the commit log. > > > > I think this commit improves " af75078fece3 ("first public release")" > > since there was no intention to aligned rte_eth_dev_data in the first > commit created it, The relevant fields in the first port probably was aligned > without any intention(if no , what's about the other ports?). > > In my view it is a bug as it missed to align to cache line from the first > release.
Can be a fix of the first release. > and before adding struct rte_vlan_filter_conf vlan_filter_conf and struct > rte_eth_dev_owner owner; it was 128B aligned just by luck for all the ports. > and further ("ethdev: add port ownership") changes the complete alignment > by introducing a container type on top of it. > So, ("ethdev: add port ownership") and rte_vlan_filter_conf vlan_filter_conf patch just exposed another issue of performance... Moreover any fields adding patch to the rte_eth_dev_data structure from the first release didn't took the performance into account.. So, all of them have a bug? I think that if you want to consider it as a fix it should be for the first commit didn't take into account the performance alignment. And you should backport it with Cc: sta...@dpdk.org. > Do you think, any reason why this fastpath structure SHOULD NOT BE cache > aligned ? > I agree it should be(actually the datapath fields should be in the same cache line). I think also it will be good to detail the fields which should be in the same cache line. > > > > My suggestion is to just explain why the rte_eth_dev_data structure > should be aligned and to align it as improvement, even to backport it to > stable branch to improve the early LTS versions for all the ports. > > I don't think, There is a VERY specific reason for rte_eth_dev_data to cache > aligned. it applies to all fastpath functions. > IMO, We are making fastpath structure cache aligned due to, > 1) Avoid sharing the element with another cache line > 2) Compiler/CPU can access the elements in natural alignment if the top most > element is aligned. Agree. > > It simple, 1 port, 1 queue l3fwd setup. > sudo ./examples/l3fwd/build/l3fwd -c 0xff00 -- -p 0x1 --config="(0,0,9)" > > It is not black and white. it will vary based on global variable alignment > etc in > the binary. > i.e if apply this patch on any RANDOM change set you will not get a fixed > improvement. > > Hope this clarifies. Yes, you should add the improved scenario to the commit log with the results.