[dpdk-dev] [PATCH] eal: fix 64bit address alignment in 32-bit builds
bruce.richardson at intel.com
Fri Apr 28 11:32:03 CEST 2017
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
> 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.
More information about the dev