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

Matan Azrad matan at mellanox.com
Mon Feb 12 10:49:55 CET 2018


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

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. 


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

> >
> > 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 at mellanox.com>
> > > Cc: Thomas Monjalon <thomas at monjalon.net>
> > > Cc: Konstantin Ananyev <konstantin.ananyev at intel.com>
> > >
> > > Signed-off-by: Jerin Jacob <jerin.jacob at caviumnetworks.com>
> > > Signed-off-by: Pavan Nikhilesh <pbhagavatula at 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
> >


More information about the dev mailing list