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

[Jerin Jacob <jerin.jacob@caviumnetworks.com>]

-----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.
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.

Do you think, any reason why this fastpath structure SHOULD NOT BE cache aligned ?

> 
> 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.

> 
> 
> > See below,
> > 
> > >
> > > I think that performance improvement results should be added to the
> > commit log.
> > 
> > I added following under comment section. Do you this want to move git
> > commit message ? If so, I can send the v3.
> > 
> >  - Some platform like thunderx + l3fwd showed 1% regression in the
> > performance with 5b7ba31148a8 ("ethdev: add port ownership") in one port
> > setup.
> 
> I think it should report the improvement of the new commit you want to add now(not the degradation of the previous commits).
> Also to details more (number of ports, number of queues per port, the forward mode, etc).

In my setup the degradation(("ethdev: add port ownership") is fixing with this patch.
So I can make it improvement of 1%.

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.

> 
> > >
> > > Moreover, Did you investigate which fields in rte_eth_dev_data structures
> > are important for performance and should not be in a different cache lines?
> > 
> > No. That can be separate patch.
> 
> I think it will be nice(not must :)), even in this commit, to explain the root cause of the performance improvement you saw by the alignment. 
> 
> > > Maybe alternative order of the fields in the structure may improve the
> > performance more...
> > 
> > Maybe.
> > 
> > >
> > > > Cc: Matan Azrad <matan@mellanox.com>
> > > > Cc: Thomas Monjalon <thomas@monjalon.net>
> > > > Cc: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > > >
> > > > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > > Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
> > > > ---
> > > > v2:
> > > > - Change the git comments based on Matan's feedback
> > > >
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fd
> > > > p
> > dk.org%2Fdev%2Fpatchwork%2Fpatch%2F35104%2F&data=02%7C01%7Cmat
> > > >
> > an%40mellanox.com%7C5c2537b12e6d4e51f12a08d571dd33a2%7Ca652971c7
> > > >
> > d2e4d9ba6a4d149256f461b%7C0%7C0%7C636540117238324576&sdata=8OOg
> > > > Zb0KzDbBce9xPVywV8ynmiKP9B%2BbYsQxgE5VlX0%3D&reserved=0
> > > >
> > > > - Some platform like thunderx + l3fwd showed 1% regression in the
> > > > performance with 5b7ba31148a8 ("ethdev: add port ownership") in one
> > > > port setup.
> > > >
> > > > - If there are no objection for this change then request to take it
> > > > for v18.02 release.
> > > > ---
> > > >  lib/librte_ether/rte_ethdev_core.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/lib/librte_ether/rte_ethdev_core.h
> > > > b/lib/librte_ether/rte_ethdev_core.h
> > > > index 315b31723..e5681e466 100644
> > > > --- a/lib/librte_ether/rte_ethdev_core.h
> > > > +++ b/lib/librte_ether/rte_ethdev_core.h
> > > > @@ -601,7 +601,7 @@ struct rte_eth_dev_data {
> > > >  	struct rte_vlan_filter_conf vlan_filter_conf;
> > > >  	/**< VLAN filter configuration. */
> > > >  	struct rte_eth_dev_owner owner; /**< The port owner. */ -};
> > > > +} __rte_cache_aligned;
> > > >
> > > >  /**
> > > >   * @internal
> > > > --
> > > > 2.16.1
> > >