Hello.
rcu_check_quiescent_state:
/*
* Races with local timer interrupt - in the worst case
* we may miss one quiescent state of that CPU. That is
* tolerable. So no need to disable interrupts.
*/
if (rdp->qsctr == rdp->last_qsctr)
return;
Afaics, this comment is misleading. rcu_check_quiescent_state()
is executed in softirq context, while rcu_check_callbacks() checks
in_softirq() before ++qsctr.
Also, replace (1 << HARDIRQ_SHIFT) by HARDIRQ_OFFSET.
On top of the 'rcu: eliminate rcu_ctrlblk.lock', see
http://marc.theaimsgroup.com/?l=linux-kernel&m=110156786721526
Oleg.
Signed-off-by: Oleg Nesterov <[email protected]>
--- 2.6.10-rc2/kernel/rcupdate.c~ 2004-11-27 21:40:02.000000000 +0300
+++ 2.6.10-rc2/kernel/rcupdate.c 2004-11-28 17:29:19.084446040 +0300
@@ -229,11 +229,6 @@ static void rcu_check_quiescent_state(st
if (!rdp->qs_pending)
return;
- /*
- * Races with local timer interrupt - in the worst case
- * we may miss one quiescent state of that CPU. That is
- * tolerable. So no need to disable interrupts.
- */
if (rdp->qsctr == rdp->last_qsctr)
return;
rdp->qs_pending = 0;
@@ -358,7 +353,7 @@ void rcu_check_callbacks(int cpu, int us
{
if (user ||
(idle_cpu(cpu) && !in_softirq() &&
- hardirq_count() <= (1 << HARDIRQ_SHIFT))) {
+ hardirq_count() <= HARDIRQ_OFFSET)) {
rcu_qsctr_inc(cpu);
rcu_bh_qsctr_inc(cpu);
} else if (!in_softirq())
On Sun, Nov 28, 2004 at 06:06:52PM +0300, Oleg Nesterov wrote:
> rcu_check_quiescent_state:
> /*
> * Races with local timer interrupt - in the worst case
> * we may miss one quiescent state of that CPU. That is
> * tolerable. So no need to disable interrupts.
> */
> if (rdp->qsctr == rdp->last_qsctr)
> return;
> Afaics, this comment is misleading. rcu_check_quiescent_state()
> is executed in softirq context, while rcu_check_callbacks() checks
> in_softirq() before ++qsctr.
> Also, replace (1 << HARDIRQ_SHIFT) by HARDIRQ_OFFSET.
> On top of the 'rcu: eliminate rcu_ctrlblk.lock', see
> http://marc.theaimsgroup.com/?l=linux-kernel&m=110156786721526
rcu_qsctr_inc() does *NOT* check in_softirq(), and yes, scheduling
does occur directly off the timer interrupt. For instance, for
userspace tasks whose timeslices have expired.
-- wli
William Lee Irwin III wrote:
>
> On Sun, Nov 28, 2004 at 06:06:52PM +0300, Oleg Nesterov wrote:
> >
> > Afaics, this comment is misleading. rcu_check_quiescent_state()
> > is executed in softirq context, while rcu_check_callbacks() checks
> > in_softirq() before ++qsctr.
>
> rcu_qsctr_inc() does *NOT* check in_softirq(), and yes, scheduling
> does occur directly off the timer interrupt. For instance, for
> userspace tasks whose timeslices have expired.
But schedule()->rcu_qsctr_inc() can happen only after return from
do_softirq(), so where is the race?
Oleg.
On Sun, Nov 28, 2004 at 06:06:52PM +0300, Oleg Nesterov wrote:
> Hello.
>
> rcu_check_quiescent_state:
> /*
> * Races with local timer interrupt - in the worst case
> * we may miss one quiescent state of that CPU. That is
> * tolerable. So no need to disable interrupts.
> */
> if (rdp->qsctr == rdp->last_qsctr)
> return;
>
> Afaics, this comment is misleading. rcu_check_quiescent_state()
> is executed in softirq context, while rcu_check_callbacks() checks
> in_softirq() before ++qsctr.
>
> Also, replace (1 << HARDIRQ_SHIFT) by HARDIRQ_OFFSET.
>
Looks good to me. IIRC, that comment has been around since very
early prototypes, so it is probably leftover trash.
Thanks
Dipankar
Dipankar Sarma wrote:
>On Sun, Nov 28, 2004 at 06:06:52PM +0300, Oleg Nesterov wrote:
>
>
>>Afaics, this comment is misleading. rcu_check_quiescent_state()
>>is executed in softirq context, while rcu_check_callbacks() checks
>>in_softirq() before ++qsctr.
>>
>>Also, replace (1 << HARDIRQ_SHIFT) by HARDIRQ_OFFSET.
>>
>>
>>
>
>Looks good to me. IIRC, that comment has been around since very
>early prototypes, so it is probably leftover trash.
>
>
>
I agree. I think I only moved it around.
But I don't like the HARDIRQ_OFFSET change. If I understand the code
correctly it checks that there is no hardirq reentrancy, i.e. the count
is 0 or 1. Shifted to the appropriate position for the actual test.
I'd either leave it as it is or use "1*HARDIRQ_OFFSET" - otherwise the
information that the count should be less of equal one is lost.
--
Manfred
On Sun, Nov 28, 2004 at 04:24:39PM +0100, Manfred Spraul wrote:
> Dipankar Sarma wrote:
>
> >On Sun, Nov 28, 2004 at 06:06:52PM +0300, Oleg Nesterov wrote:
> >
> >
> >>Afaics, this comment is misleading. rcu_check_quiescent_state()
> >>is executed in softirq context, while rcu_check_callbacks() checks
> >>in_softirq() before ++qsctr.
> >>
> >>Also, replace (1 << HARDIRQ_SHIFT) by HARDIRQ_OFFSET.
> >>
> >>
> >>
> >
> >Looks good to me. IIRC, that comment has been around since very
> >early prototypes, so it is probably leftover trash.
> >
> >
> >
> I agree. I think I only moved it around.
> But I don't like the HARDIRQ_OFFSET change. If I understand the code
> correctly it checks that there is no hardirq reentrancy, i.e. the count
> is 0 or 1. Shifted to the appropriate position for the actual test.
> I'd either leave it as it is or use "1*HARDIRQ_OFFSET" - otherwise the
> information that the count should be less of equal one is lost.
Hmm. I agree with Manfred. hardirq_count() <= (1 << HARDIRQ_SHIFT)
was the test I arrived at since it was most explicit - One level
of (local timer) interrupt over idle task and no softirq in between
is OK to indicate that the cpu had seen an idle task. A bigger
hardirq_count() indicates reentrant hardirq over idle task and we
are no longer safe.
So, let's drop the HARDIRQ_OFFSET change.
Thanks
Dipankar
Dipankar Sarma wrote:
>
> Hmm. I agree with Manfred. hardirq_count() <= (1 << HARDIRQ_SHIFT)
> was the test I arrived at since it was most explicit - One level
> of (local timer) interrupt over idle task and no softirq in between
> is OK to indicate that the cpu had seen an idle task. A bigger
> hardirq_count() indicates reentrant hardirq over idle task and we
> are no longer safe.
>
> So, let's drop the HARDIRQ_OFFSET change.
Ok. I am resending these two patches in one.
Oleg.