[RFC 1/3] rcu: Expedite the rcu quiescent state reporting if help needed

[RFC 1/3] rcu: Expedite the rcu quiescent state reporting if help needed

[Joel Fernandes (Google)]

The t->rcu_read_unlock_special union's need_qs bit can be set by the
scheduler tick (in rcu_flavor_sched_clock_irq) to indicate that help is
needed from the rcu_read_unlock path. When this help arrives however, we
can do better to speed up the quiescent state reporting which if
rcu_read_unlock_special::need_qs is set might be quite urgent. Make use
of this information in deciding when to do heavy-weight softirq raising
where possible.

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 kernel/rcu/tree_plugin.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index c588ef98efd3..bff6410fac06 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -622,7 +622,8 @@ static void rcu_read_unlock_special(struct task_struct *t)
 		t->rcu_read_unlock_special.b.exp_hint = false;
 		exp = (t->rcu_blocked_node && t->rcu_blocked_node->exp_tasks) ||
 		      (rdp->grpmask & rnp->expmask) ||
-		      tick_nohz_full_cpu(rdp->cpu);
+		      tick_nohz_full_cpu(rdp->cpu)  ||
+		      t->rcu_read_unlock_special.b.need_qs;
 		// Need to defer quiescent state until everything is enabled.
 		if (irqs_were_disabled && use_softirq &&
 		    (in_interrupt() ||
-- 
2.22.0.410.gd8fdbe21b5-goog

[RFC 2/3] rcu: Simplify rcu_note_context_switch exit from critical section

[Joel Fernandes (Google)]

The rcu_preempt_note_context_switch() tries to handle cases where
__rcu_read_unlock() got preempted and then the context switch path does
the reporting of the quiscent state along with clearing any bits in the
rcu_read_unlock_special union.

This can be handled by just calling rcu_deferred_qs() which was added
during the RCU consolidation work and already does these checks.

Tested RCU config TREE03 for an hour which succeeds.

Cc: rcu@vger.kernel.org
Cc: kernel-team@android.com
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 kernel/rcu/tree_plugin.h | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index bff6410fac06..ebb4d46a6267 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -313,15 +313,6 @@ void rcu_note_context_switch(bool preempt)
 				       ? rnp->gp_seq
 				       : rcu_seq_snap(&rnp->gp_seq));
 		rcu_preempt_ctxt_queue(rnp, rdp);
-	} else if (t->rcu_read_lock_nesting < 0 &&
-		   t->rcu_read_unlock_special.s) {
-
-		/*
-		 * Complete exit from RCU read-side critical section on
-		 * behalf of preempted instance of __rcu_read_unlock().
-		 */
-		rcu_read_unlock_special(t);
-		rcu_preempt_deferred_qs(t);
 	} else {
 		rcu_preempt_deferred_qs(t);
 	}
-- 
2.22.0.410.gd8fdbe21b5-goog

Re: [RFC 2/3] rcu: Simplify rcu_note_context_switch exit from critical section

[Paul E. McKenney]

On Mon, Jul 01, 2019 at 12:04:14AM -0400, Joel Fernandes (Google) wrote:
> The rcu_preempt_note_context_switch() tries to handle cases where
> __rcu_read_unlock() got preempted and then the context switch path does
> the reporting of the quiscent state along with clearing any bits in the
> rcu_read_unlock_special union.
> 
> This can be handled by just calling rcu_deferred_qs() which was added
> during the RCU consolidation work and already does these checks.
> 
> Tested RCU config TREE03 for an hour which succeeds.
> 
> Cc: rcu@vger.kernel.org
> Cc: kernel-team@android.com
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

My first reaction was "that cannot possibly work", but after a bit of
digging, it really does appear to work just fine.  I therefore expanded
the commit log a bit, so please check it to catch any messups on my part.

Very cool, thank you very much!  ;-)

							Thanx, Paul

------------------------------------------------------------------------

