Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1357216 > unrolled thread

[GIT PULL] NOHZ updates for v4.6

Back to article view | Back to linux.kernel

Contents

  [GIT PULL] NOHZ updates for v4.6 Ingo Molnar <mingo@kernel.org> - 2016-03-14 13:40 +0100
    Re: [GIT PULL] NOHZ updates for v4.6 Linus Torvalds <torvalds@linux-foundation.org> - 2016-03-15 03:50 +0100
      Re: [GIT PULL] NOHZ updates for v4.6 Peter Zijlstra <peterz@infradead.org> - 2016-03-15 09:50 +0100
        Re: [GIT PULL] NOHZ updates for v4.6 Ingo Molnar <mingo@kernel.org> - 2016-03-15 10:50 +0100
      [PATCH] atomic: Fix bugs in 'fetch_or()' and rename it to 'xchg_or()' Ingo Molnar <mingo@kernel.org> - 2016-03-15 10:40 +0100
        Re: [PATCH] atomic: Fix bugs in 'fetch_or()' and rename it to  'xchg_or()' Peter Zijlstra <peterz@infradead.org> - 2016-03-15 12:00 +0100
          Re: [PATCH] atomic: Fix bugs in 'fetch_or()' and rename it to  'xchg_or()' Ingo Molnar <mingo@kernel.org> - 2016-03-15 13:10 +0100
            Re: [PATCH] atomic: Fix bugs in 'fetch_or()' and rename it to  'xchg_or()' Peter Zijlstra <peterz@infradead.org> - 2016-03-15 13:50 +0100
        Re: [PATCH] atomic: Fix bugs in 'fetch_or()' and rename it to  'xchg_or()' Peter Zijlstra <peterz@infradead.org> - 2016-03-15 12:10 +0100
        Re: [PATCH] atomic: Fix bugs in 'fetch_or()' and rename it to  'xchg_or()' Ingo Molnar <mingo@kernel.org> - 2016-03-15 13:10 +0100
          Re: [PATCH] atomic: Fix bugs in 'fetch_or()' and rename it to  'xchg_or()' Ingo Molnar <mingo@kernel.org> - 2016-03-15 13:40 +0100
            Re: [PATCH] atomic: Fix bugs in 'fetch_or()' and rename it to  'xchg_or()' Ingo Molnar <mingo@kernel.org> - 2016-03-15 13:40 +0100
            Re: [PATCH] atomic: Fix bugs in 'fetch_or()' and rename it to  'xchg_or()' Peter Zijlstra <peterz@infradead.org> - 2016-03-15 14:20 +0100
        Re: [PATCH] atomic: Fix bugs in 'fetch_or()' and rename it to  'xchg_or()' Peter Zijlstra <peterz@infradead.org> - 2016-03-15 13:30 +0100
        [PATCH v2] atomic: Fix bugs in 'fetch_or()' and rename it to  'xchg_or()' Ingo Molnar <mingo@kernel.org> - 2016-03-15 13:30 +0100
          Re: [PATCH v2] atomic: Fix bugs in 'fetch_or()' and rename it to  'xchg_or()' Peter Zijlstra <peterz@infradead.org> - 2016-03-15 14:30 +0100
            Re: [PATCH v2] atomic: Fix bugs in 'fetch_or()' and rename it to  'xchg_or()' Ingo Molnar <mingo@kernel.org> - 2016-03-16 09:10 +0100
              Re: [PATCH v2] atomic: Fix bugs in 'fetch_or()' and rename it to  'xchg_or()' Peter Zijlstra <peterz@infradead.org> - 2016-03-16 09:40 +0100
          Re: [PATCH v2] atomic: Fix bugs in 'fetch_or()' and rename it to  'xchg_or()' Frederic Weisbecker <fweisbec@gmail.com> - 2016-03-15 18:10 +0100
            Re: [PATCH v2] atomic: Fix bugs in 'fetch_or()' and rename it to  'xchg_or()' Ingo Molnar <mingo@kernel.org> - 2016-03-16 09:20 +0100
              Re: [PATCH v2] atomic: Fix bugs in 'fetch_or()' and rename it to  'xchg_or()' Frederic Weisbecker <fweisbec@gmail.com> - 2016-03-17 02:00 +0100
        Re: [PATCH] atomic: Fix bugs in 'fetch_or()' and rename it to 'xchg_or()' Linus Torvalds <torvalds@linux-foundation.org> - 2016-03-15 17:20 +0100
      [PATCH] nohz: Change tick_dep_mask from 'unsigned long' to 'unsigned  int' Ingo Molnar <mingo@kernel.org> - 2016-03-15 11:00 +0100
        Re: [PATCH] nohz: Change tick_dep_mask from 'unsigned long' to  'unsigned int' Ingo Molnar <mingo@kernel.org> - 2016-03-15 13:20 +0100
          Re: [PATCH] nohz: Change tick_dep_mask from 'unsigned long' to  'unsigned int' Linus Torvalds <torvalds@linux-foundation.org> - 2016-03-15 17:40 +0100
            Re: [PATCH] nohz: Change tick_dep_mask from 'unsigned long' to  'unsigned int' Frederic Weisbecker <fweisbec@gmail.com> - 2016-03-15 18:30 +0100
              Re: [PATCH] nohz: Change tick_dep_mask from 'unsigned long' to  'unsigned int' Linus Torvalds <torvalds@linux-foundation.org> - 2016-03-15 18:40 +0100

