[dpdk-dev] [PATCH] eal: fix 64bit address alignment in 32-bit builds

[dpdk-dev] [PATCH] eal: fix 64bit address alignment in 32-bit builds

[Bruce Richardson]

On i686 builds, the uint64_t type is 64-bits in size but is aligned to
32-bits only. This causes mbuf fields for rearm_data to not be 16-byte
aligned on 32-bit builds, which causes errors with some vector PMDs which
expect the rearm data to be aligned as on 64-bit.

Given that we cannot use the extra space in the data structures anyway, as
it's already used on 64-bit builds, we can just force alignment of physical
address structure members to 8-bytes in all cases. This has no effect on
64-bit systems, but fixes the updated PMDs on 32-bit.

Fixes: f4356d7ca168 ("net/i40e: eliminate mbuf write on rearm")
Fixes: f160666a1073 ("net/ixgbe: eliminate mbuf write on rearm")

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/librte_eal/common/include/rte_memory.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/include/rte_memory.h b/lib/librte_eal/common/include/rte_memory.h
index 4aa5d1f..ad14875 100644
--- a/lib/librte_eal/common/include/rte_memory.h
+++ b/lib/librte_eal/common/include/rte_memory.h
@@ -98,7 +98,8 @@ enum rte_page_sizes {
  */
 #define __rte_cache_min_aligned __rte_aligned(RTE_CACHE_LINE_MIN_SIZE)
 
-typedef uint64_t phys_addr_t; /**< Physical address definition. */
+/** Physical address definition. */
+typedef uint64_t phys_addr_t __rte_aligned(sizeof(uint64_t));
 #define RTE_BAD_PHYS_ADDR ((phys_addr_t)-1)
 
 /**
-- 
2.9.3

Re: [dpdk-dev] [PATCH] eal: fix 64bit address alignment in 32-bit builds

[Bruce Richardson]

On Fri, Apr 28, 2017 at 09:15:51AM +0100, Bruce Richardson wrote:
> On i686 builds, the uint64_t type is 64-bits in size but is aligned to
> 32-bits only. This causes mbuf fields for rearm_data to not be 16-byte
> aligned on 32-bit builds, which causes errors with some vector PMDs which
> expect the rearm data to be aligned as on 64-bit.
> 
> Given that we cannot use the extra space in the data structures anyway, as
> it's already used on 64-bit builds, we can just force alignment of physical
> address structure members to 8-bytes in all cases. This has no effect on
> 64-bit systems, but fixes the updated PMDs on 32-bit.
> 
> Fixes: f4356d7ca168 ("net/i40e: eliminate mbuf write on rearm")
> Fixes: f160666a1073 ("net/ixgbe: eliminate mbuf write on rearm")
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  lib/librte_eal/common/include/rte_memory.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_eal/common/include/rte_memory.h b/lib/librte_eal/common/include/rte_memory.h
> index 4aa5d1f..ad14875 100644
> --- a/lib/librte_eal/common/include/rte_memory.h
> +++ b/lib/librte_eal/common/include/rte_memory.h
> @@ -98,7 +98,8 @@ enum rte_page_sizes {
>   */
>  #define __rte_cache_min_aligned __rte_aligned(RTE_CACHE_LINE_MIN_SIZE)
>  
> -typedef uint64_t phys_addr_t; /**< Physical address definition. */
> +/** Physical address definition. */
> +typedef uint64_t phys_addr_t __rte_aligned(sizeof(uint64_t));
>  #define RTE_BAD_PHYS_ADDR ((phys_addr_t)-1)
>  

Note that this is obviously not the only way to fix the issues with the
PMDs, but it seems to me to be the simplest and most logical way to do
so. I've verified with testpmd that 
a) things still work for 64-bit, and
b) the md5sums of the testpmd 64-bit binary do not change before and
after the change, so it really only affects 32-bit builds.

Regards,
/Bruce

Re: [dpdk-dev] [PATCH] eal: fix 64bit address alignment in 32-bit builds

[Thomas Monjalon]

28/04/2017 10:15, Bruce Richardson:
> On i686 builds, the uint64_t type is 64-bits in size but is aligned to
> 32-bits only. This causes mbuf fields for rearm_data to not be 16-byte
> aligned on 32-bit builds, which causes errors with some vector PMDs which
> expect the rearm data to be aligned as on 64-bit.
> 
> Given that we cannot use the extra space in the data structures anyway, as
> it's already used on 64-bit builds, we can just force alignment of physical
> address structure members to 8-bytes in all cases. This has no effect on
> 64-bit systems, but fixes the updated PMDs on 32-bit.

