Re: [RFC v6 08/23] RISC-V: Define __NR_* as __NR_*_time64/64 for 32-bit

[Arnd Bergmann <arnd at arndb dot de>]

On Mon, Jan 27, 2020 at 2:17 PM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Arnd Bergmann:
>
> > What is the problem with using a plain uintptr_t or struct timespec* and
> > requiring callers to add a cast? Is this just a matter of ugly calling
> > conventions, or are there cases where it causes bigger problems?
>
> The problem with casts is that they make code compile which uses the
> wrong timespec type.  Old 32-bit ports will have at least three of them,
> and we should provide as much guidance as possible to programmers to get
> this right.  If we require them to write casts, I don't think we can
> achieve that.

I see, that is definitely a problem if callers mix the kernel types with the
glibc types, even if glibc only provides a single (conditional) definition of
struct timespec in its user-facing headers.

> > Would it help to have the libc futex wrapper take seven arguments with
> > separate timeout and val2 arguments, and requiring at least one of them
> > to be 0 or NULL? I think the only reason for the awkward interface in the
> > kernel is that a syscall with seven arguments would require passing
> > a structure on most architectures.
>
> I hope that nowadays, we'd just add separate system calls instead of
> multiplexers (even though futex_time64 doesn't give much hope).  Then
> the different operations can use whatever argument types they need.

I think that is the general trend, but for futex_time64() the goal was to
stay close to the existing futex() despite its problems. In case of
io_uring() and fs{mount,config,mount,pick}() we fortunately avoided
adding new multiplexers.

Coming back to futex, do you think this would work in a libc header?

/* plain kernel syscalls */
extern inline int __futex_time64(int *uaddr, int futex_op, int val,
                 const struct __timespec64 *timeout,
                 int *uaddr2, int val3);
extern inline int __futex_old(int *uaddr, int futex_op, int val,
                 const struct __timespec_old *timeout,
                 int *uaddr2, int val3);

/* typesafe wrapper, could be inline or out of line */
static __inline int
futex(int *uaddr, int futex_op, int val,
                 int val2, int *uaddr2, int val3,
                 const struct timespec *timeout)
{
         if (sizeof(time_t) > sizeof(long)) {
                  int ret;
                  if (!timeout)
                         return futex_time64(uaddr, futex_op, val,
val2, uaddr2, val3);
                 ret = futex_time64(uaddr, futex_op, val, val2, uaddr2, val3);
                 if (ret == -1 && errno = -ENOSYS) {
                           int ts[2] = { timeout.tv_sec, timeout.tv_nsec };
                           return futex_old(uaddr, futex_op, (void
*)ts, val2, uaddr2, val3);
                 }
       }

       if (!timeout)
                 return futex_old(uaddr, futex_op, val, val2, uaddr2, val3);

       return futex_old(uaddr, futex_op, (void *)timeout, val2, uaddr2, val3);
}

That would be roughly as efficient as the call to syscall() but do the
right thing in all cases, and not require adding an unsafe dummy
__NR_futex definition.

     Arnd