[dpdk-dev] [PATCH] ethdev: fix a regression due to cache alignment issue

[dpdk-dev] [PATCH] ethdev: fix a regression due to cache alignment issue

[Jerin Jacob]

Prior to "ethdev: add port ownership" change, the fast
path structure struct rte_eth_dev->data was cache aligned
due to the fact that eth_dev_data was allocated from
rte_malloc with cache aligned attribute.
"ethdev: add port ownership" change set introduced a
rte_eth_dev_shared_data container for port ownership change,
This resulted in rte_eth_dev->data memory as cache unaligned.
Added a compiler alignment attribute to make sure
rte_eth_dev->data always cache aligned and hence compiler
can load/store the elements in struct rte_eth_dev_data
as naturally aligned.

Some platform like thunderx + l3fwd showed 1% regression in
the performance with the offending changeset.

Fixes: 5b7ba31148a8 ("ethdev: add port ownership")

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

Re: [dpdk-dev] [PATCH] ethdev: fix a regression due to cache alignment issue

[Matan Azrad]

Hi Jerin

From: Jerin Jacob,Sent: Saturday, February 10, 2018 11:42 AM
> Prior to "ethdev: add port ownership" change, the fast path structure struct
> rte_eth_dev->data was cache aligned due to the fact that eth_dev_data was
> allocated from rte_malloc with cache aligned attribute.
> "ethdev: add port ownership" change set introduced a
> rte_eth_dev_shared_data container for port ownership change, This
> resulted in rte_eth_dev->data memory as cache unaligned.
> Added a compiler alignment attribute to make sure rte_eth_dev->data
> always cache aligned and hence compiler can load/store the elements in
> struct rte_eth_dev_data as naturally aligned.
> 
> Some platform like thunderx + l3fwd showed 1% regression in the
> performance with the offending changeset.
> 
> Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> 

Are you sure the previous port data allocation(before port ownership) was cache aligned for all the ports for every system (cache line size 16\32\64\128...) or maybe it was only aligned to the first port (port_id=0)?

I think that if the answer is no, this fix fixes early patch, or just improvement. 

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

Re: [dpdk-dev] [PATCH] ethdev: fix a regression due to cache alignment issue

[Jerin Jacob]

-----Original Message-----
> Date: Sat, 10 Feb 2018 18:23:42 +0000
> From: Matan Azrad <matan@mellanox.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, "dev@dpdk.org"
>  <dev@dpdk.org>
> CC: Thomas Monjalon <thomas@monjalon.net>, Konstantin Ananyev
>  <konstantin.ananyev@intel.com>, Pavan Nikhilesh
>  <pbhagavatula@caviumnetworks.com>
> Subject: RE: [dpdk-dev] [PATCH] ethdev: fix a regression due to cache
>  alignment issue
> 
> Hi Jerin

Hi Matan,

> 
> From: Jerin Jacob,Sent: Saturday, February 10, 2018 11:42 AM
> > Prior to "ethdev: add port ownership" change, the fast path structure struct
> > rte_eth_dev->data was cache aligned due to the fact that eth_dev_data was
> > allocated from rte_malloc with cache aligned attribute.
> > "ethdev: add port ownership" change set introduced a
> > rte_eth_dev_shared_data container for port ownership change, This
> > resulted in rte_eth_dev->data memory as cache unaligned.
> > Added a compiler alignment attribute to make sure rte_eth_dev->data
> > always cache aligned and hence compiler can load/store the elements in
> > struct rte_eth_dev_data as naturally aligned.
> > 
> > Some platform like thunderx + l3fwd showed 1% regression in the
> > performance with the offending changeset.
> > 
> > Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> > 
> 
> Are you sure the previous port data allocation(before port ownership) was cache aligned for all the ports for every system (cache line size 16\32\64\128...) or maybe it was only aligned to the first port (port_id=0)?

Yes, It was aligned to only for the first port.

> 
> I think that if the answer is no, this fix fixes early patch, or just improvement. 

I will send v2 as fixes early patch/improvement.

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

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

[Jerin Jacob]

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

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://dpdk.org/dev/patchwork/patch/35104/

