Re: [PATCH v12 2/7] linux: Introduce INTERNAL_VSYSCALL

[Richard Henderson <richard.henderson@linaro.org>]

On 2/23/24 13:29, Gabriel Ravier wrote:
> On 2/23/24 23:12, Evan Green wrote:
>> On Thu, Feb 15, 2024 at 11:44 PM Florian Weimer <fweimer@redhat.com> wrote:
>>> * Evan Green:
>>>
>>>> Add an INTERNAL_VSYSCALL() macro that makes a vDSO call, falling back to
>>>> a regular syscall, but without setting errno. Instead, the return value
>>>> is plumbed straight out of the macro.
>>>>
>>>> Signed-off-by: Evan Green <evan@rivosinc.com>
>>>> ---
>>>>
>>>> (no changes since v10)
>>>>
>>>> Changes in v10:
>>>>   - Introduced INTERNAL_VSYSCALL patch
>>>>
>>>>   sysdeps/unix/sysv/linux/sysdep-vdso.h | 19 +++++++++++++++++++
>>>>   1 file changed, 19 insertions(+)
>>>>
>>>> diff --git a/sysdeps/unix/sysv/linux/sysdep-vdso.h 
>>>> b/sysdeps/unix/sysv/linux/sysdep-vdso.h
>>>> index 189319ad98..f7d4b29b25 100644
>>>> --- a/sysdeps/unix/sysv/linux/sysdep-vdso.h
>>>> +++ b/sysdeps/unix/sysv/linux/sysdep-vdso.h
>>>> @@ -53,4 +53,23 @@
>>>>       sc_ret;                                                                \
>>>>     })
>>>>
>>>> +#define INTERNAL_VSYSCALL(name, nr, args...)                               \
>>>> +  ({                                                                       \
>>>> +    __label__ out;                                                         \
>>>> +    long int sc_ret;                                                       \
>>>> +                                                                           \
>>>> +    __typeof (GLRO(dl_vdso_##name)) vdsop = GLRO(dl_vdso_##name);          \
>>>> +    if (vdsop != NULL)                                                             \
>>>> +      {                                                                            \
>>>> +     sc_ret = INTERNAL_VSYSCALL_CALL (vdsop, nr, ##args);                  \
>>>> +     if ((!INTERNAL_SYSCALL_ERROR_P (sc_ret)) ||                           \
>>>> +         (INTERNAL_SYSCALL_ERRNO (sc_ret) != ENOSYS))                      \
>>>> +       goto out;                                                           \
>>>> +      }                                                                            \
>>>> +                                                                           \
>>>> +    sc_ret = INTERNAL_SYSCALL_CALL (name, ##args);                         \
>>>> +  out:                                                                             \
>>>> +    sc_ret;                                                                \
>>>> +  })
>>>> +
>>>>   #endif /* SYSDEP_VDSO_LINUX_H  */
>>> I wonder if this is clearer without the label and with a conditional
>>> expression instead?
>> It's kinda awkward without the label because the regular syscall is
>> made either if vdsop is NULL or returns ENOSYS (which is two checks on
>> the return value, so doesn't lend itself to inlining in the if
>> statement). The best I could come up with without the label is to
>> duplicate the syscall_call:
>>
>> #define INTERNAL_VSYSCALL(name, nr, args...)                      \
>>    ({                                          \
>>      long int sc_ret;                                  \
>>                                            \
>>      __typeof (GLRO(dl_vdso_##name)) vdsop = GLRO(dl_vdso_##name);          \
>>      if (vdsop != NULL)                                  \
>>        {                                          \
>>      sc_ret = INTERNAL_VSYSCALL_CALL (vdsop, nr, ##args);              \
>>      if ((INTERNAL_SYSCALL_ERROR_P (sc_ret)) &&                  \
>>          (INTERNAL_SYSCALL_ERRNO (sc_ret) == ENOSYS))              \
>>        sc_ret = INTERNAL_SYSCALL_CALL (name, ##args);              \
>>        }                                          \
>>      else                                      \
>>        {                                          \
>>      sc_ret = INTERNAL_SYSCALL_CALL (name, ##args);                  \
>>        }                                          \
>>      sc_ret;                                      \
>>    })
>>
>> I think I prefer the version with the label, but I'm not attuned to
>> the glibc style, so let me know if you like this and I'll send out a
>> new version.
>> -Evan
> 
> Is GCC able to de-duplicate the duplicated syscall call ? The label version already seems 
> readable enough in any case and if GCC is not able to de-duplicate the syscall it seems 
> preferable from a performance/code size standpoint as well.

Or a switch?

   switch (vdsop != NULL)
     {
     case 1:
       sc_ret = INTERNAL_VSYSCALL_CALL(...);
       if (!error_p (sc_ret) || errno (sc_ret) != ENOSYS)
         break;
       /* FALLTHRU */
     default:
       sc_ret = INTERVAL_SYSCALL_CALL(...);
       break;
     }

I haven't actually looked at the generated code, but I'd think it would be identical to 
the gotos.


r~