Re: [PATCH 1/2] rcu: cosmetic, delete wrong comment, use HARDIRQ_OFFSET

Re: [PATCH 1/2] rcu: cosmetic, delete wrong comment, use HARDIRQ_OFFSET

[William Lee Irwin III]

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

[PATCH 1/2] rcu: cosmetic, delete wrong comment, use HARDIRQ_OFFSET

[Oleg Nesterov]

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 <oleg@tv-sign.ru>

--- 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())

Re: [PATCH 1/2] rcu: cosmetic, delete wrong comment, use HARDIRQ_OFFSET

[Dipankar Sarma]

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

Re: [PATCH 1/2] rcu: cosmetic, delete wrong comment, use HARDIRQ_OFFSET

[Manfred Spraul]

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

Re: [PATCH 1/2] rcu: cosmetic, delete wrong comment, use HARDIRQ_OFFSET

[Dipankar Sarma]

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

Re: [PATCH 1/2] rcu: cosmetic, delete wrong comment, use HARDIRQ_OFFSET

[Oleg Nesterov]

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.

Re: [PATCH 1/2] rcu: cosmetic, delete wrong comment, use HARDIRQ_OFFSET

[Oleg Nesterov]

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.