- 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

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

[Matan Azrad]

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 that performance improvement results should be added to the commit log.

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?
Maybe alternative order of the fields in the structure may improve the performance more... 
 
> 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%2Fdp
> 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

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

[Jerin Jacob]

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

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.

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

> 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%2Fdp
> > 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
> 

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

[Matan Azrad]

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

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

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

[Jerin Jacob]

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

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

[Matan Azrad]

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.

[dpdk-dev] [PATCH v3] ethdev: fix ethdev data alignment

[Jerin Jacob]

The struct rte_eth_dev_data is used in ethdev fastpath routines
and it not aligned to cache line size. This patch fixes the ethdev
data alignment.

The alignment was broken from the "first public release" changeset
where ethdev data address was aligned only to the first port.
Remaining ports alignment was defined by the size of the struct
(rte_eth_dev_data). This scheme is not guaranteed to be cache line
aligned all the time.

"ethdev: add port ownership" change set introduced a
rte_eth_dev_shared_data container for port ownership change,
This resulted in rte_eth_dev->data memory for the first port also
as cache unaligned.

Added a compiler alignment attribute to make sure
rte_eth_dev->data always cache aligned so that CPU/compiler
1) Avoid sharing the element with another cache line
2) Can load/store the elements in struct rte_eth_dev_data as
naturally aligned.

Some platform like thunderX could see performance regression of 1%
at "ethdev: add port ownership" change set with
1 port/1 queue l3fwd application and this patch fixes that regression.

example command:
sudo ./examples/l3fwd/build/l3fwd -c 0xff00 -- -p 0x1 --config="(0,0,9)"

Fixes: af75078fece3 ("first public release")
Fixes: 5b7ba31148a8 ("ethdev: add port ownership")

Cc: stable@dpdk.org

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

v3:
- added more history/data in the git log
- added stable@dpdk.org in Cc

v2:
 - Change the git comments based on Matan's feedback
http://dpdk.org/dev/patchwork/patch/35104/ 

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

Re: [dpdk-dev] [PATCH v3] ethdev: fix ethdev data alignment

[Matan Azrad]

Hi Jerin

From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> The struct rte_eth_dev_data is used in ethdev fastpath routines and it not
> aligned to cache line size. This patch fixes the ethdev data alignment.
> 
> The alignment was broken from the "first public release" changeset where
> ethdev data address was aligned only to the first port.
> Remaining ports alignment was defined by the size of the struct
> (rte_eth_dev_data). This scheme is not guaranteed to be cache line aligned
> all the time.
> 
> "ethdev: add port ownership" change set introduced a
> rte_eth_dev_shared_data container for port ownership change, This
> resulted in rte_eth_dev->data memory for the first port also as cache
> unaligned.
> 
> Added a compiler alignment attribute to make sure rte_eth_dev->data
> always cache aligned so that CPU/compiler
> 1) Avoid sharing the element with another cache line
> 2) Can load/store the elements in struct rte_eth_dev_data as naturally
> aligned.
> 
> Some platform like thunderX could see performance regression of 1% at
> "ethdev: add port ownership" change set with
> 1 port/1 queue l3fwd application and this patch fixes that regression.
> 
> example command:
> sudo ./examples/l3fwd/build/l3fwd -c 0xff00 -- -p 0x1 --config="(0,0,9)"
> 
> Fixes: af75078fece3 ("first public release")
> Fixes: 5b7ba31148a8 ("ethdev: add port ownership")

I don't think you need the add the 5b7ba31148a8 fix line.
Maybe think about it in the next way:
Is your fix can stay as a same fix before the port ownership feature addition?
If yes, You are not fixing it.

I think that the port ownership just exposed the issue, did not initiated it.