I agree to align on 64-bit in mbuf.

> Fixes: f4356d7ca168 ("net/i40e: eliminate mbuf write on rearm")
> Fixes: f160666a1073 ("net/ixgbe: eliminate mbuf write on rearm")
[...]
> --- a/lib/librte_eal/common/include/rte_memory.h
> +++ b/lib/librte_eal/common/include/rte_memory.h
> -typedef uint64_t phys_addr_t; /**< Physical address definition. */
> +/** Physical address definition. */
> +typedef uint64_t phys_addr_t __rte_aligned(sizeof(uint64_t));

Why setting this constraint for everyone?

Re: [dpdk-dev] [PATCH] eal: fix 64bit address alignment in 32-bit builds

[Bruce Richardson]

On Fri, Apr 28, 2017 at 10:56:56AM +0200, Thomas Monjalon wrote:
> 28/04/2017 10:15, Bruce Richardson:
> > On i686 builds, the uint64_t type is 64-bits in size but is aligned to
> > 32-bits only. This causes mbuf fields for rearm_data to not be 16-byte
> > aligned on 32-bit builds, which causes errors with some vector PMDs which
> > expect the rearm data to be aligned as on 64-bit.
> > 
> > Given that we cannot use the extra space in the data structures anyway, as
> > it's already used on 64-bit builds, we can just force alignment of physical
> > address structure members to 8-bytes in all cases. This has no effect on
> > 64-bit systems, but fixes the updated PMDs on 32-bit.
> 
> I agree to align on 64-bit in mbuf.
> 
> > Fixes: f4356d7ca168 ("net/i40e: eliminate mbuf write on rearm")
> > Fixes: f160666a1073 ("net/ixgbe: eliminate mbuf write on rearm")
> [...]
> > --- a/lib/librte_eal/common/include/rte_memory.h
> > +++ b/lib/librte_eal/common/include/rte_memory.h
> > -typedef uint64_t phys_addr_t; /**< Physical address definition. */
> > +/** Physical address definition. */
> > +typedef uint64_t phys_addr_t __rte_aligned(sizeof(uint64_t));
> 
> Why setting this constraint for everyone?
>
Well, it only has an effect on 32-bit builds, and unless there is a
problem, I don't see why not always align them to the extra 8 bytes. If
this does cause an issue, I'm happy enough to use #ifdefs, but in the
absense of a confirmed problem, I'd rather keep the code clean.

/Bruce

Re: [dpdk-dev] [PATCH] eal: fix 64bit address alignment in 32-bit builds

[Thomas Monjalon]

28/04/2017 11:03, Bruce Richardson:
> On Fri, Apr 28, 2017 at 10:56:56AM +0200, Thomas Monjalon wrote:
> > 28/04/2017 10:15, Bruce Richardson:
> > > On i686 builds, the uint64_t type is 64-bits in size but is aligned to
> > > 32-bits only. This causes mbuf fields for rearm_data to not be 16-byte
> > > aligned on 32-bit builds, which causes errors with some vector PMDs which
> > > expect the rearm data to be aligned as on 64-bit.
> > > 
> > > Given that we cannot use the extra space in the data structures anyway, as
> > > it's already used on 64-bit builds, we can just force alignment of physical
> > > address structure members to 8-bytes in all cases. This has no effect on
> > > 64-bit systems, but fixes the updated PMDs on 32-bit.
> > 
> > I agree to align on 64-bit in mbuf.
> > 
> > > Fixes: f4356d7ca168 ("net/i40e: eliminate mbuf write on rearm")
> > > Fixes: f160666a1073 ("net/ixgbe: eliminate mbuf write on rearm")
> > [...]
> > > --- a/lib/librte_eal/common/include/rte_memory.h
> > > +++ b/lib/librte_eal/common/include/rte_memory.h
> > > -typedef uint64_t phys_addr_t; /**< Physical address definition. */
> > > +/** Physical address definition. */
> > > +typedef uint64_t phys_addr_t __rte_aligned(sizeof(uint64_t));
> > 
> > Why setting this constraint for everyone?
> >
> Well, it only has an effect on 32-bit builds, and unless there is a
> problem, I don't see why not always align them to the extra 8 bytes. If
> this does cause an issue, I'm happy enough to use #ifdefs, but in the
> absense of a confirmed problem, I'd rather keep the code clean.

Is it expected for everyone to have every physical addresses aligned on 64?
I think it can be weird for some applications.
Why do you think it is cleaner than adding the alignment to the mbuf fields?

PS: It is yet another macro which is not rte_ prefixed.

