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: "dev@dpdk.org" <dev@dpdk.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>, "dev@dpdk.org"
> > > > <dev@dpdk.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.