commit ce547cb41ed7662f70d0b07d4c7f7555ba130c61
Author: Joel Fernandes (Google) <joel@joelfernandes.org>
Date:   Mon Jul 1 00:04:14 2019 -0400

    rcu: Simplify rcu_note_context_switch exit from critical section
    
    Because __rcu_read_unlock() can be preempted just before the call to
    rcu_read_unlock_special(), it is possible for a task to be preempted just
    before it would have fully exited its RCU read-side critical section.
    This would result in a needless extension of that critical section until
    that task was resumed, which might in turn result in a needlessly
    long grace period, needless RCU priority boosting, and needless
    force-quiescent-state actions.  Therefore, rcu_note_context_switch()
    invokes __rcu_read_unlock() followed by rcu_preempt_deferred_qs() when
    it detects this situation.  This action by rcu_note_context_switch()
    ends the RCU read-side critical section immediately.
    
    Of course, once the task resumes, it will invoke rcu_read_unlock_special()
    redundantly.  This is harmless because the fact that a preemption
    happened means that interrupts, preemption, and softirqs cannot
    have been disabled, so there would be no deferred quiescent state.
    While ->rcu_read_lock_nesting remains less than zero, none of the
    ->rcu_read_unlock_special.b bits can be set, and they were all zeroed by
    the call to rcu_note_context_switch() at task-preemption time.  Therefore,
    setting ->rcu_read_unlock_special.b.exp_hint to false has no effect.
    
    Therefore, the extra call to rcu_preempt_deferred_qs_irqrestore()
    would return immediately.  With one possible exception, which is
    if an expedited grace period started just as the task was being
    resumed, which could leave ->exp_deferred_qs set.  This will cause
    rcu_preempt_deferred_qs_irqrestore() to invoke rcu_report_exp_rdp(),
    reporting the quiescent state, just as it should.  (Such an expedited
    grace period won't affect the preemption code path due to interrupts
    having already been disabled.)
    
    But when rcu_note_context_switch() invokes __rcu_read_unlock(), it
    is doing so with preemption disabled, hence __rcu_read_unlock() will
    unconditionally defer the quiescent state, only to immediately invoke
    rcu_preempt_deferred_qs(), thus immediately reporting the deferred
    quiescent state.  It turns out to be safe (and faster) to instead
    just invoke rcu_preempt_deferred_qs() without the __rcu_read_unlock()
    middleman.
    
    Because this is the invocation during the preemption (as opposed to
    the invocation just after the resume), at least one of the bits in
    ->rcu_read_unlock_special.b must be set and ->rcu_read_lock_nesting
    must be negative.  This means that rcu_preempt_need_deferred_qs() must
    return true, avoiding the early exit from rcu_preempt_deferred_qs().
    Thus, rcu_preempt_deferred_qs_irqrestore() will be invoked immediately,
    as required.
    
    This commit therefore simplifies the CONFIG_PREEMPT=y version of
    rcu_note_context_switch() by removing the "else if" branch of its
    "if" statement.  This change means that all callers that would have
    invoked rcu_read_unlock_special() followed by rcu_preempt_deferred_qs()
    will now simply invoke rcu_preempt_deferred_qs(), thus avoiding the
    rcu_read_unlock_special() middleman when __rcu_read_unlock() is preempted.
    
    Cc: rcu@vger.kernel.org
    Cc: kernel-team@android.com
    Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
    Signed-off-by: Paul E. McKenney <paulmck@linux.ibm.com>

diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index 187dc076c497..214e4689c29d 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -313,15 +313,6 @@ void rcu_note_context_switch(bool preempt)
 				       ? rnp->gp_seq
 				       : rcu_seq_snap(&rnp->gp_seq));
 		rcu_preempt_ctxt_queue(rnp, rdp);
-	} else if (t->rcu_read_lock_nesting < 0 &&
-		   t->rcu_read_unlock_special.s) {
-
-		/*
-		 * Complete exit from RCU read-side critical section on
-		 * behalf of preempted instance of __rcu_read_unlock().
-		 */
-		rcu_read_unlock_special(t);
-		rcu_preempt_deferred_qs(t);
 	} else {
 		rcu_preempt_deferred_qs(t);
 	}

Re: [RFC 2/3] rcu: Simplify rcu_note_context_switch exit from critical section

[Joel Fernandes]

On Mon, Jul 01, 2019 at 01:03:10PM -0700, Paul E. McKenney wrote:
> On Mon, Jul 01, 2019 at 12:04:14AM -0400, Joel Fernandes (Google) wrote:
> > The rcu_preempt_note_context_switch() tries to handle cases where
> > __rcu_read_unlock() got preempted and then the context switch path does
> > the reporting of the quiscent state along with clearing any bits in the
> > rcu_read_unlock_special union.
> > 
> > This can be handled by just calling rcu_deferred_qs() which was added
> > during the RCU consolidation work and already does these checks.
> > 
> > Tested RCU config TREE03 for an hour which succeeds.
> > 
> > Cc: rcu@vger.kernel.org
> > Cc: kernel-team@android.com
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> 
> My first reaction was "that cannot possibly work", but after a bit of
> digging, it really does appear to work just fine.  I therefore expanded
> the commit log a bit, so please check it to catch any messups on my part.
> 
> Very cool, thank you very much!  ;-)

