DPDK patches and discussions
 help / color / Atom feed
* [dpdk-dev] [PATCH v1] lib/table: fix cache alignment issue
@ 2020-06-16 16:27 Ting Xu
  2020-06-17  5:43 ` [dpdk-dev] [PATCH v2] " Ting Xu
                   ` (2 more replies)
  0 siblings, 3 replies; 22+ messages in thread
From: Ting Xu @ 2020-06-16 16:27 UTC (permalink / raw)
  To: dev; +Cc: cristian.dumitrescu, stable

When create softnic hash table with 16 keys, it failed on 32bit
environment because of the structure rte_bucket_4_16 alignment
issue. Add __rte_cache_aligned to ensure correct cache align.

Fixes: 8aa327214c ("table: hash")
Cc: stable@dpdk.org

Signed-off-by: Ting Xu <ting.xu@intel.com>
---
 lib/librte_table/rte_table_hash_key16.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_table/rte_table_hash_key16.c b/lib/librte_table/rte_table_hash_key16.c
index 2cca1c924..5e1665c15 100644
--- a/lib/librte_table/rte_table_hash_key16.c
+++ b/lib/librte_table/rte_table_hash_key16.c
@@ -44,7 +44,7 @@ struct rte_bucket_4_16 {
 	uint64_t key[4][2];
 
 	/* Cache line 2 */
-	uint8_t data[0];
+	uint8_t data[0] __rte_cache_aligned;
 };
 
 struct rte_table_hash {
-- 
2.17.1


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [dpdk-dev] [PATCH v2] lib/table: fix cache alignment issue
  2020-06-16 16:27 [dpdk-dev] [PATCH v1] lib/table: fix cache alignment issue Ting Xu
@ 2020-06-17  5:43 ` " Ting Xu
  2020-07-02  8:06   ` Zhou, JunX W
  2020-07-09  1:48 ` [dpdk-dev] [PATCH v3] " Ting Xu
  2020-07-22  2:16 ` [dpdk-dev] [PATCH v4] " Ting Xu
  2 siblings, 1 reply; 22+ messages in thread
From: Ting Xu @ 2020-06-17  5:43 UTC (permalink / raw)
  To: dev; +Cc: cristian.dumitrescu, stable

When create softnic hash table with 16 keys, it failed on 32bit
environment because of the structure rte_bucket_4_16 alignment
issue. Add __rte_cache_aligned to ensure correct cache align.

Fixes: 8aa327214c ("table: hash")
Cc: stable@dpdk.org

Signed-off-by: Ting Xu <ting.xu@intel.com>

---
v1->v2: Correct patch time
---
 lib/librte_table/rte_table_hash_key16.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_table/rte_table_hash_key16.c b/lib/librte_table/rte_table_hash_key16.c
index 2cca1c924..5e1665c15 100644
--- a/lib/librte_table/rte_table_hash_key16.c
+++ b/lib/librte_table/rte_table_hash_key16.c
@@ -44,7 +44,7 @@ struct rte_bucket_4_16 {
 	uint64_t key[4][2];
 
 	/* Cache line 2 */
-	uint8_t data[0];
+	uint8_t data[0] __rte_cache_aligned;
 };
 
 struct rte_table_hash {
-- 
2.17.1


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v2] lib/table: fix cache alignment issue
  2020-06-17  5:43 ` [dpdk-dev] [PATCH v2] " Ting Xu
@ 2020-07-02  8:06   ` Zhou, JunX W
  0 siblings, 0 replies; 22+ messages in thread
From: Zhou, JunX W @ 2020-07-02  8:06 UTC (permalink / raw)
  To: Xu, Ting, dev; +Cc: Dumitrescu, Cristian, stable

Tested-by: junx.w.zhou@intel.com

-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ting Xu
Sent: Wednesday, June 17, 2020 1:44 PM
To: dev@dpdk.org
Cc: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; stable@dpdk.org
Subject: [dpdk-dev] [PATCH v2] lib/table: fix cache alignment issue

When create softnic hash table with 16 keys, it failed on 32bit environment because of the structure rte_bucket_4_16 alignment issue. Add __rte_cache_aligned to ensure correct cache align.

Fixes: 8aa327214c ("table: hash")
Cc: stable@dpdk.org

Signed-off-by: Ting Xu <ting.xu@intel.com>

---
v1->v2: Correct patch time
---
 lib/librte_table/rte_table_hash_key16.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_table/rte_table_hash_key16.c b/lib/librte_table/rte_table_hash_key16.c
index 2cca1c924..5e1665c15 100644
--- a/lib/librte_table/rte_table_hash_key16.c
+++ b/lib/librte_table/rte_table_hash_key16.c
@@ -44,7 +44,7 @@ struct rte_bucket_4_16 {
 	uint64_t key[4][2];
 
 	/* Cache line 2 */
-	uint8_t data[0];
+	uint8_t data[0] __rte_cache_aligned;
 };
 
 struct rte_table_hash {
--
2.17.1


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [dpdk-dev] [PATCH v3] lib/table: fix cache alignment issue
  2020-06-16 16:27 [dpdk-dev] [PATCH v1] lib/table: fix cache alignment issue Ting Xu
  2020-06-17  5:43 ` [dpdk-dev] [PATCH v2] " Ting Xu
@ 2020-07-09  1:48 ` " Ting Xu
  2020-07-20 14:37   ` Dumitrescu, Cristian
  2020-07-22  2:16 ` [dpdk-dev] [PATCH v4] " Ting Xu
  2 siblings, 1 reply; 22+ messages in thread
From: Ting Xu @ 2020-07-09  1:48 UTC (permalink / raw)
  To: dev; +Cc: cristian.dumitrescu, Ting Xu, stable

When create softnic hash table with 16 keys, it failed on 32bit
environment because of the structure rte_bucket_4_16 alignment
issue. Add __rte_cache_aligned to ensure correct cache align.

Fixes: 8aa327214c ("table: hash")
Cc: stable@dpdk.org

Signed-off-by: Ting Xu <ting.xu@intel.com>

---
v2->v3: Rebase
v1->v2: Correct patch time
---
 lib/librte_table/rte_table_hash_key16.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_table/rte_table_hash_key16.c b/lib/librte_table/rte_table_hash_key16.c
index 2cca1c924..5e1665c15 100644
--- a/lib/librte_table/rte_table_hash_key16.c
+++ b/lib/librte_table/rte_table_hash_key16.c
@@ -44,7 +44,7 @@ struct rte_bucket_4_16 {
 	uint64_t key[4][2];
 
 	/* Cache line 2 */
-	uint8_t data[0];
+	uint8_t data[0] __rte_cache_aligned;
 };
 
 struct rte_table_hash {
-- 
2.17.1


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v3] lib/table: fix cache alignment issue
  2020-07-09  1:48 ` [dpdk-dev] [PATCH v3] " Ting Xu
@ 2020-07-20 14:37   ` Dumitrescu, Cristian
  2020-07-21  5:15     ` Xu, Ting
  0 siblings, 1 reply; 22+ messages in thread
From: Dumitrescu, Cristian @ 2020-07-20 14:37 UTC (permalink / raw)
  To: Xu, Ting, dev; +Cc: stable



> -----Original Message-----
> From: Xu, Ting <ting.xu@intel.com>
> Sent: Thursday, July 9, 2020 2:48 AM
> To: dev@dpdk.org
> Cc: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Xu, Ting
> <ting.xu@intel.com>; stable@dpdk.org
> Subject: [PATCH v3] lib/table: fix cache alignment issue
> 
> When create softnic hash table with 16 keys, it failed on 32bit
> environment because of the structure rte_bucket_4_16 alignment
> issue. Add __rte_cache_aligned to ensure correct cache align.
> 
> Fixes: 8aa327214c ("table: hash")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ting Xu <ting.xu@intel.com>
> 
> ---
> v2->v3: Rebase
> v1->v2: Correct patch time
> ---
>  lib/librte_table/rte_table_hash_key16.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_table/rte_table_hash_key16.c
> b/lib/librte_table/rte_table_hash_key16.c
> index 2cca1c924..5e1665c15 100644
> --- a/lib/librte_table/rte_table_hash_key16.c
> +++ b/lib/librte_table/rte_table_hash_key16.c
> @@ -44,7 +44,7 @@ struct rte_bucket_4_16 {
>  	uint64_t key[4][2];
> 
>  	/* Cache line 2 */
> -	uint8_t data[0];
> +	uint8_t data[0] __rte_cache_aligned;
>  };
> 
>  struct rte_table_hash {
> --
> 2.17.1

Hi Ting,

This fix is breaking the execution for systems with cache line of 128 bytes, as typically (on 64-bit systems) this structure would be 64-byte in size and adding the __rte_cache_aligned would force doubling the size of this structure through padding enforced by the compiler.

Can you please confirm this is caused by check below failing in the table create function:
	sizeof(struct rte_bucket_4_16) % 64) != 0

Since all the other fields in this data structure are explicitly declared as 64-bit fields, due to the alignment rules I was expecting the compiler to add a 32-bit padding field after the "next" field, which is a pointer that would only take 32 bits on 32-bit systems. I am not sure why this did not take place in your case, any thoughts?

Not sure why we would run Soft NIC on 32-bit systems, might be better to disable Soft NIC for 32-bit systems.

Thanks,
Cristian

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v3] lib/table: fix cache alignment issue
  2020-07-20 14:37   ` Dumitrescu, Cristian
@ 2020-07-21  5:15     ` Xu, Ting
  2020-07-21 21:16       ` Dumitrescu, Cristian
  0 siblings, 1 reply; 22+ messages in thread
From: Xu, Ting @ 2020-07-21  5:15 UTC (permalink / raw)
  To: Dumitrescu, Cristian, dev; +Cc: stable

Hi, Cristian

> -----Original Message-----
> From: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Sent: Monday, July 20, 2020 10:38 PM
> To: Xu, Ting <ting.xu@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: RE: [PATCH v3] lib/table: fix cache alignment issue
> 
> 
> 
> > -----Original Message-----
> > From: Xu, Ting <ting.xu@intel.com>
> > Sent: Thursday, July 9, 2020 2:48 AM
> > To: dev@dpdk.org
> > Cc: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Xu, Ting
> > <ting.xu@intel.com>; stable@dpdk.org
> > Subject: [PATCH v3] lib/table: fix cache alignment issue
> >
> > When create softnic hash table with 16 keys, it failed on 32bit
> > environment because of the structure rte_bucket_4_16 alignment issue.
> > Add __rte_cache_aligned to ensure correct cache align.
> >
> > Fixes: 8aa327214c ("table: hash")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Ting Xu <ting.xu@intel.com>
> >
> > ---
> > v2->v3: Rebase
> > v1->v2: Correct patch time
> > ---
> >  lib/librte_table/rte_table_hash_key16.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/librte_table/rte_table_hash_key16.c
> > b/lib/librte_table/rte_table_hash_key16.c
> > index 2cca1c924..5e1665c15 100644
> > --- a/lib/librte_table/rte_table_hash_key16.c
> > +++ b/lib/librte_table/rte_table_hash_key16.c
> > @@ -44,7 +44,7 @@ struct rte_bucket_4_16 {
> >  	uint64_t key[4][2];
> >
> >  	/* Cache line 2 */
> > -	uint8_t data[0];
> > +	uint8_t data[0] __rte_cache_aligned;
> >  };
> >
> >  struct rte_table_hash {
> > --
> > 2.17.1
> 
> Hi Ting,
> 
> This fix is breaking the execution for systems with cache line of 128 bytes, as
> typically (on 64-bit systems) this structure would be 64-byte in size and
> adding the __rte_cache_aligned would force doubling the size of this
> structure through padding enforced by the compiler.
> 
> Can you please confirm this is caused by check below failing in the table
> create function:
> 	sizeof(struct rte_bucket_4_16) % 64) != 0
> 

The result of sizeof(struct rte_bucket_4_16) is 124 byte in this case, and this is the direct reason causing this failure.

> Since all the other fields in this data structure are explicitly declared as 64-bit
> fields, due to the alignment rules I was expecting the compiler to add a 32-bit
> padding field after the "next" field, which is a pointer that would only take 32
> bits on 32-bit systems. I am not sure why this did not take place in your case,
> any thoughts?
> 

It shows that the size of the field struct rte_bucket_4_16 *next in struct rte_bucket_4_16 is only 32 bits. And there is no padding added by the compiler in my and the reporter's case.
I tried to add a 32 bits pad field after the field next manually, and the result is correct then.
Is it because in 32-bit system, the compiler will not extend the 32 bits pointer to 64 bits, since the 32 bits size has already matched the cache line?

> Not sure why we would run Soft NIC on 32-bit systems, might be better to
> disable Soft NIC for 32-bit systems.
> 

To be honest, I do not know why we should run softnic on 32-bit system, I was just assigned this specific bug. It seems there is a complete test case for validation team to test softnic in 32-bit system.
I am not sure is it OK to tell the validation team that we should disable softnic in 32-bit system now. Or we should fix this issue this time and discuss about the problem later?

Thanks!

> Thanks,
> Cristian

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v3] lib/table: fix cache alignment issue
  2020-07-21  5:15     ` Xu, Ting
@ 2020-07-21 21:16       ` Dumitrescu, Cristian
  2020-07-22  2:16         ` Xu, Ting
  0 siblings, 1 reply; 22+ messages in thread
From: Dumitrescu, Cristian @ 2020-07-21 21:16 UTC (permalink / raw)
  To: Xu, Ting, dev; +Cc: stable



> -----Original Message-----
> From: Xu, Ting <ting.xu@intel.com>
> Sent: Tuesday, July 21, 2020 6:16 AM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: RE: [PATCH v3] lib/table: fix cache alignment issue
> 
> Hi, Cristian
> 
> > -----Original Message-----
> > From: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> > Sent: Monday, July 20, 2020 10:38 PM
> > To: Xu, Ting <ting.xu@intel.com>; dev@dpdk.org
> > Cc: stable@dpdk.org
> > Subject: RE: [PATCH v3] lib/table: fix cache alignment issue
> >
> >
> >
> > > -----Original Message-----
> > > From: Xu, Ting <ting.xu@intel.com>
> > > Sent: Thursday, July 9, 2020 2:48 AM
> > > To: dev@dpdk.org
> > > Cc: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Xu, Ting
> > > <ting.xu@intel.com>; stable@dpdk.org
> > > Subject: [PATCH v3] lib/table: fix cache alignment issue
> > >
> > > When create softnic hash table with 16 keys, it failed on 32bit
> > > environment because of the structure rte_bucket_4_16 alignment issue.
> > > Add __rte_cache_aligned to ensure correct cache align.
> > >
> > > Fixes: 8aa327214c ("table: hash")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Ting Xu <ting.xu@intel.com>
> > >
> > > ---
> > > v2->v3: Rebase
> > > v1->v2: Correct patch time
> > > ---
> > >  lib/librte_table/rte_table_hash_key16.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/lib/librte_table/rte_table_hash_key16.c
> > > b/lib/librte_table/rte_table_hash_key16.c
> > > index 2cca1c924..5e1665c15 100644
> > > --- a/lib/librte_table/rte_table_hash_key16.c
> > > +++ b/lib/librte_table/rte_table_hash_key16.c
> > > @@ -44,7 +44,7 @@ struct rte_bucket_4_16 {
> > >  	uint64_t key[4][2];
> > >
> > >  	/* Cache line 2 */
> > > -	uint8_t data[0];
> > > +	uint8_t data[0] __rte_cache_aligned;
> > >  };
> > >
> > >  struct rte_table_hash {
> > > --
> > > 2.17.1
> >
> > Hi Ting,
> >
> > This fix is breaking the execution for systems with cache line of 128 bytes,
> as
> > typically (on 64-bit systems) this structure would be 64-byte in size and
> > adding the __rte_cache_aligned would force doubling the size of this
> > structure through padding enforced by the compiler.
> >
> > Can you please confirm this is caused by check below failing in the table
> > create function:
> > 	sizeof(struct rte_bucket_4_16) % 64) != 0
> >
> 
> The result of sizeof(struct rte_bucket_4_16) is 124 byte in this case, and this
> is the direct reason causing this failure.
> 
> > Since all the other fields in this data structure are explicitly declared as 64-
> bit
> > fields, due to the alignment rules I was expecting the compiler to add a 32-
> bit
> > padding field after the "next" field, which is a pointer that would only take
> 32
> > bits on 32-bit systems. I am not sure why this did not take place in your
> case,
> > any thoughts?
> >
> 
> It shows that the size of the field struct rte_bucket_4_16 *next in struct
> rte_bucket_4_16 is only 32 bits. And there is no padding added by the
> compiler in my and the reporter's case.
> I tried to add a 32 bits pad field after the field next manually, and the result is
> correct then.
> Is it because in 32-bit system, the compiler will not extend the 32 bits pointer
> to 64 bits, since the 32 bits size has already matched the cache line?
> 
> > Not sure why we would run Soft NIC on 32-bit systems, might be better to
> > disable Soft NIC for 32-bit systems.
> >
> 

My proposed solution, which IMO provides the cleanest and most readable way to fix / maintain this code:

#ifdef RTE_ARCH_64

struct rte_bucket_4_16 {
	//current definition of this struct
};

#else

struct rte_bucket_4_16 {
	//definition with padding fields for the 32-bit pointers to keep this struct to a multiple of 64 bytes
};

#endif

We need to apply the same for 8-byte key and 32-byte key hash functions from the same folder.

What do you think, Ting?

> To be honest, I do not know why we should run softnic on 32-bit system, I
> was just assigned this specific bug. It seems there is a complete test case for
> validation team to test softnic in 32-bit system.
> I am not sure is it OK to tell the validation team that we should disable softnic
> in 32-bit system now. Or we should fix this issue this time and discuss about
> the problem later?
> 
> Thanks!
> 
> > Thanks,
> > Cristian

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v3] lib/table: fix cache alignment issue
  2020-07-21 21:16       ` Dumitrescu, Cristian
@ 2020-07-22  2:16         ` Xu, Ting
  0 siblings, 0 replies; 22+ messages in thread
From: Xu, Ting @ 2020-07-22  2:16 UTC (permalink / raw)
  To: Dumitrescu, Cristian, dev; +Cc: stable

Hi, Cristian,

> -----Original Message-----
> From: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Sent: Wednesday, July 22, 2020 5:17 AM
> To: Xu, Ting <ting.xu@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: RE: [PATCH v3] lib/table: fix cache alignment issue
> 
> 
> 
> > -----Original Message-----
> > From: Xu, Ting <ting.xu@intel.com>
> > Sent: Tuesday, July 21, 2020 6:16 AM
> > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; dev@dpdk.org
> > Cc: stable@dpdk.org
> > Subject: RE: [PATCH v3] lib/table: fix cache alignment issue
> >
> > Hi, Cristian
> >
> > > -----Original Message-----
> > > From: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> > > Sent: Monday, July 20, 2020 10:38 PM
> > > To: Xu, Ting <ting.xu@intel.com>; dev@dpdk.org
> > > Cc: stable@dpdk.org
> > > Subject: RE: [PATCH v3] lib/table: fix cache alignment issue
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Xu, Ting <ting.xu@intel.com>
> > > > Sent: Thursday, July 9, 2020 2:48 AM
> > > > To: dev@dpdk.org
> > > > Cc: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Xu, Ting
> > > > <ting.xu@intel.com>; stable@dpdk.org
> > > > Subject: [PATCH v3] lib/table: fix cache alignment issue
> > > >
> > > > When create softnic hash table with 16 keys, it failed on 32bit
> > > > environment because of the structure rte_bucket_4_16 alignment issue.
> > > > Add __rte_cache_aligned to ensure correct cache align.
> > > >
> > > > Fixes: 8aa327214c ("table: hash")
> > > > Cc: stable@dpdk.org
> > > >
> > > > Signed-off-by: Ting Xu <ting.xu@intel.com>
> > > >
> > > > ---
> > > > v2->v3: Rebase
> > > > v1->v2: Correct patch time
> > > > ---
> > > >  lib/librte_table/rte_table_hash_key16.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/lib/librte_table/rte_table_hash_key16.c
> > > > b/lib/librte_table/rte_table_hash_key16.c
> > > > index 2cca1c924..5e1665c15 100644
> > > > --- a/lib/librte_table/rte_table_hash_key16.c
> > > > +++ b/lib/librte_table/rte_table_hash_key16.c
> > > > @@ -44,7 +44,7 @@ struct rte_bucket_4_16 {
> > > >  	uint64_t key[4][2];
> > > >
> > > >  	/* Cache line 2 */
> > > > -	uint8_t data[0];
> > > > +	uint8_t data[0] __rte_cache_aligned;
> > > >  };
> > > >
> > > >  struct rte_table_hash {
> > > > --
> > > > 2.17.1
> > >
> > > Hi Ting,
> > >
> > > This fix is breaking the execution for systems with cache line of
> > > 128 bytes,
> > as
> > > typically (on 64-bit systems) this structure would be 64-byte in
> > > size and adding the __rte_cache_aligned would force doubling the
> > > size of this structure through padding enforced by the compiler.
> > >
> > > Can you please confirm this is caused by check below failing in the
> > > table create function:
> > > 	sizeof(struct rte_bucket_4_16) % 64) != 0
> > >
> >
> > The result of sizeof(struct rte_bucket_4_16) is 124 byte in this case,
> > and this is the direct reason causing this failure.
> >
> > > Since all the other fields in this data structure are explicitly
> > > declared as 64-
> > bit
> > > fields, due to the alignment rules I was expecting the compiler to
> > > add a 32-
> > bit
> > > padding field after the "next" field, which is a pointer that would
> > > only take
> > 32
> > > bits on 32-bit systems. I am not sure why this did not take place in
> > > your
> > case,
> > > any thoughts?
> > >
> >
> > It shows that the size of the field struct rte_bucket_4_16 *next in
> > struct
> > rte_bucket_4_16 is only 32 bits. And there is no padding added by the
> > compiler in my and the reporter's case.
> > I tried to add a 32 bits pad field after the field next manually, and
> > the result is correct then.
> > Is it because in 32-bit system, the compiler will not extend the 32
> > bits pointer to 64 bits, since the 32 bits size has already matched the cache
> line?
> >
> > > Not sure why we would run Soft NIC on 32-bit systems, might be
> > > better to disable Soft NIC for 32-bit systems.
> > >
> >
> 
> My proposed solution, which IMO provides the cleanest and most readable
> way to fix / maintain this code:
> 
> #ifdef RTE_ARCH_64
> 
> struct rte_bucket_4_16 {
> 	//current definition of this struct
> };
> 
> #else
> 
> struct rte_bucket_4_16 {
> 	//definition with padding fields for the 32-bit pointers to keep this
> struct to a multiple of 64 bytes };
> 
> #endif
> 
> We need to apply the same for 8-byte key and 32-byte key hash functions
> from the same folder.
> 
> What do you think, Ting?
> 

Thanks for your advice. I think it makes sense.
I have updated a new patch version based on this method, could you please help review?

Thanks!

> > To be honest, I do not know why we should run softnic on 32-bit
> > system, I was just assigned this specific bug. It seems there is a
> > complete test case for validation team to test softnic in 32-bit system.
> > I am not sure is it OK to tell the validation team that we should
> > disable softnic in 32-bit system now. Or we should fix this issue this
> > time and discuss about the problem later?
> >
> > Thanks!
> >
> > > Thanks,
> > > Cristian

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [dpdk-dev] [PATCH v4] lib/table: fix cache alignment issue
  2020-06-16 16:27 [dpdk-dev] [PATCH v1] lib/table: fix cache alignment issue Ting Xu
  2020-06-17  5:43 ` [dpdk-dev] [PATCH v2] " Ting Xu
  2020-07-09  1:48 ` [dpdk-dev] [PATCH v3] " Ting Xu
@ 2020-07-22  2:16 ` " Ting Xu
  2020-07-22  8:26   ` Dumitrescu, Cristian
                     ` (2 more replies)
  2 siblings, 3 replies; 22+ messages in thread
From: Ting Xu @ 2020-07-22  2:16 UTC (permalink / raw)
  To: dev; +Cc: cristian.dumitrescu, Ting Xu, stable

When create softnic hash table with 16 keys, it failed on 32-bit
environment, because the pointer field in structure rte_bucket_4_16
is only 32 bits. Add a padding field in 32-bit environment to keep
the structure to a multiple of 64 bytes. Apply this to 8-byte and
32-byte key hash function as well.

Fixes: 8aa327214c ("table: hash")
Cc: stable@dpdk.org

Signed-off-by: Ting Xu <ting.xu@intel.com>

---
v3->v4: Change design based on comment
v2->v3: Rebase
v1->v2: Correct patch time
---
 lib/librte_table/rte_table_hash_key16.c | 17 +++++++++++++++++
 lib/librte_table/rte_table_hash_key32.c | 17 +++++++++++++++++
 lib/librte_table/rte_table_hash_key8.c  | 16 ++++++++++++++++
 3 files changed, 50 insertions(+)

diff --git a/lib/librte_table/rte_table_hash_key16.c b/lib/librte_table/rte_table_hash_key16.c
index 2cca1c924..c4384b114 100644
--- a/lib/librte_table/rte_table_hash_key16.c
+++ b/lib/librte_table/rte_table_hash_key16.c
@@ -33,6 +33,7 @@
 
 #endif
 
+#ifdef RTE_ARCH_64
 struct rte_bucket_4_16 {
 	/* Cache line 0 */
 	uint64_t signature[4 + 1];
@@ -46,6 +47,22 @@ struct rte_bucket_4_16 {
 	/* Cache line 2 */
 	uint8_t data[0];
 };
+#else
+struct rte_bucket_4_16 {
+	/* Cache line 0 */
+	uint64_t signature[4 + 1];
+	uint64_t lru_list;
+	struct rte_bucket_4_16 *next;
+	uint32_t pad;
+	uint64_t next_valid;
+
+	/* Cache line 1 */
+	uint64_t key[4][2];
+
+	/* Cache line 2 */
+	uint8_t data[0];
+};
+#endif
 
 struct rte_table_hash {
 	struct rte_table_stats stats;
diff --git a/lib/librte_table/rte_table_hash_key32.c b/lib/librte_table/rte_table_hash_key32.c
index a137c5028..3e0031fe1 100644
--- a/lib/librte_table/rte_table_hash_key32.c
+++ b/lib/librte_table/rte_table_hash_key32.c
@@ -33,6 +33,7 @@
 
 #endif
 
+#ifdef RTE_ARCH_64
 struct rte_bucket_4_32 {
 	/* Cache line 0 */
 	uint64_t signature[4 + 1];
@@ -46,6 +47,22 @@ struct rte_bucket_4_32 {
 	/* Cache line 3 */
 	uint8_t data[0];
 };
+#else
+struct rte_bucket_4_32 {
+	/* Cache line 0 */
+	uint64_t signature[4 + 1];
+	uint64_t lru_list;
+	struct rte_bucket_4_32 *next;
+	uint32_t pad;
+	uint64_t next_valid;
+
+	/* Cache lines 1 and 2 */
+	uint64_t key[4][4];
+
+	/* Cache line 3 */
+	uint8_t data[0];
+};
+#endif
 
 struct rte_table_hash {
 	struct rte_table_stats stats;
diff --git a/lib/librte_table/rte_table_hash_key8.c b/lib/librte_table/rte_table_hash_key8.c
index 1811ad8d0..34e3ed1af 100644
--- a/lib/librte_table/rte_table_hash_key8.c
+++ b/lib/librte_table/rte_table_hash_key8.c
@@ -31,6 +31,7 @@
 
 #endif
 
+#ifdef RTE_ARCH_64
 struct rte_bucket_4_8 {
 	/* Cache line 0 */
 	uint64_t signature;
@@ -43,6 +44,21 @@ struct rte_bucket_4_8 {
 	/* Cache line 1 */
 	uint8_t data[0];
 };
+#else
+struct rte_bucket_4_8 {
+	/* Cache line 0 */
+	uint64_t signature;
+	uint64_t lru_list;
+	struct rte_bucket_4_8 *next;
+	uint32_t pad;
+	uint64_t next_valid;
+
+	uint64_t key[4];
+
+	/* Cache line 1 */
+	uint8_t data[0];
+};
+#endif
 
 struct rte_table_hash {
 	struct rte_table_stats stats;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v4] lib/table: fix cache alignment issue
  2020-07-22  2:16 ` [dpdk-dev] [PATCH v4] " Ting Xu
@ 2020-07-22  8:26   ` Dumitrescu, Cristian
  2020-07-22  8:30     ` Xu, Ting
  2020-07-22  8:48   ` Dumitrescu, Cristian
  2020-07-29 12:01   ` David Marchand
  2 siblings, 1 reply; 22+ messages in thread
From: Dumitrescu, Cristian @ 2020-07-22  8:26 UTC (permalink / raw)
  To: Xu, Ting, dev; +Cc: stable



> +#ifdef RTE_ARCH_64
>  struct rte_bucket_4_32 {
>  	/* Cache line 0 */
>  	uint64_t signature[4 + 1];
> @@ -46,6 +47,22 @@ struct rte_bucket_4_32 {
>  	/* Cache line 3 */
>  	uint8_t data[0];
>  };
> +#else
> +struct rte_bucket_4_32 {
> +	/* Cache line 0 */
> +	uint64_t signature[4 + 1];
> +	uint64_t lru_list;
> +	struct rte_bucket_4_32 *next;
> +	uint32_t pad;
> +	uint64_t next_valid;
> +
> +	/* Cache lines 1 and 2 */
> +	uint64_t key[4][4];
> +
> +	/* Cache line 3 */
> +	uint8_t data[0];
> +};
> +#endif
> 

Hi Ting,

Yes, it looks good, but as mentioned previously please do the same on the other files in the same folder and add the changes to your patch, as we need to keep all these files in sync:

rte_table_hash_key8.c, struct rte_bucket_4_8
rte_table_hash_key32.c, struct rte_bucket_4_32

Thanks,
Cristian

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v4] lib/table: fix cache alignment issue
  2020-07-22  8:26   ` Dumitrescu, Cristian
@ 2020-07-22  8:30     ` Xu, Ting
  2020-07-22  8:49       ` Dumitrescu, Cristian
  0 siblings, 1 reply; 22+ messages in thread
From: Xu, Ting @ 2020-07-22  8:30 UTC (permalink / raw)
  To: Dumitrescu, Cristian, dev; +Cc: stable

Hi, Cristian,

> -----Original Message-----
> From: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Sent: Wednesday, July 22, 2020 4:27 PM
> To: Xu, Ting <ting.xu@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: RE: [PATCH v4] lib/table: fix cache alignment issue
> 
> 
> 
> > +#ifdef RTE_ARCH_64
> >  struct rte_bucket_4_32 {
> >  	/* Cache line 0 */
> >  	uint64_t signature[4 + 1];
> > @@ -46,6 +47,22 @@ struct rte_bucket_4_32 {
> >  	/* Cache line 3 */
> >  	uint8_t data[0];
> >  };
> > +#else
> > +struct rte_bucket_4_32 {
> > +	/* Cache line 0 */
> > +	uint64_t signature[4 + 1];
> > +	uint64_t lru_list;
> > +	struct rte_bucket_4_32 *next;
> > +	uint32_t pad;
> > +	uint64_t next_valid;
> > +
> > +	/* Cache lines 1 and 2 */
> > +	uint64_t key[4][4];
> > +
> > +	/* Cache line 3 */
> > +	uint8_t data[0];
> > +};
> > +#endif
> >
> 
> Hi Ting,
> 
> Yes, it looks good, but as mentioned previously please do the same on the
> other files in the same folder and add the changes to your patch, as we need
> to keep all these files in sync:
> 
> rte_table_hash_key8.c, struct rte_bucket_4_8 rte_table_hash_key32.c, struct
> rte_bucket_4_32
> 

I have did the changes to 8 and 32 bytes in this patch.

> Thanks,
> Cristian

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v4] lib/table: fix cache alignment issue
  2020-07-22  2:16 ` [dpdk-dev] [PATCH v4] " Ting Xu
  2020-07-22  8:26   ` Dumitrescu, Cristian
@ 2020-07-22  8:48   ` Dumitrescu, Cristian
  2020-07-29 12:01   ` David Marchand
  2 siblings, 0 replies; 22+ messages in thread
From: Dumitrescu, Cristian @ 2020-07-22  8:48 UTC (permalink / raw)
  To: Xu, Ting, dev; +Cc: stable



> -----Original Message-----
> From: Xu, Ting <ting.xu@intel.com>
> Sent: Wednesday, July 22, 2020 3:16 AM
> To: dev@dpdk.org
> Cc: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Xu, Ting
> <ting.xu@intel.com>; stable@dpdk.org
> Subject: [PATCH v4] lib/table: fix cache alignment issue
> 
> When create softnic hash table with 16 keys, it failed on 32-bit
> environment, because the pointer field in structure rte_bucket_4_16
> is only 32 bits. Add a padding field in 32-bit environment to keep
> the structure to a multiple of 64 bytes. Apply this to 8-byte and
> 32-byte key hash function as well.
> 
> Fixes: 8aa327214c ("table: hash")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ting Xu <ting.xu@intel.com>
> 
> ---
> v3->v4: Change design based on comment
> v2->v3: Rebase
> v1->v2: Correct patch time
> ---

Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v4] lib/table: fix cache alignment issue
  2020-07-22  8:30     ` Xu, Ting
@ 2020-07-22  8:49       ` Dumitrescu, Cristian
  0 siblings, 0 replies; 22+ messages in thread
From: Dumitrescu, Cristian @ 2020-07-22  8:49 UTC (permalink / raw)
  To: Xu, Ting, dev; +Cc: stable



> -----Original Message-----
> From: Xu, Ting <ting.xu@intel.com>
> Sent: Wednesday, July 22, 2020 9:31 AM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: RE: [PATCH v4] lib/table: fix cache alignment issue
> 
> Hi, Cristian,
> 
> > -----Original Message-----
> > From: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> > Sent: Wednesday, July 22, 2020 4:27 PM
> > To: Xu, Ting <ting.xu@intel.com>; dev@dpdk.org
> > Cc: stable@dpdk.org
> > Subject: RE: [PATCH v4] lib/table: fix cache alignment issue
> >
> >
> >
> > > +#ifdef RTE_ARCH_64
> > >  struct rte_bucket_4_32 {
> > >  	/* Cache line 0 */
> > >  	uint64_t signature[4 + 1];
> > > @@ -46,6 +47,22 @@ struct rte_bucket_4_32 {
> > >  	/* Cache line 3 */
> > >  	uint8_t data[0];
> > >  };
> > > +#else
> > > +struct rte_bucket_4_32 {
> > > +	/* Cache line 0 */
> > > +	uint64_t signature[4 + 1];
> > > +	uint64_t lru_list;
> > > +	struct rte_bucket_4_32 *next;
> > > +	uint32_t pad;
> > > +	uint64_t next_valid;
> > > +
> > > +	/* Cache lines 1 and 2 */
> > > +	uint64_t key[4][4];
> > > +
> > > +	/* Cache line 3 */
> > > +	uint8_t data[0];
> > > +};
> > > +#endif
> > >
> >
> > Hi Ting,
> >
> > Yes, it looks good, but as mentioned previously please do the same on the
> > other files in the same folder and add the changes to your patch, as we
> need
> > to keep all these files in sync:
> >
> > rte_table_hash_key8.c, struct rte_bucket_4_8 rte_table_hash_key32.c,
> struct
> > rte_bucket_4_32
> >
> 
> I have did the changes to 8 and 32 bytes in this patch.
> 
> > Thanks,
> > Cristian

Upss, sorry, somehow I missed it. I just acked this patch. Thanks, Ting!

Regards,
Cristian

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v4] lib/table: fix cache alignment issue
  2020-07-22  2:16 ` [dpdk-dev] [PATCH v4] " Ting Xu
  2020-07-22  8:26   ` Dumitrescu, Cristian
  2020-07-22  8:48   ` Dumitrescu, Cristian
@ 2020-07-29 12:01   ` David Marchand
  2020-07-29 13:13     ` Dumitrescu, Cristian
  2 siblings, 1 reply; 22+ messages in thread
From: David Marchand @ 2020-07-29 12:01 UTC (permalink / raw)
  To: Ting Xu, Cristian Dumitrescu; +Cc: dev, dpdk stable

On Wed, Jul 22, 2020 at 4:13 AM Ting Xu <ting.xu@intel.com> wrote:
>
> When create softnic hash table with 16 keys, it failed on 32-bit
> environment, because the pointer field in structure rte_bucket_4_16
> is only 32 bits. Add a padding field in 32-bit environment to keep
> the structure to a multiple of 64 bytes. Apply this to 8-byte and
> 32-byte key hash function as well.

Please correct me if I am wrong, but it simply means this part of the
table library never worked for 32-bit.
It seems more adding 32-bit support rather than a fix and then I
wonder if it has its place in rc3.



Now, looking at the details:

For 64-bit on my x86, we have:

struct rte_bucket_4_8 {
    uint64_t                   signature;            /*     0     8 */
    uint64_t                   lru_list;             /*     8     8 */
    struct rte_bucket_4_8 *    next;                 /*    16     8 */
    uint64_t                   next_valid;           /*    24     8 */
    uint64_t                   key[4];               /*    32    32 */
    /* --- cacheline 1 boundary (64 bytes) --- */
    uint8_t                    data[];               /*    64     0 */

    /* size: 64, cachelines: 1, members: 6 */
};


For 32-bit, we have:

struct rte_bucket_4_8 {
    uint64_t                   signature;            /*     0     8 */
    uint64_t                   lru_list;             /*     8     8 */
    struct rte_bucket_4_8 *    next;                 /*    16     4 */
    uint64_t                   next_valid;           /*    20     8 */
    uint64_t                   key[4];               /*    28    32 */
    uint8_t                    data[];               /*    60     0 */

    /* size: 60, cachelines: 1, members: 6 */
    /* last cacheline: 60 bytes */
} __attribute__((__packed__));

^^ it is interesting that a packed attribute ends up here.
I saw no such attribute in the library code.
Compiler black magic at work I guess...


>
> Fixes: 8aa327214c ("table: hash")
> Cc: stable@dpdk.org
>
> Signed-off-by: Ting Xu <ting.xu@intel.com>
>
> ---
> v3->v4: Change design based on comment
> v2->v3: Rebase
> v1->v2: Correct patch time
> ---
>  lib/librte_table/rte_table_hash_key16.c | 17 +++++++++++++++++
>  lib/librte_table/rte_table_hash_key32.c | 17 +++++++++++++++++
>  lib/librte_table/rte_table_hash_key8.c  | 16 ++++++++++++++++
>  3 files changed, 50 insertions(+)
>
> diff --git a/lib/librte_table/rte_table_hash_key16.c b/lib/librte_table/rte_table_hash_key16.c
> index 2cca1c924..c4384b114 100644
> --- a/lib/librte_table/rte_table_hash_key16.c
> +++ b/lib/librte_table/rte_table_hash_key16.c
> @@ -33,6 +33,7 @@
>
>  #endif
>
> +#ifdef RTE_ARCH_64
>  struct rte_bucket_4_16 {
>         /* Cache line 0 */
>         uint64_t signature[4 + 1];
> @@ -46,6 +47,22 @@ struct rte_bucket_4_16 {
>         /* Cache line 2 */
>         uint8_t data[0];
>  };
> +#else
> +struct rte_bucket_4_16 {
> +       /* Cache line 0 */
> +       uint64_t signature[4 + 1];
> +       uint64_t lru_list;
> +       struct rte_bucket_4_16 *next;
> +       uint32_t pad;
> +       uint64_t next_valid;
> +
> +       /* Cache line 1 */
> +       uint64_t key[4][2];
> +
> +       /* Cache line 2 */
> +       uint8_t data[0];
> +};
> +#endif

The change could simply be:

@@ -38,6 +38,9 @@ struct rte_bucket_4_16 {
        uint64_t signature[4 + 1];
        uint64_t lru_list;
        struct rte_bucket_4_16 *next;
+#ifndef RTE_ARCH_64
+       uint32_t pad;
+#endif
        uint64_t next_valid;

        /* Cache line 1 */

It avoids duplicating the whole structure definition (we could miss
updating one side of the #ifdef later).
Idem for the other "8" and "32" structures.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v4] lib/table: fix cache alignment issue
  2020-07-29 12:01   ` David Marchand
@ 2020-07-29 13:13     ` Dumitrescu, Cristian
  2020-07-29 13:28       ` [dpdk-dev] [dpdk-stable] " David Marchand
  0 siblings, 1 reply; 22+ messages in thread
From: Dumitrescu, Cristian @ 2020-07-29 13:13 UTC (permalink / raw)
  To: David Marchand, Xu, Ting; +Cc: dev, dpdk stable



> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Wednesday, July 29, 2020 1:01 PM
> To: Xu, Ting <ting.xu@intel.com>; Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com>
> Cc: dev <dev@dpdk.org>; dpdk stable <stable@dpdk.org>
> Subject: Re: [dpdk-dev] [PATCH v4] lib/table: fix cache alignment issue
> 
> On Wed, Jul 22, 2020 at 4:13 AM Ting Xu <ting.xu@intel.com> wrote:
> >
> > When create softnic hash table with 16 keys, it failed on 32-bit
> > environment, because the pointer field in structure rte_bucket_4_16
> > is only 32 bits. Add a padding field in 32-bit environment to keep
> > the structure to a multiple of 64 bytes. Apply this to 8-byte and
> > 32-byte key hash function as well.
> 
> Please correct me if I am wrong, but it simply means this part of the
> table library never worked for 32-bit.
> It seems more adding 32-bit support rather than a fix and then I
> wonder if it has its place in rc3.
> 

Functionally. the code works, but performance is affected.

The only thing that prevents the code from working is the check in the table create function that checks the size of the above structure is 64 bytes, which caught this issue.

> 
> 
> Now, looking at the details:
> 
> For 64-bit on my x86, we have:
> 
> struct rte_bucket_4_8 {
>     uint64_t                   signature;            /*     0     8 */
>     uint64_t                   lru_list;             /*     8     8 */
>     struct rte_bucket_4_8 *    next;                 /*    16     8 */
>     uint64_t                   next_valid;           /*    24     8 */
>     uint64_t                   key[4];               /*    32    32 */
>     /* --- cacheline 1 boundary (64 bytes) --- */
>     uint8_t                    data[];               /*    64     0 */
> 
>     /* size: 64, cachelines: 1, members: 6 */
> };
> 
> 
> For 32-bit, we have:
> 
> struct rte_bucket_4_8 {
>     uint64_t                   signature;            /*     0     8 */
>     uint64_t                   lru_list;             /*     8     8 */
>     struct rte_bucket_4_8 *    next;                 /*    16     4 */
>     uint64_t                   next_valid;           /*    20     8 */
>     uint64_t                   key[4];               /*    28    32 */
>     uint8_t                    data[];               /*    60     0 */
> 
>     /* size: 60, cachelines: 1, members: 6 */
>     /* last cacheline: 60 bytes */
> } __attribute__((__packed__));
> 
> ^^ it is interesting that a packed attribute ends up here.
> I saw no such attribute in the library code.
> Compiler black magic at work I guess...
> 

Where do you see the packet attribute? I don't see it in the code.

A packet attribute would explain this issue, i.e. why did the compiler decide not to insert an expected padfing of 4 bytes right after the "next" field, that would allow the field "next_valid" to be aligned to its natural boundary of 8 bytes.

> 
> >
> > Fixes: 8aa327214c ("table: hash")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Ting Xu <ting.xu@intel.com>
> >
> > ---
> > v3->v4: Change design based on comment
> > v2->v3: Rebase
> > v1->v2: Correct patch time
> > ---
> >  lib/librte_table/rte_table_hash_key16.c | 17 +++++++++++++++++
> >  lib/librte_table/rte_table_hash_key32.c | 17 +++++++++++++++++
> >  lib/librte_table/rte_table_hash_key8.c  | 16 ++++++++++++++++
> >  3 files changed, 50 insertions(+)
> >
> > diff --git a/lib/librte_table/rte_table_hash_key16.c
> b/lib/librte_table/rte_table_hash_key16.c
> > index 2cca1c924..c4384b114 100644
> > --- a/lib/librte_table/rte_table_hash_key16.c
> > +++ b/lib/librte_table/rte_table_hash_key16.c
> > @@ -33,6 +33,7 @@
> >
> >  #endif
> >
> > +#ifdef RTE_ARCH_64
> >  struct rte_bucket_4_16 {
> >         /* Cache line 0 */
> >         uint64_t signature[4 + 1];
> > @@ -46,6 +47,22 @@ struct rte_bucket_4_16 {
> >         /* Cache line 2 */
> >         uint8_t data[0];
> >  };
> > +#else
> > +struct rte_bucket_4_16 {
> > +       /* Cache line 0 */
> > +       uint64_t signature[4 + 1];
> > +       uint64_t lru_list;
> > +       struct rte_bucket_4_16 *next;
> > +       uint32_t pad;
> > +       uint64_t next_valid;
> > +
> > +       /* Cache line 1 */
> > +       uint64_t key[4][2];
> > +
> > +       /* Cache line 2 */
> > +       uint8_t data[0];
> > +};
> > +#endif
> 
> The change could simply be:
> 
> @@ -38,6 +38,9 @@ struct rte_bucket_4_16 {
>         uint64_t signature[4 + 1];
>         uint64_t lru_list;
>         struct rte_bucket_4_16 *next;
> +#ifndef RTE_ARCH_64
> +       uint32_t pad;
> +#endif
>         uint64_t next_valid;
> 
>         /* Cache line 1 */
> 
> It avoids duplicating the whole structure definition (we could miss
> updating one side of the #ifdef later).
> Idem for the other "8" and "32" structures.
> 
> 
> --
> David Marchand


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH v4] lib/table: fix cache alignment issue
  2020-07-29 13:13     ` Dumitrescu, Cristian
@ 2020-07-29 13:28       ` " David Marchand
  2020-07-29 13:54         ` Dumitrescu, Cristian
  2020-07-30 10:35         ` Kevin Traynor
  0 siblings, 2 replies; 22+ messages in thread
From: David Marchand @ 2020-07-29 13:28 UTC (permalink / raw)
  To: Dumitrescu, Cristian
  Cc: Xu, Ting, dev, dpdk stable, Kevin Traynor, Luca Boccassi

On Wed, Jul 29, 2020 at 3:14 PM Dumitrescu, Cristian
<cristian.dumitrescu@intel.com> wrote:
> > Please correct me if I am wrong, but it simply means this part of the
> > table library never worked for 32-bit.
> > It seems more adding 32-bit support rather than a fix and then I
> > wonder if it has its place in rc3.
> >
>
> Functionally. the code works, but performance is affected.
>
> The only thing that prevents the code from working is the check in the table create function that checks the size of the above structure is 64 bytes, which caught this issue.

Yes, and that's my point.
It was not working.
It was not tested.


This patch asks for backport in stable branches, I will let Kevin and
Luca comment.


>
> >
> >
> > Now, looking at the details:
> >
> > For 64-bit on my x86, we have:
> >
> > struct rte_bucket_4_8 {
> >     uint64_t                   signature;            /*     0     8 */
> >     uint64_t                   lru_list;             /*     8     8 */
> >     struct rte_bucket_4_8 *    next;                 /*    16     8 */
> >     uint64_t                   next_valid;           /*    24     8 */
> >     uint64_t                   key[4];               /*    32    32 */
> >     /* --- cacheline 1 boundary (64 bytes) --- */
> >     uint8_t                    data[];               /*    64     0 */
> >
> >     /* size: 64, cachelines: 1, members: 6 */
> > };
> >
> >
> > For 32-bit, we have:
> >
> > struct rte_bucket_4_8 {
> >     uint64_t                   signature;            /*     0     8 */
> >     uint64_t                   lru_list;             /*     8     8 */
> >     struct rte_bucket_4_8 *    next;                 /*    16     4 */
> >     uint64_t                   next_valid;           /*    20     8 */
> >     uint64_t                   key[4];               /*    28    32 */
> >     uint8_t                    data[];               /*    60     0 */
> >
> >     /* size: 60, cachelines: 1, members: 6 */
> >     /* last cacheline: 60 bytes */
> > } __attribute__((__packed__));
> >
> > ^^ it is interesting that a packed attribute ends up here.
> > I saw no such attribute in the library code.
> > Compiler black magic at work I guess...
> >
>
> Where do you see the packet attribute? I don't see it in the code.

That's pahole reporting this.
Maybe the tool extrapolates this attribute based on the next_valid
field placement... I don't know.

> A packet attribute would explain this issue, i.e. why did the compiler decide not to insert an expected padfing of 4 bytes right after the "next" field, that would allow the field "next_valid" to be aligned to its natural boundary of 8 bytes.

Or a 64-bit field on 32-bit has a special alignment that I am not aware of.


>
> >
> > >
> > > Fixes: 8aa327214c ("table: hash")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Ting Xu <ting.xu@intel.com>
> > >
> > > ---
> > > v3->v4: Change design based on comment
> > > v2->v3: Rebase
> > > v1->v2: Correct patch time
> > > ---
> > >  lib/librte_table/rte_table_hash_key16.c | 17 +++++++++++++++++
> > >  lib/librte_table/rte_table_hash_key32.c | 17 +++++++++++++++++
> > >  lib/librte_table/rte_table_hash_key8.c  | 16 ++++++++++++++++
> > >  3 files changed, 50 insertions(+)
> > >
> > > diff --git a/lib/librte_table/rte_table_hash_key16.c
> > b/lib/librte_table/rte_table_hash_key16.c
> > > index 2cca1c924..c4384b114 100644
> > > --- a/lib/librte_table/rte_table_hash_key16.c
> > > +++ b/lib/librte_table/rte_table_hash_key16.c
> > > @@ -33,6 +33,7 @@
> > >
> > >  #endif
> > >
> > > +#ifdef RTE_ARCH_64
> > >  struct rte_bucket_4_16 {
> > >         /* Cache line 0 */
> > >         uint64_t signature[4 + 1];
> > > @@ -46,6 +47,22 @@ struct rte_bucket_4_16 {
> > >         /* Cache line 2 */
> > >         uint8_t data[0];
> > >  };
> > > +#else
> > > +struct rte_bucket_4_16 {
> > > +       /* Cache line 0 */
> > > +       uint64_t signature[4 + 1];
> > > +       uint64_t lru_list;
> > > +       struct rte_bucket_4_16 *next;
> > > +       uint32_t pad;
> > > +       uint64_t next_valid;
> > > +
> > > +       /* Cache line 1 */
> > > +       uint64_t key[4][2];
> > > +
> > > +       /* Cache line 2 */
> > > +       uint8_t data[0];
> > > +};
> > > +#endif
> >
> > The change could simply be:
> >
> > @@ -38,6 +38,9 @@ struct rte_bucket_4_16 {
> >         uint64_t signature[4 + 1];
> >         uint64_t lru_list;
> >         struct rte_bucket_4_16 *next;
> > +#ifndef RTE_ARCH_64
> > +       uint32_t pad;
> > +#endif
> >         uint64_t next_valid;
> >
> >         /* Cache line 1 */
> >
> > It avoids duplicating the whole structure definition (we could miss
> > updating one side of the #ifdef later).
> > Idem for the other "8" and "32" structures.


What about this comment?


-- 
David Marchand


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH v4] lib/table: fix cache alignment issue
  2020-07-29 13:28       ` [dpdk-dev] [dpdk-stable] " David Marchand
@ 2020-07-29 13:54         ` Dumitrescu, Cristian
  2020-07-29 13:59           ` David Marchand
  2020-07-30 10:35         ` Kevin Traynor
  1 sibling, 1 reply; 22+ messages in thread
From: Dumitrescu, Cristian @ 2020-07-29 13:54 UTC (permalink / raw)
  To: David Marchand; +Cc: Xu, Ting, dev, dpdk stable, Kevin Traynor, Luca Boccassi



> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Wednesday, July 29, 2020 2:28 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: Xu, Ting <ting.xu@intel.com>; dev <dev@dpdk.org>; dpdk stable
> <stable@dpdk.org>; Kevin Traynor <ktraynor@redhat.com>; Luca Boccassi
> <bluca@debian.org>
> Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH v4] lib/table: fix cache
> alignment issue
> 
> On Wed, Jul 29, 2020 at 3:14 PM Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com> wrote:
> > > Please correct me if I am wrong, but it simply means this part of the
> > > table library never worked for 32-bit.
> > > It seems more adding 32-bit support rather than a fix and then I
> > > wonder if it has its place in rc3.
> > >
> >
> > Functionally. the code works, but performance is affected.
> >
> > The only thing that prevents the code from working is the check in the
> table create function that checks the size of the above structure is 64 bytes,
> which caught this issue.
> 
> Yes, and that's my point.
> It was not working.
> It was not tested.
> 
> 

Not sure when this code was last tested on 32-bit systems, I'll let the validation folks comment on this, but I cannot rule out a change in compiler behavior either.

This is a low complexity and low impact change, hence low risk IMO.

> This patch asks for backport in stable branches, I will let Kevin and
> Luca comment.
> 
> 
> >
> > >
> > >
> > > Now, looking at the details:
> > >
> > > For 64-bit on my x86, we have:
> > >
> > > struct rte_bucket_4_8 {
> > >     uint64_t                   signature;            /*     0     8 */
> > >     uint64_t                   lru_list;             /*     8     8 */
> > >     struct rte_bucket_4_8 *    next;                 /*    16     8 */
> > >     uint64_t                   next_valid;           /*    24     8 */
> > >     uint64_t                   key[4];               /*    32    32 */
> > >     /* --- cacheline 1 boundary (64 bytes) --- */
> > >     uint8_t                    data[];               /*    64     0 */
> > >
> > >     /* size: 64, cachelines: 1, members: 6 */
> > > };
> > >
> > >
> > > For 32-bit, we have:
> > >
> > > struct rte_bucket_4_8 {
> > >     uint64_t                   signature;            /*     0     8 */
> > >     uint64_t                   lru_list;             /*     8     8 */
> > >     struct rte_bucket_4_8 *    next;                 /*    16     4 */
> > >     uint64_t                   next_valid;           /*    20     8 */
> > >     uint64_t                   key[4];               /*    28    32 */
> > >     uint8_t                    data[];               /*    60     0 */
> > >
> > >     /* size: 60, cachelines: 1, members: 6 */
> > >     /* last cacheline: 60 bytes */
> > > } __attribute__((__packed__));
> > >
> > > ^^ it is interesting that a packed attribute ends up here.
> > > I saw no such attribute in the library code.
> > > Compiler black magic at work I guess...
> > >
> >
> > Where do you see the packet attribute? I don't see it in the code.
> 
> That's pahole reporting this.
> Maybe the tool extrapolates this attribute based on the next_valid
> field placement... I don't know.
> 
> > A packet attribute would explain this issue, i.e. why did the compiler decide
> not to insert an expected padfing of 4 bytes right after the "next" field, that
> would allow the field "next_valid" to be aligned to its natural boundary of 8
> bytes.
> 
> Or a 64-bit field on 32-bit has a special alignment that I am not aware of.
> 
> 
> >
> > >
> > > >
> > > > Fixes: 8aa327214c ("table: hash")
> > > > Cc: stable@dpdk.org
> > > >
> > > > Signed-off-by: Ting Xu <ting.xu@intel.com>
> > > >
> > > > ---
> > > > v3->v4: Change design based on comment
> > > > v2->v3: Rebase
> > > > v1->v2: Correct patch time
> > > > ---
> > > >  lib/librte_table/rte_table_hash_key16.c | 17 +++++++++++++++++
> > > >  lib/librte_table/rte_table_hash_key32.c | 17 +++++++++++++++++
> > > >  lib/librte_table/rte_table_hash_key8.c  | 16 ++++++++++++++++
> > > >  3 files changed, 50 insertions(+)
> > > >
> > > > diff --git a/lib/librte_table/rte_table_hash_key16.c
> > > b/lib/librte_table/rte_table_hash_key16.c
> > > > index 2cca1c924..c4384b114 100644
> > > > --- a/lib/librte_table/rte_table_hash_key16.c
> > > > +++ b/lib/librte_table/rte_table_hash_key16.c
> > > > @@ -33,6 +33,7 @@
> > > >
> > > >  #endif
> > > >
> > > > +#ifdef RTE_ARCH_64
> > > >  struct rte_bucket_4_16 {
> > > >         /* Cache line 0 */
> > > >         uint64_t signature[4 + 1];
> > > > @@ -46,6 +47,22 @@ struct rte_bucket_4_16 {
> > > >         /* Cache line 2 */
> > > >         uint8_t data[0];
> > > >  };
> > > > +#else
> > > > +struct rte_bucket_4_16 {
> > > > +       /* Cache line 0 */
> > > > +       uint64_t signature[4 + 1];
> > > > +       uint64_t lru_list;
> > > > +       struct rte_bucket_4_16 *next;
> > > > +       uint32_t pad;
> > > > +       uint64_t next_valid;
> > > > +
> > > > +       /* Cache line 1 */
> > > > +       uint64_t key[4][2];
> > > > +
> > > > +       /* Cache line 2 */
> > > > +       uint8_t data[0];
> > > > +};
> > > > +#endif
> > >
> > > The change could simply be:
> > >
> > > @@ -38,6 +38,9 @@ struct rte_bucket_4_16 {
> > >         uint64_t signature[4 + 1];
> > >         uint64_t lru_list;
> > >         struct rte_bucket_4_16 *next;
> > > +#ifndef RTE_ARCH_64
> > > +       uint32_t pad;
> > > +#endif
> > >         uint64_t next_valid;
> > >
> > >         /* Cache line 1 */
> > >
> > > It avoids duplicating the whole structure definition (we could miss
> > > updating one side of the #ifdef later).
> > > Idem for the other "8" and "32" structures.
> 
> 
> What about this comment?
> 
> 
> --
> David Marchand


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH v4] lib/table: fix cache alignment issue
  2020-07-29 13:54         ` Dumitrescu, Cristian
@ 2020-07-29 13:59           ` David Marchand
  2020-07-29 14:53             ` Dumitrescu, Cristian
  0 siblings, 1 reply; 22+ messages in thread
From: David Marchand @ 2020-07-29 13:59 UTC (permalink / raw)
  To: Dumitrescu, Cristian
  Cc: Xu, Ting, dev, dpdk stable, Kevin Traynor, Luca Boccassi

On Wed, Jul 29, 2020 at 3:54 PM Dumitrescu, Cristian
<cristian.dumitrescu@intel.com> wrote:
> > -----Original Message-----
> > From: David Marchand <david.marchand@redhat.com>
> > Sent: Wednesday, July 29, 2020 2:28 PM
> > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> > Cc: Xu, Ting <ting.xu@intel.com>; dev <dev@dpdk.org>; dpdk stable
> > <stable@dpdk.org>; Kevin Traynor <ktraynor@redhat.com>; Luca Boccassi
> > <bluca@debian.org>
> > Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH v4] lib/table: fix cache
> > alignment issue
> >
> > On Wed, Jul 29, 2020 at 3:14 PM Dumitrescu, Cristian
> > <cristian.dumitrescu@intel.com> wrote:
> > > > Please correct me if I am wrong, but it simply means this part of the
> > > > table library never worked for 32-bit.
> > > > It seems more adding 32-bit support rather than a fix and then I
> > > > wonder if it has its place in rc3.
> > > >
> > >
> > > Functionally. the code works, but performance is affected.
> > >
> > > The only thing that prevents the code from working is the check in the
> > table create function that checks the size of the above structure is 64 bytes,
> > which caught this issue.
> >
> > Yes, and that's my point.
> > It was not working.
> > It was not tested.
> >
> >
>
> Not sure when this code was last tested on 32-bit systems, I'll let the validation folks comment on this, but I cannot rule out a change in compiler behavior either.
>
> This is a low complexity and low impact change, hence low risk IMO.

Risk is to be evaluated when there is a need.
I got pinged on this, like it was the end of the times.

Then I find something that is not worth looking at, hence I am a bit irritated.

And please, for the 2nd time, can you look at my comment below?


> > > > > diff --git a/lib/librte_table/rte_table_hash_key16.c
> > > > b/lib/librte_table/rte_table_hash_key16.c
> > > > > index 2cca1c924..c4384b114 100644
> > > > > --- a/lib/librte_table/rte_table_hash_key16.c
> > > > > +++ b/lib/librte_table/rte_table_hash_key16.c
> > > > > @@ -33,6 +33,7 @@
> > > > >
> > > > >  #endif
> > > > >
> > > > > +#ifdef RTE_ARCH_64
> > > > >  struct rte_bucket_4_16 {
> > > > >         /* Cache line 0 */
> > > > >         uint64_t signature[4 + 1];
> > > > > @@ -46,6 +47,22 @@ struct rte_bucket_4_16 {
> > > > >         /* Cache line 2 */
> > > > >         uint8_t data[0];
> > > > >  };
> > > > > +#else
> > > > > +struct rte_bucket_4_16 {
> > > > > +       /* Cache line 0 */
> > > > > +       uint64_t signature[4 + 1];
> > > > > +       uint64_t lru_list;
> > > > > +       struct rte_bucket_4_16 *next;
> > > > > +       uint32_t pad;
> > > > > +       uint64_t next_valid;
> > > > > +
> > > > > +       /* Cache line 1 */
> > > > > +       uint64_t key[4][2];
> > > > > +
> > > > > +       /* Cache line 2 */
> > > > > +       uint8_t data[0];
> > > > > +};
> > > > > +#endif
> > > >
> > > > The change could simply be:
> > > >
> > > > @@ -38,6 +38,9 @@ struct rte_bucket_4_16 {
> > > >         uint64_t signature[4 + 1];
> > > >         uint64_t lru_list;
> > > >         struct rte_bucket_4_16 *next;
> > > > +#ifndef RTE_ARCH_64
> > > > +       uint32_t pad;
> > > > +#endif
> > > >         uint64_t next_valid;
> > > >
> > > >         /* Cache line 1 */
> > > >
> > > > It avoids duplicating the whole structure definition (we could miss
> > > > updating one side of the #ifdef later).
> > > > Idem for the other "8" and "32" structures.
> >
> >
> > What about this comment?

What about this comment?


-- 
David Marchand


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH v4] lib/table: fix cache alignment issue
  2020-07-29 13:59           ` David Marchand
@ 2020-07-29 14:53             ` Dumitrescu, Cristian
  2020-07-30  6:57               ` Xu, Ting
  0 siblings, 1 reply; 22+ messages in thread
From: Dumitrescu, Cristian @ 2020-07-29 14:53 UTC (permalink / raw)
  To: David Marchand; +Cc: Xu, Ting, dev, dpdk stable, Kevin Traynor, Luca Boccassi



> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Wednesday, July 29, 2020 3:00 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: Xu, Ting <ting.xu@intel.com>; dev <dev@dpdk.org>; dpdk stable
> <stable@dpdk.org>; Kevin Traynor <ktraynor@redhat.com>; Luca Boccassi
> <bluca@debian.org>
> Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH v4] lib/table: fix cache
> alignment issue
> 
> On Wed, Jul 29, 2020 at 3:54 PM Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com> wrote:
> > > -----Original Message-----
> > > From: David Marchand <david.marchand@redhat.com>
> > > Sent: Wednesday, July 29, 2020 2:28 PM
> > > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> > > Cc: Xu, Ting <ting.xu@intel.com>; dev <dev@dpdk.org>; dpdk stable
> > > <stable@dpdk.org>; Kevin Traynor <ktraynor@redhat.com>; Luca
> Boccassi
> > > <bluca@debian.org>
> > > Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH v4] lib/table: fix cache
> > > alignment issue
> > >
> > > On Wed, Jul 29, 2020 at 3:14 PM Dumitrescu, Cristian
> > > <cristian.dumitrescu@intel.com> wrote:
> > > > > Please correct me if I am wrong, but it simply means this part of the
> > > > > table library never worked for 32-bit.
> > > > > It seems more adding 32-bit support rather than a fix and then I
> > > > > wonder if it has its place in rc3.
> > > > >
> > > >
> > > > Functionally. the code works, but performance is affected.
> > > >
> > > > The only thing that prevents the code from working is the check in the
> > > table create function that checks the size of the above structure is 64
> bytes,
> > > which caught this issue.
> > >
> > > Yes, and that's my point.
> > > It was not working.
> > > It was not tested.
> > >
> > >
> >
> > Not sure when this code was last tested on 32-bit systems, I'll let the
> validation folks comment on this, but I cannot rule out a change in compiler
> behavior either.
> >
> > This is a low complexity and low impact change, hence low risk IMO.
> 
> Risk is to be evaluated when there is a need.
> I got pinged on this, like it was the end of the times.
> 
> Then I find something that is not worth looking at, hence I am a bit irritated.
> 

I got pinged as well, and I also had to allocate time on this patch. It probably means it is important for somebody.

> And please, for the 2nd time, can you look at my comment below?
> 
Sorry, I missed it first.

> 
> > > > > > diff --git a/lib/librte_table/rte_table_hash_key16.c
> > > > > b/lib/librte_table/rte_table_hash_key16.c
> > > > > > index 2cca1c924..c4384b114 100644
> > > > > > --- a/lib/librte_table/rte_table_hash_key16.c
> > > > > > +++ b/lib/librte_table/rte_table_hash_key16.c
> > > > > > @@ -33,6 +33,7 @@
> > > > > >
> > > > > >  #endif
> > > > > >
> > > > > > +#ifdef RTE_ARCH_64
> > > > > >  struct rte_bucket_4_16 {
> > > > > >         /* Cache line 0 */
> > > > > >         uint64_t signature[4 + 1];
> > > > > > @@ -46,6 +47,22 @@ struct rte_bucket_4_16 {
> > > > > >         /* Cache line 2 */
> > > > > >         uint8_t data[0];
> > > > > >  };
> > > > > > +#else
> > > > > > +struct rte_bucket_4_16 {
> > > > > > +       /* Cache line 0 */
> > > > > > +       uint64_t signature[4 + 1];
> > > > > > +       uint64_t lru_list;
> > > > > > +       struct rte_bucket_4_16 *next;
> > > > > > +       uint32_t pad;
> > > > > > +       uint64_t next_valid;
> > > > > > +
> > > > > > +       /* Cache line 1 */
> > > > > > +       uint64_t key[4][2];
> > > > > > +
> > > > > > +       /* Cache line 2 */
> > > > > > +       uint8_t data[0];
> > > > > > +};
> > > > > > +#endif
> > > > >
> > > > > The change could simply be:
> > > > >
> > > > > @@ -38,6 +38,9 @@ struct rte_bucket_4_16 {
> > > > >         uint64_t signature[4 + 1];
> > > > >         uint64_t lru_list;
> > > > >         struct rte_bucket_4_16 *next;
> > > > > +#ifndef RTE_ARCH_64
> > > > > +       uint32_t pad;
> > > > > +#endif
> > > > >         uint64_t next_valid;
> > > > >
> > > > >         /* Cache line 1 */
> > > > >
> > > > > It avoids duplicating the whole structure definition (we could miss
> > > > > updating one side of the #ifdef later).
> > > > > Idem for the other "8" and "32" structures.
> > >
> > >
> > > What about this comment?
> 
> What about this comment?
> 

You might suspect I also thought about this option. My preference is for the option in the patch for the reasons that IMO it is easier to read and understand the reason for the difference, even though the code is slightly larger. It also leaves the 64-bit code untouched, so it is easier to remove when we finally decide at some point to drop the 32-bit support.

But I can live with the option you describe as well. Thanks for the input.

For me, it would be great if somebody on this list could indicate why the 4-byte padding was not inserted by the compiler automatically, and hence the need for this fix.

> 
> --
> David Marchand


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH v4] lib/table: fix cache alignment issue
  2020-07-29 14:53             ` Dumitrescu, Cristian
@ 2020-07-30  6:57               ` Xu, Ting
  0 siblings, 0 replies; 22+ messages in thread
From: Xu, Ting @ 2020-07-30  6:57 UTC (permalink / raw)
  To: Dumitrescu, Cristian, David Marchand
  Cc: dev, dpdk stable, Kevin Traynor, Luca Boccassi

Hi, all,

> -----Original Message-----
> From: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Sent: Wednesday, July 29, 2020 10:53 PM
> To: David Marchand <david.marchand@redhat.com>
> Cc: Xu, Ting <ting.xu@intel.com>; dev <dev@dpdk.org>; dpdk stable
> <stable@dpdk.org>; Kevin Traynor <ktraynor@redhat.com>; Luca Boccassi
> <bluca@debian.org>
> Subject: RE: [dpdk-stable] [dpdk-dev] [PATCH v4] lib/table: fix cache alignment
> issue
> 
> 
> 
> > -----Original Message-----
> > From: David Marchand <david.marchand@redhat.com>
> > Sent: Wednesday, July 29, 2020 3:00 PM
> > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> > Cc: Xu, Ting <ting.xu@intel.com>; dev <dev@dpdk.org>; dpdk stable
> > <stable@dpdk.org>; Kevin Traynor <ktraynor@redhat.com>; Luca Boccassi
> > <bluca@debian.org>
> > Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH v4] lib/table: fix cache
> > alignment issue
> >
> > On Wed, Jul 29, 2020 at 3:54 PM Dumitrescu, Cristian
> > <cristian.dumitrescu@intel.com> wrote:
> > > > -----Original Message-----
> > > > From: David Marchand <david.marchand@redhat.com>
> > > > Sent: Wednesday, July 29, 2020 2:28 PM
> > > > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> > > > Cc: Xu, Ting <ting.xu@intel.com>; dev <dev@dpdk.org>; dpdk stable
> > > > <stable@dpdk.org>; Kevin Traynor <ktraynor@redhat.com>; Luca
> > Boccassi
> > > > <bluca@debian.org>
> > > > Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH v4] lib/table: fix
> > > > cache alignment issue
> > > >
> > > > On Wed, Jul 29, 2020 at 3:14 PM Dumitrescu, Cristian
> > > > <cristian.dumitrescu@intel.com> wrote:
> > > > > > Please correct me if I am wrong, but it simply means this part
> > > > > > of the table library never worked for 32-bit.
> > > > > > It seems more adding 32-bit support rather than a fix and then
> > > > > > I wonder if it has its place in rc3.
> > > > > >
> > > > >
> > > > > Functionally. the code works, but performance is affected.
> > > > >
> > > > > The only thing that prevents the code from working is the check
> > > > > in the
> > > > table create function that checks the size of the above structure
> > > > is 64
> > bytes,
> > > > which caught this issue.
> > > >
> > > > Yes, and that's my point.
> > > > It was not working.
> > > > It was not tested.
> > > >
> > > >
> > >
> > > Not sure when this code was last tested on 32-bit systems, I'll let
> > > the
> > validation folks comment on this, but I cannot rule out a change in
> > compiler behavior either.
> > >
> > > This is a low complexity and low impact change, hence low risk IMO.
> >
> > Risk is to be evaluated when there is a need.
> > I got pinged on this, like it was the end of the times.
> >
> > Then I find something that is not worth looking at, hence I am a bit irritated.
> >
> 
> I got pinged as well, and I also had to allocate time on this patch. It probably
> means it is important for somebody.
> 
> > And please, for the 2nd time, can you look at my comment below?
> >
> Sorry, I missed it first.
> 
> >
> > > > > > > diff --git a/lib/librte_table/rte_table_hash_key16.c
> > > > > > b/lib/librte_table/rte_table_hash_key16.c
> > > > > > > index 2cca1c924..c4384b114 100644
> > > > > > > --- a/lib/librte_table/rte_table_hash_key16.c
> > > > > > > +++ b/lib/librte_table/rte_table_hash_key16.c
> > > > > > > @@ -33,6 +33,7 @@
> > > > > > >
> > > > > > >  #endif
> > > > > > >
> > > > > > > +#ifdef RTE_ARCH_64
> > > > > > >  struct rte_bucket_4_16 {
> > > > > > >         /* Cache line 0 */
> > > > > > >         uint64_t signature[4 + 1]; @@ -46,6 +47,22 @@ struct
> > > > > > > rte_bucket_4_16 {
> > > > > > >         /* Cache line 2 */
> > > > > > >         uint8_t data[0];
> > > > > > >  };
> > > > > > > +#else
> > > > > > > +struct rte_bucket_4_16 {
> > > > > > > +       /* Cache line 0 */
> > > > > > > +       uint64_t signature[4 + 1];
> > > > > > > +       uint64_t lru_list;
> > > > > > > +       struct rte_bucket_4_16 *next;
> > > > > > > +       uint32_t pad;
> > > > > > > +       uint64_t next_valid;
> > > > > > > +
> > > > > > > +       /* Cache line 1 */
> > > > > > > +       uint64_t key[4][2];
> > > > > > > +
> > > > > > > +       /* Cache line 2 */
> > > > > > > +       uint8_t data[0];
> > > > > > > +};
> > > > > > > +#endif
> > > > > >
> > > > > > The change could simply be:
> > > > > >
> > > > > > @@ -38,6 +38,9 @@ struct rte_bucket_4_16 {
> > > > > >         uint64_t signature[4 + 1];
> > > > > >         uint64_t lru_list;
> > > > > >         struct rte_bucket_4_16 *next;
> > > > > > +#ifndef RTE_ARCH_64
> > > > > > +       uint32_t pad;
> > > > > > +#endif
> > > > > >         uint64_t next_valid;
> > > > > >
> > > > > >         /* Cache line 1 */
> > > > > >
> > > > > > It avoids duplicating the whole structure definition (we could
> > > > > > miss updating one side of the #ifdef later).
> > > > > > Idem for the other "8" and "32" structures.
> > > >
> > > >
> > > > What about this comment?
> >
> > What about this comment?
> >
> 
> You might suspect I also thought about this option. My preference is for the
> option in the patch for the reasons that IMO it is easier to read and
> understand the reason for the difference, even though the code is slightly
> larger. It also leaves the 64-bit code untouched, so it is easier to remove when
> we finally decide at some point to drop the 32-bit support.
> 
> But I can live with the option you describe as well. Thanks for the input.
> 
> For me, it would be great if somebody on this list could indicate why the 4-
> byte padding was not inserted by the compiler automatically, and hence the
> need for this fix.
> 

Thanks for your help and additional works on this patch.
The validation team tested this case in a 32-bit environment, besides, there are a series of similar tests in 32-bit environment as well. There might be some practical needs for this.
Therefore, before we decide to drop 32-bit support formally, I think such modification is OK, if we cannot fix the compiler issue directly.

Shall I update the patch as David suggested to make it simpler?

> >
> > --
> > David Marchand


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH v4] lib/table: fix cache alignment issue
  2020-07-29 13:28       ` [dpdk-dev] [dpdk-stable] " David Marchand
  2020-07-29 13:54         ` Dumitrescu, Cristian
@ 2020-07-30 10:35         ` Kevin Traynor
  1 sibling, 0 replies; 22+ messages in thread
From: Kevin Traynor @ 2020-07-30 10:35 UTC (permalink / raw)
  To: David Marchand, Dumitrescu, Cristian
  Cc: Xu, Ting, dev, dpdk stable, Luca Boccassi



On 29/07/2020 14:28, David Marchand wrote:
> On Wed, Jul 29, 2020 at 3:14 PM Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com> wrote:
>>> Please correct me if I am wrong, but it simply means this part of the
>>> table library never worked for 32-bit.
>>> It seems more adding 32-bit support rather than a fix and then I
>>> wonder if it has its place in rc3.
>>>
>>
>> Functionally. the code works, but performance is affected.
>>
>> The only thing that prevents the code from working is the check in the table create function that checks the size of the above structure is 64 bytes, which caught this issue.
> 
> Yes, and that's my point.
> It was not working.
> It was not tested.
> 
> 
> This patch asks for backport in stable branches, I will let Kevin and
> Luca comment.
> 

It should be in master first, but then it's a fix so I think it can go
to stable if requested and supported by the table maintainer in the
event of any (future) regressions.

> 
>>
>>>
>>>
>>> Now, looking at the details:
>>>
>>> For 64-bit on my x86, we have:
>>>
>>> struct rte_bucket_4_8 {
>>>     uint64_t                   signature;            /*     0     8 */
>>>     uint64_t                   lru_list;             /*     8     8 */
>>>     struct rte_bucket_4_8 *    next;                 /*    16     8 */
>>>     uint64_t                   next_valid;           /*    24     8 */
>>>     uint64_t                   key[4];               /*    32    32 */
>>>     /* --- cacheline 1 boundary (64 bytes) --- */
>>>     uint8_t                    data[];               /*    64     0 */
>>>
>>>     /* size: 64, cachelines: 1, members: 6 */
>>> };
>>>
>>>
>>> For 32-bit, we have:
>>>
>>> struct rte_bucket_4_8 {
>>>     uint64_t                   signature;            /*     0     8 */
>>>     uint64_t                   lru_list;             /*     8     8 */
>>>     struct rte_bucket_4_8 *    next;                 /*    16     4 */
>>>     uint64_t                   next_valid;           /*    20     8 */
>>>     uint64_t                   key[4];               /*    28    32 */
>>>     uint8_t                    data[];               /*    60     0 */
>>>
>>>     /* size: 60, cachelines: 1, members: 6 */
>>>     /* last cacheline: 60 bytes */
>>> } __attribute__((__packed__));
>>>
>>> ^^ it is interesting that a packed attribute ends up here.
>>> I saw no such attribute in the library code.
>>> Compiler black magic at work I guess...
>>>
>>
>> Where do you see the packet attribute? I don't see it in the code.
> 
> That's pahole reporting this.
> Maybe the tool extrapolates this attribute based on the next_valid
> field placement... I don't know.
> 
>> A packet attribute would explain this issue, i.e. why did the compiler decide not to insert an expected padfing of 4 bytes right after the "next" field, that would allow the field "next_valid" to be aligned to its natural boundary of 8 bytes.
> 
> Or a 64-bit field on 32-bit has a special alignment that I am not aware of.
> 
> 
>>
>>>
>>>>
>>>> Fixes: 8aa327214c ("table: hash")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Ting Xu <ting.xu@intel.com>
>>>>
>>>> ---
>>>> v3->v4: Change design based on comment
>>>> v2->v3: Rebase
>>>> v1->v2: Correct patch time
>>>> ---
>>>>  lib/librte_table/rte_table_hash_key16.c | 17 +++++++++++++++++
>>>>  lib/librte_table/rte_table_hash_key32.c | 17 +++++++++++++++++
>>>>  lib/librte_table/rte_table_hash_key8.c  | 16 ++++++++++++++++
>>>>  3 files changed, 50 insertions(+)
>>>>
>>>> diff --git a/lib/librte_table/rte_table_hash_key16.c
>>> b/lib/librte_table/rte_table_hash_key16.c
>>>> index 2cca1c924..c4384b114 100644
>>>> --- a/lib/librte_table/rte_table_hash_key16.c
>>>> +++ b/lib/librte_table/rte_table_hash_key16.c
>>>> @@ -33,6 +33,7 @@
>>>>
>>>>  #endif
>>>>
>>>> +#ifdef RTE_ARCH_64
>>>>  struct rte_bucket_4_16 {
>>>>         /* Cache line 0 */
>>>>         uint64_t signature[4 + 1];
>>>> @@ -46,6 +47,22 @@ struct rte_bucket_4_16 {
>>>>         /* Cache line 2 */
>>>>         uint8_t data[0];
>>>>  };
>>>> +#else
>>>> +struct rte_bucket_4_16 {
>>>> +       /* Cache line 0 */
>>>> +       uint64_t signature[4 + 1];
>>>> +       uint64_t lru_list;
>>>> +       struct rte_bucket_4_16 *next;
>>>> +       uint32_t pad;
>>>> +       uint64_t next_valid;
>>>> +
>>>> +       /* Cache line 1 */
>>>> +       uint64_t key[4][2];
>>>> +
>>>> +       /* Cache line 2 */
>>>> +       uint8_t data[0];
>>>> +};
>>>> +#endif
>>>
>>> The change could simply be:
>>>
>>> @@ -38,6 +38,9 @@ struct rte_bucket_4_16 {
>>>         uint64_t signature[4 + 1];
>>>         uint64_t lru_list;
>>>         struct rte_bucket_4_16 *next;
>>> +#ifndef RTE_ARCH_64
>>> +       uint32_t pad;
>>> +#endif
>>>         uint64_t next_valid;
>>>
>>>         /* Cache line 1 */
>>>
>>> It avoids duplicating the whole structure definition (we could miss
>>> updating one side of the #ifdef later).
>>> Idem for the other "8" and "32" structures.
> 
> 
> What about this comment?
> 
> 

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [dpdk-dev] [PATCH v1] lib/table: fix cache alignment issue
@ 2020-07-09  1:43 " Ting Xu
  0 siblings, 0 replies; 22+ messages in thread
From: Ting Xu @ 2020-07-09  1:43 UTC (permalink / raw)
  To: dev; +Cc: cristian.dumitrescu, Ting Xu, stable

When create softnic hash table with 16 keys, it failed on 32bit
environment because of the structure rte_bucket_4_16 alignment
issue. Add __rte_cache_aligned to ensure correct cache align.

Fixes: 8aa327214c ("table: hash")
Cc: stable@dpdk.org

Signed-off-by: Ting Xu <ting.xu@intel.com>
---
 lib/librte_table/rte_table_hash_key16.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_table/rte_table_hash_key16.c b/lib/librte_table/rte_table_hash_key16.c
index 2cca1c924..5e1665c15 100644
--- a/lib/librte_table/rte_table_hash_key16.c
+++ b/lib/librte_table/rte_table_hash_key16.c
@@ -44,7 +44,7 @@ struct rte_bucket_4_16 {
 	uint64_t key[4][2];
 
 	/* Cache line 2 */
-	uint8_t data[0];
+	uint8_t data[0] __rte_cache_aligned;
 };
 
 struct rte_table_hash {
-- 
2.17.1


^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, back to index

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-16 16:27 [dpdk-dev] [PATCH v1] lib/table: fix cache alignment issue Ting Xu
2020-06-17  5:43 ` [dpdk-dev] [PATCH v2] " Ting Xu
2020-07-02  8:06   ` Zhou, JunX W
2020-07-09  1:48 ` [dpdk-dev] [PATCH v3] " Ting Xu
2020-07-20 14:37   ` Dumitrescu, Cristian
2020-07-21  5:15     ` Xu, Ting
2020-07-21 21:16       ` Dumitrescu, Cristian
2020-07-22  2:16         ` Xu, Ting
2020-07-22  2:16 ` [dpdk-dev] [PATCH v4] " Ting Xu
2020-07-22  8:26   ` Dumitrescu, Cristian
2020-07-22  8:30     ` Xu, Ting
2020-07-22  8:49       ` Dumitrescu, Cristian
2020-07-22  8:48   ` Dumitrescu, Cristian
2020-07-29 12:01   ` David Marchand
2020-07-29 13:13     ` Dumitrescu, Cristian
2020-07-29 13:28       ` [dpdk-dev] [dpdk-stable] " David Marchand
2020-07-29 13:54         ` Dumitrescu, Cristian
2020-07-29 13:59           ` David Marchand
2020-07-29 14:53             ` Dumitrescu, Cristian
2020-07-30  6:57               ` Xu, Ting
2020-07-30 10:35         ` Kevin Traynor
2020-07-09  1:43 [dpdk-dev] [PATCH v1] " Ting Xu

DPDK patches and discussions

Archives are clonable:
	git clone --mirror http://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ http://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox