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

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

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
  

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?
  

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
  

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.
  

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
  

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
  

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