Awesome! Thanks. I am glad you agree with the change and I agree with your
changes to the commit log.

thanks,

 - Joel


> 
> 							Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
> commit ce547cb41ed7662f70d0b07d4c7f7555ba130c61
> Author: Joel Fernandes (Google) <joel@joelfernandes.org>
> Date:   Mon Jul 1 00:04:14 2019 -0400
> 
>     rcu: Simplify rcu_note_context_switch exit from critical section
>     
>     Because __rcu_read_unlock() can be preempted just before the call to
>     rcu_read_unlock_special(), it is possible for a task to be preempted just
>     before it would have fully exited its RCU read-side critical section.
>     This would result in a needless extension of that critical section until
>     that task was resumed, which might in turn result in a needlessly
>     long grace period, needless RCU priority boosting, and needless
>     force-quiescent-state actions.  Therefore, rcu_note_context_switch()
>     invokes __rcu_read_unlock() followed by rcu_preempt_deferred_qs() when
>     it detects this situation.  This action by rcu_note_context_switch()
>     ends the RCU read-side critical section immediately.
>     
>     Of course, once the task resumes, it will invoke rcu_read_unlock_special()
>     redundantly.  This is harmless because the fact that a preemption
>     happened means that interrupts, preemption, and softirqs cannot
>     have been disabled, so there would be no deferred quiescent state.
>     While ->rcu_read_lock_nesting remains less than zero, none of the
>     ->rcu_read_unlock_special.b bits can be set, and they were all zeroed by
>     the call to rcu_note_context_switch() at task-preemption time.  Therefore,
>     setting ->rcu_read_unlock_special.b.exp_hint to false has no effect.
>     
>     Therefore, the extra call to rcu_preempt_deferred_qs_irqrestore()
>     would return immediately.  With one possible exception, which is
>     if an expedited grace period started just as the task was being
>     resumed, which could leave ->exp_deferred_qs set.  This will cause
>     rcu_preempt_deferred_qs_irqrestore() to invoke rcu_report_exp_rdp(),
>     reporting the quiescent state, just as it should.  (Such an expedited
>     grace period won't affect the preemption code path due to interrupts
>     having already been disabled.)
>     
>     But when rcu_note_context_switch() invokes __rcu_read_unlock(), it
>     is doing so with preemption disabled, hence __rcu_read_unlock() will
>     unconditionally defer the quiescent state, only to immediately invoke
>     rcu_preempt_deferred_qs(), thus immediately reporting the deferred
>     quiescent state.  It turns out to be safe (and faster) to instead
>     just invoke rcu_preempt_deferred_qs() without the __rcu_read_unlock()
>     middleman.
>     
>     Because this is the invocation during the preemption (as opposed to
>     the invocation just after the resume), at least one of the bits in
>     ->rcu_read_unlock_special.b must be set and ->rcu_read_lock_nesting
>     must be negative.  This means that rcu_preempt_need_deferred_qs() must
>     return true, avoiding the early exit from rcu_preempt_deferred_qs().
>     Thus, rcu_preempt_deferred_qs_irqrestore() will be invoked immediately,
>     as required.
>     
>     This commit therefore simplifies the CONFIG_PREEMPT=y version of
>     rcu_note_context_switch() by removing the "else if" branch of its
>     "if" statement.  This change means that all callers that would have
>     invoked rcu_read_unlock_special() followed by rcu_preempt_deferred_qs()
>     will now simply invoke rcu_preempt_deferred_qs(), thus avoiding the
>     rcu_read_unlock_special() middleman when __rcu_read_unlock() is preempted.
>     
>     Cc: rcu@vger.kernel.org
>     Cc: kernel-team@android.com
>     Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>     Signed-off-by: Paul E. McKenney <paulmck@linux.ibm.com>
> 
> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> index 187dc076c497..214e4689c29d 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -313,15 +313,6 @@ void rcu_note_context_switch(bool preempt)
>  				       ? rnp->gp_seq
>  				       : rcu_seq_snap(&rnp->gp_seq));
>  		rcu_preempt_ctxt_queue(rnp, rdp);
> -	} else if (t->rcu_read_lock_nesting < 0 &&
> -		   t->rcu_read_unlock_special.s) {
> -
> -		/*
> -		 * Complete exit from RCU read-side critical section on
> -		 * behalf of preempted instance of __rcu_read_unlock().
> -		 */
> -		rcu_read_unlock_special(t);
> -		rcu_preempt_deferred_qs(t);
>  	} else {
>  		rcu_preempt_deferred_qs(t);
>  	}
> 