Re: [dpdk-dev] [PATCH] eal: fix 64bit address alignment in 32-bit builds

[Bruce Richardson]

On Fri, Apr 28, 2017 at 11:21:27AM +0200, Thomas Monjalon wrote:
> 28/04/2017 11:03, Bruce Richardson:
> > On Fri, Apr 28, 2017 at 10:56:56AM +0200, Thomas Monjalon wrote:
> > > 28/04/2017 10:15, Bruce Richardson:
> > > > On i686 builds, the uint64_t type is 64-bits in size but is aligned to
> > > > 32-bits only. This causes mbuf fields for rearm_data to not be 16-byte
> > > > aligned on 32-bit builds, which causes errors with some vector PMDs which
> > > > expect the rearm data to be aligned as on 64-bit.
> > > > 
> > > > Given that we cannot use the extra space in the data structures anyway, as
> > > > it's already used on 64-bit builds, we can just force alignment of physical
> > > > address structure members to 8-bytes in all cases. This has no effect on
> > > > 64-bit systems, but fixes the updated PMDs on 32-bit.
> > > 
> > > I agree to align on 64-bit in mbuf.
> > > 
> > > > Fixes: f4356d7ca168 ("net/i40e: eliminate mbuf write on rearm")
> > > > Fixes: f160666a1073 ("net/ixgbe: eliminate mbuf write on rearm")
> > > [...]
> > > > --- a/lib/librte_eal/common/include/rte_memory.h
> > > > +++ b/lib/librte_eal/common/include/rte_memory.h
> > > > -typedef uint64_t phys_addr_t; /**< Physical address definition. */
> > > > +/** Physical address definition. */
> > > > +typedef uint64_t phys_addr_t __rte_aligned(sizeof(uint64_t));
> > > 
> > > Why setting this constraint for everyone?
> > >
> > Well, it only has an effect on 32-bit builds, and unless there is a
> > problem, I don't see why not always align them to the extra 8 bytes. If
> > this does cause an issue, I'm happy enough to use #ifdefs, but in the
> > absense of a confirmed problem, I'd rather keep the code clean.
> 
> Is it expected for everyone to have every physical addresses aligned on 64?
> I think it can be weird for some applications.
> Why do you think it is cleaner than adding the alignment to the mbuf fields?
> 
I'm ok to redo the patch to only make the change to the mbuf value.
However, when researching this, I discovered that gcc apparently already
aligns all non-structure-member uint64_t values on an 8-byte boundary on
32-bit x86 anyway*. [Don't know if this also applies e.g. to 32-bit arm,
but I wouldn't be surprised if it did.] That means the scope of this
only applies to structures with phys_addr values, so it's not a huge
scope.
*Ref: https://gcc.gnu.org/ml/gcc/2009-06/msg00333.html

> PS: It is yet another macro which is not rte_ prefixed.
> 
Yes. Not going to fix that in this patch though!

So, do you want a V2 to limit the alignment change to the phys_addr in
the mbuf, rather than generally to physical addresses? I prefer the way
I have it here, but I'm ok to change.

Regards,
/Bruce

Re: [dpdk-dev] [PATCH] eal: fix 64bit address alignment in 32-bit builds

[Olivier Matz]

Hi,

On Fri, 28 Apr 2017 10:32:03 +0100, Bruce Richardson <bruce.richardson@intel.com> wrote:
> On Fri, Apr 28, 2017 at 11:21:27AM +0200, Thomas Monjalon wrote:
> > 28/04/2017 11:03, Bruce Richardson:  
> > > On Fri, Apr 28, 2017 at 10:56:56AM +0200, Thomas Monjalon wrote:  
> > > > 28/04/2017 10:15, Bruce Richardson:  
> > > > > On i686 builds, the uint64_t type is 64-bits in size but is aligned to
> > > > > 32-bits only. This causes mbuf fields for rearm_data to not be 16-byte
> > > > > aligned on 32-bit builds, which causes errors with some vector PMDs which
> > > > > expect the rearm data to be aligned as on 64-bit.
> > > > > 
> > > > > Given that we cannot use the extra space in the data structures anyway, as
> > > > > it's already used on 64-bit builds, we can just force alignment of physical
> > > > > address structure members to 8-bytes in all cases. This has no effect on
> > > > > 64-bit systems, but fixes the updated PMDs on 32-bit.  
> > > > 
> > > > I agree to align on 64-bit in mbuf.
> > > >   
> > > > > Fixes: f4356d7ca168 ("net/i40e: eliminate mbuf write on rearm")
> > > > > Fixes: f160666a1073 ("net/ixgbe: eliminate mbuf write on rearm")  
> > > > [...]  
> > > > > --- a/lib/librte_eal/common/include/rte_memory.h
> > > > > +++ b/lib/librte_eal/common/include/rte_memory.h
> > > > > -typedef uint64_t phys_addr_t; /**< Physical address definition. */
> > > > > +/** Physical address definition. */
> > > > > +typedef uint64_t phys_addr_t __rte_aligned(sizeof(uint64_t));  
> > > > 
> > > > Why setting this constraint for everyone?
> > > >  
> > > Well, it only has an effect on 32-bit builds, and unless there is a
> > > problem, I don't see why not always align them to the extra 8 bytes. If
> > > this does cause an issue, I'm happy enough to use #ifdefs, but in the
> > > absense of a confirmed problem, I'd rather keep the code clean.  
> > 
> > Is it expected for everyone to have every physical addresses aligned on 64?
> > I think it can be weird for some applications.
> > Why do you think it is cleaner than adding the alignment to the mbuf fields?
> >   
> I'm ok to redo the patch to only make the change to the mbuf value.
> However, when researching this, I discovered that gcc apparently already
> aligns all non-structure-member uint64_t values on an 8-byte boundary on
> 32-bit x86 anyway*. [Don't know if this also applies e.g. to 32-bit arm,
> but I wouldn't be surprised if it did.] That means the scope of this
> only applies to structures with phys_addr values, so it's not a huge
> scope.
> *Ref: https://gcc.gnu.org/ml/gcc/2009-06/msg00333.html
> 
> > PS: It is yet another macro which is not rte_ prefixed.
> >   
> Yes. Not going to fix that in this patch though!
> 
> So, do you want a V2 to limit the alignment change to the phys_addr in
> the mbuf, rather than generally to physical addresses? I prefer the way
> I have it here, but I'm ok to change.

Since the need comes from vector pmd, I think it's better to limit
the alignment in the mbuf.

Also, it would be good to progressively add some compile-time BUG_ON() in
vector PMDs that have some hidden field alignment/ordering constraints.

Olivier

Re: [dpdk-dev] [PATCH] eal: fix 64bit address alignment in 32-bit builds

[Bruce Richardson]

On Fri, Apr 28, 2017 at 11:56:54AM +0200, Olivier Matz wrote:
> Hi,
> 
> On Fri, 28 Apr 2017 10:32:03 +0100, Bruce Richardson <bruce.richardson@intel.com> wrote:
> > On Fri, Apr 28, 2017 at 11:21:27AM +0200, Thomas Monjalon wrote:
> > > 28/04/2017 11:03, Bruce Richardson:  
> > > > On Fri, Apr 28, 2017 at 10:56:56AM +0200, Thomas Monjalon wrote:  
> > > > > 28/04/2017 10:15, Bruce Richardson:  
> > > > > > On i686 builds, the uint64_t type is 64-bits in size but is aligned to
> > > > > > 32-bits only. This causes mbuf fields for rearm_data to not be 16-byte
> > > > > > aligned on 32-bit builds, which causes errors with some vector PMDs which
> > > > > > expect the rearm data to be aligned as on 64-bit.
> > > > > > 
> > > > > > Given that we cannot use the extra space in the data structures anyway, as
> > > > > > it's already used on 64-bit builds, we can just force alignment of physical
> > > > > > address structure members to 8-bytes in all cases. This has no effect on
> > > > > > 64-bit systems, but fixes the updated PMDs on 32-bit.  
> > > > > 
> > > > > I agree to align on 64-bit in mbuf.
> > > > >   
> > > > > > Fixes: f4356d7ca168 ("net/i40e: eliminate mbuf write on rearm")
> > > > > > Fixes: f160666a1073 ("net/ixgbe: eliminate mbuf write on rearm")  
> > > > > [...]  
> > > > > > --- a/lib/librte_eal/common/include/rte_memory.h
> > > > > > +++ b/lib/librte_eal/common/include/rte_memory.h
> > > > > > -typedef uint64_t phys_addr_t; /**< Physical address definition. */
> > > > > > +/** Physical address definition. */
> > > > > > +typedef uint64_t phys_addr_t __rte_aligned(sizeof(uint64_t));  
> > > > > 
> > > > > Why setting this constraint for everyone?
> > > > >  
> > > > Well, it only has an effect on 32-bit builds, and unless there is a
> > > > problem, I don't see why not always align them to the extra 8 bytes. If
> > > > this does cause an issue, I'm happy enough to use #ifdefs, but in the
> > > > absense of a confirmed problem, I'd rather keep the code clean.  
> > > 
> > > Is it expected for everyone to have every physical addresses aligned on 64?
> > > I think it can be weird for some applications.
> > > Why do you think it is cleaner than adding the alignment to the mbuf fields?
> > >   
> > I'm ok to redo the patch to only make the change to the mbuf value.
> > However, when researching this, I discovered that gcc apparently already
> > aligns all non-structure-member uint64_t values on an 8-byte boundary on
> > 32-bit x86 anyway*. [Don't know if this also applies e.g. to 32-bit arm,
> > but I wouldn't be surprised if it did.] That means the scope of this
> > only applies to structures with phys_addr values, so it's not a huge
> > scope.
> > *Ref: https://gcc.gnu.org/ml/gcc/2009-06/msg00333.html
> > 
> > > PS: It is yet another macro which is not rte_ prefixed.
> > >   
> > Yes. Not going to fix that in this patch though!
> > 
> > So, do you want a V2 to limit the alignment change to the phys_addr in
> > the mbuf, rather than generally to physical addresses? I prefer the way
> > I have it here, but I'm ok to change.
> 
> Since the need comes from vector pmd, I think it's better to limit
> the alignment in the mbuf.

Ok, I'll do a V2.

> 
> Also, it would be good to progressively add some compile-time BUG_ON() in
> vector PMDs that have some hidden field alignment/ordering constraints.
> 
Yes, good idea. I'll see about patching that too.

/Bruce

[dpdk-dev] [PATCH v2] mbuf: fix 64bit address alignment in 32-bit builds

[Bruce Richardson]

On i686 builds, the uin64_t type is 64-bits in size but is aligned to
32-bits only. This causes mbuf fields for rearm_data to not be 16-byte
aligned on 32-bit builds, which causes errors with some vector PMDs which
expect the rearm data to be aligned as on 64-bit.

Given that we cannot use the extra space in the data structures anyway, as
it's already used on 64-bit builds, we can just force alignment of the
physical address in the mbuf to 8-bytes in all cases. This has no effect on
64-bit systems, but fixes the updated PMDs on 32-bit.

Fixes: f4356d7ca168 ("net/i40e: eliminate mbuf write on rearm")
Fixes: f160666a1073 ("net/ixgbe: eliminate mbuf write on rearm")

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
v2: change alignment fix from being for all phys_addr_t vars to just
the one in the mbuf structure. This is a lower risk fix. Additional
patches promised to put in build-checks for alignment in vpmds will
be sent separately.
---
 lib/librte_mbuf/rte_mbuf.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 9dd8e80..66f8013 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -403,7 +403,13 @@ struct rte_mbuf {
 	MARKER cacheline0;
 
 	void *buf_addr;           /**< Virtual address of segment buffer. */
-	phys_addr_t buf_physaddr; /**< Physical address of segment buffer. */
+	/**
+	 * Physical address of segment buffer.
+	 * Force alignment to 8-bytes, so as to ensure we have the exact
+	 * same mbuf cacheline0 layout for 32-bit and 64-bit. This makes
+	 * working on vector drivers easier.
+	 */
+	phys_addr_t buf_physaddr __rte_aligned(sizeof(phys_addr_t));
 
 	/* next 8 bytes are initialised on RX descriptor rearm */
 	MARKER64 rearm_data;
-- 
2.9.3

Re: [dpdk-dev] [PATCH v2] mbuf: fix 64bit address alignment in 32-bit builds

[Thomas Monjalon]

28/04/2017 15:10, Bruce Richardson:
> On i686 builds, the uin64_t type is 64-bits in size but is aligned to
> 32-bits only. This causes mbuf fields for rearm_data to not be 16-byte
> aligned on 32-bit builds, which causes errors with some vector PMDs which
> expect the rearm data to be aligned as on 64-bit.
> 
> Given that we cannot use the extra space in the data structures anyway, as
> it's already used on 64-bit builds, we can just force alignment of the
> physical address in the mbuf to 8-bytes in all cases. This has no effect on
> 64-bit systems, but fixes the updated PMDs on 32-bit.
> 
> Fixes: f4356d7ca168 ("net/i40e: eliminate mbuf write on rearm")
> Fixes: f160666a1073 ("net/ixgbe: eliminate mbuf write on rearm")
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
> v2: change alignment fix from being for all phys_addr_t vars to just
> the one in the mbuf structure. This is a lower risk fix. Additional
> patches promised to put in build-checks for alignment in vpmds will
> be sent separately.

Applied, thanks