[ovs-dev] [PATCH] Revert "dpif_netdev: Refactor dp_netdev_pmd_thread structure."

[Ben Pfaff]

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/