Re: [RFC 2/3] rcu: Simplify rcu_note_context_switch exit from critical section

[Paul E. McKenney]

On Mon, Jul 01, 2019 at 05:33:28PM -0400, Joel Fernandes wrote:
> On Mon, Jul 01, 2019 at 01:03:10PM -0700, Paul E. McKenney wrote:
> > On Mon, Jul 01, 2019 at 12:04:14AM -0400, Joel Fernandes (Google) wrote:
> > > The rcu_preempt_note_context_switch() tries to handle cases where
> > > __rcu_read_unlock() got preempted and then the context switch path does
> > > the reporting of the quiscent state along with clearing any bits in the
> > > rcu_read_unlock_special union.
> > > 
> > > This can be handled by just calling rcu_deferred_qs() which was added
> > > during the RCU consolidation work and already does these checks.
> > > 
> > > Tested RCU config TREE03 for an hour which succeeds.
> > > 
> > > Cc: rcu@vger.kernel.org
> > > Cc: kernel-team@android.com
> > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > 
> > My first reaction was "that cannot possibly work", but after a bit of
> > digging, it really does appear to work just fine.  I therefore expanded
> > the commit log a bit, so please check it to catch any messups on my part.
> > 
> > Very cool, thank you very much!  ;-)
> 
> Awesome! Thanks. I am glad you agree with the change and I agree with your
> changes to the commit log.

Very good, I will push it to -rcu shortly.

							Thanx, Paul

> thanks,
> 
>  - Joel
> 
> 
> > 
> > 							Thanx, Paul
> > 
> > ------------------------------------------------------------------------
> > 
> > commit ce547cb41ed7662f70d0b07d4c7f7555ba130c61
> > Author: Joel Fernandes (Google) <joel@joelfernandes.org>
> > Date:   Mon Jul 1 00:04:14 2019 -0400
> > 
> >     rcu: Simplify rcu_note_context_switch exit from critical section
> >     
> >     Because __rcu_read_unlock() can be preempted just before the call to
> >     rcu_read_unlock_special(), it is possible for a task to be preempted just
> >     before it would have fully exited its RCU read-side critical section.
> >     This would result in a needless extension of that critical section until
> >     that task was resumed, which might in turn result in a needlessly
> >     long grace period, needless RCU priority boosting, and needless
> >     force-quiescent-state actions.  Therefore, rcu_note_context_switch()
> >     invokes __rcu_read_unlock() followed by rcu_preempt_deferred_qs() when
> >     it detects this situation.  This action by rcu_note_context_switch()
> >     ends the RCU read-side critical section immediately.
> >     
> >     Of course, once the task resumes, it will invoke rcu_read_unlock_special()
> >     redundantly.  This is harmless because the fact that a preemption
> >     happened means that interrupts, preemption, and softirqs cannot
> >     have been disabled, so there would be no deferred quiescent state.
> >     While ->rcu_read_lock_nesting remains less than zero, none of the
> >     ->rcu_read_unlock_special.b bits can be set, and they were all zeroed by
> >     the call to rcu_note_context_switch() at task-preemption time.  Therefore,
> >     setting ->rcu_read_unlock_special.b.exp_hint to false has no effect.
> >     
> >     Therefore, the extra call to rcu_preempt_deferred_qs_irqrestore()
> >     would return immediately.  With one possible exception, which is
> >     if an expedited grace period started just as the task was being
> >     resumed, which could leave ->exp_deferred_qs set.  This will cause
> >     rcu_preempt_deferred_qs_irqrestore() to invoke rcu_report_exp_rdp(),
> >     reporting the quiescent state, just as it should.  (Such an expedited
> >     grace period won't affect the preemption code path due to interrupts
> >     having already been disabled.)
> >     
> >     But when rcu_note_context_switch() invokes __rcu_read_unlock(), it
> >     is doing so with preemption disabled, hence __rcu_read_unlock() will
> >     unconditionally defer the quiescent state, only to immediately invoke
> >     rcu_preempt_deferred_qs(), thus immediately reporting the deferred
> >     quiescent state.  It turns out to be safe (and faster) to instead
> >     just invoke rcu_preempt_deferred_qs() without the __rcu_read_unlock()
> >     middleman.
> >     
> >     Because this is the invocation during the preemption (as opposed to
> >     the invocation just after the resume), at least one of the bits in
> >     ->rcu_read_unlock_special.b must be set and ->rcu_read_lock_nesting
> >     must be negative.  This means that rcu_preempt_need_deferred_qs() must
> >     return true, avoiding the early exit from rcu_preempt_deferred_qs().
> >     Thus, rcu_preempt_deferred_qs_irqrestore() will be invoked immediately,
> >     as required.
> >     
> >     This commit therefore simplifies the CONFIG_PREEMPT=y version of
> >     rcu_note_context_switch() by removing the "else if" branch of its
> >     "if" statement.  This change means that all callers that would have
> >     invoked rcu_read_unlock_special() followed by rcu_preempt_deferred_qs()
> >     will now simply invoke rcu_preempt_deferred_qs(), thus avoiding the
> >     rcu_read_unlock_special() middleman when __rcu_read_unlock() is preempted.
> >     
> >     Cc: rcu@vger.kernel.org
> >     Cc: kernel-team@android.com
> >     Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> >     Signed-off-by: Paul E. McKenney <paulmck@linux.ibm.com>
> > 
> > diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> > index 187dc076c497..214e4689c29d 100644
> > --- a/kernel/rcu/tree_plugin.h
> > +++ b/kernel/rcu/tree_plugin.h
> > @@ -313,15 +313,6 @@ void rcu_note_context_switch(bool preempt)
> >  				       ? rnp->gp_seq
> >  				       : rcu_seq_snap(&rnp->gp_seq));
> >  		rcu_preempt_ctxt_queue(rnp, rdp);
> > -	} else if (t->rcu_read_lock_nesting < 0 &&
> > -		   t->rcu_read_unlock_special.s) {
> > -
> > -		/*
> > -		 * Complete exit from RCU read-side critical section on
> > -		 * behalf of preempted instance of __rcu_read_unlock().
> > -		 */
> > -		rcu_read_unlock_special(t);
> > -		rcu_preempt_deferred_qs(t);
> >  	} else {
> >  		rcu_preempt_deferred_qs(t);
> >  	}
> > 
> 

