Re: [RFC v2 08/20] sysdeps/wait: Use waitid if avaliable

[Arnd Bergmann <arnd at arndb dot de>]

On Wed, Jun 26, 2019 at 5:48 PM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Arnd Bergmann:
>
> [Exposing futex as __NR_futex for 64-bit-only-time_t]


I'm realizing now that I was missing your point as I took futex as
an example assuming we would handle all 21 time64 syscalls the
same way, but you point out some issues that are specific to futex,
so I should try to separate those two things.

> > Is there any long-term advantage for your approach that I'm
> > missing?
>
> Userspace that calls futex and does not care about timeouts would just
> work on RV32 without any porting.

In general, my hope is that anything should just work on rv32 without
porting, regardless of the timeouts.

One problem specific to futex is the fact that glibc does not come with
a wrapper for it, so it's up to the applications to decide on which macro
to pass.

For most of the other syscalls, having rv32 not define the macro means
that we break applications at compile-time, and are hopefully able to
fix them in a way that works on all architectures. In case of futex,
I agree that it would be better not to break compilation when you
don't pass a timeout, but I see no good way to have it both ways.

> For example, consider this code in glib:
>
> | static void
> | g_futex_wait (const volatile gint *address,
> |               gint                 value)
> | {
> |   syscall (__NR_futex, address, (gsize) FUTEX_WAIT_PRIVATE, (gsize) value, NULL);
> | }
>
> This will not work on RV32 for no good reason at all.  You would
> actually have to add an #ifdef for RV32 to fix it because you clearly
> don't want to do this:
>
> | static void
> | g_futex_wait (const volatile gint *address,
> |               gint                 value)
> | {
> | #ifdef __NR_futex_time64
> |   syscall (__NR_futex_time64, address, (gsize) FUTEX_WAIT_PRIVATE, (gsize) value, NULL);
> | #else
> |   syscall (__NR_futex, address, (gsize) FUTEX_WAIT_PRIVATE, (gsize) value, NULL);
> | #endif
> | }
>
> because that would break if the library was used on an older kernel
> (older than the kernel headers installed at build time).

*nod*

> Maybe you could use this:
>
> | static void
> | g_futex_wait (const volatile gint *address,
> |               gint                 value)
> | {
> | #ifdef __NR_futex
> |   syscall (__NR_futex_time64, address, (gsize) FUTEX_WAIT_PRIVATE, (gsize) value, NULL);
> | #else
> |   syscall (__NR_futex_time64, address, (gsize) FUTEX_WAIT_PRIVATE, (gsize) value, NULL);
> | #endif
> | }
>
> But this would still break if people actually go ahead with the removal
> of the 32-bit system calls (something I think is quite impossible to do,
> but some people seem to disagree).
>
> Fallback on ENOSYS requires introducing global variable to avoid
> pointless future system calls.

I hope the example of g_futex_wait() becomes a little easier after
we have come up with a solution for fixing the much harder case
of using __NR_futex /with/ timeouts, such as (also from glib)

gboolean
g_cond_wait_until (GCond  *cond, GMutex *mutex, gint64  end_time)
{
 struct timespec span;
  ...
  res = syscall (__NR_futex, &cond->i[0], (gsize) FUTEX_WAIT_PRIVATE,
(gsize) sampled, &span);
 ...
}

The problem obviously is that on existing 32-bit architectures,
the first argument (__NR_futex) must correspond to the type
of the 'span' variable. Fixing this is always ugly, but has to be
done. The best I can think of is

gboolean
g_cond_wait_until (GCond  *cond, GMutex *mutex, gint64  end_time)
{
  struct timespec span;
  ...
  res = -ENOSYS;
  if (sizeof(time_t) > sizeof(__syscall_slong_t)) {
#ifdef __NR_futex_time64
     res = syscall (__NR_futex_time64, &cond->i[0], (gsize) FUTEX_WAIT_PRIVATE,
                 (gsize) sampled, &span);
#endif
 } else {
#ifdef __NR_futex
     res = syscall (__NR_futex, &cond->i[0], (gsize) FUTEX_WAIT_PRIVATE,
                 (gsize) sampled, &span);
#endif
  }
...
}

The version above will
- always use __NR_futex on 64-bit architectures and x32
- use __NR_futex on 32-bit architectures with 32-bit time_t, this will always
  work except on kernels that don't support time32 syscalls (as intended)
- use __NR_futex_time64 on 32-bit architectures with 64-bit time_t, which
  works when built with linux-5.1+ headers and running on linux-5.1+.
  This is probably good enough, as many other things are also broken
  when trying to use time64 with older kernels or headers.

One could achieve something similar by defining SYS_futex in glibc like

#if __WORDSIZE == 64 || __TIMESIZE == 32 || (defined __x86_64__ &&
defined __ILP32__)
#define SYS_futex __NR_futex
#else
#define SYS_futex __NR_futex_time64
#endif

and then requiring applications to use SYS_futex rather than __NR_futex.
Again, there would be no fallback for time64 with older kernels or
headers this way.

We can't really redefine __NR_futex in the kernel headers this way
unfortunately, because
a) there is no reliable way for kernel headers to check __TIMESIZE
    in an #ifdef (users might include asm/unistd.h before libc headers);
    the best approximation would be
  #define __NR_futex (sizeof(time_t) > sizeof(__kernel_long_t) ? \
               __NR_futex_time64 : __NR_futex_time32)
    which has a few downsides as well.
b) we need to provide both plain __NR_futex and __NR_futex_time64
    for libraries that want to call both depending on the timeout argument
    type.

        Arnd