> Cc: stable@dpdk.org
> 
> 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>
> ---
> 
> v3:
> - added more history/data in the git log
> - added stable@dpdk.org in Cc
> 
> v2:
>  - Change the git comments based on Matan's feedback
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdpd
> k.org%2Fdev%2Fpatchwork%2Fpatch%2F35104%2F&data=02%7C01%7Cmata
> n%40mellanox.com%7Cd491698ead5f48626c1408d5721a8331%7C8c5c71989d
> be4cf9a2099e499881116c%7C0%7C0%7C636540380570748030&sdata=if8cCrk3
> U0XqWp8NE%2BEP1BjuPHX95HHN0f1yjk6krV0%3D&reserved=0
> 
> ---
>  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

Re: [dpdk-dev] [PATCH v3] ethdev: fix ethdev data alignment

[Jerin Jacob]

-----Original Message-----
> Date: Mon, 12 Feb 2018 13:44:54 +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>, "stable@dpdk.org"
>  <stable@dpdk.org>, Thomas Monjalon <thomas@monjalon.net>, Konstantin
>  Ananyev <konstantin.ananyev@intel.com>, Pavan Nikhilesh
>  <pbhagavatula@caviumnetworks.com>
> Subject: RE: [dpdk-dev] [PATCH v3] ethdev: fix ethdev data alignment
> 
> Hi Jerin
> 
> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > The struct rte_eth_dev_data is used in ethdev fastpath routines and it not
> > aligned to cache line size. This patch fixes the ethdev data alignment.
> > 
> > The alignment was broken from the "first public release" changeset where
> > ethdev data address was aligned only to the first port.
> > Remaining ports alignment was defined by the size of the struct
> > (rte_eth_dev_data). This scheme is not guaranteed to be cache line aligned
> > all the time.
> > 
> > "ethdev: add port ownership" change set introduced a
> > rte_eth_dev_shared_data container for port ownership change, This
> > resulted in rte_eth_dev->data memory for the first port also as cache
> > unaligned.
> > 
> > Added a compiler alignment attribute to make sure rte_eth_dev->data
> > always cache aligned so that CPU/compiler
> > 1) Avoid sharing the element with another cache line
> > 2) Can load/store the elements in struct rte_eth_dev_data as naturally
> > aligned.
> > 
> > Some platform like thunderX could see performance regression of 1% at
> > "ethdev: add port ownership" change set with
> > 1 port/1 queue l3fwd application and this patch fixes that regression.
> > 
> > example command:
> > sudo ./examples/l3fwd/build/l3fwd -c 0xff00 -- -p 0x1 --config="(0,0,9)"
> > 
> > Fixes: af75078fece3 ("first public release")
> > Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> 
> I don't think you need the add the 5b7ba31148a8 fix line.
> Maybe think about it in the next way:
> Is your fix can stay as a same fix before the port ownership feature addition?
> If yes, You are not fixing it.

I don't think so. 5b7ba31148a8 breaking the first port alignment case(as
mentioned in the commit log clearly).
First port alignment was correct prior to 5b7ba31148a8 change set

Do you agree?

> 
> I think that the port ownership just exposed the issue, did not initiated it.
> 
> > Cc: stable@dpdk.org
> > 
> > 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>
> > ---
> > 
> > v3:
> > - added more history/data in the git log
> > - added stable@dpdk.org in Cc
> > 
> > v2:
> >  - Change the git comments based on Matan's feedback
> > https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdpd
> > k.org%2Fdev%2Fpatchwork%2Fpatch%2F35104%2F&data=02%7C01%7Cmata
> > n%40mellanox.com%7Cd491698ead5f48626c1408d5721a8331%7C8c5c71989d
> > be4cf9a2099e499881116c%7C0%7C0%7C636540380570748030&sdata=if8cCrk3
> > U0XqWp8NE%2BEP1BjuPHX95HHN0f1yjk6krV0%3D&reserved=0
> > 
> > ---
> >  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
> 

Re: [dpdk-dev] [PATCH v3] ethdev: fix ethdev data alignment

[Matan Azrad]