[RFC 3/3] Revert "rcutorture: Tweak kvm options"

[Joel Fernandes (Google)]

This reverts commit a6fda6dab93c2c06ef4b8cb4b9258df6674d2438 which
causes kvm.sh to not run on my machines. The qemu-system-x86_64 command
runs but does nothing.

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
I am Ok if we want to drop this patch but it is in my tree because
without it I can't run the tests.

 tools/testing/selftests/rcutorture/bin/functions.sh | 13 +------------
 .../selftests/rcutorture/configs/rcu/CFcommon       |  3 ---
 2 files changed, 1 insertion(+), 15 deletions(-)

diff --git a/tools/testing/selftests/rcutorture/bin/functions.sh b/tools/testing/selftests/rcutorture/bin/functions.sh
index c3a49fb4d6f6..6bcb8b5b2ff2 100644
--- a/tools/testing/selftests/rcutorture/bin/functions.sh
+++ b/tools/testing/selftests/rcutorture/bin/functions.sh
@@ -172,7 +172,7 @@ identify_qemu_append () {
 	local console=ttyS0
 	case "$1" in
 	qemu-system-x86_64|qemu-system-i386)
-		echo selinux=0 initcall_debug debug
+		echo noapic selinux=0 initcall_debug debug
 		;;
 	qemu-system-aarch64)
 		console=ttyAMA0
@@ -191,19 +191,8 @@ identify_qemu_append () {
 # Output arguments for qemu arguments based on the TORTURE_QEMU_MAC
 # and TORTURE_QEMU_INTERACTIVE environment variables.
 identify_qemu_args () {
-	local KVM_CPU=""
-	case "$1" in
-	qemu-system-x86_64)
-		KVM_CPU=kvm64
-		;;
-	qemu-system-i386)
-		KVM_CPU=kvm32
-		;;
-	esac
 	case "$1" in
 	qemu-system-x86_64|qemu-system-i386)
-		echo -machine q35,accel=kvm
-		echo -cpu ${KVM_CPU}
 		;;
 	qemu-system-aarch64)
 		echo -machine virt,gic-version=host -cpu host
diff --git a/tools/testing/selftests/rcutorture/configs/rcu/CFcommon b/tools/testing/selftests/rcutorture/configs/rcu/CFcommon
index e19a444a0684..d2d2a86139db 100644
--- a/tools/testing/selftests/rcutorture/configs/rcu/CFcommon
+++ b/tools/testing/selftests/rcutorture/configs/rcu/CFcommon
@@ -1,5 +1,2 @@
 CONFIG_RCU_TORTURE_TEST=y
 CONFIG_PRINTK_TIME=y
-CONFIG_HYPERVISOR_GUEST=y
-CONFIG_PARAVIRT=y
-CONFIG_KVM_GUEST=y
-- 
2.22.0.410.gd8fdbe21b5-goog

