Re: [RFC/PoC] malloc: use wfcqueue to speed up remote frees

[Eric Wong <normalperson@yhbt.net>]

Carlos O'Donell <carlos@redhat.com> wrote:
> On 07/31/2018 07:18 PM, Eric Wong wrote:
> >> - Can you explain the RSS reduction given this patch? You
> >> might think that just adding the frees to a queue wouldn't
> >> result in any RSS gains.
> > 
> > At least two reasons I can see:
> > 
> > 1) With lock contention, the freeing thread can lose to the
> >    allocating thread.  This makes the allocating thread hit
> >    sysmalloc since it prevented the freeing thread from doing
> >    its job.  sysmalloc is the slow path, so the lock gets held
> >    even longer and the problem compounds from there.
> 
> How does this impact RSS? It would only block the remote thread
> from freeing in a timely fashion, but it would eventually make
> progress.

Blocking the freeing thread causes the allocating thread to
sysmalloc more.  If the freeing thread could always beat the
allocating thread, then the freed memory would be available in
the arena by the time the allocating thread takes the lock.

> > 2) thread caching - memory ends up in the wrong thread and
> >    could never get used in some cases.  Fortunately this is
> >    bounded, but still a waste.
> 
> We can't have memory end up in the wrong thread. The remote thread
> computes the arena from the chunk it has, and then frees back to
> the appropriate arena, even if it's not the arena that the thread
> is attached to.

Really?  I see:

   __libc_free -> MAYBE_INIT_TCACHE && _int_free -> tcache_put

I am not seeing anything in _int_free which makes the tcache_put
arena-aware.  If we drop MAYBE_INIT_TCACHE from __libc_free,
then the tcache_put could be avoided.

> > I'm still new to the code, but it looks like threads are pinned
> > to the arena and the memory used for arenas never gets released.
> > Is that correct?
> 
> Threads are pinned to their arenas, but they can move in the event
> of allocation failures, particularly to the main arena to attempt
> sbrk to get more memory.

OK.

> > I was wondering if there was another possibility: the allocating
> > thread gives up the arena and creates a new one because the
> > freeing thread locked it, but I don't think that's the case.
> 
> No.
> 
> > Also, if I spawn a bunch of threads and get a bunch of
> > arenas early in the program lifetime; and then only have few
> > threads later, there can be a lot of idle arenas.
>  
> Yes. That is true. We don't coalesce arenas to match the thread
> demand.

Eep :<    If contention can be avoided (which tcache seems to
work well for), limiting arenas to CPU count seems desirable and
worth trying.

<snip>

> >> - Adding urcu as a build-time dependency is not acceptable for
> >> bootstrap, instead we would bundle a copy of urcu and keep it
> >> in sync with upstream. Would that make your work easier?
> > 
> > Yes, bundling that sounds great.  I assume it's something for
> > you or one of the regular contributors to work on (build systems
> > scare me :x)
> 
> Yes, that is something we'd have to do.

OK, I noticed my patch fails conformance tests because
(despite my use of __cds_wfcq_splice_nonblocking) it references
poll(), despite poll() being in an impossible code path:

   __cds_wfcq_splice_nonblocking -> ___cds_wfcq_splice
	   -> ___cds_wfcq_busy_wait -> poll

The poll call is impossible because the `blocking' parameter is 0;
but I guess the linker doesn't know that?

> >> - What problems are you having with `make -j4 check?' Try
> >> master and report back.  We are about to release 2.28 so it
> >> should build and pass.
> > 
> > My fault.  It seems like tests aren't automatically rerun when I
> > change the code; so some of my broken work-in-progress changes
> > ended up being false positives :x.  When working on this, I made
> > the mistake of doing remote_free_step inside malloc_consolidate,
> > which could recurse into _int_free or _int_malloc
> 
> This depends a bit on what you touch.

Alright, I'll keep that in mind.  Thanks!