* Re: [PATCH v6 04/16] sched/core: uclamp: Add CPU's clamp buckets refcounting
From: Patrick Bellasi @ 2019-01-21 15:54 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190121151717.GK27931@hirez.programming.kicks-ass.net>
On 21-Jan 16:17, Peter Zijlstra wrote:
> On Tue, Jan 15, 2019 at 10:15:01AM +0000, Patrick Bellasi wrote:
> > +#ifdef CONFIG_UCLAMP_TASK
>
> > +struct uclamp_bucket {
> > + unsigned long value : bits_per(SCHED_CAPACITY_SCALE);
> > + unsigned long tasks : BITS_PER_LONG - bits_per(SCHED_CAPACITY_SCALE);
> > +};
>
> > +struct uclamp_cpu {
> > + unsigned int value;
>
> /* 4 byte hole */
>
> > + struct uclamp_bucket bucket[UCLAMP_BUCKETS];
> > +};
>
> With the default of 5, this UCLAMP_BUCKETS := 6, so struct uclamp_cpu
> ends up being 7 'unsigned long's, or 56 bytes on 64bit (with a 4 byte
> hole).
Yes, that's dimensioned and configured to fit into a single cache line
for all the possible 5 (by default) clamp values of a clamp index
(i.e. min or max util).
>
> > +#endif /* CONFIG_UCLAMP_TASK */
> > +
> > /*
> > * This is the main, per-CPU runqueue data structure.
> > *
> > @@ -835,6 +879,11 @@ struct rq {
> > unsigned long nr_load_updates;
> > u64 nr_switches;
> >
> > +#ifdef CONFIG_UCLAMP_TASK
> > + /* Utilization clamp values based on CPU's RUNNABLE tasks */
> > + struct uclamp_cpu uclamp[UCLAMP_CNT] ____cacheline_aligned;
>
> Which makes this 112 bytes with 8 bytes in 2 holes, which is short of 2
> 64 byte cachelines.
Right, we have 2 cache lines where:
- the first $L tracks 5 different util_min values
- the second $L tracks 5 different util_max values
> Is that the best layout?
It changed few times and that's what I found more reasonable for both
for fitting the default configuration and also for code readability.
Notice that we access RQ and SE clamp values with the same patter,
for example:
{rq|p}->uclamp[clamp_idx].value
Are you worried about the holes or something else specific ?
> > +#endif
> > +
> > struct cfs_rq cfs;
> > struct rt_rq rt;
> > struct dl_rq dl;
> > --
> > 2.19.2
> >
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v6 04/16] sched/core: uclamp: Add CPU's clamp buckets refcounting
From: Peter Zijlstra @ 2019-01-21 16:12 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190121152311.7u7bwbjopuptnzcy@e110439-lin>
On Mon, Jan 21, 2019 at 03:23:11PM +0000, Patrick Bellasi wrote:
> On 21-Jan 15:59, Peter Zijlstra wrote:
> > On Tue, Jan 15, 2019 at 10:15:01AM +0000, Patrick Bellasi wrote:
> > > @@ -835,6 +954,28 @@ static void uclamp_bucket_inc(struct uclamp_se *uc_se, unsigned int clamp_id,
> > > } while (!atomic_long_try_cmpxchg(&uc_maps[bucket_id].adata,
> > > &uc_map_old.data, uc_map_new.data));
> > >
> > > + /*
> > > + * Ensure each CPU tracks the correct value for this clamp bucket.
> > > + * This initialization of per-CPU variables is required only when a
> > > + * clamp value is requested for the first time from a slow-path.
> > > + */
> >
> > I'm confused; why is this needed?
>
> That's a lazy initialization of the per-CPU uclamp data for a given
> bucket, i.e. the clamp value assigned to a bucket, which happens only
> when new clamp values are requested... usually only at system
> boot/configuration time.
>
> For example, let say we have these buckets mapped to given clamp
> values:
>
> bucket_#0: clamp value: 10% (mapped)
> bucket_#1: clamp value: 20% (mapped)
> bucket_#2: clamp value: 30% (mapped)
>
> and then let's assume all the users of bucket_#1 are "destroyed", i.e.
> there are no more tasks, system defaults or cgroups asking for a
> 20% clamp value. The corresponding bucket will become free:
>
> bucket_#0: clamp value: 10% (mapped)
> bucket_#1: clamp value: 20% (free)
> bucket_#2: clamp value: 30% (mapped)
>
> If, in the future, we ask for a new clamp value, let say a task ask
> for a 40% clamp value, then we need to map that value into a bucket.
> Since bucket_#1 is free we can use it to fill up the hold and keep all
> the buckets in use at the beginning of a cache line.
>
> However, since now bucket_#1 tracks a different clamp value (40
> instead of 20) we need to walk all the CPUs and updated the cached
> value:
>
> bucket_#0: clamp value: 10% (mapped)
> bucket_#1: clamp value: 40% (mapped)
> bucket_#2: clamp value: 30% (mapped)
>
> Is that more clear ?
Yes, and I realized this a little while after sending this; but I'm not
sure I have an answer to why though.
That is; why isn't the whole thing hard coded to have:
bucket_n: clamp value: n*UCLAMP_BUCKET_DELTA
We already do that division anyway (clamp_value / UCLAMP_BUCKET_DELTA),
and from that we instantly have the right bucket index. And that allows
us to initialize all this beforehand.
> and keep all
> the buckets in use at the beginning of a cache line.
That; is that the rationale for all this? Note that per the defaults
everything is in a single line already.
^ permalink raw reply
* Re: [PATCH v2 29/29] y2038: add 64-bit time_t syscalls to all 32-bit architectures
From: Arnd Bergmann @ 2019-01-21 16:31 UTC (permalink / raw)
To: y2038 Mailman List, Linux API, Linux Kernel Mailing List,
linux-arch
Cc: Matt Turner, Russell King - ARM Linux, Catalin Marinas,
Will Deacon, Tony Luck, Fenghua Yu, Geert Uytterhoeven,
Michal Simek, Paul Burton, Helge Deller, Benjamin Herrenschmidt,
Michael Ellerman, Martin Schwidefsky, Heiko Carstens, Rich Felker,
David Miller, Andy Lutomirski, Thomas Gleixner, Ingo Molnar,
"H. Peter Anvin" <hpa>
In-Reply-To: <20190118161835.2259170-30-arnd@arndb.de>
On Fri, Jan 18, 2019 at 5:25 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> This adds 21 new system calls on each ABI that has 32-bit time_t
> today. All of these have the exact same semantics as their existing
> counterparts, and the new ones all have macro names that end in 'time64'
> for clarification.
>
> This gets us to the point of being able to safely use a C library
> that has 64-bit time_t in user space. There are still a couple of
> loose ends to tie up in various areas of the code, but this is the
> big one, and should be entirely uncontroversial at this point.
I've successfully tested this with musl and LTP now, using an
i386 kernel. The musl port I used is at
https://git.linaro.org/people/arnd.bergmann/musl-y2038.git/
This is just an updated version of what I used for testing last
year, using the current syscall assignment, and going back
to the time32 versions of getitimer/setitimer and
wait4/waitid/getusage.
It's certainly not intended for merging like this, but a proper
musl port is under discussion now, and this should be
sufficient if anyone else wants to try out the new syscall
ABI before we merge it.
The LTP I have is heavily hacked, and has a number of
failures resulting from differences between musl and glibc,
or from the way we convert between the kernel types and
the user space types.
The testing found one minor bug in all the kernel syscall tables:
> +418 common mq_timedsend_time64 sys_mq_timedsend
> +419 common mq_timedreceiv_time64 sys_mq_timedreceive
While this would have fit in with umount(), creat() and mknod(),
it was unintentional, and I've changed it back to
mq_timedreceive_time64 (with an added 'e').
Arnd
^ permalink raw reply
* Re: [PATCH v6 04/16] sched/core: uclamp: Add CPU's clamp buckets refcounting
From: Patrick Bellasi @ 2019-01-21 16:33 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190121161237.GB13777@hirez.programming.kicks-ass.net>
On 21-Jan 17:12, Peter Zijlstra wrote:
> On Mon, Jan 21, 2019 at 03:23:11PM +0000, Patrick Bellasi wrote:
> > On 21-Jan 15:59, Peter Zijlstra wrote:
> > > On Tue, Jan 15, 2019 at 10:15:01AM +0000, Patrick Bellasi wrote:
> > > > @@ -835,6 +954,28 @@ static void uclamp_bucket_inc(struct uclamp_se *uc_se, unsigned int clamp_id,
> > > > } while (!atomic_long_try_cmpxchg(&uc_maps[bucket_id].adata,
> > > > &uc_map_old.data, uc_map_new.data));
> > > >
> > > > + /*
> > > > + * Ensure each CPU tracks the correct value for this clamp bucket.
> > > > + * This initialization of per-CPU variables is required only when a
> > > > + * clamp value is requested for the first time from a slow-path.
> > > > + */
> > >
> > > I'm confused; why is this needed?
> >
> > That's a lazy initialization of the per-CPU uclamp data for a given
> > bucket, i.e. the clamp value assigned to a bucket, which happens only
> > when new clamp values are requested... usually only at system
> > boot/configuration time.
> >
> > For example, let say we have these buckets mapped to given clamp
> > values:
> >
> > bucket_#0: clamp value: 10% (mapped)
> > bucket_#1: clamp value: 20% (mapped)
> > bucket_#2: clamp value: 30% (mapped)
> >
> > and then let's assume all the users of bucket_#1 are "destroyed", i.e.
> > there are no more tasks, system defaults or cgroups asking for a
> > 20% clamp value. The corresponding bucket will become free:
> >
> > bucket_#0: clamp value: 10% (mapped)
> > bucket_#1: clamp value: 20% (free)
> > bucket_#2: clamp value: 30% (mapped)
> >
> > If, in the future, we ask for a new clamp value, let say a task ask
> > for a 40% clamp value, then we need to map that value into a bucket.
> > Since bucket_#1 is free we can use it to fill up the hold and keep all
> > the buckets in use at the beginning of a cache line.
> >
> > However, since now bucket_#1 tracks a different clamp value (40
> > instead of 20) we need to walk all the CPUs and updated the cached
> > value:
> >
> > bucket_#0: clamp value: 10% (mapped)
> > bucket_#1: clamp value: 40% (mapped)
> > bucket_#2: clamp value: 30% (mapped)
> >
> > Is that more clear ?
>
> Yes, and I realized this a little while after sending this; but I'm not
> sure I have an answer to why though.
>
> That is; why isn't the whole thing hard coded to have:
>
> bucket_n: clamp value: n*UCLAMP_BUCKET_DELTA
>
> We already do that division anyway (clamp_value / UCLAMP_BUCKET_DELTA),
> and from that we instantly have the right bucket index. And that allows
> us to initialize all this beforehand.
>
> > and keep all
> > the buckets in use at the beginning of a cache line.
>
> That; is that the rationale for all this? Note that per the defaults
> everything is in a single line already.
Yes, that's because of the loop in:
dequeue_task()
uclamp_cpu_dec()
uclamp_cpu_dec_id()
uclamp_cpu_update()
where buckets needs sometimes to be scanned to find a new max.
Consider also that, with mapping, we can more easily increase the
buckets count to 20 in order to have a finer clamping granularity if
needed without warring too much about performance impact especially
when we use anyway few different clamp values.
So, I agree that mapping adds (code) complexity but it can also save
few cycles in the fast path... do you think it's not worth the added
complexity?
TBH I never did a proper profiling w/-w/o mapping... I'm just worried
in principle for a loop on 20 entries spanning 4 cache lines. :/
NOTE: the loop is currently going through all the entries anyway,
but we can add later a guard to bail out once we covered the
number of active entries.
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v2 29/29] y2038: add 64-bit time_t syscalls to all 32-bit architectures
From: Arnd Bergmann @ 2019-01-21 17:08 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Russell King - ARM Linux admin, Andy Lutomirski,
y2038 Mailman List, Linux API, LKML, linux-arch, Matt Turner,
Catalin Marinas, Will Deacon, Tony Luck, Fenghua Yu, Michal Simek,
Paul Burton, Helge Deller, Benjamin Herrenschmidt,
Michael Ellerman, Martin Schwidefsky, Heiko Carstens, Rich Felker
In-Reply-To: <CAMuHMdXzQNEEDjWrmTph8Krovj1g2WhnBUaM=FvKB+J2fZqctA@mail.gmail.com>
On Mon, Jan 21, 2019 at 9:19 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Sat, Jan 19, 2019 at 3:29 PM Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> > On Fri, Jan 18, 2019 at 11:53:25AM -0800, Andy Lutomirski wrote:
> > > On Fri, Jan 18, 2019 at 11:33 AM Arnd Bergmann <arnd@arndb.de> wrote:
> > > > On Fri, Jan 18, 2019 at 7:50 PM Andy Lutomirski <luto@kernel.org> wrote:
> > >
> > > Can we perhaps just start the consistent numbers above 547 or maybe
> > > block out 512..547 in the new regime?
> >
> > I don't think you gain much with that kind of scheme - it won't take
> > very long before an architecture misses having a syscall added, and
> > then someone else adds their own. Been there with ARM - I was keeping
> > the syscall table in the same order as x86 for new syscalls, but now
>
> Same for m68k, and probably other architectures.
>
> > that others have been adding syscalls to the table since I converted
> > ARM to the tabular form, that's now gone out the window.
> >
> > So, I think it's completely pointless to do what you're suggesting.
> > We'll just end up with a big hole in the middle of the syscall table
> > and then revert back to random numbering of syscalls thereafter again.
>
> I believe the plan is to add future syscalls for all architectures in a
> single commit, to keep everything in sync.
Yes, that is the idea. This was not realistic before, since each one
of the old architectures had its own way of describing the system call
tables, and many needed a different set of quirks.
Since (almost) everything is now converted to the syscall.tbl format,
we have removed all obsolete architectures, and a lot of the quirks
(x32, spu, s390-31) won't matter as much in the future, I think it is
now possible to do it.
We could even extend scripts/checksyscalls.sh to warn if a new
syscall above 423 is not added to all 16 tables at the same time.
> Regardless, I'm wondering what to do with the holes marked "room for
> arch specific calls".
> When is a syscall really arch-specific, and can it be added there, and
> when does it turn out (later) that it isn't, breaking the
> synchronization again?
We've had a bit of that already, with cacheflush(), which exists on
a couple of architectures, including some that use the first
'arch specific' slot (244) of the asm-generic table. I think this
will be rare enough that we can figure out a solution when we
get there.
> The pkey syscalls may be a bad example, as AFAIU they can be implemented
> on some architectures, but not on some others. Still, I had skipped them
> when adding new syscalls to m68k.
>
> Perhaps we should get rid of the notion of "arch-specific syscalls", and
> reserve a slot everywhere anyway?
I don't mind calling the hole something else if that helps. Out of
principle I would already assume that anything we add for x86
or the generic table should be added everywhere, but we can
make it broader than that.
Arnd
^ permalink raw reply
* Re: [PATCH v2 14/29] arch: add pkey and rseq syscall numbers everywhere
From: Arnd Bergmann @ 2019-01-21 20:28 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: y2038 Mailman List, Linux API, Linux Kernel Mailing List,
Linux-Arch, Matt Turner, Russell King, Catalin Marinas,
Will Deacon, Tony Luck, Fenghua Yu, Michal Simek, Paul Burton,
Helge Deller, Benjamin Herrenschmidt, Michael Ellerman,
Martin Schwidefsky, Heiko Carstens, Rich Felker, David S. Miller
In-Reply-To: <CAMuHMdWaJyNqUeq4qu3AgU0fYrQdZ_zbo0DFFiM97Y5HESYYnA@mail.gmail.com>
On Mon, Jan 21, 2019 at 9:56 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Note that all architectures that already define pkey syscalls, list
> pkey_mprotect first.
It's easy enough to change, so I've reordered them for consistency now.
> Regardless, for m68k:
> Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
Thanks,
Arnd
^ permalink raw reply
* Re: [PATCH v2 29/29] y2038: add 64-bit time_t syscalls to all 32-bit architectures
From: Arnd Bergmann @ 2019-01-21 20:40 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Rich Felker, linux-ia64@vger.kernel.org, Linux-sh list,
Benjamin Herrenschmidt, Heiko Carstens, linux-mips, Max Filippov,
Network Development, Deepa Dinamani, H. Peter Anvin, sparclinux,
linux-arch, linux-s390, y2038 Mailman List, Michael Ellerman,
Helge Deller, X86 ML, Russell King - ARM Linux admin, Ingo Molnar,
Firoz Khan, Catalin Marinas, Matt Turner, Fengh
In-Reply-To: <CAK8P3a04UC2dHVqx1gHXJQzsDw446h1ghLEuRe0xmUyJgrOktw@mail.gmail.com>
On Mon, Jan 21, 2019 at 6:08 PM Arnd Bergmann <arnd@arndb.de> wrote:
> On Mon, Jan 21, 2019 at 9:19 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > Regardless, I'm wondering what to do with the holes marked "room for
> > arch specific calls".
> > When is a syscall really arch-specific, and can it be added there, and
> > when does it turn out (later) that it isn't, breaking the
> > synchronization again?
>
> We've had a bit of that already, with cacheflush(), which exists on
> a couple of architectures, including some that use the first
> 'arch specific' slot (244) of the asm-generic table. I think this
> will be rare enough that we can figure out a solution when we
> get there.
>
> > The pkey syscalls may be a bad example, as AFAIU they can be implemented
> > on some architectures, but not on some others. Still, I had skipped them
> > when adding new syscalls to m68k.
> >
> > Perhaps we should get rid of the notion of "arch-specific syscalls", and
> > reserve a slot everywhere anyway?
>
> I don't mind calling the hole something else if that helps. Out of
> principle I would already assume that anything we add for x86
> or the generic table should be added everywhere, but we can
> make it broader than that.
Applying this fixup below,
ARnd
diff --git a/arch/x86/entry/syscalls/syscall_32.tbl
b/arch/x86/entry/syscalls/syscall_32.tbl
index d9c2d2eea044..955ab6a3b61f 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -398,7 +398,7 @@
384 i386 arch_prctl sys_arch_prctl
__ia32_compat_sys_arch_prctl
385 i386 io_pgetevents sys_io_pgetevents_time32
__ia32_compat_sys_io_pgetevents
386 i386 rseq sys_rseq
__ia32_sys_rseq
-# room for arch specific syscalls
+# don't use numbers 387 through 392, add new calls at the end
393 i386 semget sys_semget
__ia32_sys_semget
394 i386 semctl sys_semctl
__ia32_compat_sys_semctl
395 i386 shmget sys_shmget
__ia32_sys_shmget
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl
b/arch/x86/entry/syscalls/syscall_64.tbl
index 43a622aec07e..2ae92fddb6d5 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -343,6 +343,8 @@
332 common statx __x64_sys_statx
333 common io_pgetevents __x64_sys_io_pgetevents
334 common rseq __x64_sys_rseq
+# don't use numbers 387 through 423, add new calls after the last
+# 'common' entry
#
# x32-specific system call numbers start at 512 to avoid cache impact
diff --git a/include/uapi/asm-generic/unistd.h
b/include/uapi/asm-generic/unistd.h
index 53831e4a4c86..acf9a07ab2ff 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -740,7 +740,7 @@ __SC_COMP_3264(__NR_io_pgetevents,
sys_io_pgetevents_time32, sys_io_pgetevents,
__SYSCALL(__NR_rseq, sys_rseq)
#define __NR_kexec_file_load 294
__SYSCALL(__NR_kexec_file_load, sys_kexec_file_load)
-/* 295 through 402 are unassigned to sync up with generic numbers */
+/* 295 through 402 are unassigned to sync up with generic numbers, don't use */
#if __BITS_PER_LONG == 32
#define __NR_clock_gettime64 403
__SYSCALL(__NR_clock_gettime64, sys_clock_gettime)
^ permalink raw reply related
* Re: [RFC PATCH glibc 1/4] glibc: Perform rseq(2) registration at C startup and thread creation (v5)
From: Mathieu Desnoyers @ 2019-01-21 21:23 UTC (permalink / raw)
To: carlos
Cc: Florian Weimer, Joseph Myers, Szabolcs Nagy, libc-alpha,
Thomas Gleixner, Ben Maurer, Peter Zijlstra, Paul E. McKenney,
Boqun Feng, Will Deacon, Dave Watson, Paul Turner, Rich Felker,
linux-kernel, linux-api
In-Reply-To: <1887968822.1146.1547833305059.JavaMail.zimbra@efficios.com>
----- On Jan 18, 2019, at 12:41 PM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:
> ----- On Jan 14, 2019, at 8:51 PM, Mathieu Desnoyers
> mathieu.desnoyers@efficios.com wrote:
>
> [...]
>
>> diff --git a/sysdeps/unix/sysv/linux/rseq-sym.c
>> b/sysdeps/unix/sysv/linux/rseq-sym.c
>> new file mode 100644
>> index 0000000000..6856d0388a
> [...]
>> +/* volatile because fields can be read/updated by the kernel. */
>> +__thread volatile struct rseq __rseq_abi = {
>> + .cpu_id = RSEQ_CPU_ID_UNINITIALIZED,
>> +};
>> +
>> +/* volatile because refcount can be read/updated by signal handlers. */
>> +__thread volatile uint32_t __rseq_refcount;
>
> Back to the weak vs non-weak question about those two symbols. I understand
> that tagging them as weak symbols has little effect on the dynamic loader
> when it loads libc.so. However, I'm worried about that happens when
> libc is statically linked into an application, and there happens to
> be more than one instance of those symbols (e.g. libc and another library
> define the same symbols, and both are statically linked into the same
> application). Isn't it a situation where tagging those symbols as "weak"
> becomes useful ?
Testing shows that it seems fine to statically link two archives within an
executable in a scenario where each .a defines the same symbol, without
using "weak", so I won't worry about this further.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* [RFC PATCH glibc 1/4] glibc: Perform rseq(2) registration at C startup and thread creation (v6)
From: Mathieu Desnoyers @ 2019-01-21 21:35 UTC (permalink / raw)
To: Carlos O'Donell
Cc: Florian Weimer, Joseph Myers, Szabolcs Nagy, libc-alpha,
Mathieu Desnoyers, Thomas Gleixner, Ben Maurer, Peter Zijlstra,
Paul E. McKenney, Boqun Feng, Will Deacon, Dave Watson,
Paul Turner, Rich Felker, linux-kernel, linux-api
Register rseq(2) TLS for each thread (including main), and unregister
for each thread (excluding main). "rseq" stands for Restartable
Sequences.
See the rseq(2) man page proposed here:
https://lkml.org/lkml/2018/9/19/647
This patch is based on glibc commit a502c5294. The rseq(2) system call
was merged into Linux 4.18.
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
CC: Carlos O'Donell <carlos@redhat.com>
CC: Florian Weimer <fweimer@redhat.com>
CC: Joseph Myers <joseph@codesourcery.com>
CC: Szabolcs Nagy <szabolcs.nagy@arm.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ben Maurer <bmaurer@fb.com>
CC: Peter Zijlstra <peterz@infradead.org>
CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
CC: Boqun Feng <boqun.feng@gmail.com>
CC: Will Deacon <will.deacon@arm.com>
CC: Dave Watson <davejwatson@fb.com>
CC: Paul Turner <pjt@google.com>
CC: Rich Felker <dalias@libc.org>
CC: libc-alpha@sourceware.org
CC: linux-kernel@vger.kernel.org
CC: linux-api@vger.kernel.org
---
Changes since v1:
- Move __rseq_refcount to an extra field at the end of __rseq_abi to
eliminate one symbol.
All libraries/programs which try to register rseq (glibc,
early-adopter applications, early-adopter libraries) should use the
rseq refcount. It becomes part of the ABI within a user-space
process, but it's not part of the ABI shared with the kernel per se.
- Restructure how this code is organized so glibc keeps building on
non-Linux targets.
- Use non-weak symbol for __rseq_abi.
- Move rseq registration/unregistration implementation into its own
nptl/rseq.c compile unit.
- Move __rseq_abi symbol under GLIBC_2.29.
Changes since v2:
- Move __rseq_refcount to its own symbol, which is less ugly than
trying to play tricks with the rseq uapi.
- Move __rseq_abi from nptl to csu (C start up), so it can be used
across glibc, including memory allocator and sched_getcpu(). The
__rseq_refcount symbol is kept in nptl, because there is no reason
to use it elsewhere in glibc.
Changes since v3:
- Set __rseq_refcount TLS to 1 on register/set to 0 on unregister
because glibc is the first/last user.
- Unconditionally register/unregister rseq at thread start/exit, because
glibc is the first/last user.
- Add missing abilist items.
- Rebase on glibc master commit a502c5294.
- Add NEWS entry.
Changes since v4:
- Do not use "weak" symbols for __rseq_abi and __rseq_refcount. Based on
"System V Application Binary Interface", weak only affects the link
editor, not the dynamic linker.
- Install a new sys/rseq.h system header on Linux, which contains the
RSEQ_SIG definition, __rseq_abi declaration and __rseq_refcount
declaration. Move those definition/declarations from rseq-internal.h
to the installed sys/rseq.h header.
- Considering that rseq is only available on Linux, move csu/rseq.c to
sysdeps/unix/sysv/linux/rseq-sym.c.
- Move __rseq_refcount from nptl/rseq.c to
sysdeps/unix/sysv/linux/rseq-sym.c, so it is only defined on Linux.
- Move both ABI definitions for __rseq_abi and __rseq_refcount to
sysdeps/unix/sysv/linux/Versions, so they only appear on Linux.
- Document __rseq_abi and __rseq_refcount volatile.
- Document the RSEQ_SIG signature define.
- Move registration functions from rseq.c to rseq-internal.h static
inline functions. Introduce empty stubs in misc/rseq-internal.h,
which can be overridden by architecture code in
sysdeps/unix/sysv/linux/rseq-internal.h.
- Rename __rseq_register_current_thread and __rseq_unregister_current_thread
to rseq_register_current_thread and rseq_unregister_current_thread,
now that those are only visible as internal static inline functions.
- Invoke rseq_register_current_thread() from libc-start.c LIBC_START_MAIN
rather than nptl init, so applications not linked against
libpthread.so have rseq registered for their main() thread. Note that
it is invoked separately for SHARED and !SHARED builds.
Changes since v5:
- Replace __rseq_refcount by __rseq_lib_abi, which contains two
uint32_t: register_state and refcount. The "register_state" field
allows inhibiting rseq registration from signal handlers nested on top
of glibc registration and occuring after rseq unregistration by glibc.
- Introduce enum rseq_register_state, which contains the states allowed
for the struct rseq_lib_abi register_state field.
---
NEWS | 6 ++
csu/libc-start.c | 12 ++-
misc/Makefile | 3 +-
misc/rseq-internal.h | 34 +++++++
nptl/pthread_create.c | 9 ++
sysdeps/unix/sysv/linux/Makefile | 5 +-
sysdeps/unix/sysv/linux/Versions | 4 +
sysdeps/unix/sysv/linux/aarch64/libc.abilist | 2 +
sysdeps/unix/sysv/linux/alpha/libc.abilist | 2 +
sysdeps/unix/sysv/linux/arm/libc.abilist | 2 +
sysdeps/unix/sysv/linux/hppa/libc.abilist | 2 +
sysdeps/unix/sysv/linux/i386/libc.abilist | 2 +
sysdeps/unix/sysv/linux/ia64/libc.abilist | 2 +
.../sysv/linux/m68k/coldfire/libc.abilist | 2 +
.../unix/sysv/linux/m68k/m680x0/libc.abilist | 2 +
.../unix/sysv/linux/microblaze/libc.abilist | 2 +
.../sysv/linux/mips/mips32/fpu/libc.abilist | 2 +
.../sysv/linux/mips/mips32/nofpu/libc.abilist | 2 +
.../sysv/linux/mips/mips64/n32/libc.abilist | 2 +
.../sysv/linux/mips/mips64/n64/libc.abilist | 2 +
sysdeps/unix/sysv/linux/nios2/libc.abilist | 2 +
.../linux/powerpc/powerpc32/fpu/libc.abilist | 2 +
.../powerpc/powerpc32/nofpu/libc.abilist | 2 +
.../linux/powerpc/powerpc64/libc-le.abilist | 2 +
.../sysv/linux/powerpc/powerpc64/libc.abilist | 2 +
.../unix/sysv/linux/riscv/rv64/libc.abilist | 2 +
sysdeps/unix/sysv/linux/rseq-internal.h | 91 +++++++++++++++++++
sysdeps/unix/sysv/linux/rseq-sym.c | 54 +++++++++++
.../unix/sysv/linux/s390/s390-32/libc.abilist | 2 +
.../unix/sysv/linux/s390/s390-64/libc.abilist | 2 +
sysdeps/unix/sysv/linux/sh/libc.abilist | 2 +
.../sysv/linux/sparc/sparc32/libc.abilist | 2 +
.../sysv/linux/sparc/sparc64/libc.abilist | 2 +
sysdeps/unix/sysv/linux/sys/rseq.h | 64 +++++++++++++
.../unix/sysv/linux/x86_64/64/libc.abilist | 2 +
.../unix/sysv/linux/x86_64/x32/libc.abilist | 2 +
36 files changed, 328 insertions(+), 6 deletions(-)
create mode 100644 misc/rseq-internal.h
create mode 100644 sysdeps/unix/sysv/linux/rseq-internal.h
create mode 100644 sysdeps/unix/sysv/linux/rseq-sym.c
create mode 100644 sysdeps/unix/sysv/linux/sys/rseq.h
diff --git a/NEWS b/NEWS
index f488821af1..b238eaa391 100644
--- a/NEWS
+++ b/NEWS
@@ -35,6 +35,12 @@ Major new features:
different directory. This is a GNU extension and similar to the
Solaris function of the same name.
+* Support for automatically registering threads with the Linux rseq(2)
+ system call has been added. This system call is implemented starting
+ from Linux 4.18. In order to be activated, it requires that glibc is built
+ against kernel headers that include this system call, and that glibc
+ detects availability of that system call at runtime.
+
Deprecated and removed features, and other changes affecting compatibility:
* The glibc.tune tunable namespace has been renamed to glibc.cpu and the
diff --git a/csu/libc-start.c b/csu/libc-start.c
index 494132368f..dc39e09685 100644
--- a/csu/libc-start.c
+++ b/csu/libc-start.c
@@ -22,6 +22,7 @@
#include <ldsodefs.h>
#include <exit-thread.h>
#include <libc-internal.h>
+#include <rseq-internal.h>
#include <elf/dl-tunables.h>
@@ -140,7 +141,10 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
__libc_multiple_libcs = &_dl_starting_up && !_dl_starting_up;
-#ifndef SHARED
+#ifdef SHARED
+ /* Register rseq ABI to the kernel. */
+ (void) rseq_register_current_thread ();
+#else
_dl_relocate_static_pie ();
char **ev = &argv[argc + 1];
@@ -218,6 +222,9 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
}
# endif
+ /* Register rseq ABI to the kernel. */
+ (void) rseq_register_current_thread ();
+
/* Initialize libpthread if linked in. */
if (__pthread_initialize_minimal != NULL)
__pthread_initialize_minimal ();
@@ -230,8 +237,7 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
# else
__pointer_chk_guard_local = pointer_chk_guard;
# endif
-
-#endif /* !SHARED */
+#endif
/* Register the destructor of the dynamic linker if there is any. */
if (__glibc_likely (rtld_fini != NULL))
diff --git a/misc/Makefile b/misc/Makefile
index c2c9994d17..4175cbb3d3 100644
--- a/misc/Makefile
+++ b/misc/Makefile
@@ -36,7 +36,8 @@ headers := sys/uio.h bits/uio-ext.h bits/uio_lim.h \
syslog.h sys/syslog.h \
bits/syslog.h bits/syslog-ldbl.h bits/syslog-path.h bits/error.h \
bits/select2.h bits/hwcap.h sys/auxv.h \
- sys/sysmacros.h bits/sysmacros.h bits/types/struct_iovec.h
+ sys/sysmacros.h bits/sysmacros.h bits/types/struct_iovec.h \
+ rseq-internal.h
routines := brk sbrk sstk ioctl \
readv writev preadv preadv64 pwritev pwritev64 \
diff --git a/misc/rseq-internal.h b/misc/rseq-internal.h
new file mode 100644
index 0000000000..915122e4bf
--- /dev/null
+++ b/misc/rseq-internal.h
@@ -0,0 +1,34 @@
+/* Copyright (C) 2018 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+ Contributed by Mathieu Desnoyers <mathieu.desnoyers@efficios.com>, 2018.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#ifndef RSEQ_INTERNAL_H
+#define RSEQ_INTERNAL_H
+
+static inline int
+rseq_register_current_thread (void)
+{
+ return -1;
+}
+
+static inline int
+rseq_unregister_current_thread (void)
+{
+ return -1;
+}
+
+#endif /* rseq-internal.h */
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index fe75d04113..bb80f97d2d 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -33,6 +33,7 @@
#include <default-sched.h>
#include <futex-internal.h>
#include <tls-setup.h>
+#include <rseq-internal.h>
#include "libioP.h"
#include <shlib-compat.h>
@@ -378,6 +379,7 @@ __free_tcb (struct pthread *pd)
START_THREAD_DEFN
{
struct pthread *pd = START_THREAD_SELF;
+ bool has_rseq = false;
#if HP_TIMING_AVAIL
/* Remember the time when the thread was started. */
@@ -396,6 +398,9 @@ START_THREAD_DEFN
if (__glibc_unlikely (atomic_exchange_acq (&pd->setxid_futex, 0) == -2))
futex_wake (&pd->setxid_futex, 1, FUTEX_PRIVATE);
+ /* Register rseq TLS to the kernel. */
+ has_rseq = !rseq_register_current_thread ();
+
#ifdef __NR_set_robust_list
# ifndef __ASSUME_SET_ROBUST_LIST
if (__set_robust_list_avail >= 0)
@@ -573,6 +578,10 @@ START_THREAD_DEFN
}
#endif
+ /* Unregister rseq TLS from kernel. */
+ if (has_rseq && rseq_unregister_current_thread ())
+ abort();
+
advise_stack_range (pd->stackblock, pd->stackblock_size, (uintptr_t) pd,
pd->guardsize);
diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
index 72b6b641d5..b18e1cd450 100644
--- a/sysdeps/unix/sysv/linux/Makefile
+++ b/sysdeps/unix/sysv/linux/Makefile
@@ -1,5 +1,5 @@
ifeq ($(subdir),csu)
-sysdep_routines += errno-loc
+sysdep_routines += errno-loc rseq-sym
endif
ifeq ($(subdir),assert)
@@ -43,7 +43,8 @@ sysdep_headers += sys/mount.h sys/acct.h sys/sysctl.h \
bits/siginfo-arch.h bits/siginfo-consts-arch.h \
bits/procfs.h bits/procfs-id.h bits/procfs-extra.h \
bits/procfs-prregset.h bits/mman-map-flags-generic.h \
- bits/msq-pad.h bits/sem-pad.h bits/shmlba.h bits/shm-pad.h
+ bits/msq-pad.h bits/sem-pad.h bits/shmlba.h bits/shm-pad.h \
+ sys/rseq.h
tests += tst-clone tst-clone2 tst-clone3 tst-fanotify tst-personality \
tst-quota tst-sync_file_range tst-sysconf-iov_max tst-ttyname \
diff --git a/sysdeps/unix/sysv/linux/Versions b/sysdeps/unix/sysv/linux/Versions
index 336c13b57d..777ea723f8 100644
--- a/sysdeps/unix/sysv/linux/Versions
+++ b/sysdeps/unix/sysv/linux/Versions
@@ -171,6 +171,10 @@ libc {
mlock2;
pkey_alloc; pkey_free; pkey_set; pkey_get; pkey_mprotect;
}
+ GLIBC_2.29 {
+ __rseq_abi;
+ __rseq_lib_abi;
+ }
GLIBC_PRIVATE {
# functions used in other libraries
__syscall_rt_sigqueueinfo;
diff --git a/sysdeps/unix/sysv/linux/aarch64/libc.abilist b/sysdeps/unix/sysv/linux/aarch64/libc.abilist
index e66c741d04..8877199b6f 100644
--- a/sysdeps/unix/sysv/linux/aarch64/libc.abilist
+++ b/sysdeps/unix/sysv/linux/aarch64/libc.abilist
@@ -2138,4 +2138,6 @@ GLIBC_2.28 thrd_current F
GLIBC_2.28 thrd_equal F
GLIBC_2.28 thrd_sleep F
GLIBC_2.28 thrd_yield F
+GLIBC_2.29 __rseq_abi T 0x20
+GLIBC_2.29 __rseq_lib_abi T 0x8
GLIBC_2.29 posix_spawn_file_actions_addchdir_np F
diff --git a/sysdeps/unix/sysv/linux/alpha/libc.abilist b/sysdeps/unix/sysv/linux/alpha/libc.abilist
index 8df162fe99..0a0db6c29f 100644
--- a/sysdeps/unix/sysv/linux/alpha/libc.abilist
+++ b/sysdeps/unix/sysv/linux/alpha/libc.abilist
@@ -2033,6 +2033,8 @@ GLIBC_2.28 thrd_current F
GLIBC_2.28 thrd_equal F
GLIBC_2.28 thrd_sleep F
GLIBC_2.28 thrd_yield F
+GLIBC_2.29 __rseq_abi T 0x20
+GLIBC_2.29 __rseq_lib_abi T 0x8
GLIBC_2.29 posix_spawn_file_actions_addchdir_np F
GLIBC_2.3 __ctype_b_loc F
GLIBC_2.3 __ctype_tolower_loc F
diff --git a/sysdeps/unix/sysv/linux/arm/libc.abilist b/sysdeps/unix/sysv/linux/arm/libc.abilist
index 43c804f9dc..65746b1050 100644
--- a/sysdeps/unix/sysv/linux/arm/libc.abilist
+++ b/sysdeps/unix/sysv/linux/arm/libc.abilist
@@ -123,6 +123,8 @@ GLIBC_2.28 thrd_current F
GLIBC_2.28 thrd_equal F
GLIBC_2.28 thrd_sleep F
GLIBC_2.28 thrd_yield F
+GLIBC_2.29 __rseq_abi T 0x20
+GLIBC_2.29 __rseq_lib_abi T 0x8
GLIBC_2.29 posix_spawn_file_actions_addchdir_np F
GLIBC_2.4 _Exit F
GLIBC_2.4 _IO_2_1_stderr_ D 0xa0
diff --git a/sysdeps/unix/sysv/linux/hppa/libc.abilist b/sysdeps/unix/sysv/linux/hppa/libc.abilist
index 88b01c2e75..52c33c08a5 100644
--- a/sysdeps/unix/sysv/linux/hppa/libc.abilist
+++ b/sysdeps/unix/sysv/linux/hppa/libc.abilist
@@ -1880,6 +1880,8 @@ GLIBC_2.28 thrd_current F
GLIBC_2.28 thrd_equal F
GLIBC_2.28 thrd_sleep F
GLIBC_2.28 thrd_yield F
+GLIBC_2.29 __rseq_abi T 0x20
+GLIBC_2.29 __rseq_lib_abi T 0x8
GLIBC_2.29 posix_spawn_file_actions_addchdir_np F
GLIBC_2.3 __ctype_b_loc F
GLIBC_2.3 __ctype_tolower_loc F
diff --git a/sysdeps/unix/sysv/linux/i386/libc.abilist b/sysdeps/unix/sysv/linux/i386/libc.abilist
index 6d02f31612..fe4a3e4f83 100644
--- a/sysdeps/unix/sysv/linux/i386/libc.abilist
+++ b/sysdeps/unix/sysv/linux/i386/libc.abilist
@@ -2045,6 +2045,8 @@ GLIBC_2.28 thrd_current F
GLIBC_2.28 thrd_equal F
GLIBC_2.28 thrd_sleep F
GLIBC_2.28 thrd_yield F
+GLIBC_2.29 __rseq_abi T 0x20
+GLIBC_2.29 __rseq_lib_abi T 0x8
GLIBC_2.29 posix_spawn_file_actions_addchdir_np F
GLIBC_2.3 __ctype_b_loc F
GLIBC_2.3 __ctype_tolower_loc F
diff --git a/sysdeps/unix/sysv/linux/ia64/libc.abilist b/sysdeps/unix/sysv/linux/ia64/libc.abilist
index 4249712611..8db1b3a508 100644
--- a/sysdeps/unix/sysv/linux/ia64/libc.abilist
+++ b/sysdeps/unix/sysv/linux/ia64/libc.abilist
@@ -1914,6 +1914,8 @@ GLIBC_2.28 thrd_current F
GLIBC_2.28 thrd_equal F
GLIBC_2.28 thrd_sleep F
GLIBC_2.28 thrd_yield F
+GLIBC_2.29 __rseq_abi T 0x20
+GLIBC_2.29 __rseq_lib_abi T 0x8
GLIBC_2.29 posix_spawn_file_actions_addchdir_np F
GLIBC_2.3 __ctype_b_loc F
GLIBC_2.3 __ctype_tolower_loc F
diff --git a/sysdeps/unix/sysv/linux/m68k/coldfire/libc.abilist b/sysdeps/unix/sysv/linux/m68k/coldfire/libc.abilist
index d47b808862..d2ee6f238f 100644
--- a/sysdeps/unix/sysv/linux/m68k/coldfire/libc.abilist
+++ b/sysdeps/unix/sysv/linux/m68k/coldfire/libc.abilist
@@ -124,6 +124,8 @@ GLIBC_2.28 thrd_current F
GLIBC_2.28 thrd_equal F
GLIBC_2.28 thrd_sleep F
GLIBC_2.28 thrd_yield F
+GLIBC_2.29 __rseq_abi T 0x20
+GLIBC_2.29 __rseq_lib_abi T 0x8
GLIBC_2.29 posix_spawn_file_actions_addchdir_np F
GLIBC_2.4 _Exit F
GLIBC_2.4 _IO_2_1_stderr_ D 0x98
diff --git a/sysdeps/unix/sysv/linux/m68k/m680x0/libc.abilist b/sysdeps/unix/sysv/linux/m68k/m680x0/libc.abilist
index d5e38308be..3250be752f 100644
--- a/sysdeps/unix/sysv/linux/m68k/m680x0/libc.abilist
+++ b/sysdeps/unix/sysv/linux/m68k/m680x0/libc.abilist
@@ -1989,6 +1989,8 @@ GLIBC_2.28 thrd_current F
GLIBC_2.28 thrd_equal F
GLIBC_2.28 thrd_sleep F
GLIBC_2.28 thrd_yield F
+GLIBC_2.29 __rseq_abi T 0x20
+GLIBC_2.29 __rseq_lib_abi T 0x8
GLIBC_2.29 posix_spawn_file_actions_addchdir_np F
GLIBC_2.3 __ctype_b_loc F
GLIBC_2.3 __ctype_tolower_loc F
diff --git a/sysdeps/unix/sysv/linux/microblaze/libc.abilist b/sysdeps/unix/sysv/linux/microblaze/libc.abilist
index 8596b84399..90507bd369 100644
--- a/sysdeps/unix/sysv/linux/microblaze/libc.abilist
+++ b/sysdeps/unix/sysv/linux/microblaze/libc.abilist
@@ -2130,4 +2130,6 @@ GLIBC_2.28 thrd_current F
GLIBC_2.28 thrd_equal F
GLIBC_2.28 thrd_sleep F
GLIBC_2.28 thrd_yield F
+GLIBC_2.29 __rseq_abi T 0x20
+GLIBC_2.29 __rseq_lib_abi T 0x8
GLIBC_2.29 posix_spawn_file_actions_addchdir_np F
diff --git a/sysdeps/unix/sysv/linux/mips/mips32/fpu/libc.abilist b/sysdeps/unix/sysv/linux/mips/mips32/fpu/libc.abilist
index 88e0f896d5..f5ad6e893f 100644
--- a/sysdeps/unix/sysv/linux/mips/mips32/fpu/libc.abilist
+++ b/sysdeps/unix/sysv/linux/mips/mips32/fpu/libc.abilist
@@ -1967,6 +1967,8 @@ GLIBC_2.28 thrd_current F
GLIBC_2.28 thrd_equal F
GLIBC_2.28 thrd_sleep F
GLIBC_2.28 thrd_yield F
+GLIBC_2.29 __rseq_abi T 0x20
+GLIBC_2.29 __rseq_lib_abi T 0x8
GLIBC_2.29 posix_spawn_file_actions_addchdir_np F
GLIBC_2.3 __ctype_b_loc F
GLIBC_2.3 __ctype_tolower_loc F
diff --git a/sysdeps/unix/sysv/linux/mips/mips32/nofpu/libc.abilist b/sysdeps/unix/sysv/linux/mips/mips32/nofpu/libc.abilist
index aff7462c34..1549be6a0e 100644
--- a/sysdeps/unix/sysv/linux/mips/mips32/nofpu/libc.abilist
+++ b/sysdeps/unix/sysv/linux/mips/mips32/nofpu/libc.abilist
@@ -1965,6 +1965,8 @@ GLIBC_2.28 thrd_current F
GLIBC_2.28 thrd_equal F
GLIBC_2.28 thrd_sleep F
GLIBC_2.28 thrd_yield F
+GLIBC_2.29 __rseq_abi T 0x20
+GLIBC_2.29 __rseq_lib_abi T 0x8
GLIBC_2.29 posix_spawn_file_actions_addchdir_np F
GLIBC_2.3 __ctype_b_loc F
GLIBC_2.3 __ctype_tolower_loc F
diff --git a/sysdeps/unix/sysv/linux/mips/mips64/n32/libc.abilist b/sysdeps/unix/sysv/linux/mips/mips64/n32/libc.abilist
index 71d82444aa..33aea43f05 100644
--- a/sysdeps/unix/sysv/linux/mips/mips64/n32/libc.abilist
+++ b/sysdeps/unix/sysv/linux/mips/mips64/n32/libc.abilist
@@ -1973,6 +1973,8 @@ GLIBC_2.28 thrd_current F
GLIBC_2.28 thrd_equal F
GLIBC_2.28 thrd_sleep F
GLIBC_2.28 thrd_yield F
+GLIBC_2.29 __rseq_abi T 0x20
+GLIBC_2.29 __rseq_lib_abi T 0x8
GLIBC_2.29 posix_spawn_file_actions_addchdir_np F
GLIBC_2.3 __ctype_b_loc F
GLIBC_2.3 __ctype_tolower_loc F
diff --git a/sysdeps/unix/sysv/linux/mips/mips64/n64/libc.abilist b/sysdeps/unix/sysv/linux/mips/mips64/n64/libc.abilist
index de6c53d293..8a43cbde55 100644
--- a/sysdeps/unix/sysv/linux/mips/mips64/n64/libc.abilist
+++ b/sysdeps/unix/sysv/linux/mips/mips64/n64/libc.abilist
@@ -1968,6 +1968,8 @@ GLIBC_2.28 thrd_current F
GLIBC_2.28 thrd_equal F
GLIBC_2.28 thrd_sleep F
GLIBC_2.28 thrd_yield F
+GLIBC_2.29 __rseq_abi T 0x20
+GLIBC_2.29 __rseq_lib_abi T 0x8
GLIBC_2.29 posix_spawn_file_actions_addchdir_np F
GLIBC_2.3 __ctype_b_loc F
GLIBC_2.3 __ctype_tolower_loc F
diff --git a/sysdeps/unix/sysv/linux/nios2/libc.abilist b/sysdeps/unix/sysv/linux/nios2/libc.abilist
index e724bab9fb..7cc6936aa3 100644
--- a/sysdeps/unix/sysv/linux/nios2/libc.abilist
+++ b/sysdeps/unix/sysv/linux/nios2/libc.abilist
@@ -2171,4 +2171,6 @@ GLIBC_2.28 thrd_current F
GLIBC_2.28 thrd_equal F
GLIBC_2.28 thrd_sleep F
GLIBC_2.28 thrd_yield F
+GLIBC_2.29 __rseq_abi T 0x20
+GLIBC_2.29 __rseq_lib_abi T 0x8
GLIBC_2.29 posix_spawn_file_actions_addchdir_np F
diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/libc.abilist b/sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/libc.abilist
index e9ecbccb71..aed1d82ebe 100644
--- a/sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/libc.abilist
+++ b/sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/libc.abilist
@@ -1993,6 +1993,8 @@ GLIBC_2.28 thrd_current F
GLIBC_2.28 thrd_equal F
GLIBC_2.28 thrd_sleep F
GLIBC_2.28 thrd_yield F
+GLIBC_2.29 __rseq_abi T 0x20
+GLIBC_2.29 __rseq_lib_abi T 0x8
GLIBC_2.29 posix_spawn_file_actions_addchdir_np F
GLIBC_2.3 __ctype_b_loc F
GLIBC_2.3 __ctype_tolower_loc F
diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/libc.abilist b/sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/libc.abilist
index da83ea6028..bbd918191c 100644
--- a/sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/libc.abilist
+++ b/sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/libc.abilist
@@ -1997,6 +1997,8 @@ GLIBC_2.28 thrd_current F
GLIBC_2.28 thrd_equal F
GLIBC_2.28 thrd_sleep F
GLIBC_2.28 thrd_yield F
+GLIBC_2.29 __rseq_abi T 0x20
+GLIBC_2.29 __rseq_lib_abi T 0x8
GLIBC_2.29 posix_spawn_file_actions_addchdir_np F
GLIBC_2.3 __ctype_b_loc F
GLIBC_2.3 __ctype_tolower_loc F
diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/libc-le.abilist b/sysdeps/unix/sysv/linux/powerpc/powerpc64/libc-le.abilist
index 4535b40d15..9be7eccefc 100644
--- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/libc-le.abilist
+++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/libc-le.abilist
@@ -2228,4 +2228,6 @@ GLIBC_2.28 thrd_current F
GLIBC_2.28 thrd_equal F
GLIBC_2.28 thrd_sleep F
GLIBC_2.28 thrd_yield F
+GLIBC_2.29 __rseq_abi T 0x20
+GLIBC_2.29 __rseq_lib_abi T 0x8
GLIBC_2.29 posix_spawn_file_actions_addchdir_np F
diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/libc.abilist b/sysdeps/unix/sysv/linux/powerpc/powerpc64/libc.abilist
index 65725de4f0..7c2820c28f 100644
--- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/libc.abilist
+++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/libc.abilist
@@ -123,6 +123,8 @@ GLIBC_2.28 thrd_current F
GLIBC_2.28 thrd_equal F
GLIBC_2.28 thrd_sleep F
GLIBC_2.28 thrd_yield F
+GLIBC_2.29 __rseq_abi T 0x20
+GLIBC_2.29 __rseq_lib_abi T 0x8
GLIBC_2.29 posix_spawn_file_actions_addchdir_np F
GLIBC_2.3 _Exit F
GLIBC_2.3 _IO_2_1_stderr_ D 0xe0
diff --git a/sysdeps/unix/sysv/linux/riscv/rv64/libc.abilist b/sysdeps/unix/sysv/linux/riscv/rv64/libc.abilist
index bbb3c4a8e7..0dbcc7d565 100644
--- a/sysdeps/unix/sysv/linux/riscv/rv64/libc.abilist
+++ b/sysdeps/unix/sysv/linux/riscv/rv64/libc.abilist
@@ -2100,4 +2100,6 @@ GLIBC_2.28 thrd_current F
GLIBC_2.28 thrd_equal F
GLIBC_2.28 thrd_sleep F
GLIBC_2.28 thrd_yield F
+GLIBC_2.29 __rseq_abi T 0x20
+GLIBC_2.29 __rseq_lib_abi T 0x8
GLIBC_2.29 posix_spawn_file_actions_addchdir_np F
diff --git a/sysdeps/unix/sysv/linux/rseq-internal.h b/sysdeps/unix/sysv/linux/rseq-internal.h
new file mode 100644
index 0000000000..cb7b2c6f8c
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/rseq-internal.h
@@ -0,0 +1,91 @@
+/* Copyright (C) 2018 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+ Contributed by Mathieu Desnoyers <mathieu.desnoyers@efficios.com>, 2018.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#ifndef RSEQ_INTERNAL_H
+#define RSEQ_INTERNAL_H
+
+#include <sysdep.h>
+
+#ifdef __NR_rseq
+
+#include <errno.h>
+#include <sys/rseq.h>
+
+static inline int
+rseq_register_current_thread (void)
+{
+ int rc, ret = 0;
+ INTERNAL_SYSCALL_DECL (err);
+
+ if (__rseq_abi.cpu_id == RSEQ_CPU_ID_REGISTRATION_FAILED)
+ return -1;
+ /* Temporarily prevent nested signal handlers from registering rseq. */
+ __rseq_lib_abi.register_state = RSEQ_REGISTER_NESTED;
+ if (__rseq_lib_abi.refcount == UINT_MAX)
+ {
+ ret = -1;
+ goto end;
+ }
+ if (__rseq_lib_abi.refcount++)
+ goto end;
+ rc = INTERNAL_SYSCALL_CALL (rseq, err, &__rseq_abi, sizeof (struct rseq),
+ 0, RSEQ_SIG);
+ if (!rc)
+ goto end;
+ if (INTERNAL_SYSCALL_ERRNO (rc, err) != EBUSY)
+ __rseq_abi.cpu_id = RSEQ_CPU_ID_REGISTRATION_FAILED;
+ ret = -1;
+end:
+ __rseq_lib_abi.register_state = RSEQ_REGISTER_ALLOWED;
+ return ret;
+}
+
+static inline int
+rseq_unregister_current_thread (void)
+{
+ int rc, ret = 0;
+ INTERNAL_SYSCALL_DECL (err);
+
+ /* Setting __rseq_register_state = RSEQ_REGISTER_EXITING for the rest of the
+ thread lifetime. Ensures signal handlers nesting just before thread exit
+ don't try to register rseq. */
+ __rseq_lib_abi.register_state = RSEQ_REGISTER_EXITING;
+ __rseq_lib_abi.refcount = 0;
+ rc = INTERNAL_SYSCALL_CALL (rseq, err, &__rseq_abi, sizeof (struct rseq),
+ RSEQ_FLAG_UNREGISTER, RSEQ_SIG);
+ if (!rc)
+ goto end;
+ ret = -1;
+end:
+ return ret;
+}
+#else
+static inline int
+rseq_register_current_thread (void)
+{
+ return -1;
+}
+
+static inline int
+rseq_unregister_current_thread (void)
+{
+ return -1;
+}
+#endif
+
+#endif /* rseq-internal.h */
diff --git a/sysdeps/unix/sysv/linux/rseq-sym.c b/sysdeps/unix/sysv/linux/rseq-sym.c
new file mode 100644
index 0000000000..99b277e9d6
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/rseq-sym.c
@@ -0,0 +1,54 @@
+/* Copyright (C) 2018 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+ Contributed by Mathieu Desnoyers <mathieu.desnoyers@efficios.com>, 2018.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#include <sys/syscall.h>
+#include <stdint.h>
+
+#ifdef __NR_rseq
+#include <sys/rseq.h>
+#else
+
+enum rseq_cpu_id_state {
+ RSEQ_CPU_ID_UNINITIALIZED = -1,
+ RSEQ_CPU_ID_REGISTRATION_FAILED = -2,
+};
+
+/* linux/rseq.h defines struct rseq as aligned on 32 bytes. The kernel ABI
+ size is 20 bytes. */
+struct rseq {
+ uint32_t cpu_id_start;
+ uint32_t cpu_id;
+ uint64_t rseq_cs;
+ uint32_t flags;
+} __attribute__ ((aligned(4 * sizeof(uint64_t))));
+
+struct rseq_lib_abi
+{
+ uint32_t register_state;
+ uint32_t refcount;
+};
+
+#endif
+
+/* volatile because fields can be read/updated by the kernel. */
+__thread volatile struct rseq __rseq_abi = {
+ .cpu_id = RSEQ_CPU_ID_UNINITIALIZED,
+};
+
+/* volatile because fields can be read/updated by signal handlers. */
+__thread volatile struct rseq_lib_abi __rseq_lib_abi;
diff --git a/sysdeps/unix/sysv/linux/s390/s390-32/libc.abilist b/sysdeps/unix/sysv/linux/s390/s390-32/libc.abilist
index e85ac2a178..eff957032a 100644
--- a/sysdeps/unix/sysv/linux/s390/s390-32/libc.abilist
+++ b/sysdeps/unix/sysv/linux/s390/s390-32/libc.abilist
@@ -2002,6 +2002,8 @@ GLIBC_2.28 thrd_current F
GLIBC_2.28 thrd_equal F
GLIBC_2.28 thrd_sleep F
GLIBC_2.28 thrd_yield F
+GLIBC_2.29 __rseq_abi T 0x20
+GLIBC_2.29 __rseq_lib_abi T 0x8
GLIBC_2.29 posix_spawn_file_actions_addchdir_np F
GLIBC_2.3 __ctype_b_loc F
GLIBC_2.3 __ctype_tolower_loc F
diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/libc.abilist b/sysdeps/unix/sysv/linux/s390/s390-64/libc.abilist
index d56931022c..32bfb25c1f 100644
--- a/sysdeps/unix/sysv/linux/s390/s390-64/libc.abilist
+++ b/sysdeps/unix/sysv/linux/s390/s390-64/libc.abilist
@@ -1908,6 +1908,8 @@ GLIBC_2.28 thrd_equal F
GLIBC_2.28 thrd_sleep F
GLIBC_2.28 thrd_yield F
GLIBC_2.29 __fentry__ F
+GLIBC_2.29 __rseq_abi T 0x20
+GLIBC_2.29 __rseq_lib_abi T 0x8
GLIBC_2.29 posix_spawn_file_actions_addchdir_np F
GLIBC_2.3 __ctype_b_loc F
GLIBC_2.3 __ctype_tolower_loc F
diff --git a/sysdeps/unix/sysv/linux/sh/libc.abilist b/sysdeps/unix/sysv/linux/sh/libc.abilist
index ff939a15c4..a12d738d86 100644
--- a/sysdeps/unix/sysv/linux/sh/libc.abilist
+++ b/sysdeps/unix/sysv/linux/sh/libc.abilist
@@ -1884,6 +1884,8 @@ GLIBC_2.28 thrd_current F
GLIBC_2.28 thrd_equal F
GLIBC_2.28 thrd_sleep F
GLIBC_2.28 thrd_yield F
+GLIBC_2.29 __rseq_abi T 0x20
+GLIBC_2.29 __rseq_lib_abi T 0x8
GLIBC_2.29 posix_spawn_file_actions_addchdir_np F
GLIBC_2.3 __ctype_b_loc F
GLIBC_2.3 __ctype_tolower_loc F
diff --git a/sysdeps/unix/sysv/linux/sparc/sparc32/libc.abilist b/sysdeps/unix/sysv/linux/sparc/sparc32/libc.abilist
index 64fa9e10a5..3333634dfa 100644
--- a/sysdeps/unix/sysv/linux/sparc/sparc32/libc.abilist
+++ b/sysdeps/unix/sysv/linux/sparc/sparc32/libc.abilist
@@ -1996,6 +1996,8 @@ GLIBC_2.28 thrd_current F
GLIBC_2.28 thrd_equal F
GLIBC_2.28 thrd_sleep F
GLIBC_2.28 thrd_yield F
+GLIBC_2.29 __rseq_abi T 0x20
+GLIBC_2.29 __rseq_lib_abi T 0x8
GLIBC_2.29 posix_spawn_file_actions_addchdir_np F
GLIBC_2.3 __ctype_b_loc F
GLIBC_2.3 __ctype_tolower_loc F
diff --git a/sysdeps/unix/sysv/linux/sparc/sparc64/libc.abilist b/sysdeps/unix/sysv/linux/sparc/sparc64/libc.abilist
index db909d1506..02db9f6669 100644
--- a/sysdeps/unix/sysv/linux/sparc/sparc64/libc.abilist
+++ b/sysdeps/unix/sysv/linux/sparc/sparc64/libc.abilist
@@ -1937,6 +1937,8 @@ GLIBC_2.28 thrd_current F
GLIBC_2.28 thrd_equal F
GLIBC_2.28 thrd_sleep F
GLIBC_2.28 thrd_yield F
+GLIBC_2.29 __rseq_abi T 0x20
+GLIBC_2.29 __rseq_lib_abi T 0x8
GLIBC_2.29 posix_spawn_file_actions_addchdir_np F
GLIBC_2.3 __ctype_b_loc F
GLIBC_2.3 __ctype_tolower_loc F
diff --git a/sysdeps/unix/sysv/linux/sys/rseq.h b/sysdeps/unix/sysv/linux/sys/rseq.h
new file mode 100644
index 0000000000..61937fb193
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/sys/rseq.h
@@ -0,0 +1,64 @@
+/* Copyright (C) 2019 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+ Contributed by Mathieu Desnoyers <mathieu.desnoyers@efficios.com>, 2019.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#ifndef _SYS_RSEQ_H
+#define _SYS_RSEQ_H 1
+
+/* We use the structures declarations from the kernel headers. */
+#include <linux/rseq.h>
+#include <stdint.h>
+
+/* Signature required before each abort handler code. */
+#define RSEQ_SIG 0x53053053
+
+enum rseq_register_state
+{
+ /* Value RSEQ_REGISTER_ALLOWED means it is allowed to update
+ the refcount field and to register/unregister rseq. */
+ RSEQ_REGISTER_ALLOWED = 0,
+ /* Value RSEQ_REGISTER_NESTED means it is temporarily forbidden
+ to update the refcount field or to register/unregister rseq. */
+ RSEQ_REGISTER_NESTED = 1,
+ /* Value RSEQ_REGISTER_EXITING means it is forbidden to update the
+ refcount field or to register/unregister rseq for the rest of the
+ thread's lifetime. */
+ RSEQ_REGISTER_EXITING = 2,
+};
+
+struct rseq_lib_abi
+{
+ uint32_t register_state; /* enum rseq_register_state. */
+ /* The refcount field keeps track of rseq users, so early adopters
+ of rseq can cooperate amongst each other and with glibc to
+ share rseq thread registration. The refcount field can only be
+ updated when allowed by the value of field register_state.
+ Registering rseq should be performed when incrementing refcount
+ from 0 to 1, and unregistering rseq should be performed when
+ decrementing refcount from 1 to 0. */
+ uint32_t refcount;
+};
+
+/* volatile because fields can be read/updated by the kernel. */
+extern __thread volatile struct rseq __rseq_abi
+__attribute__ ((tls_model ("initial-exec")));
+
+/* volatile because fields can be read/updated by signal handlers. */
+extern __thread volatile struct rseq_lib_abi __rseq_lib_abi
+__attribute__ ((tls_model ("initial-exec")));
+
+#endif /* sys/rseq.h */
diff --git a/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist b/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist
index 3b175f104b..417d8ab9a6 100644
--- a/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist
+++ b/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist
@@ -1895,6 +1895,8 @@ GLIBC_2.28 thrd_current F
GLIBC_2.28 thrd_equal F
GLIBC_2.28 thrd_sleep F
GLIBC_2.28 thrd_yield F
+GLIBC_2.29 __rseq_abi T 0x20
+GLIBC_2.29 __rseq_lib_abi T 0x8
GLIBC_2.29 posix_spawn_file_actions_addchdir_np F
GLIBC_2.3 __ctype_b_loc F
GLIBC_2.3 __ctype_tolower_loc F
diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/libc.abilist b/sysdeps/unix/sysv/linux/x86_64/x32/libc.abilist
index 1b57710477..ef5ad4160d 100644
--- a/sysdeps/unix/sysv/linux/x86_64/x32/libc.abilist
+++ b/sysdeps/unix/sysv/linux/x86_64/x32/libc.abilist
@@ -2146,4 +2146,6 @@ GLIBC_2.28 thrd_current F
GLIBC_2.28 thrd_equal F
GLIBC_2.28 thrd_sleep F
GLIBC_2.28 thrd_yield F
+GLIBC_2.29 __rseq_abi T 0x20
+GLIBC_2.29 __rseq_lib_abi T 0x8
GLIBC_2.29 posix_spawn_file_actions_addchdir_np F
--
2.17.1
^ permalink raw reply related
* [RFC PATCH glibc 2/4] glibc: sched_getcpu(): use rseq cpu_id TLS on Linux
From: Mathieu Desnoyers @ 2019-01-21 21:35 UTC (permalink / raw)
To: Carlos O'Donell
Cc: Florian Weimer, Joseph Myers, Szabolcs Nagy, libc-alpha,
Mathieu Desnoyers, Thomas Gleixner, Ben Maurer, Peter Zijlstra,
Paul E. McKenney, Boqun Feng, Will Deacon, Dave Watson,
Paul Turner, linux-kernel, linux-api
In-Reply-To: <20190121213530.23803-1-mathieu.desnoyers@efficios.com>
When available, use the cpu_id field from __rseq_abi on Linux to
implement sched_getcpu(). Fall-back on the vgetcpu vDSO if unavailable.
Benchmarks:
x86-64: Intel E5-2630 v3@2.40GHz, 16-core, hyperthreading
glibc sched_getcpu(): 13.7 ns (baseline)
glibc sched_getcpu() using rseq: 2.5 ns (speedup: 5.5x)
inline load cpuid from __rseq_abi TLS: 0.8 ns (speedup: 17.1x)
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
CC: Carlos O'Donell <carlos@redhat.com>
CC: Florian Weimer <fweimer@redhat.com>
CC: Joseph Myers <joseph@codesourcery.com>
CC: Szabolcs Nagy <szabolcs.nagy@arm.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ben Maurer <bmaurer@fb.com>
CC: Peter Zijlstra <peterz@infradead.org>
CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
CC: Boqun Feng <boqun.feng@gmail.com>
CC: Will Deacon <will.deacon@arm.com>
CC: Dave Watson <davejwatson@fb.com>
CC: Paul Turner <pjt@google.com>
CC: libc-alpha@sourceware.org
CC: linux-kernel@vger.kernel.org
CC: linux-api@vger.kernel.org
---
sysdeps/unix/sysv/linux/sched_getcpu.c | 25 +++++++++++++++++++++++--
1 file changed, 23 insertions(+), 2 deletions(-)
diff --git a/sysdeps/unix/sysv/linux/sched_getcpu.c b/sysdeps/unix/sysv/linux/sched_getcpu.c
index b69eeda15c..37fda59d36 100644
--- a/sysdeps/unix/sysv/linux/sched_getcpu.c
+++ b/sysdeps/unix/sysv/linux/sched_getcpu.c
@@ -24,8 +24,8 @@
#endif
#include <sysdep-vdso.h>
-int
-sched_getcpu (void)
+static int
+vsyscall_sched_getcpu (void)
{
#ifdef __NR_getcpu
unsigned int cpu;
@@ -37,3 +37,24 @@ sched_getcpu (void)
return -1;
#endif
}
+
+#ifdef __NR_rseq
+#include <linux/rseq.h>
+
+extern __attribute__ ((tls_model ("initial-exec")))
+__thread volatile struct rseq __rseq_abi;
+
+int
+sched_getcpu (void)
+{
+ int cpu_id = __rseq_abi.cpu_id;
+
+ return cpu_id >= 0 ? cpu_id : vsyscall_sched_getcpu ();
+}
+#else
+int
+sched_getcpu (void)
+{
+ return vsyscall_sched_getcpu ();
+}
+#endif
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v4 1/3] fs: hoist EFSCORRUPTED definition into uapi header
From: Theodore Y. Ts'o @ 2019-01-21 21:54 UTC (permalink / raw)
To: Jann Horn
Cc: Richard Henderson, Ivan Kokshaysky, Matt Turner, Alexander Viro,
linux-fsdevel, Arnd Bergmann, Eric W. Biederman, Andreas Dilger,
linux-alpha, linux-kernel, Dave Chinner, Pavel Machek, linux-arch,
linux-api
In-Reply-To: <20190118161440.220134-1-jannh@google.com>
On Fri, Jan 18, 2019 at 05:14:38PM +0100, Jann Horn wrote:
> Multiple filesystems can already return EFSCORRUPTED errors to userspace;
> however, so far, definitions of EFSCORRUPTED were in filesystem-private
> headers.
>
> I wanted to use EUCLEAN to indicate data corruption in the VFS layer;
> Dave Chinner says that I should instead hoist the definitions of
> EFSCORRUPTED into the UAPI header and then use EFSCORRUPTED.
>
> This patch is marked for stable backport because it is a prerequisite for
> the following patch.
>
> Cc: stable@vger.kernel.org
> Suggested-by: Dave Chinner <david@fromorbit.com>
> Signed-off-by: Jann Horn <jannh@google.com>
Before we enshrine the overloading of EUCLEAN and EFSCORRUPTED, I
wonder if we should at least consider the option of assigning a new
error code number for EFSCORRUPTED. The downside of doing this is
that for a while, older versions glibc won't have strerror/perror
translation for the new error code. On the other hand, I'm not sure
it will be that much more confusing to the average user than
"Structure needs cleaning". :-)
The upside of assigning a new error code is that in a year or two,
we'll actually have an intelligible error message showing up in log
files and in user's terminals.
- Ted
^ permalink raw reply
* Re: [PATCH v4 1/3] fs: hoist EFSCORRUPTED definition into uapi header
From: Dave Chinner @ 2019-01-21 22:13 UTC (permalink / raw)
To: Theodore Y. Ts'o, Jann Horn, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Alexander Viro, linux-fsdevel,
Arnd Bergmann, Eric W. Biederman, Andreas Dilger, linux-alpha,
linux-kernel, Pavel Machek, linux-arch, linux-api
In-Reply-To: <20190121215454.GA12996@mit.edu>
On Mon, Jan 21, 2019 at 04:54:54PM -0500, Theodore Y. Ts'o wrote:
> On Fri, Jan 18, 2019 at 05:14:38PM +0100, Jann Horn wrote:
> > Multiple filesystems can already return EFSCORRUPTED errors to userspace;
> > however, so far, definitions of EFSCORRUPTED were in filesystem-private
> > headers.
> >
> > I wanted to use EUCLEAN to indicate data corruption in the VFS layer;
> > Dave Chinner says that I should instead hoist the definitions of
> > EFSCORRUPTED into the UAPI header and then use EFSCORRUPTED.
> >
> > This patch is marked for stable backport because it is a prerequisite for
> > the following patch.
> >
> > Cc: stable@vger.kernel.org
> > Suggested-by: Dave Chinner <david@fromorbit.com>
> > Signed-off-by: Jann Horn <jannh@google.com>
>
> Before we enshrine the overloading of EUCLEAN and EFSCORRUPTED, I
> wonder if we should at least consider the option of assigning a new
> error code number for EFSCORRUPTED.
No.
We've exposed filesystem corruption errors to userspace as errno 117
for many years now, so people are already familiar with this error
as indicating a filesystem problem.
> The downside of doing this is
> that for a while, older versions glibc won't have strerror/perror
> translation for the new error code.
And everyone will end up asking "WTF is this undefined error the
application just received?" until glibc, man pages and enough
occurrences of the question have been asked that the search engines
develop enough of a history record that they return useful results.
Not only won't users have a clue, but the app developers the users
first ask "what's this error mean" won't have a clue, either.
> On the other hand, I'm not sure
> it will be that much more confusing to the average user than
> "Structure needs cleaning". :-)
Go search for "XFS structure needs cleaning" on your preferred
search engine and will you get lots and lots of hits indicating what
you should do when you get that error. It's taken years to build up
that history such that it's extremely useful to the average user....
> The upside of assigning a new error code is that in a year or two,
> we'll actually have an intelligible error message showing up in log
> files and in user's terminals.
The downside is that it will take several years before people will
become familiar with the new error, and we'll have to deal with the
fallout repeatedly from it. Hence, IMO, there's no upside to
changing the errno of EFSCORRUPTED now that it is largely ubiquitous
in userspace.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply
* Re: [PATCH v4 1/3] fs: hoist EFSCORRUPTED definition into uapi header
From: David Sterba @ 2019-01-21 22:14 UTC (permalink / raw)
To: Theodore Y. Ts'o, Jann Horn, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Alexander Viro, linux-fsdevel,
Arnd Bergmann, Eric W. Biederman, Andreas Dilger, linux-alpha,
linux-kernel, Dave Chinner, Pavel Machek, linux-arch, linux-api
In-Reply-To: <20190121215454.GA12996@mit.edu>
On Mon, Jan 21, 2019 at 04:54:54PM -0500, Theodore Y. Ts'o wrote:
> On Fri, Jan 18, 2019 at 05:14:38PM +0100, Jann Horn wrote:
> > Multiple filesystems can already return EFSCORRUPTED errors to userspace;
> > however, so far, definitions of EFSCORRUPTED were in filesystem-private
> > headers.
> >
> > I wanted to use EUCLEAN to indicate data corruption in the VFS layer;
> > Dave Chinner says that I should instead hoist the definitions of
> > EFSCORRUPTED into the UAPI header and then use EFSCORRUPTED.
> >
> > This patch is marked for stable backport because it is a prerequisite for
> > the following patch.
> >
> > Cc: stable@vger.kernel.org
> > Suggested-by: Dave Chinner <david@fromorbit.com>
> > Signed-off-by: Jann Horn <jannh@google.com>
>
> Before we enshrine the overloading of EUCLEAN and EFSCORRUPTED, I
> wonder if we should at least consider the option of assigning a new
> error code number for EFSCORRUPTED. The downside of doing this is
> that for a while, older versions glibc won't have strerror/perror
> translation for the new error code. On the other hand, I'm not sure
> it will be that much more confusing to the average user than
> "Structure needs cleaning". :-)
>
> The upside of assigning a new error code is that in a year or two,
> we'll actually have an intelligible error message showing up in log
> files and in user's terminals.
I vote for a new code with a better message than EUCLEAN provides.
^ permalink raw reply
* Re: [PATCH v4 3/3] fs: let filldir_t return bool instead of an error code
From: Dave Chinner @ 2019-01-21 22:24 UTC (permalink / raw)
To: Jann Horn
Cc: Richard Henderson, Ivan Kokshaysky, Matt Turner, Alexander Viro,
linux-fsdevel, Arnd Bergmann, Eric W. Biederman,
Theodore Ts'o, Andreas Dilger, linux-alpha, kernel list,
Pavel Machek, linux-arch, Linux API
In-Reply-To: <CAG48ez3SgejgGbctHwxON1B1k26xk-z16xdozKZkx7=-Li0aHw@mail.gmail.com>
On Mon, Jan 21, 2019 at 04:49:45PM +0100, Jann Horn wrote:
> On Sun, Jan 20, 2019 at 11:41 PM Dave Chinner <david@fromorbit.com> wrote:
> > On Fri, Jan 18, 2019 at 05:14:40PM +0100, Jann Horn wrote:
> > > As Al Viro pointed out, many filldir_t functions return error codes, but
> > > all callers of filldir_t functions just check whether the return value is
> > > non-zero (to determine whether to continue reading the directory); more
> > > precise errors have to be signalled via struct dir_context.
> > > Change all filldir_t functions to return bool instead of int.
> > >
> > > Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> > > Signed-off-by: Jann Horn <jannh@google.com>
> > > ---
> > > arch/alpha/kernel/osf_sys.c | 12 +++----
> > > fs/afs/dir.c | 30 +++++++++--------
> > > fs/ecryptfs/file.c | 13 ++++----
> > > fs/exportfs/expfs.c | 8 ++---
> > > fs/fat/dir.c | 8 ++---
> > > fs/gfs2/export.c | 6 ++--
> > > fs/nfsd/nfs4recover.c | 8 ++---
> > > fs/nfsd/vfs.c | 6 ++--
> > > fs/ocfs2/dir.c | 10 +++---
> > > fs/ocfs2/journal.c | 14 ++++----
> > > fs/overlayfs/readdir.c | 24 +++++++-------
> > > fs/readdir.c | 64 ++++++++++++++++++-------------------
> > > fs/reiserfs/xattr.c | 20 ++++++------
> > > fs/xfs/scrub/dir.c | 8 ++---
> > > fs/xfs/scrub/parent.c | 4 +--
> > > include/linux/fs.h | 10 +++---
> > > 16 files changed, 125 insertions(+), 120 deletions(-)
> > >
> > > diff --git a/arch/alpha/kernel/osf_sys.c b/arch/alpha/kernel/osf_sys.c
> > > index db1c2144d477..14e5ae0dac50 100644
> > > --- a/arch/alpha/kernel/osf_sys.c
> > > +++ b/arch/alpha/kernel/osf_sys.c
> > > @@ -108,7 +108,7 @@ struct osf_dirent_callback {
> > > int error;
> > > };
> > >
> > > -static int
> > > +static bool
> > > osf_filldir(struct dir_context *ctx, const char *name, int namlen,
> > > loff_t offset, u64 ino, unsigned int d_type)
> > > {
> > > @@ -120,14 +120,14 @@ osf_filldir(struct dir_context *ctx, const char *name, int namlen,
> > >
> > > buf->error = check_dirent_name(name, namlen);
> > > if (unlikely(buf->error))
> > > - return -EFSCORRUPTED;
> > > + return false;
> > > buf->error = -EINVAL; /* only used if we fail */
> > > if (reclen > buf->count)
> > > - return -EINVAL;
> > > + return false;
> >
> > Oh, it's because the error being returned is being squashed by
> > dir_emit():
>
> Yeah.
>
> > > struct dir_context {
> > > @@ -3469,17 +3471,17 @@ static inline bool dir_emit(struct dir_context *ctx,
> > > const char *name, int namelen,
> > > u64 ino, unsigned type)
> > > {
> > > - return ctx->actor(ctx, name, namelen, ctx->pos, ino, type) == 0;
> > > + return ctx->actor(ctx, name, namelen, ctx->pos, ino, type);
> > > }
> >
> > /me wonders if it would be cleaner to do:
> >
> > static inline bool dir_emit(...)
> > {
> > buf->error = ctx->actor(....)
> > if (buf->error)
> > return false;
> > return true;
> > }
> >
> > And clean up all filldir actors just to return the error state
> > rather than have to jump through hoops to stash the error state in
> > the context buffer and return the error state?
>
> One negative thing about that, IMO, is that it mixes up the request
> for termination of the loop and the presence of an error.
Doesn't the code already do that, only worse?
> > That then allows callers who want/need the full error info can
> > continue to call ctx->actor directly,
>
> "continue to call ctx->actor directly"? I don't remember any code that
> calls ctx->actor directly.
ovl_fill_real().
And the XFS directory scrubber could probably make better use of the
error return from ctx->actor when validating the directory contents
rather than just calling dir_emit() and aborting the scan at the
first error encountered. We eventually want to know exactly what
error was encountered here to determine if it is safe to continue,
not just a "stop processing" flag. e.g. a bad name length will need
to stop traversal because we can't trust the underlying structure,
but an invalid file type isn't a structural flaw that prevents us
from continuing to traverse and check the rest of the directory....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply
* Re: [PATCH v4 1/3] fs: hoist EFSCORRUPTED definition into uapi header
From: Darrick J. Wong @ 2019-01-21 23:51 UTC (permalink / raw)
To: Theodore Y. Ts'o, Jann Horn, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Alexander Viro, linux-fsdevel,
Arnd Bergmann, Eric W. Biederman, Andreas Dilger, linux-alpha,
linux-kernel, Dave Chinner, Pavel Machek, linux-arch, linux-api
In-Reply-To: <20190121215454.GA12996@mit.edu>
On Mon, Jan 21, 2019 at 04:54:54PM -0500, Theodore Y. Ts'o wrote:
> On Fri, Jan 18, 2019 at 05:14:38PM +0100, Jann Horn wrote:
> > Multiple filesystems can already return EFSCORRUPTED errors to userspace;
> > however, so far, definitions of EFSCORRUPTED were in filesystem-private
> > headers.
> >
> > I wanted to use EUCLEAN to indicate data corruption in the VFS layer;
> > Dave Chinner says that I should instead hoist the definitions of
> > EFSCORRUPTED into the UAPI header and then use EFSCORRUPTED.
> >
> > This patch is marked for stable backport because it is a prerequisite for
> > the following patch.
> >
> > Cc: stable@vger.kernel.org
> > Suggested-by: Dave Chinner <david@fromorbit.com>
> > Signed-off-by: Jann Horn <jannh@google.com>
>
> Before we enshrine the overloading of EUCLEAN and EFSCORRUPTED, I
> wonder if we should at least consider the option of assigning a new
> error code number for EFSCORRUPTED. The downside of doing this is
> that for a while, older versions glibc won't have strerror/perror
> translation for the new error code. On the other hand, I'm not sure
> it will be that much more confusing to the average user than
> "Structure needs cleaning". :-)
>
> The upside of assigning a new error code is that in a year or two,
> we'll actually have an intelligible error message showing up in log
> files and in user's terminals.
Uh... Ted? Back in ~2009 we had a discussion on the ext4 conference
call about what error codes to return for "metadata is garbage" and
"metadata crc doesn't validate". Back then you said that it would have
been great if someone had thought of defining error codes for that so
that by the time we got around to merging metadata checksums for ext4,
we'd have some error codes ready to go.
I pointed out that "the XFS people" already returned EUCLEAN /
EFSCORRUPTED and EILSEQ / EBADFSCRC for those cases and that upstream
had been doing that for a couple of years already, so we decided that
we'd just make ext4 behave like XFS because they'd already started
training everyone and their pet search engines that these oddly phrased
error messages in the context of a filesystem means they need to run
some sort of fsck/repair tool. We also decided that while the messaging
was weird, it would be less work for both of us than to try to push
disruptive changes through Linux uapi, glibc, man pages, strerror
localization catalogs, etc.
Now it's a *decade* later, and ext4 / XFS have both converged on more or
less the same behavioral patterns w.r.t. when they return EFSCORRUPTED
and EFSBADCRC. btrfs, hpfs, jffs2, and ubifs seem to use EUCLEAN to
signal bad metadata just like XFS and ext4 do.
I disagree with upending 13 years of established precedent for user
visible behavior. We possibly could've pulled this off ten years ago,
but it's waaaay too late now. Too much work, too little gain.
--D
> - Ted
^ permalink raw reply
* Re: [PATCH v4 1/3] fs: hoist EFSCORRUPTED definition into uapi header
From: Theodore Y. Ts'o @ 2019-01-22 0:38 UTC (permalink / raw)
To: Darrick J. Wong
Cc: Jann Horn, Richard Henderson, Ivan Kokshaysky, Matt Turner,
Alexander Viro, linux-fsdevel, Arnd Bergmann, Eric W. Biederman,
Andreas Dilger, linux-alpha, linux-kernel, Dave Chinner,
Pavel Machek, linux-arch, linux-api
In-Reply-To: <20190121235158.GA4363@magnolia>
On Mon, Jan 21, 2019 at 03:51:58PM -0800, Darrick J. Wong wrote:
>
> I disagree with upending 13 years of established precedent for user
> visible behavior. We possibly could've pulled this off ten years ago,
> but it's waaaay too late now. Too much work, too little gain.
I remember the discussion; but now that we're adding it to uapi header
files, it's really going to be impossible. And I have had some
regrets about that decision ten years ago. I agree it would cause
confusion if we do it now, but it's basically the our last
opportunity.
How about this then? We could ask glibc to change the string returned
by strerror for EUCLEAN/EFSCORRUPTED to be something like "File system
or block device corrupted". This is how the errno is used in the
kernel; and if we don't want to change the error code, changing the
string returned by glibc should be less problematic.
- Ted
^ permalink raw reply
* Re: [PATCH v6 05/16] sched/core: uclamp: Update CPU's refcount on clamp changes
From: Peter Zijlstra @ 2019-01-22 9:37 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190121154412.fak2t2iquj3aixtu@e110439-lin>
On Mon, Jan 21, 2019 at 03:44:12PM +0000, Patrick Bellasi wrote:
> On 21-Jan 16:33, Peter Zijlstra wrote:
> > On Tue, Jan 15, 2019 at 10:15:02AM +0000, Patrick Bellasi wrote:
> >
> > > +static inline void
> > > +uclamp_task_update_active(struct task_struct *p, unsigned int clamp_id)
> > > +{
> > > + struct rq_flags rf;
> > > + struct rq *rq;
> > > +
> > > + /*
> > > + * Lock the task and the CPU where the task is (or was) queued.
> > > + *
> > > + * We might lock the (previous) rq of a !RUNNABLE task, but that's the
> > > + * price to pay to safely serialize util_{min,max} updates with
> > > + * enqueues, dequeues and migration operations.
> > > + * This is the same locking schema used by __set_cpus_allowed_ptr().
> > > + */
> > > + rq = task_rq_lock(p, &rf);
> > > +
> > > + /*
> > > + * Setting the clamp bucket is serialized by task_rq_lock().
> > > + * If the task is not yet RUNNABLE and its task_struct is not
> > > + * affecting a valid clamp bucket, the next time it's enqueued,
> > > + * it will already see the updated clamp bucket value.
> > > + */
> > > + if (!p->uclamp[clamp_id].active)
> > > + goto done;
> > > +
> > > + uclamp_cpu_dec_id(p, rq, clamp_id);
> > > + uclamp_cpu_inc_id(p, rq, clamp_id);
> > > +
> > > +done:
> > > + task_rq_unlock(rq, p, &rf);
> > > +}
> >
> > > @@ -1008,11 +1043,11 @@ static int __setscheduler_uclamp(struct task_struct *p,
> > >
> > > mutex_lock(&uclamp_mutex);
> > > if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_MIN) {
> > > - uclamp_bucket_inc(&p->uclamp[UCLAMP_MIN],
> > > + uclamp_bucket_inc(p, &p->uclamp[UCLAMP_MIN],
> > > UCLAMP_MIN, lower_bound);
> > > }
> > > if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_MAX) {
> > > - uclamp_bucket_inc(&p->uclamp[UCLAMP_MAX],
> > > + uclamp_bucket_inc(p, &p->uclamp[UCLAMP_MAX],
> > > UCLAMP_MAX, upper_bound);
> > > }
> > > mutex_unlock(&uclamp_mutex);
> >
> >
> > But.... __sched_setscheduler() actually does the whole dequeue + enqueue
> > thing already ?!? See where it does __setscheduler().
>
> This is slow-path accounting, not fast path.
Sure; but that's still no reason for duplicate or unneeded code.
> There are two refcounting going on here:
>
> 1) mapped buckets:
>
> clamp_value <--(M1)--> bucket_id
>
> 2) RUNNABLE tasks:
>
> bucket_id <--(M2)--> RUNNABLE tasks in a bucket
>
> What we fix here is the refcounting for the buckets mapping. If a task
> does not have a task specific clamp value it does not refcount any
> bucket. The moment we assign a task specific clamp value, we need to
> refcount the task in the bucket corresponding to that clamp value.
>
> This will keep the bucket in use at least as long as the task will
> need that clamp value.
Sure, I get that. What I don't get is why you're adding that (2) here.
Like said, __sched_setscheduler() already does a dequeue/enqueue under
rq->lock, which should already take care of that.
^ permalink raw reply
* Re: [PATCH v2 29/29] y2038: add 64-bit time_t syscalls to all 32-bit architectures
From: Arnd Bergmann @ 2019-01-22 9:37 UTC (permalink / raw)
To: Andy Lutomirski
Cc: y2038 Mailman List, Linux API, LKML, linux-arch, Thomas Gleixner,
Ingo Molnar, H. Peter Anvin, X86 ML, alpha
In-Reply-To: <CALCETrXqM5mhvwreN5y-9K99h1j9rs9MAVK-cNLC54s1fdHA6w@mail.gmail.com>
On Fri, Jan 18, 2019 at 7:50 PM Andy Lutomirski <luto@kernel.org> wrote:
> On Fri, Jan 18, 2019 at 8:25 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> I have a patch that I'll send soon to make x32 use its own table. As
> far as I'm concerned, 547 is *it*. 548 is just a normal number and is
> not special. But let's please not reuse 512..547 for other purposes
> on x86 variants -- that way lies even more confusion, IMO.
(trimming Cc list, as this is getting a little off-topic most most)
Just so I understand: do you mean duplicating the .tbl file, or just
the resulting table of entry points?
In either way, how will that work with the new io_uring_setup()
system call that will have to use the compat entry point?
Are you planning to use the same syscall number as x86_64
but point it to the compat function, or do we still need a new
syscall number for x32 in the regular range?
Arnd
^ permalink raw reply
* Re: [PATCH v6 04/16] sched/core: uclamp: Add CPU's clamp buckets refcounting
From: Peter Zijlstra @ 2019-01-22 9:45 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190121163337.6l7hkggicndtpzjs@e110439-lin>
On Mon, Jan 21, 2019 at 04:33:38PM +0000, Patrick Bellasi wrote:
> On 21-Jan 17:12, Peter Zijlstra wrote:
> > On Mon, Jan 21, 2019 at 03:23:11PM +0000, Patrick Bellasi wrote:
> > > and keep all
> > > the buckets in use at the beginning of a cache line.
> >
> > That; is that the rationale for all this? Note that per the defaults
> > everything is in a single line already.
>
> Yes, that's because of the loop in:
>
> dequeue_task()
> uclamp_cpu_dec()
> uclamp_cpu_dec_id()
> uclamp_cpu_update()
>
> where buckets needs sometimes to be scanned to find a new max.
>
> Consider also that, with mapping, we can more easily increase the
> buckets count to 20 in order to have a finer clamping granularity if
> needed without warring too much about performance impact especially
> when we use anyway few different clamp values.
>
> So, I agree that mapping adds (code) complexity but it can also save
> few cycles in the fast path... do you think it's not worth the added
> complexity?
Then maybe split this out in a separate patch? Do the trivial linear
bucket thing first and then do this smarty pants thing on top.
One problem with the scheme is that it doesn't defrag; so if you get a
peak usage, you can still end up with only two active buckets in
different lines.
Also; if it is it's own patch, you get a much better view of the
additional complexity and a chance to justify it ;-)
Also; would it make sense to do s/cpu/rq/ on much of this? All this
uclamp_cpu_*() stuff really is per rq and takes rq arguments, so why
does it have cpu in the name... no strong feelings, just noticed it and
thought is a tad inconsistent.
^ permalink raw reply
* Re: [PATCH v6 04/16] sched/core: uclamp: Add CPU's clamp buckets refcounting
From: Peter Zijlstra @ 2019-01-22 10:03 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190121155407.gv4cxpg2njqmdlj5@e110439-lin>
On Mon, Jan 21, 2019 at 03:54:07PM +0000, Patrick Bellasi wrote:
> On 21-Jan 16:17, Peter Zijlstra wrote:
> > On Tue, Jan 15, 2019 at 10:15:01AM +0000, Patrick Bellasi wrote:
> > > +#ifdef CONFIG_UCLAMP_TASK
> >
> > > +struct uclamp_bucket {
> > > + unsigned long value : bits_per(SCHED_CAPACITY_SCALE);
> > > + unsigned long tasks : BITS_PER_LONG - bits_per(SCHED_CAPACITY_SCALE);
> > > +};
> >
> > > +struct uclamp_cpu {
> > > + unsigned int value;
> >
> > /* 4 byte hole */
> >
> > > + struct uclamp_bucket bucket[UCLAMP_BUCKETS];
> > > +};
> >
> > With the default of 5, this UCLAMP_BUCKETS := 6, so struct uclamp_cpu
> > ends up being 7 'unsigned long's, or 56 bytes on 64bit (with a 4 byte
> > hole).
>
> Yes, that's dimensioned and configured to fit into a single cache line
> for all the possible 5 (by default) clamp values of a clamp index
> (i.e. min or max util).
And I suppose you picked 5 because 20% is a 'nice' number? whereas
16./666/% is a bit odd?
> > > +#endif /* CONFIG_UCLAMP_TASK */
> > > +
> > > /*
> > > * This is the main, per-CPU runqueue data structure.
> > > *
> > > @@ -835,6 +879,11 @@ struct rq {
> > > unsigned long nr_load_updates;
> > > u64 nr_switches;
> > >
> > > +#ifdef CONFIG_UCLAMP_TASK
> > > + /* Utilization clamp values based on CPU's RUNNABLE tasks */
> > > + struct uclamp_cpu uclamp[UCLAMP_CNT] ____cacheline_aligned;
> >
> > Which makes this 112 bytes with 8 bytes in 2 holes, which is short of 2
> > 64 byte cachelines.
>
> Right, we have 2 cache lines where:
> - the first $L tracks 5 different util_min values
> - the second $L tracks 5 different util_max values
Well, not quite so, if you want that you should put
____cacheline_aligned on struct uclamp_cpu. Such that the individual
array entries are each aligned, the above only alignes the whole array,
so the second uclamp_cpu is spread over both lines.
But I think this is actually better, since you have to scan both
min/max anyway, and allowing one the straddle a line you have to touch
anyway, allows for using less lines in total.
Consider for example the case where UCLAMP_BUCKETS=8, then each
uclamp_cpu would be 9 words or 72 bytes. If you force align the member,
then you end up with 4 lines, whereas now it would be 3.
> > Is that the best layout?
>
> It changed few times and that's what I found more reasonable for both
> for fitting the default configuration and also for code readability.
> Notice that we access RQ and SE clamp values with the same patter,
> for example:
>
> {rq|p}->uclamp[clamp_idx].value
>
> Are you worried about the holes or something else specific ?
Not sure; just mostly asking if this was by design or by accident.
One thing I did wonder though; since bucket[0] is counting the tasks
that are unconstrained and it's bucket value is basically fixed (0 /
1024), can't we abuse that value field to store uclamp_cpu::value ?
OTOH, doing that might make the code really ugly with all them:
if (!bucket_id)
exceptions all over the place.
^ permalink raw reply
* Re: [PATCH v6 04/16] sched/core: uclamp: Add CPU's clamp buckets refcounting
From: Patrick Bellasi @ 2019-01-22 10:31 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190122094507.GN27931@hirez.programming.kicks-ass.net>
On 22-Jan 10:45, Peter Zijlstra wrote:
> On Mon, Jan 21, 2019 at 04:33:38PM +0000, Patrick Bellasi wrote:
> > On 21-Jan 17:12, Peter Zijlstra wrote:
> > > On Mon, Jan 21, 2019 at 03:23:11PM +0000, Patrick Bellasi wrote:
>
> > > > and keep all
> > > > the buckets in use at the beginning of a cache line.
> > >
> > > That; is that the rationale for all this? Note that per the defaults
> > > everything is in a single line already.
> >
> > Yes, that's because of the loop in:
> >
> > dequeue_task()
> > uclamp_cpu_dec()
> > uclamp_cpu_dec_id()
> > uclamp_cpu_update()
> >
> > where buckets needs sometimes to be scanned to find a new max.
> >
> > Consider also that, with mapping, we can more easily increase the
> > buckets count to 20 in order to have a finer clamping granularity if
> > needed without warring too much about performance impact especially
> > when we use anyway few different clamp values.
> >
> > So, I agree that mapping adds (code) complexity but it can also save
> > few cycles in the fast path... do you think it's not worth the added
> > complexity?
>
> Then maybe split this out in a separate patch? Do the trivial linear
> bucket thing first and then do this smarty pants thing on top.
>
> One problem with the scheme is that it doesn't defrag; so if you get a
> peak usage, you can still end up with only two active buckets in
> different lines.
You right, that was saved for a later optimization. :/
Mainly in consideration of the fact that, at least for the main usage
we have in mind on Android, we will likely configure all the required
clamps once for all at boot time.
> Also; if it is it's own patch, you get a much better view of the
> additional complexity and a chance to justify it ;-)
What about ditching the mapping for the time being and see if we
get a real overhead hit in the future ?
At that point we will revamp the mapping patch with also a proper
defrag support.
> Also; would it make sense to do s/cpu/rq/ on much of this? All this
> uclamp_cpu_*() stuff really is per rq and takes rq arguments, so why
> does it have cpu in the name... no strong feelings, just noticed it and
> thought is a tad inconsistent.
The idea behind using "cpu" instead of "rq" was that we use those only at
root rq level and the clamps are aggregated per-CPU.
I remember one of the first versions used "cpu" instead of "rq" as a
parameter name and you proposed to change it as an optimization since
we call it from dequeue_task() where we already have a *rq.
... but, since we have those uclamp data within struct rq, I think you
are right: it makes more sense to rename the functions.
Will do it in v7, thanks.
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v6 08/16] sched/cpufreq: uclamp: Add utilization clamping for FAIR tasks
From: Rafael J. Wysocki @ 2019-01-22 10:37 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Peter Zijlstra,
Tejun Heo, Rafael J . Wysocki, Vincent Guittot, Viresh Kumar,
Paul Turner, Quentin Perret, Dietmar Eggemann, Morten Rasmussen,
Juri Lelli, Todd Kjos, Joel Fernandes, Steve Muckle,
Suren Baghdasaryan
In-Reply-To: <20190115101513.2822-9-patrick.bellasi@arm.com>
On Tuesday, January 15, 2019 11:15:05 AM CET Patrick Bellasi wrote:
> Each time a frequency update is required via schedutil, a frequency is
> selected to (possibly) satisfy the utilization reported by each
> scheduling class. However, when utilization clamping is in use, the
> frequency selection should consider userspace utilization clamping
> hints. This will allow, for example, to:
>
> - boost tasks which are directly affecting the user experience
> by running them at least at a minimum "requested" frequency
>
> - cap low priority tasks not directly affecting the user experience
> by running them only up to a maximum "allowed" frequency
>
> These constraints are meant to support a per-task based tuning of the
> frequency selection thus supporting a fine grained definition of
> performance boosting vs energy saving strategies in kernel space.
>
> Add support to clamp the utilization and IOWait boost of RUNNABLE FAIR
> tasks within the boundaries defined by their aggregated utilization
> clamp constraints.
> Based on the max(min_util, max_util) of each task, max-aggregated the
> CPU clamp value in a way to give the boosted tasks the performance they
> need when they happen to be co-scheduled with other capped tasks.
>
> Signed-off-by: Patrick Bellasi <patrick.bellasi@arm.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> ---
> Changes in v6:
> Message-ID: <20181107113849.GC14309@e110439-lin>
> - sanity check util_max >= util_min
> Others:
> - wholesale s/group/bucket/
> - wholesale s/_{get,put}/_{inc,dec}/ to match refcount APIs
> ---
> kernel/sched/cpufreq_schedutil.c | 27 ++++++++++++++++++++++++---
> kernel/sched/sched.h | 23 +++++++++++++++++++++++
> 2 files changed, 47 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 033ec7c45f13..520ee2b785e7 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -218,8 +218,15 @@ unsigned long schedutil_freq_util(int cpu, unsigned long util_cfs,
> * CFS tasks and we use the same metric to track the effective
> * utilization (PELT windows are synchronized) we can directly add them
> * to obtain the CPU's actual utilization.
> + *
> + * CFS utilization can be boosted or capped, depending on utilization
> + * clamp constraints requested by currently RUNNABLE tasks.
> + * When there are no CFS RUNNABLE tasks, clamps are released and
> + * frequency will be gracefully reduced with the utilization decay.
> */
> - util = util_cfs;
> + util = (type == ENERGY_UTIL)
> + ? util_cfs
> + : uclamp_util(rq, util_cfs);
> util += cpu_util_rt(rq);
>
> dl_util = cpu_util_dl(rq);
> @@ -327,6 +334,7 @@ static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, u64 time,
> unsigned int flags)
> {
> bool set_iowait_boost = flags & SCHED_CPUFREQ_IOWAIT;
> + unsigned int max_boost;
>
> /* Reset boost if the CPU appears to have been idle enough */
> if (sg_cpu->iowait_boost &&
> @@ -342,11 +350,24 @@ static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, u64 time,
> return;
> sg_cpu->iowait_boost_pending = true;
>
> + /*
> + * Boost FAIR tasks only up to the CPU clamped utilization.
> + *
> + * Since DL tasks have a much more advanced bandwidth control, it's
> + * safe to assume that IO boost does not apply to those tasks.
> + * Instead, since RT tasks are not utilization clamped, we don't want
> + * to apply clamping on IO boost while there is blocked RT
> + * utilization.
> + */
> + max_boost = sg_cpu->iowait_boost_max;
> + if (!cpu_util_rt(cpu_rq(sg_cpu->cpu)))
> + max_boost = uclamp_util(cpu_rq(sg_cpu->cpu), max_boost);
> +
> /* Double the boost at each request */
> if (sg_cpu->iowait_boost) {
> sg_cpu->iowait_boost <<= 1;
> - if (sg_cpu->iowait_boost > sg_cpu->iowait_boost_max)
> - sg_cpu->iowait_boost = sg_cpu->iowait_boost_max;
> + if (sg_cpu->iowait_boost > max_boost)
> + sg_cpu->iowait_boost = max_boost;
> return;
> }
>
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index b7f3ee8ba164..95d62a2a0b44 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -2267,6 +2267,29 @@ static inline unsigned int uclamp_none(int clamp_id)
> return SCHED_CAPACITY_SCALE;
> }
>
> +#ifdef CONFIG_UCLAMP_TASK
> +static inline unsigned int uclamp_util(struct rq *rq, unsigned int util)
> +{
> + unsigned int min_util = READ_ONCE(rq->uclamp[UCLAMP_MIN].value);
> + unsigned int max_util = READ_ONCE(rq->uclamp[UCLAMP_MAX].value);
> +
> + /*
> + * Since CPU's {min,max}_util clamps are MAX aggregated considering
> + * RUNNABLE tasks with _different_ clamps, we can end up with an
> + * invertion, which we can fix at usage time.
> + */
> + if (unlikely(min_util >= max_util))
> + return min_util;
> +
> + return clamp(util, min_util, max_util);
> +}
> +#else /* CONFIG_UCLAMP_TASK */
> +static inline unsigned int uclamp_util(struct rq *rq, unsigned int util)
> +{
> + return util;
> +}
> +#endif /* CONFIG_UCLAMP_TASK */
> +
> #ifdef arch_scale_freq_capacity
> # ifndef arch_scale_freq_invariant
> # define arch_scale_freq_invariant() true
>
IMO it would be better to combine this patch with the next one.
At least some things in it I was about to ask about would go away
then. :-)
Besides, I don't really see a reason for the split here.
^ permalink raw reply
* Re: [PATCH v6 05/16] sched/core: uclamp: Update CPU's refcount on clamp changes
From: Patrick Bellasi @ 2019-01-22 10:43 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190122093704.GM27931@hirez.programming.kicks-ass.net>
On 22-Jan 10:37, Peter Zijlstra wrote:
> On Mon, Jan 21, 2019 at 03:44:12PM +0000, Patrick Bellasi wrote:
> > On 21-Jan 16:33, Peter Zijlstra wrote:
> > > On Tue, Jan 15, 2019 at 10:15:02AM +0000, Patrick Bellasi wrote:
> > >
> > > > +static inline void
> > > > +uclamp_task_update_active(struct task_struct *p, unsigned int clamp_id)
> > > > +{
> > > > + struct rq_flags rf;
> > > > + struct rq *rq;
> > > > +
> > > > + /*
> > > > + * Lock the task and the CPU where the task is (or was) queued.
> > > > + *
> > > > + * We might lock the (previous) rq of a !RUNNABLE task, but that's the
> > > > + * price to pay to safely serialize util_{min,max} updates with
> > > > + * enqueues, dequeues and migration operations.
> > > > + * This is the same locking schema used by __set_cpus_allowed_ptr().
> > > > + */
> > > > + rq = task_rq_lock(p, &rf);
> > > > +
> > > > + /*
> > > > + * Setting the clamp bucket is serialized by task_rq_lock().
> > > > + * If the task is not yet RUNNABLE and its task_struct is not
> > > > + * affecting a valid clamp bucket, the next time it's enqueued,
> > > > + * it will already see the updated clamp bucket value.
> > > > + */
> > > > + if (!p->uclamp[clamp_id].active)
> > > > + goto done;
> > > > +
> > > > + uclamp_cpu_dec_id(p, rq, clamp_id);
> > > > + uclamp_cpu_inc_id(p, rq, clamp_id);
> > > > +
> > > > +done:
> > > > + task_rq_unlock(rq, p, &rf);
> > > > +}
> > >
> > > > @@ -1008,11 +1043,11 @@ static int __setscheduler_uclamp(struct task_struct *p,
> > > >
> > > > mutex_lock(&uclamp_mutex);
> > > > if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_MIN) {
> > > > - uclamp_bucket_inc(&p->uclamp[UCLAMP_MIN],
> > > > + uclamp_bucket_inc(p, &p->uclamp[UCLAMP_MIN],
> > > > UCLAMP_MIN, lower_bound);
> > > > }
> > > > if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_MAX) {
> > > > - uclamp_bucket_inc(&p->uclamp[UCLAMP_MAX],
> > > > + uclamp_bucket_inc(p, &p->uclamp[UCLAMP_MAX],
> > > > UCLAMP_MAX, upper_bound);
> > > > }
> > > > mutex_unlock(&uclamp_mutex);
> > >
> > >
> > > But.... __sched_setscheduler() actually does the whole dequeue + enqueue
> > > thing already ?!? See where it does __setscheduler().
> >
> > This is slow-path accounting, not fast path.
>
> Sure; but that's still no reason for duplicate or unneeded code.
>
> > There are two refcounting going on here:
> >
> > 1) mapped buckets:
> >
> > clamp_value <--(M1)--> bucket_id
> >
> > 2) RUNNABLE tasks:
> >
> > bucket_id <--(M2)--> RUNNABLE tasks in a bucket
> >
> > What we fix here is the refcounting for the buckets mapping. If a task
> > does not have a task specific clamp value it does not refcount any
> > bucket. The moment we assign a task specific clamp value, we need to
> > refcount the task in the bucket corresponding to that clamp value.
> >
> > This will keep the bucket in use at least as long as the task will
> > need that clamp value.
>
> Sure, I get that. What I don't get is why you're adding that (2) here.
> Like said, __sched_setscheduler() already does a dequeue/enqueue under
> rq->lock, which should already take care of that.
Oh, ok... got it what you mean now.
With:
[PATCH v6 01/16] sched/core: Allow sched_setattr() to use the current policy
<20190115101513.2822-2-patrick.bellasi@arm.com>
we can call __sched_setscheduler() with:
attr->sched_flags & SCHED_FLAG_KEEP_POLICY
whenever we want just to change the clamp values of a task without
changing its class. Thus, we can end up returning from
__sched_setscheduler() without doing an actual dequeue/enqueue.
This is likely the most common use-case.
I'll better check if I can propagate this info and avoid M2 if we
actually did a dequeue/enqueue.
Cheers Patrick
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v6 04/16] sched/core: uclamp: Add CPU's clamp buckets refcounting
From: Patrick Bellasi @ 2019-01-22 10:53 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190122100342.GO27931@hirez.programming.kicks-ass.net>
On 22-Jan 11:03, Peter Zijlstra wrote:
> On Mon, Jan 21, 2019 at 03:54:07PM +0000, Patrick Bellasi wrote:
> > On 21-Jan 16:17, Peter Zijlstra wrote:
> > > On Tue, Jan 15, 2019 at 10:15:01AM +0000, Patrick Bellasi wrote:
> > > > +#ifdef CONFIG_UCLAMP_TASK
> > >
> > > > +struct uclamp_bucket {
> > > > + unsigned long value : bits_per(SCHED_CAPACITY_SCALE);
> > > > + unsigned long tasks : BITS_PER_LONG - bits_per(SCHED_CAPACITY_SCALE);
> > > > +};
> > >
> > > > +struct uclamp_cpu {
> > > > + unsigned int value;
> > >
> > > /* 4 byte hole */
> > >
> > > > + struct uclamp_bucket bucket[UCLAMP_BUCKETS];
> > > > +};
> > >
> > > With the default of 5, this UCLAMP_BUCKETS := 6, so struct uclamp_cpu
> > > ends up being 7 'unsigned long's, or 56 bytes on 64bit (with a 4 byte
> > > hole).
> >
> > Yes, that's dimensioned and configured to fit into a single cache line
> > for all the possible 5 (by default) clamp values of a clamp index
> > (i.e. min or max util).
>
> And I suppose you picked 5 because 20% is a 'nice' number? whereas
> 16./666/% is a bit odd?
Yes, UCLAMP_BUCKETS:=6 gives me 5 20% buckets:
0-19%, 20-39%, 40-59%, 60-79%, 80-99%
plus a 100% bucket to track the max boosted tasks.
Does that makes sense ?
> > > > +#endif /* CONFIG_UCLAMP_TASK */
> > > > +
> > > > /*
> > > > * This is the main, per-CPU runqueue data structure.
> > > > *
> > > > @@ -835,6 +879,11 @@ struct rq {
> > > > unsigned long nr_load_updates;
> > > > u64 nr_switches;
> > > >
> > > > +#ifdef CONFIG_UCLAMP_TASK
> > > > + /* Utilization clamp values based on CPU's RUNNABLE tasks */
> > > > + struct uclamp_cpu uclamp[UCLAMP_CNT] ____cacheline_aligned;
> > >
> > > Which makes this 112 bytes with 8 bytes in 2 holes, which is short of 2
> > > 64 byte cachelines.
> >
> > Right, we have 2 cache lines where:
> > - the first $L tracks 5 different util_min values
> > - the second $L tracks 5 different util_max values
>
> Well, not quite so, if you want that you should put
> ____cacheline_aligned on struct uclamp_cpu. Such that the individual
> array entries are each aligned, the above only alignes the whole array,
> so the second uclamp_cpu is spread over both lines.
That's true... I was considering more important to save space if we
have a buckets number which can fit in let say 3 cache lines.
... but if you prefer the other way around I'll move it.
> But I think this is actually better, since you have to scan both
> min/max anyway, and allowing one the straddle a line you have to touch
> anyway, allows for using less lines in total.
Right.
> Consider for example the case where UCLAMP_BUCKETS=8, then each
> uclamp_cpu would be 9 words or 72 bytes. If you force align the member,
> then you end up with 4 lines, whereas now it would be 3.
Exactly :)
> > > Is that the best layout?
> >
> > It changed few times and that's what I found more reasonable for both
> > for fitting the default configuration and also for code readability.
> > Notice that we access RQ and SE clamp values with the same patter,
> > for example:
> >
> > {rq|p}->uclamp[clamp_idx].value
> >
> > Are you worried about the holes or something else specific ?
>
> Not sure; just mostly asking if this was by design or by accident.
>
> One thing I did wonder though; since bucket[0] is counting the tasks
> that are unconstrained and it's bucket value is basically fixed (0 /
> 1024), can't we abuse that value field to store uclamp_cpu::value ?
Mmm... should be possible, just worried about adding special cases
which can make the code even more complex of what it's not already.
.... moreover, if we ditch the mapping, the 1024 will be indexed at
the top of the array... so...
> OTOH, doing that might make the code really ugly with all them:
>
> if (!bucket_id)
>
> exceptions all over the place.
Exactly... I should read all your comments before replying :)
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v6 08/16] sched/cpufreq: uclamp: Add utilization clamping for FAIR tasks
From: Patrick Bellasi @ 2019-01-22 11:02 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Peter Zijlstra,
Tejun Heo, Rafael J . Wysocki, Vincent Guittot, Viresh Kumar,
Paul Turner, Quentin Perret, Dietmar Eggemann, Morten Rasmussen,
Juri Lelli, Todd Kjos, Joel Fernandes, Steve Muckle,
Suren Baghdasaryan
In-Reply-To: <3006911.57lVBuUGX3@aspire.rjw.lan>
On 22-Jan 11:37, Rafael J. Wysocki wrote:
> On Tuesday, January 15, 2019 11:15:05 AM CET Patrick Bellasi wrote:
> > Each time a frequency update is required via schedutil, a frequency is
> > selected to (possibly) satisfy the utilization reported by each
> > scheduling class. However, when utilization clamping is in use, the
> > frequency selection should consider userspace utilization clamping
> > hints. This will allow, for example, to:
> >
> > - boost tasks which are directly affecting the user experience
> > by running them at least at a minimum "requested" frequency
> >
> > - cap low priority tasks not directly affecting the user experience
> > by running them only up to a maximum "allowed" frequency
> >
> > These constraints are meant to support a per-task based tuning of the
> > frequency selection thus supporting a fine grained definition of
> > performance boosting vs energy saving strategies in kernel space.
> >
> > Add support to clamp the utilization and IOWait boost of RUNNABLE FAIR
> > tasks within the boundaries defined by their aggregated utilization
> > clamp constraints.
> > Based on the max(min_util, max_util) of each task, max-aggregated the
> > CPU clamp value in a way to give the boosted tasks the performance they
> > need when they happen to be co-scheduled with other capped tasks.
> >
> > Signed-off-by: Patrick Bellasi <patrick.bellasi@arm.com>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > ---
> > Changes in v6:
> > Message-ID: <20181107113849.GC14309@e110439-lin>
> > - sanity check util_max >= util_min
> > Others:
> > - wholesale s/group/bucket/
> > - wholesale s/_{get,put}/_{inc,dec}/ to match refcount APIs
> > ---
> > kernel/sched/cpufreq_schedutil.c | 27 ++++++++++++++++++++++++---
> > kernel/sched/sched.h | 23 +++++++++++++++++++++++
> > 2 files changed, 47 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > index 033ec7c45f13..520ee2b785e7 100644
> > --- a/kernel/sched/cpufreq_schedutil.c
> > +++ b/kernel/sched/cpufreq_schedutil.c
> > @@ -218,8 +218,15 @@ unsigned long schedutil_freq_util(int cpu, unsigned long util_cfs,
> > * CFS tasks and we use the same metric to track the effective
> > * utilization (PELT windows are synchronized) we can directly add them
> > * to obtain the CPU's actual utilization.
> > + *
> > + * CFS utilization can be boosted or capped, depending on utilization
> > + * clamp constraints requested by currently RUNNABLE tasks.
> > + * When there are no CFS RUNNABLE tasks, clamps are released and
> > + * frequency will be gracefully reduced with the utilization decay.
> > */
> > - util = util_cfs;
> > + util = (type == ENERGY_UTIL)
> > + ? util_cfs
> > + : uclamp_util(rq, util_cfs);
> > util += cpu_util_rt(rq);
> >
> > dl_util = cpu_util_dl(rq);
> > @@ -327,6 +334,7 @@ static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, u64 time,
> > unsigned int flags)
> > {
> > bool set_iowait_boost = flags & SCHED_CPUFREQ_IOWAIT;
> > + unsigned int max_boost;
> >
> > /* Reset boost if the CPU appears to have been idle enough */
> > if (sg_cpu->iowait_boost &&
> > @@ -342,11 +350,24 @@ static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, u64 time,
> > return;
> > sg_cpu->iowait_boost_pending = true;
> >
> > + /*
> > + * Boost FAIR tasks only up to the CPU clamped utilization.
> > + *
> > + * Since DL tasks have a much more advanced bandwidth control, it's
> > + * safe to assume that IO boost does not apply to those tasks.
> > + * Instead, since RT tasks are not utilization clamped, we don't want
> > + * to apply clamping on IO boost while there is blocked RT
> > + * utilization.
> > + */
> > + max_boost = sg_cpu->iowait_boost_max;
> > + if (!cpu_util_rt(cpu_rq(sg_cpu->cpu)))
> > + max_boost = uclamp_util(cpu_rq(sg_cpu->cpu), max_boost);
> > +
> > /* Double the boost at each request */
> > if (sg_cpu->iowait_boost) {
> > sg_cpu->iowait_boost <<= 1;
> > - if (sg_cpu->iowait_boost > sg_cpu->iowait_boost_max)
> > - sg_cpu->iowait_boost = sg_cpu->iowait_boost_max;
> > + if (sg_cpu->iowait_boost > max_boost)
> > + sg_cpu->iowait_boost = max_boost;
> > return;
> > }
> >
> > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> > index b7f3ee8ba164..95d62a2a0b44 100644
> > --- a/kernel/sched/sched.h
> > +++ b/kernel/sched/sched.h
> > @@ -2267,6 +2267,29 @@ static inline unsigned int uclamp_none(int clamp_id)
> > return SCHED_CAPACITY_SCALE;
> > }
> >
> > +#ifdef CONFIG_UCLAMP_TASK
> > +static inline unsigned int uclamp_util(struct rq *rq, unsigned int util)
> > +{
> > + unsigned int min_util = READ_ONCE(rq->uclamp[UCLAMP_MIN].value);
> > + unsigned int max_util = READ_ONCE(rq->uclamp[UCLAMP_MAX].value);
> > +
> > + /*
> > + * Since CPU's {min,max}_util clamps are MAX aggregated considering
> > + * RUNNABLE tasks with _different_ clamps, we can end up with an
> > + * invertion, which we can fix at usage time.
> > + */
> > + if (unlikely(min_util >= max_util))
> > + return min_util;
> > +
> > + return clamp(util, min_util, max_util);
> > +}
> > +#else /* CONFIG_UCLAMP_TASK */
> > +static inline unsigned int uclamp_util(struct rq *rq, unsigned int util)
> > +{
> > + return util;
> > +}
> > +#endif /* CONFIG_UCLAMP_TASK */
> > +
> > #ifdef arch_scale_freq_capacity
> > # ifndef arch_scale_freq_invariant
> > # define arch_scale_freq_invariant() true
> >
>
> IMO it would be better to combine this patch with the next one.
Main reason was to better document in the changelog what we do for the
two different classes...
> At least some things in it I was about to ask about would go away
> then. :-)
... but if it creates confusion I can certainly merge them.
Or maybe clarify better in this patch what's not clear: may I ask what
were your questions ?
> Besides, I don't really see a reason for the split here.
Was mainly to make the changes required for RT more self-contained.
For that class only, not for FAIR, we have additional code in the
following patch which add uclamp_default_perf which are system
defaults used to track/account tasks requesting the maximum frequency.
Again, I can either better clarify the above patch or just merge the
two together: what do you prefer ?
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox