diff mbox series

[v3] lib/table: fix cache alignment issue

Message ID 20200709014825.13690-1-ting.xu@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series [v3] lib/table: fix cache alignment issue |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-broadcom-Performance success Performance Testing PASS
ci/travis-robot warning Travis build: failed
ci/iol-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/Intel-compilation fail Compilation issues

Commit Message

Xu, Ting July 9, 2020, 1:48 a.m. UTC 
  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(-)

Comments

Cristian Dumitrescu July 20, 2020, 2:37 p.m. UTC | #1 
> -----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
Xu, Ting July 21, 2020, 5:15 a.m. UTC | #2 
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
Cristian Dumitrescu July 21, 2020, 9:16 p.m. UTC | #3 
> -----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
Xu, Ting July 22, 2020, 2:16 a.m. UTC | #4 
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
diff mbox series

Patch

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 {