From: Jerin Jacob, Sent: Monday, February 12, 2018 3:51 PM
> -----Original Message-----
> > Date: Mon, 12 Feb 2018 13:44:54 +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>, "stable@dpdk.org"
> >  <stable@dpdk.org>, Thomas Monjalon <thomas@monjalon.net>,
> Konstantin
> > Ananyev <konstantin.ananyev@intel.com>, Pavan Nikhilesh
> > <pbhagavatula@caviumnetworks.com>
> > Subject: RE: [dpdk-dev] [PATCH v3] ethdev: fix ethdev data alignment
> >
> > Hi Jerin
> >
> > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > > The struct rte_eth_dev_data is used in ethdev fastpath routines and
> > > it not aligned to cache line size. This patch fixes the ethdev data
> alignment.
> > >
> > > The alignment was broken from the "first public release" changeset
> > > where ethdev data address was aligned only to the first port.
> > > Remaining ports alignment was defined by the size of the struct
> > > (rte_eth_dev_data). This scheme is not guaranteed to be cache line
> > > aligned all the time.
> > >
> > > "ethdev: add port ownership" change set introduced a
> > > rte_eth_dev_shared_data container for port ownership change, This
> > > resulted in rte_eth_dev->data memory for the first port also as
> > > cache unaligned.
> > >
> > > Added a compiler alignment attribute to make sure rte_eth_dev->data
> > > always cache aligned so that CPU/compiler
> > > 1) Avoid sharing the element with another cache line
> > > 2) Can load/store the elements in struct rte_eth_dev_data as
> > > naturally aligned.
> > >
> > > Some platform like thunderX could see performance regression of 1%
> > > at
> > > "ethdev: add port ownership" change set with
> > > 1 port/1 queue l3fwd application and this patch fixes that regression.
> > >
> > > example command:
> > > sudo ./examples/l3fwd/build/l3fwd -c 0xff00 -- -p 0x1 --config="(0,0,9)"
> > >
> > > Fixes: af75078fece3 ("first public release")
> > > Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> >
> > I don't think you need the add the 5b7ba31148a8 fix line.
> > Maybe think about it in the next way:
> > Is your fix can stay as a same fix before the port ownership feature
> addition?
> > If yes, You are not fixing it.
> 
> I don't think so. 5b7ba31148a8 breaking the first port alignment case(as
> mentioned in the commit log clearly).
> First port alignment was correct prior to 5b7ba31148a8 change set
> 
> Do you agree?

No.
The root cause of the issue this fix is fixing, is that the structure is not aligned, it is not relevant to port ownership.
The port ownership just exposes it.

> >
> > I think that the port ownership just exposed the issue, did not initiated it.
> >
> > > Cc: stable@dpdk.org
> > >
> > > 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>
> > > ---
> > >
> > > v3:
> > > - added more history/data in the git log
> > > - added stable@dpdk.org in Cc
> > >
> > > v2:
> > >  - Change the git comments based on Matan's feedback
> > >
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdp
> > > d
> k.org%2Fdev%2Fpatchwork%2Fpatch%2F35104%2F&data=02%7C01%7Cmata
> > >
> n%40mellanox.com%7Cd491698ead5f48626c1408d5721a8331%7C8c5c71989d
> > >
> be4cf9a2099e499881116c%7C0%7C0%7C636540380570748030&sdata=if8cCrk3
> > > U0XqWp8NE%2BEP1BjuPHX95HHN0f1yjk6krV0%3D&reserved=0
> > >
> > > ---
> > >  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
> >

Re: [dpdk-dev] [PATCH v3] ethdev: fix ethdev data alignment

[Jerin Jacob]

