Re: [dpdk-dev] [PATCH v2] ethdev: make ethdev data cache aligned

[Matan Azrad <matan@mellanox.com>]

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 <matan@mellanox.com>
> > To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > CC: "dev@dpdk.org" <dev@dpdk.org>, "ferruh.yigit@intel.com"
> >  <ferruh.yigit@intel.com>, Thomas Monjalon <thomas@monjalon.net>,
> > Konstantin Ananyev <konstantin.ananyev@intel.com>, Pavan Nikhilesh
> > <pbhagavatula@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 <matan@mellanox.com>
> > > > To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, "dev@dpdk.org"
> > > >  <dev@dpdk.org>
> > > > CC: "ferruh.yigit@intel.com" <ferruh.yigit@intel.com>, Thomas
> > > > Monjalon <thomas@monjalon.net>, Konstantin Ananyev
> > > > <konstantin.ananyev@intel.com>,  Pavan Nikhilesh
> > > > <pbhagavatula@caviumnetworks.com>
> > > > Subject: RE: [dpdk-dev] [PATCH v2] ethdev: make ethdev data cache
> > > > aligned
> > > >
> > > > Hi Jerin
> > > >
> > > >  From: Jerin Jacob [mailto:jerin.jacob@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: stable@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.