Page 2 of 2 — ← Prev page 1 [2]

#1359502 — Re: [PATCH v2] atomic: Fix bugs in 'fetch_or()' and rename it to 'xchg_or()'

On Wed, Mar 16, 2016 at 09:14:44AM +0100, Ingo Molnar wrote:
> Yeah, so in the perf tooling build we do have -Wshadow to catch such mishaps,
> but not in the main kernel build.
> 
> ... and yes, if I add it via the patch below the bug gets warned about:
> 
>  include/linux/atomic.h:561:15: note: shadowed declaration is here
>    typeof(ptr)  __ptr  = (ptr);     \
>                ^
>  kernel/sched/core.c:332:11: note: in expansion of macro ‘xchg_or’
>    return !(xchg_or(&ti->flags, _TIF_NEED_RESCHED) & _TIF_POLLING_NRFLA
> 
> ... but I also get a ton of other warnings, just when building a single 
> kernel/sched/core.o file:
> 
>  ./arch/x86/include/asm/bitops.h:396:28: warning: declaration of ‘ffs’ shadows a built-in function [-Wshadow]
>  ./arch/x86/include/asm/bitops.h:396:28: warning: declaration of ‘ffs’ shadows a built-in function [-Wshadow]
>  include/linux/jiffies.h:422:60: warning: declaration of ‘jiffies’ shadows a global declaration [-Wshadow]
>  ./arch/x86/include/asm/io_apic.h:187:54: warning: declaration of ‘apic’ shadows a global declaration [-Wshadow]
>  ./arch/x86/include/asm/bitops.h:396:28: warning: declaration of ‘ffs’ shadows a built-in function [-Wshadow]
>  include/linux/jiffies.h:422:60: warning: declaration of ‘jiffies’ shadows a global declaration [-Wshadow]
>  ./arch/x86/include/asm/io_apic.h:187:54: warning: declaration of ‘apic’ shadows a global declaration [-Wshadow]
>  include/linux/kernel.h:750:12: warning: declaration of ‘_min1’ shadows a previous local [-Wshadow]
>  include/linux/kernel.h:750:12: warning: declaration of ‘_min1’ shadows a previous local [-Wshadow]
>  include/linux/kernel.h:751:12: warning: declaration of ‘_min2’ shadows a previous local [-Wshadow]
>  kernel/sched/sched.h:308:43: warning: declaration of ‘down’ shadows a global declaration [-Wshadow]
>  kernel/sched/sched.h:308:60: warning: declaration of ‘up’ shadows a global declaration [-Wshadow]
>  kernel/sched/auto_group.h:44:55: warning: declaration of ‘init_task’ shadows a global declaration [-Wshadow]
>  kernel/sched/core.c:635:20: warning: declaration of ‘down’ shadows a global declaration [-Wshadow]
>  kernel/sched/core.c:635:37: warning: declaration of ‘up’ shadows a global declaration [-Wshadow]

Heh, what else can we expect from global functions named up() and down() in a millions-lines C project :-)

> 
> and yes, I'd say most of these are signatures of sloppy macros and sloppy variable 
> names - but it would be a ton of work to eliminate these warnings.

Yeah that's what I was afraid of.

Thanks for trying it though!

[toc] | [prev] | [next] | [standalone]

#1358136 — Re: [PATCH] atomic: Fix bugs in 'fetch_or()' and rename it to 'xchg_or()'

On Tue, Mar 15, 2016 at 2:32 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> I can see two other problems with it as well:
>
>  1) 'ptr' may get evaluated multiple times as well, not just 'mask'.

Right you are.

>  2) its naming sucks. "fetch_or()" does not really signal that it's a
>     fundamentally atomic operation, nor what API family it belongs to.

Actually, I like the naming, I just don't like the implementation.

It's not just the "evaluated multiple times", it's also the fact that
if we start doing "fetch_or()", then some day we'll want "fetch_and()"
or "fetch_clear()" etc.

