| Message ID | 20240214143159.2951158-3-evan@rivosinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | RISC-V: ifunced memcpy using new kernel hwprobe interface | expand |
* 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? Thanks, Florian
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
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.
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~
* Evan Green: > 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; \ > }) Indentation is off for me, for clarification: #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; \ }) ?: does not work because the comparison with -ENOSYS discards the return value. Does this look correct? #define INTERNAL_VSYSCALL(name, nr, args...) \ ({ \ long int sc_ret = -ENOSYS; \ __typeof (GLRO(dl_vdso_##name)) vdsop = GLRO(dl_vdso_##name); \ if (vdsop != NULL) \ sc_ret = INTERNAL_VSYSCALL_CALL (vdsop, nr, ##args); \ if (sc_ret == -ENOSYS) \ sc_ret = INTERNAL_SYSCALL_CALL (name, ##args); \ sc_ret; \ }) I expect GCC to generate decent code for it.
On Sat, Feb 24, 2024 at 3:40 AM Florian Weimer <fw@deneb.enyo.de> wrote: > > * Evan Green: > > > 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; \ > > }) > > Indentation is off for me, for clarification: Yeah sorry I just pasted it into the gmail client, which mangles everything. > > #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; \ > }) > > ?: does not work because the comparison with -ENOSYS discards the > return value. > > Does this look correct? > > #define INTERNAL_VSYSCALL(name, nr, args...) \ > ({ \ > long int sc_ret = -ENOSYS; \ > __typeof (GLRO(dl_vdso_##name)) vdsop = GLRO(dl_vdso_##name); \ > if (vdsop != NULL) \ > sc_ret = INTERNAL_VSYSCALL_CALL (vdsop, nr, ##args); \ > if (sc_ret == -ENOSYS) \ > sc_ret = INTERNAL_SYSCALL_CALL (name, ##args); \ > sc_ret; \ > }) > > I expect GCC to generate decent code for it. I like it! I'm guessing I should continue to use the INTERNAL_SYSCALL_ERRNO_P() and INTERNAL_SYSCALL_ERRNO(), and you omitted it for example's sake. Will plan to spin with this. -Evan
* Evan Green: > I like it! I'm guessing I should continue to use the > INTERNAL_SYSCALL_ERRNO_P() and INTERNAL_SYSCALL_ERRNO(), and you > omitted it for example's sake. Will plan to spin with this. We already hard-code the Linux convention in a few places, e.g. sysdeps/unix/sysv/linux/fstatat64.c, sysdeps/unix/sysv/linux/clock_gettime.c. I don't think the macros add value here. We also don't have a macro to inject the error code, I believe, so one -ENOSYS will stick around. But I don't feel strongly about it.
On Mon, Feb 26, 2024 at 9:07 AM Florian Weimer <fw@deneb.enyo.de> wrote: > > * Evan Green: > > > I like it! I'm guessing I should continue to use the > > INTERNAL_SYSCALL_ERRNO_P() and INTERNAL_SYSCALL_ERRNO(), and you > > omitted it for example's sake. Will plan to spin with this. > > We already hard-code the Linux convention in a few places, e.g. > sysdeps/unix/sysv/linux/fstatat64.c, > sysdeps/unix/sysv/linux/clock_gettime.c. I don't think the macros add > value here. We also don't have a macro to inject the error code, I > believe, so one -ENOSYS will stick around. But I don't feel strongly > about it. Got it, will spin with this as-is then. Thanks for the clarification. -Evan
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 */
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(+)