Message ID | 87y1amnd85.fsf@oldenburg.str.redhat.com |
---|---|
State | New |
Headers | show |
Series | [v3] linux: Use rseq area unconditionally in sched_getcpu (bug 31479) | expand |
On 2024-03-13 15:28, Florian Weimer wrote: > Originally, nptl/descr.h included <sys/rseq.h>, but we removed that > in commit 2c6b4b272e6b4d07303af25709051c3e96288f2d ("nptl: > Unconditionally use a 32-byte rseq area"). After that, it was > not ensured that the RSEQ_SIG macro was defined during sched_getcpu.c > compilation that provided a definition. This commit always checks > the rseq area for CPU number information before using the other > approaches. > > This adds an unnecessary (but well-predictable) branch on > architectures which do not define RSEQ_SIG, but its cost is small > compared to the system call. Most architectures that have vDSO > acceleration for getcpu also have rseq support. > > Fixes: 2c6b4b272e6b4d07303af25709051c3e96288f2d > Fixes: 1d350aa06091211863e41169729cee1bca39f72f Would adding back the '#include <sys/rseq.h>' in nptl/descr.h have any adverse consequences? It would restore the original behavior without any effect on architectures which do not define RSEQ_SIG.
On 2024-03-13 16:51, Michael Jeanson wrote: > On 2024-03-13 15:28, Florian Weimer wrote: >> Originally, nptl/descr.h included <sys/rseq.h>, but we removed that >> in commit 2c6b4b272e6b4d07303af25709051c3e96288f2d ("nptl: >> Unconditionally use a 32-byte rseq area"). After that, it was >> not ensured that the RSEQ_SIG macro was defined during sched_getcpu.c >> compilation that provided a definition. This commit always checks >> the rseq area for CPU number information before using the other >> approaches. >> >> This adds an unnecessary (but well-predictable) branch on >> architectures which do not define RSEQ_SIG, but its cost is small >> compared to the system call. Most architectures that have vDSO >> acceleration for getcpu also have rseq support. >> >> Fixes: 2c6b4b272e6b4d07303af25709051c3e96288f2d >> Fixes: 1d350aa06091211863e41169729cee1bca39f72f > > Would adding back the '#include <sys/rseq.h>' in nptl/descr.h have any adverse > consequences? > > It would restore the original behavior without any effect on architectures > which do not define RSEQ_SIG. Just saw the V2 discussion, please ignore this.
> Originally, nptl/descr.h included <sys/rseq.h>, but we removed that > in commit 2c6b4b272e6b4d07303af25709051c3e96288f2d ("nptl: > Unconditionally use a 32-byte rseq area"). After that, it was > not ensured that the RSEQ_SIG macro was defined during sched_getcpu.c > compilation that provided a definition. This commit always checks > the rseq area for CPU number information before using the other > approaches. > > This adds an unnecessary (but well-predictable) branch on > architectures which do not define RSEQ_SIG, but its cost is small > compared to the system call. Most architectures that have vDSO > acceleration for getcpu also have rseq support. This looks good to me. Reviewed-by: Arjun Shankar <arjun@redhat.com> > > Fixes: 2c6b4b272e6b4d07303af25709051c3e96288f2d OK. This commit removed the #include for sys/rseq.h. > Fixes: 1d350aa06091211863e41169729cee1bca39f72f OK. This commit made sched_getcpu's implementation depend on the definition of RSEQ_SIG. > > --- > v3: Remove #ifdef RSEQ_SIG instead of making it defined. > sysdeps/unix/sysv/linux/sched_getcpu.c | 8 -------- > 1 file changed, 8 deletions(-) > > diff --git a/sysdeps/unix/sysv/linux/sched_getcpu.c b/sysdeps/unix/sysv/linux/sched_getcpu.c > index dfb884568d..72a3360550 100644 > --- a/sysdeps/unix/sysv/linux/sched_getcpu.c > +++ b/sysdeps/unix/sysv/linux/sched_getcpu.c > @@ -33,17 +33,9 @@ vsyscall_sched_getcpu (void) > return r == -1 ? r : cpu; > } > > -#ifdef RSEQ_SIG > int > sched_getcpu (void) > { > int cpu_id = THREAD_GETMEM_VOLATILE (THREAD_SELF, rseq_area.cpu_id); > return __glibc_likely (cpu_id >= 0) ? cpu_id : vsyscall_sched_getcpu (); > } > -#else /* RSEQ_SIG */ > -int > -sched_getcpu (void) > -{ > - return vsyscall_sched_getcpu (); > -} > -#endif /* RSEQ_SIG */ OK. Get rid of the "backup" implementation. Now, on machines without support, cpu_id will first be set to RSEQ_CPU_ID_REGISTRATION_FAILED, which is negative. Then we will call vsyscall_sched_getcpu instead.
diff --git a/sysdeps/unix/sysv/linux/sched_getcpu.c b/sysdeps/unix/sysv/linux/sched_getcpu.c index dfb884568d..72a3360550 100644 --- a/sysdeps/unix/sysv/linux/sched_getcpu.c +++ b/sysdeps/unix/sysv/linux/sched_getcpu.c @@ -33,17 +33,9 @@ vsyscall_sched_getcpu (void) return r == -1 ? r : cpu; } -#ifdef RSEQ_SIG int sched_getcpu (void) { int cpu_id = THREAD_GETMEM_VOLATILE (THREAD_SELF, rseq_area.cpu_id); return __glibc_likely (cpu_id >= 0) ? cpu_id : vsyscall_sched_getcpu (); } -#else /* RSEQ_SIG */ -int -sched_getcpu (void) -{ - return vsyscall_sched_getcpu (); -} -#endif /* RSEQ_SIG */