Re: [RFC 3/3] Revert "rcutorture: Tweak kvm options"

[Sebastian Andrzej Siewior]

On 2019-07-01 00:04:15 [-0400], Joel Fernandes (Google) wrote:
> This reverts commit a6fda6dab93c2c06ef4b8cb4b9258df6674d2438 which
> causes kvm.sh to not run on my machines. The qemu-system-x86_64 command
> runs but does nothing.

Nope. I would like to know *why* you need 'noapic' to work. Is it a
brand new or old qemu-system-x86_64?

> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Sebastian

Re: [RFC 3/3] Revert "rcutorture: Tweak kvm options"

[Joel Fernandes]

On Mon, Jul 01, 2019 at 02:23:58PM +0200, Sebastian Andrzej Siewior wrote:
> On 2019-07-01 00:04:15 [-0400], Joel Fernandes (Google) wrote:
> > This reverts commit a6fda6dab93c2c06ef4b8cb4b9258df6674d2438 which
> > causes kvm.sh to not run on my machines. The qemu-system-x86_64 command
> > runs but does nothing.
> 
> Nope. I would like to know *why* you need 'noapic' to work. Is it a
> brand new or old qemu-system-x86_64?

I did not have time to debug yesterday and I posted this particular revert as
an 'RFC' just to make aware of this problem.

I spent some more time just now, it looks like this has nothing to do with
'noapic' and appears to be a problem on debian distros with the e1000e NIC.
May be this NIC was added to the virtual hardware because of -machine in the
patch?

Any if I add the following to the qemu command that kvm.sh runs, it works again:
-net nic,model=e1000

Without it I get:
 qemu-system-x86_64: Initialization of device e1000e failed: failed to find romfile "efi-e1000e.rom"

Seems to be mentioned here:
https://bugs.launchpad.net/ubuntu/+source/ipxe/+bug/1737211

And in syzkaller as well:
https://github.com/google/syzkaller/blob/master/vm/qemu/qemu.go#L88

Adding Dmitry who is syzkaller owner for any thoughts as well.

I'm happy to write a patch to set the nic model as e1000 and send it out if
we agree this solution is good enough.

 - Joel

Re: [RFC 3/3] Revert "rcutorture: Tweak kvm options"

[Dmitry Vyukov]

On Mon, Jul 1, 2019 at 4:14 PM Joel Fernandes <joel@joelfernandes.org> wrote:
>
> On Mon, Jul 01, 2019 at 02:23:58PM +0200, Sebastian Andrzej Siewior wrote:
> > On 2019-07-01 00:04:15 [-0400], Joel Fernandes (Google) wrote:
> > > This reverts commit a6fda6dab93c2c06ef4b8cb4b9258df6674d2438 which
> > > causes kvm.sh to not run on my machines. The qemu-system-x86_64 command
> > > runs but does nothing.
> >
> > Nope. I would like to know *why* you need 'noapic' to work. Is it a
> > brand new or old qemu-system-x86_64?
>
> I did not have time to debug yesterday and I posted this particular revert as
> an 'RFC' just to make aware of this problem.
>
> I spent some more time just now, it looks like this has nothing to do with
> 'noapic' and appears to be a problem on debian distros with the e1000e NIC.
> May be this NIC was added to the virtual hardware because of -machine in the
> patch?
>
> Any if I add the following to the qemu command that kvm.sh runs, it works again:
> -net nic,model=e1000
>
> Without it I get:
>  qemu-system-x86_64: Initialization of device e1000e failed: failed to find romfile "efi-e1000e.rom"
>
> Seems to be mentioned here:
> https://bugs.launchpad.net/ubuntu/+source/ipxe/+bug/1737211
>
> And in syzkaller as well:
> https://github.com/google/syzkaller/blob/master/vm/qemu/qemu.go#L88
>
> Adding Dmitry who is syzkaller owner for any thoughts as well.

I don't have many thoughts on this. That particular error looked like
a bug in the package in the particular distro/version.


> I'm happy to write a patch to set the nic model as e1000 and send it out if
> we agree this solution is good enough.
>
>  - Joel
>

[tip: core/rcu] torture: Remove qemu dependency on EFI firmware

[tip-bot2 for Paul E. McKenney]

The following commit has been merged into the core/rcu branch of tip:

Commit-ID:     88513ae533756d10358e406743c21e8cf61fb72a
Gitweb:        https://git.kernel.org/tip/88513ae533756d10358e406743c21e8cf61fb72a
Author:        Paul E. McKenney <paulmck@kernel.org>
AuthorDate:    Tue, 28 Apr 2020 14:41:48 -07:00
Committer:     Paul E. McKenney <paulmck@kernel.org>
CommitterDate: Mon, 29 Jun 2020 12:01:43 -07:00

