Hello, Christoph,
I have a patch that currently uses __this_cpu_inc_return() to increment a
per-CPU variable, but without preemption disabled. Of course, given that
preemption is enabled, it might well end up picking up one CPU's counter,
adding one to it, then storing the result into some other CPU's counter.
But this is OK, the test can be probabilistic. And when I run this
against v3.14 and earlier, it works fine.
But now there is 188a81409ff7 (percpu: add preemption checks to
__this_cpu ops), which gives me lots of splats.
My current admittedly crude workaround is as follows:
static inline bool rcu_should_resched(void)
{
int t;
#ifdef CONFIG_DEBUG_PREEMPT
preempt_disable();
#endif /* #ifdef CONFIG_DEBUG_PREEMPT */
t = __this_cpu_read(rcu_cond_resched_count) + 1;
if (t < RCU_COND_RESCHED_LIM) {
__this_cpu_write(rcu_cond_resched_count, t);
#ifdef CONFIG_DEBUG_PREEMPT
preempt_enable();
#endif /* #ifdef CONFIG_DEBUG_PREEMPT */
return false;
}
#ifdef CONFIG_DEBUG_PREEMPT
preempt_enable();
#endif /* #ifdef CONFIG_DEBUG_PREEMPT */
return true;
}
This is arguably better than the original __this_cpu_read() because it
avoids overflow, but I thought I should check to see if there was some
better way to do this.
Thanx, Paul
On Tue, Apr 15, 2014 at 03:17:55PM -0700, Paul E. McKenney wrote:
> My current admittedly crude workaround is as follows:
>
> static inline bool rcu_should_resched(void)
> {
> int t;
>
> #ifdef CONFIG_DEBUG_PREEMPT
> preempt_disable();
> #endif /* #ifdef CONFIG_DEBUG_PREEMPT */
> t = __this_cpu_read(rcu_cond_resched_count) + 1;
> if (t < RCU_COND_RESCHED_LIM) {
> __this_cpu_write(rcu_cond_resched_count, t);
> #ifdef CONFIG_DEBUG_PREEMPT
> preempt_enable();
> #endif /* #ifdef CONFIG_DEBUG_PREEMPT */
> return false;
> }
> #ifdef CONFIG_DEBUG_PREEMPT
> preempt_enable();
> #endif /* #ifdef CONFIG_DEBUG_PREEMPT */
> return true;
> }
Won't using DEBUG_PREEMPT instead of just CONFIG_PREEMPT here make this
silently do the wrong thing if preemption is enabled, but debugging isn't ?
I'm not seeing why you need the ifdefs at all, unless the implied
barrier() is a problem ?
Dave
On Tue, Apr 15, 2014 at 06:29:51PM -0400, Dave Jones wrote:
> On Tue, Apr 15, 2014 at 03:17:55PM -0700, Paul E. McKenney wrote:
>
> > My current admittedly crude workaround is as follows:
> >
> > static inline bool rcu_should_resched(void)
> > {
> > int t;
> >
> > #ifdef CONFIG_DEBUG_PREEMPT
> > preempt_disable();
> > #endif /* #ifdef CONFIG_DEBUG_PREEMPT */
> > t = __this_cpu_read(rcu_cond_resched_count) + 1;
> > if (t < RCU_COND_RESCHED_LIM) {
> > __this_cpu_write(rcu_cond_resched_count, t);
> > #ifdef CONFIG_DEBUG_PREEMPT
> > preempt_enable();
> > #endif /* #ifdef CONFIG_DEBUG_PREEMPT */
> > return false;
> > }
> > #ifdef CONFIG_DEBUG_PREEMPT
> > preempt_enable();
> > #endif /* #ifdef CONFIG_DEBUG_PREEMPT */
> > return true;
> > }
>
> Won't using DEBUG_PREEMPT instead of just CONFIG_PREEMPT here make this
> silently do the wrong thing if preemption is enabled, but debugging isn't ?
If preemption is enabled, but debugging is not, then yes, the above code
might force an unnecessary schedule() if the above code was preempted
between the __this_cpu_read() and the __this_cpu_write(). Which does
not cause a problem, especially given that it won't happen very often.
> I'm not seeing why you need the ifdefs at all, unless the implied
> barrier() is a problem ?
I don't think that Peter Zijlstra would be too happy about an extra
unneeded preempt_disable()/preempt_enable() pair in the cond_resched()
fastpath. Not that I necessarily expect him to be particularly happy
with the above, but perhaps someone has a better approach.
Thanx, Paul
On Tue, Apr 15, 2014 at 03:47:26PM -0700, Paul E. McKenney wrote:
> On Tue, Apr 15, 2014 at 06:29:51PM -0400, Dave Jones wrote:
> > On Tue, Apr 15, 2014 at 03:17:55PM -0700, Paul E. McKenney wrote:
> >
> > > My current admittedly crude workaround is as follows:
> > >
> > > static inline bool rcu_should_resched(void)
> > > {
> > > int t;
> > >
> > > #ifdef CONFIG_DEBUG_PREEMPT
> > > preempt_disable();
> > > #endif /* #ifdef CONFIG_DEBUG_PREEMPT */
> > > t = __this_cpu_read(rcu_cond_resched_count) + 1;
> > > if (t < RCU_COND_RESCHED_LIM) {
> > > __this_cpu_write(rcu_cond_resched_count, t);
> > > #ifdef CONFIG_DEBUG_PREEMPT
> > > preempt_enable();
> > > #endif /* #ifdef CONFIG_DEBUG_PREEMPT */
> > > return false;
> > > }
> > > #ifdef CONFIG_DEBUG_PREEMPT
> > > preempt_enable();
> > > #endif /* #ifdef CONFIG_DEBUG_PREEMPT */
> > > return true;
> > > }
> >
> > Won't using DEBUG_PREEMPT instead of just CONFIG_PREEMPT here make this
> > silently do the wrong thing if preemption is enabled, but debugging isn't ?
>
> If preemption is enabled, but debugging is not, then yes, the above code
> might force an unnecessary schedule() if the above code was preempted
> between the __this_cpu_read() and the __this_cpu_write(). Which does
> not cause a problem, especially given that it won't happen very often.
>
> > I'm not seeing why you need the ifdefs at all, unless the implied
> > barrier() is a problem ?
>
> I don't think that Peter Zijlstra would be too happy about an extra
> unneeded preempt_disable()/preempt_enable() pair in the cond_resched()
> fastpath. Not that I necessarily expect him to be particularly happy
> with the above, but perhaps someone has a better approach.
But falling back on the old ways of doing this at least looks a bit
nicer:
static inline bool rcu_should_resched(void)
{
int t;
int *tp = &per_cpu(rcu_cond_resched_count, raw_smp_processor_id());
t = ACCESS_ONCE(*tp) + 1;
if (t < RCU_COND_RESCHED_LIM) {
ACCESS_ONCE(*tp) = t;
return false;
}
return true;
}
Other thoughts?
Thanx, Paul
On Tue, Apr 15, 2014 at 03:17:55PM -0700, Paul E. McKenney wrote:
> Hello, Christoph,
>
> I have a patch that currently uses __this_cpu_inc_return() to increment a
> per-CPU variable, but without preemption disabled. Of course, given that
> preemption is enabled, it might well end up picking up one CPU's counter,
> adding one to it, then storing the result into some other CPU's counter.
> But this is OK, the test can be probabilistic. And when I run this
> against v3.14 and earlier, it works fine.
>
> But now there is 188a81409ff7 (percpu: add preemption checks to
> __this_cpu ops), which gives me lots of splats.
>
> My current admittedly crude workaround is as follows:
>
> static inline bool rcu_should_resched(void)
> {
> int t;
>
> #ifdef CONFIG_DEBUG_PREEMPT
> preempt_disable();
> #endif /* #ifdef CONFIG_DEBUG_PREEMPT */
> t = __this_cpu_read(rcu_cond_resched_count) + 1;
> if (t < RCU_COND_RESCHED_LIM) {
> __this_cpu_write(rcu_cond_resched_count, t);
> #ifdef CONFIG_DEBUG_PREEMPT
> preempt_enable();
> #endif /* #ifdef CONFIG_DEBUG_PREEMPT */
> return false;
> }
> #ifdef CONFIG_DEBUG_PREEMPT
> preempt_enable();
> #endif /* #ifdef CONFIG_DEBUG_PREEMPT */
> return true;
> }
>
> This is arguably better than the original __this_cpu_read() because it
> avoids overflow, but I thought I should check to see if there was some
> better way to do this.
you could use raw_cpu_{read,write}(). But note that without the
unconditional preempt_disable() in there your code can read a different
rcu_cond_resched_count than it writes.
So I think you very much want that preempt_disable().
On Tue, Apr 15, 2014 at 08:54:19PM -0700, Paul E. McKenney wrote:
> But falling back on the old ways of doing this at least looks a bit
> nicer:
>
> static inline bool rcu_should_resched(void)
> {
> int t;
> int *tp = &per_cpu(rcu_cond_resched_count, raw_smp_processor_id());
>
> t = ACCESS_ONCE(*tp) + 1;
> if (t < RCU_COND_RESCHED_LIM) {
<here>
> ACCESS_ONCE(*tp) = t;
> return false;
> }
> return true;
> }
>
> Other thoughts?
Still broken, if A starts out on CPU1, gets migrated to CPU0 at <here>,
then B starts the same on CPU1. It is possible for both CPU0 and CPU1 to
write a different value into your rcu_cond_resched_count.
You really want to disable preemption around there. The proper old way
would've been get_cpu_var()/put_cpu_var().
On Wed, Apr 16, 2014 at 07:21:48AM +0200, Peter Zijlstra wrote:
> On Tue, Apr 15, 2014 at 08:54:19PM -0700, Paul E. McKenney wrote:
> > But falling back on the old ways of doing this at least looks a bit
> > nicer:
> >
> > static inline bool rcu_should_resched(void)
> > {
> > int t;
> > int *tp = &per_cpu(rcu_cond_resched_count, raw_smp_processor_id());
> >
> > t = ACCESS_ONCE(*tp) + 1;
> > if (t < RCU_COND_RESCHED_LIM) {
>
> <here>
>
> > ACCESS_ONCE(*tp) = t;
> > return false;
> > }
> > return true;
> > }
> >
> > Other thoughts?
>
> Still broken, if A starts out on CPU1, gets migrated to CPU0 at <here>,
> then B starts the same on CPU1. It is possible for both CPU0 and CPU1 to
> write a different value into your rcu_cond_resched_count.
That is actually OK. The values written are guaranteed to be between
zero and RCU_COND_RESCHED_LIM-1. In theory, yes, rcu_should_resched()
could end up failing due to a horribly unlucky sequence of preemptions,
but the probability is -way- lower than that of hardware failure.
However...
> You really want to disable preemption around there. The proper old way
> would've been get_cpu_var()/put_cpu_var().
If you are OK with unconditional disabling of preemption at this point,
that would avoid worrying about probabilities and would be quite a bit
simpler.
So unconditional preempt_disable()/preempt_enable() it is.
Thanx, Paul
On Tue, 15 Apr 2014, Paul E. McKenney wrote:
> Hello, Christoph,
>
> I have a patch that currently uses __this_cpu_inc_return() to increment a
> per-CPU variable, but without preemption disabled. Of course, given that
> preemption is enabled, it might well end up picking up one CPU's counter,
> adding one to it, then storing the result into some other CPU's counter.
> But this is OK, the test can be probabilistic. And when I run this
> against v3.14 and earlier, it works fine.
We introduced raw_cpu_inc_return to squish these warnings.
> This is arguably better than the original __this_cpu_read() because it
> avoids overflow, but I thought I should check to see if there was some
> better way to do this.
If this is supposed to be totally race safe then you must disable
preemption.
On Wed, 16 Apr 2014, Peter Zijlstra wrote:
> You really want to disable preemption around there. The proper old way
> would've been get_cpu_var()/put_cpu_var().
get_cpu_var and put_cpu_var is still the correct way of doing things if
you need to access multiple per cpu variables.
The problem that I want to solve is the varied use of
__get_cpu_var for address calculations, per cpu variable assignment,
structure copying and per cpu variable access. The this_cpu ops can avoid
address calculations using segment prefixes. Plus the changes clarify what
is actuallly going on.
On Wed, Apr 16, 2014 at 10:08:03AM -0500, Christoph Lameter wrote:
> On Tue, 15 Apr 2014, Paul E. McKenney wrote:
>
> > Hello, Christoph,
> >
> > I have a patch that currently uses __this_cpu_inc_return() to increment a
> > per-CPU variable, but without preemption disabled. Of course, given that
> > preemption is enabled, it might well end up picking up one CPU's counter,
> > adding one to it, then storing the result into some other CPU's counter.
> > But this is OK, the test can be probabilistic. And when I run this
> > against v3.14 and earlier, it works fine.
>
> We introduced raw_cpu_inc_return to squish these warnings.
Cool, this is a good short-term fix.
> > This is arguably better than the original __this_cpu_read() because it
> > avoids overflow, but I thought I should check to see if there was some
> > better way to do this.
>
> If this is supposed to be totally race safe then you must disable
> preemption.
Understood!
Thanx, Paul
On Wed, Apr 16, 2014 at 09:06:21AM -0700, Paul E. McKenney wrote:
> On Wed, Apr 16, 2014 at 10:08:03AM -0500, Christoph Lameter wrote:
> > On Tue, 15 Apr 2014, Paul E. McKenney wrote:
> >
> > > Hello, Christoph,
> > >
> > > I have a patch that currently uses __this_cpu_inc_return() to increment a
> > > per-CPU variable, but without preemption disabled. Of course, given that
> > > preemption is enabled, it might well end up picking up one CPU's counter,
> > > adding one to it, then storing the result into some other CPU's counter.
> > > But this is OK, the test can be probabilistic. And when I run this
> > > against v3.14 and earlier, it works fine.
> >
> > We introduced raw_cpu_inc_return to squish these warnings.
>
> Cool, this is a good short-term fix.
Or at least it is given the following patch. Was this the intent?
Thanx, Paul
------------------------------------------------------------------------
percpu: Fix raw_cpu_inc_return()
The definition for raw_cpu_add_return() uses the operation prefix
"raw_add_return_", but the definitions in the various percpu.h files
expect "raw_cpu_add_return_". This commit therefore appropriately
adjusts the definition of raw_cpu_add_return().
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: Christoph Lameter <[email protected]>
diff --git a/include/linux/percpu.h b/include/linux/percpu.h
index e7a0b95ed527..495c6543a8f2 100644
--- a/include/linux/percpu.h
+++ b/include/linux/percpu.h
@@ -639,7 +639,7 @@ do { \
# define raw_cpu_add_return_8(pcp, val) raw_cpu_generic_add_return(pcp, val)
# endif
# define raw_cpu_add_return(pcp, val) \
- __pcpu_size_call_return2(raw_add_return_, pcp, val)
+ __pcpu_size_call_return2(raw_cpu_add_return_, pcp, val)
#endif
#define raw_cpu_sub_return(pcp, val) raw_cpu_add_return(pcp, -(typeof(pcp))(val))
On Wed, 16 Apr 2014, Paul E. McKenney wrote:
> On Wed, Apr 16, 2014 at 09:06:21AM -0700, Paul E. McKenney wrote:
> > On Wed, Apr 16, 2014 at 10:08:03AM -0500, Christoph Lameter wrote:
> > > On Tue, 15 Apr 2014, Paul E. McKenney wrote:
> > >
> > > > Hello, Christoph,
> > > >
> > > > I have a patch that currently uses __this_cpu_inc_return() to increment a
> > > > per-CPU variable, but without preemption disabled. Of course, given that
> > > > preemption is enabled, it might well end up picking up one CPU's counter,
> > > > adding one to it, then storing the result into some other CPU's counter.
> > > > But this is OK, the test can be probabilistic. And when I run this
> > > > against v3.14 and earlier, it works fine.
> > >
> > > We introduced raw_cpu_inc_return to squish these warnings.
> >
> > Cool, this is a good short-term fix.
>
> Or at least it is given the following patch. Was this the intent?
Correct. Thanks for the fix.
Acked-by: Christoph Lameter <[email protected]>
> Thanx, Paul
>
> ------------------------------------------------------------------------
>
> percpu: Fix raw_cpu_inc_return()
>
> The definition for raw_cpu_add_return() uses the operation prefix
> "raw_add_return_", but the definitions in the various percpu.h files
> expect "raw_cpu_add_return_". This commit therefore appropriately
> adjusts the definition of raw_cpu_add_return().
>
> Signed-off-by: Paul E. McKenney <[email protected]>
> Cc: Christoph Lameter <[email protected]>
>
> diff --git a/include/linux/percpu.h b/include/linux/percpu.h
> index e7a0b95ed527..495c6543a8f2 100644
> --- a/include/linux/percpu.h
> +++ b/include/linux/percpu.h
> @@ -639,7 +639,7 @@ do { \
> # define raw_cpu_add_return_8(pcp, val) raw_cpu_generic_add_return(pcp, val)
> # endif
> # define raw_cpu_add_return(pcp, val) \
> - __pcpu_size_call_return2(raw_add_return_, pcp, val)
> + __pcpu_size_call_return2(raw_cpu_add_return_, pcp, val)
> #endif
>
> #define raw_cpu_sub_return(pcp, val) raw_cpu_add_return(pcp, -(typeof(pcp))(val))
>
On Wed, Apr 16, 2014 at 01:29:21PM -0500, Christoph Lameter wrote:
> On Wed, 16 Apr 2014, Paul E. McKenney wrote:
>
> > On Wed, Apr 16, 2014 at 09:06:21AM -0700, Paul E. McKenney wrote:
> > > On Wed, Apr 16, 2014 at 10:08:03AM -0500, Christoph Lameter wrote:
> > > > On Tue, 15 Apr 2014, Paul E. McKenney wrote:
> > > >
> > > > > Hello, Christoph,
> > > > >
> > > > > I have a patch that currently uses __this_cpu_inc_return() to increment a
> > > > > per-CPU variable, but without preemption disabled. Of course, given that
> > > > > preemption is enabled, it might well end up picking up one CPU's counter,
> > > > > adding one to it, then storing the result into some other CPU's counter.
> > > > > But this is OK, the test can be probabilistic. And when I run this
> > > > > against v3.14 and earlier, it works fine.
> > > >
> > > > We introduced raw_cpu_inc_return to squish these warnings.
> > >
> > > Cool, this is a good short-term fix.
> >
> > Or at least it is given the following patch. Was this the intent?
>
> Correct. Thanks for the fix.
>
> Acked-by: Christoph Lameter <[email protected]>
Queued, thank you. I don't need this until 3.16, but I am tempted
to push it in this cycle in case others need it. Any objections?
Thanx, Paul
> > ------------------------------------------------------------------------
> >
> > percpu: Fix raw_cpu_inc_return()
> >
> > The definition for raw_cpu_add_return() uses the operation prefix
> > "raw_add_return_", but the definitions in the various percpu.h files
> > expect "raw_cpu_add_return_". This commit therefore appropriately
> > adjusts the definition of raw_cpu_add_return().
> >
> > Signed-off-by: Paul E. McKenney <[email protected]>
> > Cc: Christoph Lameter <[email protected]>
> >
> > diff --git a/include/linux/percpu.h b/include/linux/percpu.h
> > index e7a0b95ed527..495c6543a8f2 100644
> > --- a/include/linux/percpu.h
> > +++ b/include/linux/percpu.h
> > @@ -639,7 +639,7 @@ do { \
> > # define raw_cpu_add_return_8(pcp, val) raw_cpu_generic_add_return(pcp, val)
> > # endif
> > # define raw_cpu_add_return(pcp, val) \
> > - __pcpu_size_call_return2(raw_add_return_, pcp, val)
> > + __pcpu_size_call_return2(raw_cpu_add_return_, pcp, val)
> > #endif
> >
> > #define raw_cpu_sub_return(pcp, val) raw_cpu_add_return(pcp, -(typeof(pcp))(val))
> >
>
On Wed, 16 Apr 2014, Paul E. McKenney wrote:
> Queued, thank you. I don't need this until 3.16, but I am tempted
> to push it in this cycle in case others need it. Any objections?
No its a bug fix.
On Thu, Apr 17, 2014 at 12:36:06PM -0500, Christoph Lameter wrote:
> On Wed, 16 Apr 2014, Paul E. McKenney wrote:
>
> > Queued, thank you. I don't need this until 3.16, but I am tempted
> > to push it in this cycle in case others need it. Any objections?
>
> No its a bug fix.
Fair enough! I resent the patch with your Ack to Tejun.
Thanx, Paul
On Thu, 17 Apr 2014, Paul E. McKenney wrote:
> Fair enough! I resent the patch with your Ack to Tejun.
Also note that you may want to use
this_cpu_inc
instead of raw_cpu_inc.
this_cpu_inc will not disable preemption or anything on x86 but just
create a single instruction using instruction atomicity to avoid the
preempt on/off sequence.
On platforms that cannot emit such an instruction it will fallback to
disable interrupts for the sequence of instructions that increments the
value.
With such an approach incrementing the counter should be much safer. If
the other arch want to avoid irq on/off sequences then they can override
the fallback to use atomics or whatever the processor architecture permits
to avoid the overhead of interrupt on / off.
On Thu, Apr 17, 2014 at 12:53:28PM -0500, Christoph Lameter wrote:
> On Thu, 17 Apr 2014, Paul E. McKenney wrote:
>
> > Fair enough! I resent the patch with your Ack to Tejun.
>
> Also note that you may want to use
>
> this_cpu_inc
>
> instead of raw_cpu_inc.
>
> this_cpu_inc will not disable preemption or anything on x86 but just
> create a single instruction using instruction atomicity to avoid the
> preempt on/off sequence.
>
>
> On platforms that cannot emit such an instruction it will fallback to
> disable interrupts for the sequence of instructions that increments the
> value.
>
> With such an approach incrementing the counter should be much safer. If
> the other arch want to avoid irq on/off sequences then they can override
> the fallback to use atomics or whatever the processor architecture permits
> to avoid the overhead of interrupt on / off.
Fair enough, but in this case I don't need it to be safe.
Thanx, Paul