Or, in fact, "fetch_add()", which is often closer to what hardware
does than the "atomic_add_return()" we have now (the difference is
that "fetch_add()" returns the original value, while
"atomic_add_return()" returns the end result).

So what I meant with "try to match what we already have in our
existing atomic.h" is that we do the _infrastructure_ so well. We do
it well both by having separate UP and SMP versions of the underlying
helpers, but we do it well by having those helpers that are then used
to implement the different atomic versions.

So I'd like something similar for the "fetch_op" thing.

                   Linus

[toc] | [prev] | [next] | [standalone]

#1357951 — [PATCH] nohz: Change tick_dep_mask from 'unsigned long' to 'unsigned int'

Linus noticed that the new tick_dep_mask types introduced in:

  d027d45d8a17 ("nohz: New tick dependency mask")

... are sloppily defined as 'unsigned long' - which is wasteful
to carry just 4 bits and which may also create suboptimal data
types on 64-bit systems with word alignment padding holes in them.

Fix this by changing the type to the more natural 'unsigned int'.

(The xchg_or() API will work fine with 'unsigned int' as well.)

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Chris Metcalf <cmetcalf@ezchip.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Christoph Lameter <cl@linux.com>
Cc: Chris Metcalf <cmetcalf@ezchip.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Luiz Capitulino <lcapitulino@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 include/linux/sched.h    |  4 ++--
 kernel/time/tick-sched.c | 11 +++++------
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index c617ea12c6b7..6d1842bb7abd 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -718,7 +718,7 @@ struct signal_struct {
 	struct task_cputime cputime_expires;
 
 #ifdef CONFIG_NO_HZ_FULL
-	unsigned long tick_dep_mask;
+	unsigned int tick_dep_mask;
 #endif
 
 	struct list_head cpu_timers[3];
@@ -1548,7 +1548,7 @@ struct task_struct {
 #endif
 
 #ifdef CONFIG_NO_HZ_FULL
-	unsigned long tick_dep_mask;
+	unsigned int tick_dep_mask;
 #endif
 	unsigned long nvcsw, nivcsw; /* context switch counts */
 	u64 start_time;		/* monotonic time in nsec */
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 851631899352..fe0f57f3432f 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -157,9 +157,9 @@ static void tick_sched_handle(struct tick_sched *ts, struct pt_regs *regs)
 cpumask_var_t tick_nohz_full_mask;
 cpumask_var_t housekeeping_mask;
 bool tick_nohz_full_running;
-static unsigned long tick_dep_mask;
+static unsigned int tick_dep_mask;
 
-static void trace_tick_dependency(unsigned long dep)
+static void trace_tick_dependency(unsigned int dep)
 {
 	if (dep & TICK_DEP_MASK_POSIX_TIMER) {
 		trace_tick_stop(0, TICK_DEP_MASK_POSIX_TIMER);
@@ -259,10 +259,9 @@ static void tick_nohz_full_kick_all(void)
 	preempt_enable();
 }
 
-static void tick_nohz_dep_set_all(unsigned long *dep,
-				  enum tick_dep_bits bit)
+static void tick_nohz_dep_set_all(unsigned int *dep, enum tick_dep_bits bit)
 {
-	unsigned long prev;
+	unsigned int prev;
 
 	prev = xchg_or(dep, BIT_MASK(bit));
 	if (!prev)
@@ -289,7 +288,7 @@ void tick_nohz_dep_clear(enum tick_dep_bits bit)
  */
 void tick_nohz_dep_set_cpu(int cpu, enum tick_dep_bits bit)
 {
-	unsigned long prev;
+	unsigned int prev;
 	struct tick_sched *ts;
 
 	ts = per_cpu_ptr(&tick_cpu_sched, cpu);

[toc] | [prev] | [next] | [standalone]

#1358011 — Re: [PATCH] nohz: Change tick_dep_mask from 'unsigned long' to 'unsigned int'

* Ingo Molnar <mingo@kernel.org> wrote:

> 
> Linus noticed that the new tick_dep_mask types introduced in:
> 
>   d027d45d8a17 ("nohz: New tick dependency mask")
> 
> ... are sloppily defined as 'unsigned long' - which is wasteful
> to carry just 4 bits and which may also create suboptimal data
> types on 64-bit systems with word alignment padding holes in them.
> 
> Fix this by changing the type to the more natural 'unsigned int'.
> 
> (The xchg_or() API will work fine with 'unsigned int' as well.)
> 
> Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Chris Metcalf <cmetcalf@ezchip.com>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Chris Metcalf <cmetcalf@ezchip.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Luiz Capitulino <lcapitulino@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> ---
>  include/linux/sched.h    |  4 ++--
>  kernel/time/tick-sched.c | 11 +++++------
>  2 files changed, 7 insertions(+), 8 deletions(-)

Hm, so the problem I did not consider is the following:

 triton:~/tip> git grep tick_dep_mask kernel | grep _bit
 kernel/time/tick-sched.c:       clear_bit(bit, &tick_dep_mask);
 kernel/time/tick-sched.c:       clear_bit(bit, &ts->tick_dep_mask);
 kernel/time/tick-sched.c:       clear_bit(bit, &tsk->tick_dep_mask);
 kernel/time/tick-sched.c:       clear_bit(bit, &sig->tick_dep_mask);

and the bitops natural type (and in fact the only supported bitops type) is 
'unsigned long'.

So it's not that easy to change a bitmask over to unsigned int.

Suggestions?

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1358143 — Re: [PATCH] nohz: Change tick_dep_mask from 'unsigned long' to 'unsigned int'

On Tue, Mar 15, 2016 at 5:15 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> Hm, so the problem I did not consider is the following:
>
>  triton:~/tip> git grep tick_dep_mask kernel | grep _bit
>  kernel/time/tick-sched.c:       clear_bit(bit, &ts->tick_dep_mask);
>  kernel/time/tick-sched.c:       clear_bit(bit, &tsk->tick_dep_mask);
>  kernel/time/tick-sched.c:       clear_bit(bit, &sig->tick_dep_mask);

Ahh. I missed that too.

Ok, leave the 64-bit field for now, while we think about this. But one
option is to just use "atomic_andnot()" instead of clear_bit().

That would imply using an "atomic_t", which would be fairly natural
(and would be 32-bit).

And wouldn't it be so nice if "thread_info->flags" would just be
atomic_t too. Right now we use a mixture of bit-ops and ACCESS_ONCE()
(and many codepaths then doing neither, and just accessing it
directly, ignoring any races.

Oh well.

                Linus

[toc] | [prev] | [next] | [standalone]

#1358164 — Re: [PATCH] nohz: Change tick_dep_mask from 'unsigned long' to 'unsigned int'

On Tue, Mar 15, 2016 at 09:30:49AM -0700, Linus Torvalds wrote:
> On Tue, Mar 15, 2016 at 5:15 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > Hm, so the problem I did not consider is the following:
> >
> >  triton:~/tip> git grep tick_dep_mask kernel | grep _bit
> >  kernel/time/tick-sched.c:       clear_bit(bit, &ts->tick_dep_mask);
> >  kernel/time/tick-sched.c:       clear_bit(bit, &tsk->tick_dep_mask);
> >  kernel/time/tick-sched.c:       clear_bit(bit, &sig->tick_dep_mask);

Ah! Now I remember why I used unsigned long :-s

> 
> Ahh. I missed that too.
> 
> Ok, leave the 64-bit field for now, while we think about this. But one
> option is to just use "atomic_andnot()" instead of clear_bit().
> 
> That would imply using an "atomic_t", which would be fairly natural
> (and would be 32-bit).

I can try that. And then we would end up with atomic_fetch_or() and maybe
the scheduler could keep its ad-hoc version until thread_info types get unified
(if that's possible).

> 
> And wouldn't it be so nice if "thread_info->flags" would just be
> atomic_t too. Right now we use a mixture of bit-ops and ACCESS_ONCE()
> (and many codepaths then doing neither, and just accessing it
> directly, ignoring any races.
> 
> Oh well.

I can try that too while at it :-)

[toc] | [prev] | [next] | [standalone]

#1358174 — Re: [PATCH] nohz: Change tick_dep_mask from 'unsigned long' to 'unsigned int'

On Tue, Mar 15, 2016 at 10:28 AM, Frederic Weisbecker
<fweisbec@gmail.com> wrote:
>>
>> And wouldn't it be so nice if "thread_info->flags" would just be
>> atomic_t too. Right now we use a mixture of bit-ops and ACCESS_ONCE()
>> (and many codepaths then doing neither, and just accessing it
>> directly, ignoring any races.
>>
>> Oh well.
>
> I can try that too while at it :-)

The real problem with thread_info->flags is that it's
per-architecture, and people access that thing from assembly code.

So changing that is potentially really *very* painful. The patch
itself is not likely too bad, but having confidence that everything
was caught?

Nasty. It would probably be a very good cleanup, but I'm not sure how
worthwhile it is considering the pain.

(The only good news is that any problem is likely to be *very*
obvious. Switching the field to a 32-bit type on a big-endian machine
and not catching some case will mean that people will check or modify
the wrong flags, and you'd likely get breakage quite quickly)

                 Linus

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web