torture:  Remove qemu dependency on EFI firmware

On some (probably misconfigured) systems, the torture-test scripting
will cause qemu to complain about missing EFI firmware, often because
qemu is trying to traverse broken symbolic links to find that firmware.
Which is a bit silly given that the default torture-test guest OS has
but a single binary for its userspace, and thus is unlikely to do much
in the way of networking in any case.

This commit therefore avoids such problems by specifying "-net none"
to qemu unless the TORTURE_QEMU_INTERACTIVE environment variable is set
(for example, by having specified "--interactive" to kvm.sh), in which
case "-net nic -net user" is specified to qemu instead.  Either choice
may be overridden by specifying the "-net" argument of your choice to
the kvm.sh "--qemu-args" parameter.

Link: https://lore.kernel.org/lkml/20190701141403.GA246562@google.com
Reported-by: Joel Fernandes <joel@joelfernandes.org>
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 tools/testing/selftests/rcutorture/bin/functions.sh      | 21 ++++++-
 tools/testing/selftests/rcutorture/bin/kvm-test-1-run.sh |  1 +-
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/rcutorture/bin/functions.sh b/tools/testing/selftests/rcutorture/bin/functions.sh
index 1281022..436b154 100644
--- a/tools/testing/selftests/rcutorture/bin/functions.sh
+++ b/tools/testing/selftests/rcutorture/bin/functions.sh
@@ -215,9 +215,6 @@ identify_qemu_args () {
 		then
 			echo -device spapr-vlan,netdev=net0,mac=$TORTURE_QEMU_MAC
 			echo -netdev bridge,br=br0,id=net0
-		elif test -n "$TORTURE_QEMU_INTERACTIVE"
-		then
-			echo -net nic -net user
 		fi
 		;;
 	esac
@@ -275,3 +272,21 @@ specify_qemu_cpus () {
 		esac
 	fi
 }
+
+# specify_qemu_net qemu-args
+#
+# Appends a string containing "-net none" to qemu-args, unless the incoming
+# qemu-args already contains "-smp" or unless the TORTURE_QEMU_INTERACTIVE
+# environment variable is set, in which case the string that is be added is
+# instead "-net nic -net user".
+specify_qemu_net () {
+	if echo $1 | grep -q -e -net
+	then
+		echo $1
+	elif test -n "$TORTURE_QEMU_INTERACTIVE"
+	then
+		echo $1 -net nic -net user
+	else
+		echo $1 -net none
+	fi
+}
diff --git a/tools/testing/selftests/rcutorture/bin/kvm-test-1-run.sh b/tools/testing/selftests/rcutorture/bin/kvm-test-1-run.sh
index 6ff611c..1b9aebd 100755
--- a/tools/testing/selftests/rcutorture/bin/kvm-test-1-run.sh
+++ b/tools/testing/selftests/rcutorture/bin/kvm-test-1-run.sh
@@ -141,6 +141,7 @@ then
 	cpu_count=$TORTURE_ALLOTED_CPUS
 fi
 qemu_args="`specify_qemu_cpus "$QEMU" "$qemu_args" "$cpu_count"`"
+qemu_args="`specify_qemu_net "$qemu_args"`"
 
 # Generate architecture-specific and interaction-specific qemu arguments
 qemu_args="$qemu_args `identify_qemu_args "$QEMU" "$resdir/console.log"`"

Re: [RFC 1/3] rcu: Expedite the rcu quiescent state reporting if help needed

[Joel Fernandes]

On Mon, Jul 01, 2019 at 12:04:13AM -0400, Joel Fernandes (Google) wrote:
> The t->rcu_read_unlock_special union's need_qs bit can be set by the
> scheduler tick (in rcu_flavor_sched_clock_irq) to indicate that help is
> needed from the rcu_read_unlock path. When this help arrives however, we
> can do better to speed up the quiescent state reporting which if
> rcu_read_unlock_special::need_qs is set might be quite urgent. Make use
> of this information in deciding when to do heavy-weight softirq raising
> where possible.

Just fyi, TREE01-06, SRCU-N and SRCU-t passed overnight testing with this series.

Re: [RFC 1/3] rcu: Expedite the rcu quiescent state reporting if help needed

[Paul E. McKenney]

On Mon, Jul 01, 2019 at 12:04:13AM -0400, Joel Fernandes (Google) wrote:
> The t->rcu_read_unlock_special union's need_qs bit can be set by the
> scheduler tick (in rcu_flavor_sched_clock_irq) to indicate that help is
> needed from the rcu_read_unlock path. When this help arrives however, we
> can do better to speed up the quiescent state reporting which if
> rcu_read_unlock_special::need_qs is set might be quite urgent. Make use
> of this information in deciding when to do heavy-weight softirq raising
> where possible.
> 
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Cute thought, but I am going to have to pass on this one.  The reason
is that by the time that ->rcu_read_unlock_special.b.need_qs gets set,
the grace period is already one full second old.  At that point, the
extra tick of waiting is down in the noise.

Right now, we do the extra work if we really are blocking an expedited
grace period (the first two lines of the original condition) or we are
running on a nohz_full CPU (which might never execute a scheduling clock
tick, thus potentially delaying forever).  And expedited grace periods
are supposed to complete in tens or maybe hundreds of microseconds,
assuming the RCU readers are being cooperative, which is a whole
different level of urgent.

Nevertheless, thank you for looking into this!

							Thanx, Paul

> ---
>  kernel/rcu/tree_plugin.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> index c588ef98efd3..bff6410fac06 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -622,7 +622,8 @@ static void rcu_read_unlock_special(struct task_struct *t)
>  		t->rcu_read_unlock_special.b.exp_hint = false;
>  		exp = (t->rcu_blocked_node && t->rcu_blocked_node->exp_tasks) ||
>  		      (rdp->grpmask & rnp->expmask) ||
> -		      tick_nohz_full_cpu(rdp->cpu);
> +		      tick_nohz_full_cpu(rdp->cpu)  ||
> +		      t->rcu_read_unlock_special.b.need_qs;
>  		// Need to defer quiescent state until everything is enabled.
>  		if (irqs_were_disabled && use_softirq &&
>  		    (in_interrupt() ||
> -- 
> 2.22.0.410.gd8fdbe21b5-goog
> 

Re: [RFC 1/3] rcu: Expedite the rcu quiescent state reporting if help needed

[Joel Fernandes]

On Mon, Jul 01, 2019 at 08:47:30PM -0700, Paul E. McKenney wrote:
> On Mon, Jul 01, 2019 at 12:04:13AM -0400, Joel Fernandes (Google) wrote:
> > The t->rcu_read_unlock_special union's need_qs bit can be set by the
> > scheduler tick (in rcu_flavor_sched_clock_irq) to indicate that help is
> > needed from the rcu_read_unlock path. When this help arrives however, we
> > can do better to speed up the quiescent state reporting which if
> > rcu_read_unlock_special::need_qs is set might be quite urgent. Make use
> > of this information in deciding when to do heavy-weight softirq raising
> > where possible.
> > 
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> 
> Cute thought, but I am going to have to pass on this one.  The reason
> is that by the time that ->rcu_read_unlock_special.b.need_qs gets set,
> the grace period is already one full second old.  At that point, the
> extra tick of waiting is down in the noise.
> 
> Right now, we do the extra work if we really are blocking an expedited
> grace period (the first two lines of the original condition) or we are
> running on a nohz_full CPU (which might never execute a scheduling clock
> tick, thus potentially delaying forever).  And expedited grace periods
> are supposed to complete in tens or maybe hundreds of microseconds,
> assuming the RCU readers are being cooperative, which is a whole
> different level of urgent.

Makes sense, I agree the patch may not be that helpful right now. I mixed up
the different levels or urgencies. No problem dropping it.

> 
> Nevertheless, thank you for looking into this!

My pleasure! Will keep them coming.

 - Joel

> 
> 							Thanx, Paul
> 
> > ---
> >  kernel/rcu/tree_plugin.h | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> > index c588ef98efd3..bff6410fac06 100644
> > --- a/kernel/rcu/tree_plugin.h
> > +++ b/kernel/rcu/tree_plugin.h
> > @@ -622,7 +622,8 @@ static void rcu_read_unlock_special(struct task_struct *t)
> >  		t->rcu_read_unlock_special.b.exp_hint = false;
> >  		exp = (t->rcu_blocked_node && t->rcu_blocked_node->exp_tasks) ||
> >  		      (rdp->grpmask & rnp->expmask) ||
> > -		      tick_nohz_full_cpu(rdp->cpu);
> > +		      tick_nohz_full_cpu(rdp->cpu)  ||
> > +		      t->rcu_read_unlock_special.b.need_qs;
> >  		// Need to defer quiescent state until everything is enabled.
> >  		if (irqs_were_disabled && use_softirq &&
> >  		    (in_interrupt() ||
> > -- 
> > 2.22.0.410.gd8fdbe21b5-goog
> > 
>