[PATCH] Async signal safe TLS accesses

[Ondřej Bílka]

On Thu, Nov 28, 2013 at 11:03:36PM -0200, Alexandre Oliva wrote:
> On Nov 19, 2013, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
> 
> > On Wed, Nov 13, 2013 at 7:45 AM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
> >> Ping?
> >> https://sourceware.org/ml/libc-alpha/2013-11/msg00183.html
> 
> > Ping? Ping?
> 
> Apologies for the delay in this review.
> 
> I'm a bit disappointed at how wasteful the allocation strategy is,
> wasting a full page per thread even for a one-word TLS dynamic block.  I
> think using a TLS-only malloc arena, only locked with signals disabled,
> would bring us much better memory efficiency, although there might be a
> significant penalty for assigning dynamic segments of different threads
> to the same page, should they happen to be accessed very often.  This
> page sharing by different threads is something we don't necessarily
> avoid with the current AS-Unsafe allocation scheme, and avoiding that is
> an improvement that the current strategy brings, but I wonder if we
> could find some trade off that wouldn't waste so much memory but that
> would keep segments used by different threads in separate pages...  This
> first comment is not meant as a blocker against the page, but as a
> suggestion for future improvement.
>
A proper solution would be make generic malloc async-signal safe. It is 
question of what is acceptable overhead. I can make a wrapper over any
allocator that does that at cost of two tls access. You would pay a mmap
overhead only when you are in signal where we do not care about
effectivity.

As I plan to use a thread-local cache for small allocations which
eliminates most of this cost when one has cheap tls.

I attached a signal-safe part that I have now, it is part of alternative
allocator I now work.

As for freeing a malloc already uses mmap for allocation bigger than
mmap_treshold so it is possible with filling appropriate header use normal free.
> 
> I was under the impression that one of the main problems with making TLS
> AS-Safe was the fact that in some cases of TLS dynamic address
> resolution we'd take the dl lock, and if we take a signal while we hold
> the lock, and the signal handler attempts to resolve the same address
> and take the lock, we'd deadlock if the lock was non-recursive, or risk
> finding inconsistent state otherwise.  The lock is recursive, but I
> don't see anything in the patch besides the CAS loops that avoids
> inconsistent state, particularly in the case of lazy TLS relocation
> (introduced with GNU2 TLS).  Was that not taken into account?
>
In theory, in practice these are much less probable than malloc problem as 
these happen in initialization and it takes time signal handler can
access object that will cause tls access.

A lazy relocation is one of dubious features, did somebody tried to
measure if these actually save anything? I would be interested in ratio
of resolved / symbols left unresolved.

Copying mostly unused tls entries could be a problem but so can be
repeated locks and allocations when these entries are actually used.


> 
> It seems to me that, in the absence of locking,
> _dl_try_allocate_static_tls is racy, and in the presence of locking, the
> CAS loop would be unnecessary.  The race is because offset is computed
> from GL(dl_tls_static_used) once, and if l->map_tls_offset could change
> between that and its update, then so could GL(dl_tls_static_used),
> requiring offset to be recomputed based on the modified value.  But
> GL(dl_tls_static_used) would have to be updated atomically as well,
> otherwise concurrent runs for different maps could end up at the same
> offset.  The code doesn't take such cares, so I wonder if there's
> evidence we don't need to worry about that and, if so, why do we still
> need the CAS loop.  Even if we do hold a lock to prevent other threads
> from changing dl_tls_static_used concurrently, couldn't a signal handler
> that interrupts _dl_try_allocate_static_tls change it? 

See _dl_mask_all_signals (&old);

> If so, the same
> “race” applies, otherwise, I don't see why we need even a single CAS,
> let alone a CAS loop.
> 
> 
> I'm somewhat concerned about priority inversion in the atomic_delay()
> and CAS loops.  Considering that the locks the patch drops are all
> recursive, they won't pose an AS-Safety problem; if we keep them in
> place, but take the measures that have apparently been taken to avoid
> the problems of using inconsistent state when a handler interrupts a
> lock-holding operation, and of using outdated state after a signal
> handler modifies it, it should all work, and then it would be clear that
> the CAS loops are only in place for the case of functions being
> interrupted by signal handlers, not to deal with inter-thread
> concurrency.  With the locks in place, we could at least get rid of the
> atomic_delay() loop.
> 
> 
> This is all I got for now.  Thank you both for your work on this issue.
> 
> -- 
> Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
> You must be the change you wish to see in the world. -- Gandhi
> Be Free! -- http://FSFLA.org/   FSF Latin America board member
> Free Software Evangelist      Red Hat Brazil Compiler Engineer

-- 

Support staff hung over, send aspirin and come back LATER.
-------------- next part --------------
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include <sys/mman.h>
#include <stdbool.h>
#include <errno.h>
#include <stdint.h>
#include <atomic.h>

#include "alloc.h"

void *alloc_next (size_t size);
void *realloc_next (void *olddata, size_t oldsize, void *oldmetadata, size_t oldmetasize, size_t newsize);
void *free_next (void *olddata, size_t oldsize, void *oldmetadata, size_t oldmetasize);

static __thread uint32_t malloc_recursed;
struct to_free_s
{
  struct to_free_s *next;
};
static struct to_free_s *to_free;

void *
alloc_signal (size_t size)
{
   /*
     if (size < small_treshold || size > mmap_treshold)
       return alloc_next (size);
    */

   if (malloc_recursed == 1)
     {
       return alloc_by_mmap (size);
     }
   malloc_recursed = 1;
   void *ret = alloc_next (size);
   malloc_recursed = 0;
   return ret;
}

void *
realloc_signal (void *olddata, size_t oldsize, void *oldmetadata, size_t oldmetasize, size_t newsize)
{
  if (malloc_recursed == 1)
    {
      void *new = malloc (newsize);
      if (!new)
	return NULL;
      return memcpy (new, olddata, oldsize);
    }

  malloc_recursed = 1;
  void *ret = realloc_next (olddata, oldsize, oldmetadata, oldmetasize, newsize);
  malloc_recursed = 0;
  return ret;
}

void
free_signal (void *data, size_t size, void *metadata, size_t metasize)
{
  /* if (is_mmaped (metadata))
       {
         munmap (metadata, metasize);
       }

  /*
     if (metasize < small_treshold)
       return free_next (data, size, metadata, metasize);
   */

  /* We will defer frees in signal handlers to later free outside of signal.
     These are kept in sigle linked list that is modified in atomic way.
     As these are likely empty we do not need a synchronization, 
     but add atomic load to make race detectors happy.  */
  
  if (malloc_recursed == 1)
    {
      struct to_free_s *to_return = (struct to_free_s *) data;
      do
        to_return->next = atomic_forced_read (to_free);
      while (!atomic_compare_and_exchange_bool_acq (&to_free, to_return,
						    to_return->next));
    }
  else
    {
      struct to_free_s *returned = atomic_forced_read (to_free);
      if (returned != NULL)
        {
          returned = atomic_exchange_acq (&to_free, NULL);
          while (returned)
            {
              struct to_free_s *next = returned->next;
              free (returned);
              returned = next;
            }
        }
    }

  malloc_recursed = 1;
  free_next (data, size, metadata, metasize);
  malloc_recursed = 0;
}