Re: [PATCH] NUMA spinlock [BZ #23962]

["H.J. Lu" <hjl.tools@gmail.com>]

On Thu, Jan 3, 2019 at 1:21 PM Rich Felker <dalias@libc.org> wrote:
>
> On Thu, Jan 03, 2019 at 12:54:18PM -0800, H.J. Lu wrote:
> > On Thu, Jan 3, 2019 at 12:43 PM Rich Felker <dalias@libc.org> wrote:
> > >
> > > On Wed, Dec 26, 2018 at 10:50:19AM +0800, Ma Ling wrote:
> > > > From: "ling.ma" <ling.ml@antfin.com>
> > > >
> > > > On multi-socket systems, memory is shared across the entire system.
> > > > Data access to the local socket is much faster than the remote socket
> > > > and data access to the local core is faster than sibling cores on the
> > > > same socket.  For serialized workloads with conventional spinlock,
> > > > when there is high spinlock contention between threads, lock ping-pong
> > > > among sockets becomes the bottleneck and threads spend majority of
> > > > their time in spinlock overhead.
> > > >
> > > > On multi-socket systems, the keys to our NUMA spinlock performance
> > > > are to minimize cross-socket traffic as well as localize the serialized
> > > > workload to one core for execution.  The basic principles of NUMA
> > > > spinlock are mainly consisted of following approaches, which reduce
> > > > data movement and accelerate critical section, eventually give us
> > > > significant performance improvement.
> > >
> > > I question whether this belongs in glibc. It seems highly application-
> > > and kernel-specific. Is there a reason you wouldn't prefer to
> > > implement and maintain it in a library for use in the kind of
> > > application that needs it?
> >
> > This is a good question.  On the other hand,  the current spinlock
> > in glibc hasn't been changed for many years.  It doesn't scale for
> > today's hardware.  Having a scalable spinlock in glibc is desirable.
>
> "Scalable spinlock" is something of an oxymoron. Spinlocks are for
> situations where contention is extremely rare, since they inherently
> blow up badly under contention. If this is happening it means you
> wanted a mutex not a spinlock.

The critical region serialized by spinlock is very small.  Overhead
of mutex is much bigger than the critical region itself.

> > > This looks like fragile duplication of stdio-like buffering logic
> > > that's not at all specific to this file. Does glibc have a policy on
> > > whether things needing this should use stdio or some other shared code
> > > rather than open-coding it like this?
> >
> > This is borrowed from sysdeps/unix/sysv/linux/getsysstats.c.  Should it
> > be exported in GLIBC_PRIVATE name space?
>
> Possibly. I don't have a strong opinion here. At least it's good to
> know it's copied from code elsewhere that's believed to be
> correct/safe.
>
> >
> > > > [...]
> > >
> > > > +/* Allocate a NUMA spinlock and return a pointer to it.  Caller should
> > > > +   call numa_spinlock_free on the NUMA spinlock pointer to free the
> > > > +   memory when it is no longer needed.  */
> > > > +
> > > > +struct numa_spinlock *
> > > > +numa_spinlock_alloc (void)
> > > > +{
> > > > +  const size_t buffer_size = 1024;
> > > > +  char buffer[buffer_size];
> > > > +  char *buffer_end = buffer + buffer_size;
> > > > +  char *cp = buffer_end;
> > > > +  char *re = buffer_end;
> > > > +
> > > > +  const int flags = O_RDONLY | O_CLOEXEC;
> > > > +  int fd = __open_nocancel ("/sys/devices/system/node/online", flags);
> > > > +  char *l;
> > > > +  unsigned int max_node = 0;
> > > > +  unsigned int node_count = 0;
> > > > +  if (fd != -1)
> > > > +    {
> > > > +      l = next_line (fd, buffer, &cp, &re, buffer_end);
> > > > +      if (l != NULL)
> > > > +     do
> > > > +       {
> > > > +         char *endp;
> > > > +         unsigned long int n = strtoul (l, &endp, 10);
> > > > +         if (l == endp)
> > > > +           {
> > > > +             node_count = 1;
> > > > +             break;
> > > > +           }
> > > > +
> > > > +         unsigned long int m = n;
> > > > +         if (*endp == '-')
> > > > +           {
> > > > +             l = endp + 1;
> > > > +             m = strtoul (l, &endp, 10);
> > > > +             if (l == endp)
> > > > +               {
> > > > +                 node_count = 1;
> > > > +                 break;
> > > > +               }
> > > > +           }
> > > > +
> > > > +         node_count += m - n + 1;
> > > > +
> > > > +         if (m >= max_node)
> > > > +           max_node = m;
> > > > +
> > > > +         l = endp;
> > > > +         while (l < re && isspace (*l))
> > > > +           ++l;
> > > > +       }
> > > > +     while (l < re);
> > > > +
> > > > +      __close_nocancel_nostatus (fd);
> > > > +    }
> > > > +
> > > > +  /* NB: Some NUMA nodes may not be available, if the number of NUMA
> > > > +     nodes is 1, set the maximium NUMA node number to 0.  */
> > > > +  if (node_count == 1)
> > > > +    max_node = 0;
> > > > +
> > > > +  unsigned int max_cpu = 0;
> > > > +  unsigned int *physical_package_id_p = NULL;
> > > > +
> > > > +  if (max_node == 0)
> > > > +    {
> > > > +      /* There is at least 1 node.  */
> > > > +      node_count = 1;
> > > > +
> > > > +      /* If NUMA is disabled, use physical_package_id instead.  */
> > > > +      struct dirent **cpu_list;
> > > > +      int nprocs = scandir ("/sys/devices/system/cpu", &cpu_list,
> > > > +                         select_cpu, NULL);
> > > > +      if (nprocs > 0)
> > > > +     {
> > > > +       int i;
> > > > +       unsigned int *cpu_id_p = NULL;
> > > > +
> > > > +       /* Find the maximum CPU number.  */
> > > > +       if (posix_memalign ((void **) &cpu_id_p,
> > > > +                           __alignof__ (void *),
> > > > +                           nprocs * sizeof (unsigned int)) == 0)
> > >
> > > Using posix_memalign to get memory with the alignment of
> > > __alignof__(void*) makes no sense. All allocations via malloc are
> > > suitably aligned for any standard type.
> >
> > Does glibc prefer malloc over posix_memalign?
>
> If not, I think it *should*. Use of posix_memalign suggests to the
> reader that some nonstandard alignment is needed, prompting one to ask
> "what?" only to figure out it was gratuitous.
>
> What's worse -- and I missed this on the first pass --
> posix_memalign's signature is broken by design and almost universally
> gets the programmer to invoke undefined behavior, which has happened
> here. cpu_id_p has type unsigned int *, but due to the cast,
> posix_memalign accesses it as if it had type void *, which is an
> aliasing violation. For this reason alone, I would go so far as to say
> the function should never be used unless there's no way to avoid it,
> and even then it should be wrapped with a function that *returns* the
> pointer as void* so that this error can't be made.

We will change it to malloc.

> > > > +                   if (snprintf (path, sizeof (path),
> > > > +                                 "/sys/devices/system/cpu/%s/topology/physical_package_id",
> > > > +                                 d->d_name) > 0)
> > >
> > > Are these sysfs pathnames documented as stable/permanent by the
> > > kernel?
> >
> > I believe so.
>
> OK. It is worth checking though since otherwise you have an interface
> that will suddenly break when things change on the kernel side.
>

Documentation/cputopology.txt has

===========================================
How CPU topology info is exported via sysfs
===========================================

Export CPU topology info via sysfs. Items (attributes) are similar
to /proc/cpuinfo output of some architectures:

1) /sys/devices/system/cpu/cpuX/topology/physical_package_id:

physical package id of cpuX. Typically corresponds to a physical
socket number, but the actual value is architecture and platform
dependent.
....


-- 
H.J.