-----Original Message-----
> Date: Mon, 12 Feb 2018 14:02:17 +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>, "stable@dpdk.org" <stable@dpdk.org>, Thomas
>  Monjalon <thomas@monjalon.net>, Konstantin Ananyev
>  <konstantin.ananyev@intel.com>, Pavan Nikhilesh
>  <pbhagavatula@caviumnetworks.com>
> Subject: RE: [dpdk-dev] [PATCH v3] ethdev: fix ethdev data alignment
> 
> 
> 
> From: Jerin Jacob, Sent: Monday, February 12, 2018 3:51 PM
> > -----Original Message-----
> > > Date: Mon, 12 Feb 2018 13:44:54 +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>, "stable@dpdk.org"
> > >  <stable@dpdk.org>, Thomas Monjalon <thomas@monjalon.net>,
> > Konstantin
> > > Ananyev <konstantin.ananyev@intel.com>, Pavan Nikhilesh
> > > <pbhagavatula@caviumnetworks.com>
> > > Subject: RE: [dpdk-dev] [PATCH v3] ethdev: fix ethdev data alignment
> > >
> > > Hi Jerin
> > >
> > > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > > > The struct rte_eth_dev_data is used in ethdev fastpath routines and
> > > > it not aligned to cache line size. This patch fixes the ethdev data
> > alignment.
> > > >
> > > > The alignment was broken from the "first public release" changeset
> > > > where ethdev data address was aligned only to the first port.
> > > > Remaining ports alignment was defined by the size of the struct
> > > > (rte_eth_dev_data). This scheme is not guaranteed to be cache line
> > > > aligned all the time.
> > > >
> > > > "ethdev: add port ownership" change set introduced a
> > > > rte_eth_dev_shared_data container for port ownership change, This
> > > > resulted in rte_eth_dev->data memory for the first port also as
> > > > cache unaligned.
> > > >
> > > > Added a compiler alignment attribute to make sure rte_eth_dev->data
> > > > always cache aligned so that CPU/compiler
> > > > 1) Avoid sharing the element with another cache line
> > > > 2) Can load/store the elements in struct rte_eth_dev_data as
> > > > naturally aligned.
> > > >
> > > > Some platform like thunderX could see performance regression of 1%
> > > > at
> > > > "ethdev: add port ownership" change set with
> > > > 1 port/1 queue l3fwd application and this patch fixes that regression.
> > > >
> > > > example command:
> > > > sudo ./examples/l3fwd/build/l3fwd -c 0xff00 -- -p 0x1 --config="(0,0,9)"
> > > >
> > > > Fixes: af75078fece3 ("first public release")
> > > > Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> > >
> > > I don't think you need the add the 5b7ba31148a8 fix line.
> > > Maybe think about it in the next way:
> > > Is your fix can stay as a same fix before the port ownership feature
> > addition?
> > > If yes, You are not fixing it.
> > 
> > I don't think so. 5b7ba31148a8 breaking the first port alignment case(as
> > mentioned in the commit log clearly).
> > First port alignment was correct prior to 5b7ba31148a8 change set
> > 
> > Do you agree?
> 
> No.
> The root cause of the issue this fix is fixing, is that the structure is not aligned, it is not relevant to port ownership.
> The port ownership just exposes it.

My point is simple. First port alignment was correct prior to 5b7ba31148a8 change set.
it is broken from there.

It is minor thing. I let, ethdev maintainer to decide what to added in Fixes on apply.

Re: [dpdk-dev] [PATCH v3] ethdev: fix ethdev data alignment

[Ferruh Yigit]

