On Tue, Nov 28, 2017 at 07:19:26AM -0800, Ben Pfaff wrote:
> On Tue, Nov 28, 2017 at 04:22:46PM +0300, Ilya Maximets wrote:
> > On 28.11.2017 01:54, Ben Pfaff wrote:
> > > On Mon, Nov 27, 2017 at 02:59:29PM +0300, Ilya Maximets wrote:
> > >>> I also verified the other case when posix_memalign isn't available and
> > >>> even in that case
> > >>> it returns the address aligned on CACHE_LINE_SIZE boundary. I will send
> > >>> out a patch to use
> > >>> xzalloc_cacheline for allocating the memory.
> > >>
> > >> I don't know how you tested this, because it is impossible:
> > >>
> > >> 1. OVS allocates some memory:
> > >> base = xmalloc(...);
> > >>
> > >> 2. Rounds it up to the cache line start:
> > >> payload = (void **) ROUND_UP((uintptr_t) base, CACHE_LINE_SIZE);
> > >>
> > >> 3. Returns the pointer increased by 8 bytes:
> > >> return (char *) payload + MEM_ALIGN;
> > >>
> > >> So, unless you redefined MEM_ALIGN to zero, you will never get aligned
> > >> memory
> > >> address while allocating by xmalloc_cacheline() on system without
> > >> posix_memalign().
> > >
> > > Maybe we should make the non-HAVE_POSIX_MEMALIGN case better. Something
> > > like this (compile tested only);
> >
> > That's interesting, but the question here is a bit different.
> > We definitely can apply 5-10 more patches to fix this unaligned memory
> > allocation,
> > paddings, holes in case of different cache line sizes / mutex sizes,
> > whatever else
> > we forget to mention, but *do we need this*? Why we're making big efforts
> > to make
> > this work as it should?
>
> For what it's worth, I was addressing the xmalloc_cacheline() code
> independent of your comments on the PMD code. I don't know the PMD code
> well enough to take a position. I'll let others comment on that.
>
> > Beside that, looking at the code below:
> > Do we care about false sharing on alloc/free operations? That is the
> > difference between
> > old and new implementations. The new one will write and read bytes possibly
> > placed
> > on a shared cacheline on alloc/free.
>
> I'd say that the value of the change that I'm proposing is that it
> allows programmers to design their structures knowing that the first
> cacheline ends after 64 bytes, not after 64-8 bytes in some cases.
Anyway, I sent out the patch officially. I haven't tested it beyond
compile-testing:
https://patchwork.ozlabs.org/patch/842251/
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