On 2/12/2018 2:11 PM, Jerin Jacob wrote:
> -----Original Message-----
>> Date: Mon, 12 Feb 2018 14:02:17 +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>, "stable@dpdk.org" <stable@dpdk.org>, Thomas
>>  Monjalon <thomas@monjalon.net>, Konstantin Ananyev
>>  <konstantin.ananyev@intel.com>, Pavan Nikhilesh
>>  <pbhagavatula@caviumnetworks.com>
>> Subject: RE: [dpdk-dev] [PATCH v3] ethdev: fix ethdev data alignment
>>
>>
>>
>> From: Jerin Jacob, Sent: Monday, February 12, 2018 3:51 PM
>>> -----Original Message-----
>>>> Date: Mon, 12 Feb 2018 13:44:54 +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>, "stable@dpdk.org"
>>>>  <stable@dpdk.org>, Thomas Monjalon <thomas@monjalon.net>,
>>> Konstantin
>>>> Ananyev <konstantin.ananyev@intel.com>, Pavan Nikhilesh
>>>> <pbhagavatula@caviumnetworks.com>
>>>> Subject: RE: [dpdk-dev] [PATCH v3] ethdev: fix ethdev data alignment
>>>>
>>>> Hi Jerin
>>>>
>>>> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
>>>>> The struct rte_eth_dev_data is used in ethdev fastpath routines and
>>>>> it not aligned to cache line size. This patch fixes the ethdev data
>>> alignment.
>>>>>
>>>>> The alignment was broken from the "first public release" changeset
>>>>> where ethdev data address was aligned only to the first port.
>>>>> Remaining ports alignment was defined by the size of the struct
>>>>> (rte_eth_dev_data). This scheme is not guaranteed to be cache line
>>>>> aligned all the time.
>>>>>
>>>>> "ethdev: add port ownership" change set introduced a
>>>>> rte_eth_dev_shared_data container for port ownership change, This
>>>>> resulted in rte_eth_dev->data memory for the first port also as
>>>>> cache unaligned.
>>>>>
>>>>> Added a compiler alignment attribute to make sure rte_eth_dev->data
>>>>> always cache aligned so that CPU/compiler
>>>>> 1) Avoid sharing the element with another cache line
>>>>> 2) Can load/store the elements in struct rte_eth_dev_data as
>>>>> naturally aligned.
>>>>>
>>>>> Some platform like thunderX could see performance regression of 1%
>>>>> at
>>>>> "ethdev: add port ownership" change set with
>>>>> 1 port/1 queue l3fwd application and this patch fixes that regression.
>>>>>
>>>>> example command:
>>>>> sudo ./examples/l3fwd/build/l3fwd -c 0xff00 -- -p 0x1 --config="(0,0,9)"
>>>>>
>>>>> Fixes: af75078fece3 ("first public release")
>>>>> Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
>>>>
>>>> I don't think you need the add the 5b7ba31148a8 fix line.
>>>> Maybe think about it in the next way:
>>>> Is your fix can stay as a same fix before the port ownership feature
>>> addition?
>>>> If yes, You are not fixing it.
>>>
>>> I don't think so. 5b7ba31148a8 breaking the first port alignment case(as
>>> mentioned in the commit log clearly).
>>> First port alignment was correct prior to 5b7ba31148a8 change set
>>>
>>> Do you agree?
>>
>> No.
>> The root cause of the issue this fix is fixing, is that the structure is not aligned, it is not relevant to port ownership.
>> The port ownership just exposes it.
> 
> My point is simple. First port alignment was correct prior to 5b7ba31148a8 change set.
> it is broken from there.

+1 for this logic, adding fixes line makes sense for me.

> 
> It is minor thing. I let, ethdev maintainer to decide what to added in Fixes on apply.
> 

Re: [dpdk-dev] [PATCH v3] ethdev: fix ethdev data alignment

[Thomas Monjalon]

12/02/2018 14:13, Jerin Jacob:
> The struct rte_eth_dev_data is used in ethdev fastpath routines
> and it not aligned to cache line size. This patch fixes the ethdev
> data alignment.
> 
> The alignment was broken from the "first public release" changeset
> where ethdev data address was aligned only to the first port.
> Remaining ports alignment was defined by the size of the struct
> (rte_eth_dev_data). This scheme is not guaranteed to be cache line
> aligned all the time.
> 
> "ethdev: add port ownership" change set introduced a
> rte_eth_dev_shared_data container for port ownership change,
> This resulted in rte_eth_dev->data memory for the first port also
> as cache unaligned.
> 
> Added a compiler alignment attribute to make sure
> rte_eth_dev->data always cache aligned so that CPU/compiler
> 1) Avoid sharing the element with another cache line
> 2) Can load/store the elements in struct rte_eth_dev_data as
> naturally aligned.
> 
> Some platform like thunderX could see performance regression of 1%
> at "ethdev: add port ownership" change set with
> 1 port/1 queue l3fwd application and this patch fixes that regression.
> 
> example command:
> sudo ./examples/l3fwd/build/l3fwd -c 0xff00 -- -p 0x1 --config="(0,0,9)"
> 
> Fixes: af75078fece3 ("first public release")
> Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> 
> Cc: stable@dpdk.org
> 
> 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>
> ---
Acked-by: Thomas Monjalon <thomas@monjalon.net>

Applied, thanks