Re: [PATCH] x86/rcutorture move CONFIG_HYPERVISOR_GUEST to kvm-test-1-run.sh
On Tue, Apr 30, 2024 at 7:22 AM Paul E. McKenney wrote:
>
> On Sat, Apr 27, 2024 at 12:56:26AM +, Zhouyi Zhou wrote:
> > CONFIG_HYPERVISOR_GUEST is a x86 specific kernel option, move to
> > kvm-test-1-run.sh to avoid ConfigFragment.diags in non-x86 platforms.
> >
> > Tested in both PPC VM of Open Source lab of Oregon State University and
> > local x86_64 server.
> >
> > Fixes: a6fda6dab93c ("rcutorture: Tweak kvm options")
> > Signed-off-by: Zhouyi Zhou
>
> I like this general approach, but why not have something like a
> CFCOMMON.arch for the various "arch"s that need it?
>
> Better yet, I bet that there is already something somewhere in the
> Linux kernel source tree that knows what hypervisor Kconfig options
> each architecture needs. If so, it would be most excellent to reuse
> that information instead of maintaining it separately in RCU.
Thank Paul for your great guidance and valuable advice!
I will do a thorough research on the above approaches.
Thanks again ;-)
Zhouyi
>
> Thanx, Paul
>
> > ---
> > tools/testing/selftests/rcutorture/bin/kvm-test-1-run.sh | 4
> > tools/testing/selftests/rcutorture/configs/rcu/CFcommon | 1 -
> > 2 files changed, 4 insertions(+), 1 deletion(-)
> >
> > 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 b33cd8753689..75774bc70be7 100755
> > --- a/tools/testing/selftests/rcutorture/bin/kvm-test-1-run.sh
> > +++ b/tools/testing/selftests/rcutorture/bin/kvm-test-1-run.sh
> > @@ -68,6 +68,10 @@ config_override_param "--gdb options" KcList
> > "$TORTURE_KCONFIG_GDB_ARG"
> > config_override_param "--kasan options" KcList "$TORTURE_KCONFIG_KASAN_ARG"
> > config_override_param "--kcsan options" KcList "$TORTURE_KCONFIG_KCSAN_ARG"
> > config_override_param "--kconfig argument" KcList "$TORTURE_KCONFIG_ARG"
> > +if uname -a | grep -q x86
> > +then
> > + config_override_param "x86 specific option" KcList
> > "CONFIG_HYPERVISOR_GUEST=y"
> > +fi
> > cp $T/KcList $resdir/ConfigFragment
> >
> > base_resdir=`echo $resdir | sed -e 's/\.[0-9]\+$//'`
> > diff --git a/tools/testing/selftests/rcutorture/configs/rcu/CFcommon
> > b/tools/testing/selftests/rcutorture/configs/rcu/CFcommon
> > index 0e92d85313aa..cf0387ae5358 100644
> > --- a/tools/testing/selftests/rcutorture/configs/rcu/CFcommon
> > +++ b/tools/testing/selftests/rcutorture/configs/rcu/CFcommon
> > @@ -1,6 +1,5 @@
> > CONFIG_RCU_TORTURE_TEST=y
> > CONFIG_PRINTK_TIME=y
> > -CONFIG_HYPERVISOR_GUEST=y
> > CONFIG_PARAVIRT=y
> > CONFIG_KVM_GUEST=y
> > CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC=n
> > --
> > 2.34.1
> >
Re: [PATCH] x86/rcutorture move CONFIG_HYPERVISOR_GUEST to kvm-test-1-run.sh
On Sat, Apr 27, 2024 at 12:56:26AM +, Zhouyi Zhou wrote:
> CONFIG_HYPERVISOR_GUEST is a x86 specific kernel option, move to
> kvm-test-1-run.sh to avoid ConfigFragment.diags in non-x86 platforms.
>
> Tested in both PPC VM of Open Source lab of Oregon State University and
> local x86_64 server.
>
> Fixes: a6fda6dab93c ("rcutorture: Tweak kvm options")
> Signed-off-by: Zhouyi Zhou
I like this general approach, but why not have something like a
CFCOMMON.arch for the various "arch"s that need it?
Better yet, I bet that there is already something somewhere in the
Linux kernel source tree that knows what hypervisor Kconfig options
each architecture needs. If so, it would be most excellent to reuse
that information instead of maintaining it separately in RCU.
Thanx, Paul
> ---
> tools/testing/selftests/rcutorture/bin/kvm-test-1-run.sh | 4
> tools/testing/selftests/rcutorture/configs/rcu/CFcommon | 1 -
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> 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 b33cd8753689..75774bc70be7 100755
> --- a/tools/testing/selftests/rcutorture/bin/kvm-test-1-run.sh
> +++ b/tools/testing/selftests/rcutorture/bin/kvm-test-1-run.sh
> @@ -68,6 +68,10 @@ config_override_param "--gdb options" KcList
> "$TORTURE_KCONFIG_GDB_ARG"
> config_override_param "--kasan options" KcList "$TORTURE_KCONFIG_KASAN_ARG"
> config_override_param "--kcsan options" KcList "$TORTURE_KCONFIG_KCSAN_ARG"
> config_override_param "--kconfig argument" KcList "$TORTURE_KCONFIG_ARG"
> +if uname -a | grep -q x86
> +then
> + config_override_param "x86 specific option" KcList
> "CONFIG_HYPERVISOR_GUEST=y"
> +fi
> cp $T/KcList $resdir/ConfigFragment
>
> base_resdir=`echo $resdir | sed -e 's/\.[0-9]\+$//'`
> diff --git a/tools/testing/selftests/rcutorture/configs/rcu/CFcommon
> b/tools/testing/selftests/rcutorture/configs/rcu/CFcommon
> index 0e92d85313aa..cf0387ae5358 100644
> --- a/tools/testing/selftests/rcutorture/configs/rcu/CFcommon
> +++ b/tools/testing/selftests/rcutorture/configs/rcu/CFcommon
> @@ -1,6 +1,5 @@
> CONFIG_RCU_TORTURE_TEST=y
> CONFIG_PRINTK_TIME=y
> -CONFIG_HYPERVISOR_GUEST=y
> CONFIG_PARAVIRT=y
> CONFIG_KVM_GUEST=y
> CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC=n
> --
> 2.34.1
>
Re: [PATCH 0/4] rcu/nocb cleanups
On Thu, Apr 25, 2024 at 04:18:31PM +0200, Frederic Weisbecker wrote: > Hi, > > Some cleanups before converting (de-)offloading to only work on offline > CPUs. Seeing no objections, I have queued this series for testing and review. Thanx, Paul > Frederic Weisbecker (4): > rcu/nocb: Fix segcblist state machine comments about bypass > rcu/nocb: Fix segcblist state machine stale comments about timers > rcu/nocb: Use kthread parking instead of ad-hoc implementation > rcu/nocb: Remove buggy bypass lock contention mitigation > > include/linux/rcu_segcblist.h | 88 +--- > kernel/rcu/tree.h | 1 - > kernel/rcu/tree_nocb.h| 149 ++ > 3 files changed, 79 insertions(+), 159 deletions(-) > > -- > 2.44.0 >
Re: [PATCH] rcu: Fix suspicious RCU usage in __do_softirq()
On Sat, Apr 27, 2024 at 06:28:08PM +0800, Zqiang wrote:
> Currently, the condition "__this_cpu_read(ksoftirqd) == current" is
> checked to ensure the rcu_softirq_qs() is invoked in ksoftirqd tasks
> context for non-RT kernels. however, in some scenarios, this condition
> will be broken.
>
> ksoftirqd/0
> ->finish_task_switch
> ->put_task_struct_rcu_user
> ->call_rcu(>rcu, delayed_put_task_struct)
> ->__kasan_record_aux_stack
> ->pfn_valid
> ->rcu_read_lock_sched()
>
> __irq_exit_rcu
> ->__do_softirq
>-> if (!IS_ENABLED(CONFIG_PREEMPT_RT) &&
>__this_cpu_read(ksoftirqd) == current)
>->rcu_softirq_qs
> ->RCU_LOCKDEP_WARN(lock_is_held(_sched_lock_map))
>
> The rcu quiescent states is reported occurs in the rcu-read critical
> section, so the lockdep warning is triggered. this commit therefore
> remove "__this_cpu_read(ksoftirqd) == current" condition check, generate
> new "handle_softirqs(bool kirqd)" function to replace __do_softirq() in
> run_ksoftirqdt(), and set parameter kirqd to true, make rcu_softirq_qs()
> be invoked only in ksofirqd tasks context for non-RT kernels.
>
> Reported-by: syzbot+dce04ed6d1438ad69...@syzkaller.appspotmail.com
> Link:
> https://lore.kernel.org/lkml/8f281a10-b85a-4586-9586-5bbc12dc784f@paulmck-laptop/T/#mea8aba4abfcb97bbf499d169ce7f30c4cff1b0e3
> Suggested-by: Thomas Gleixner
> Signed-off-by: Zqiang
Reviewed-by: Paul E. McKenney
> ---
> kernel/softirq.c | 12
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/softirq.c b/kernel/softirq.c
> index b315b21fb28c..e991d735be0d 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -508,7 +508,7 @@ static inline bool lockdep_softirq_start(void) { return
> false; }
> static inline void lockdep_softirq_end(bool in_hardirq) { }
> #endif
>
> -asmlinkage __visible void __softirq_entry __do_softirq(void)
> +static void handle_softirqs(bool kirqd)
> {
> unsigned long end = jiffies + MAX_SOFTIRQ_TIME;
> unsigned long old_flags = current->flags;
> @@ -563,8 +563,7 @@ asmlinkage __visible void __softirq_entry
> __do_softirq(void)
> pending >>= softirq_bit;
> }
>
> - if (!IS_ENABLED(CONFIG_PREEMPT_RT) &&
> - __this_cpu_read(ksoftirqd) == current)
> + if (!IS_ENABLED(CONFIG_PREEMPT_RT) && kirqd)
> rcu_softirq_qs();
>
> local_irq_disable();
> @@ -584,6 +583,11 @@ asmlinkage __visible void __softirq_entry
> __do_softirq(void)
> current_restore_flags(old_flags, PF_MEMALLOC);
> }
>
> +asmlinkage __visible void __softirq_entry __do_softirq(void)
> +{
> + handle_softirqs(false);
> +}
> +
> /**
> * irq_enter_rcu - Enter an interrupt context with RCU watching
> */
> @@ -921,7 +925,7 @@ static void run_ksoftirqd(unsigned int cpu)
>* We can safely run softirq on inline stack, as we are not deep
>* in the task stack here.
>*/
> - __do_softirq();
> + handle_softirqs(true);
> ksoftirqd_run_end();
> cond_resched();
> return;
> --
> 2.17.1
>
Re: [PATCH] Revert "rcu-tasks: Fix synchronize_rcu_tasks() VS zap_pid_ns_processes()"
On Mon, Apr 29, 2024 at 01:03:28PM +0200, Oleg Nesterov wrote:
> On 04/25, Frederic Weisbecker wrote:
> >
> > This reverts commit 28319d6dc5e2ffefa452c2377dd0f71621b5bff0. The race
> > it fixed was subject to conditions that don't exist anymore since:
> >
> > 1612160b9127 ("rcu-tasks: Eliminate deadlocks involving do_exit() and
> > RCU tasks")
> >
> > This latter commit removes the use of SRCU that used to cover the
> > RCU-tasks blind spot on exit between the tasklist's removal and the
> > final preemption disabling. The task is now placed instead into a
> > temporary list inside which voluntary sleeps are accounted as RCU-tasks
> > quiescent states. This would disarm the deadlock initially reported
> > against PID namespace exit.
> >
> > Signed-off-by: Frederic Weisbecker
> > ---
> > include/linux/rcupdate.h | 2 --
> > kernel/pid_namespace.c | 17 -
> > kernel/rcu/tasks.h | 16 +++-
> > 3 files changed, 3 insertions(+), 32 deletions(-)
>
> LGTM. FWIW,
>
> Reviewed-by: Oleg Nesterov
Queued, thank you both!
Thanx, Paul
Re: [PATCH] rcu: Remove unreachable logic
On 2024-04-29 13:34, Alan Huang wrote:
call_rcu_core is only called from __call_rcu_common with interrupt
disabled. This patch thus removes the unreachable logic and the would-be
unused 'flags' parameter.
Nack.
call_rcu_core() receives a @flags parameter which are the _saved_ flags
as they were prior to local_irq_save().
This patch highlights a misunderstanding of what the code is actually
doing.
Thanks,
Mathieu
Signed-off-by: Alan Huang
---
kernel/rcu/tree.c | 35 ++-
1 file changed, 2 insertions(+), 33 deletions(-)
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index d5507ac1bbf1..b0ea2ebd7769 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -2983,7 +2983,7 @@ static void rcutree_enqueue(struct rcu_data *rdp, struct
rcu_head *head, rcu_cal
* Handle any core-RCU processing required by a call_rcu() invocation.
*/
static void call_rcu_core(struct rcu_data *rdp, struct rcu_head *head,
- rcu_callback_t func, unsigned long flags)
+ rcu_callback_t func)
{
rcutree_enqueue(rdp, head, func);
/*
@@ -2992,37 +2992,6 @@ static void call_rcu_core(struct rcu_data *rdp, struct
rcu_head *head,
*/
if (!rcu_is_watching())
invoke_rcu_core();
-
- /* If interrupts were disabled or CPU offline, don't invoke RCU core. */
- if (irqs_disabled_flags(flags) || cpu_is_offline(smp_processor_id()))
- return;
-
- /*
-* Force the grace period if too many callbacks or too long waiting.
-* Enforce hysteresis, and don't invoke rcu_force_quiescent_state()
-* if some other CPU has recently done so. Also, don't bother
-* invoking rcu_force_quiescent_state() if the newly enqueued callback
-* is the only one waiting for a grace period to complete.
-*/
- if (unlikely(rcu_segcblist_n_cbs(>cblist) >
-rdp->qlen_last_fqs_check + qhimark)) {
-
- /* Are we ignoring a completed grace period? */
- note_gp_changes(rdp);
-
- /* Start a new grace period if one not already started. */
- if (!rcu_gp_in_progress()) {
- rcu_accelerate_cbs_unlocked(rdp->mynode, rdp);
- } else {
- /* Give the grace period a kick. */
- rdp->blimit = DEFAULT_MAX_RCU_BLIMIT;
- if (READ_ONCE(rcu_state.n_force_qs) == rdp->n_force_qs_snap
&&
- rcu_segcblist_first_pend_cb(>cblist) != head)
- rcu_force_quiescent_state();
- rdp->n_force_qs_snap = READ_ONCE(rcu_state.n_force_qs);
- rdp->qlen_last_fqs_check =
rcu_segcblist_n_cbs(>cblist);
- }
- }
}
/*
@@ -3121,7 +3090,7 @@ __call_rcu_common(struct rcu_head *head, rcu_callback_t
func, bool lazy_in)
if (unlikely(rcu_rdp_is_offloaded(rdp)))
call_rcu_nocb(rdp, head, func, flags, lazy);
else
- call_rcu_core(rdp, head, func, flags);
+ call_rcu_core(rdp, head, func);
local_irq_restore(flags);
}
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com
[PATCH] rcu: Remove unreachable logic
call_rcu_core is only called from __call_rcu_common with interrupt
disabled. This patch thus removes the unreachable logic and the would-be
unused 'flags' parameter.
Signed-off-by: Alan Huang
---
kernel/rcu/tree.c | 35 ++-
1 file changed, 2 insertions(+), 33 deletions(-)
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index d5507ac1bbf1..b0ea2ebd7769 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -2983,7 +2983,7 @@ static void rcutree_enqueue(struct rcu_data *rdp, struct
rcu_head *head, rcu_cal
* Handle any core-RCU processing required by a call_rcu() invocation.
*/
static void call_rcu_core(struct rcu_data *rdp, struct rcu_head *head,
- rcu_callback_t func, unsigned long flags)
+ rcu_callback_t func)
{
rcutree_enqueue(rdp, head, func);
/*
@@ -2992,37 +2992,6 @@ static void call_rcu_core(struct rcu_data *rdp, struct
rcu_head *head,
*/
if (!rcu_is_watching())
invoke_rcu_core();
-
- /* If interrupts were disabled or CPU offline, don't invoke RCU core. */
- if (irqs_disabled_flags(flags) || cpu_is_offline(smp_processor_id()))
- return;
-
- /*
-* Force the grace period if too many callbacks or too long waiting.
-* Enforce hysteresis, and don't invoke rcu_force_quiescent_state()
-* if some other CPU has recently done so. Also, don't bother
-* invoking rcu_force_quiescent_state() if the newly enqueued callback
-* is the only one waiting for a grace period to complete.
-*/
- if (unlikely(rcu_segcblist_n_cbs(>cblist) >
-rdp->qlen_last_fqs_check + qhimark)) {
-
- /* Are we ignoring a completed grace period? */
- note_gp_changes(rdp);
-
- /* Start a new grace period if one not already started. */
- if (!rcu_gp_in_progress()) {
- rcu_accelerate_cbs_unlocked(rdp->mynode, rdp);
- } else {
- /* Give the grace period a kick. */
- rdp->blimit = DEFAULT_MAX_RCU_BLIMIT;
- if (READ_ONCE(rcu_state.n_force_qs) ==
rdp->n_force_qs_snap &&
- rcu_segcblist_first_pend_cb(>cblist) != head)
- rcu_force_quiescent_state();
- rdp->n_force_qs_snap = READ_ONCE(rcu_state.n_force_qs);
- rdp->qlen_last_fqs_check =
rcu_segcblist_n_cbs(>cblist);
- }
- }
}
/*
@@ -3121,7 +3090,7 @@ __call_rcu_common(struct rcu_head *head, rcu_callback_t
func, bool lazy_in)
if (unlikely(rcu_rdp_is_offloaded(rdp)))
call_rcu_nocb(rdp, head, func, flags, lazy);
else
- call_rcu_core(rdp, head, func, flags);
+ call_rcu_core(rdp, head, func);
local_irq_restore(flags);
}
--
2.44.0
Re: [PATCH] Revert "rcu-tasks: Fix synchronize_rcu_tasks() VS zap_pid_ns_processes()"
On 04/25, Frederic Weisbecker wrote:
>
> This reverts commit 28319d6dc5e2ffefa452c2377dd0f71621b5bff0. The race
> it fixed was subject to conditions that don't exist anymore since:
>
> 1612160b9127 ("rcu-tasks: Eliminate deadlocks involving do_exit() and
> RCU tasks")
>
> This latter commit removes the use of SRCU that used to cover the
> RCU-tasks blind spot on exit between the tasklist's removal and the
> final preemption disabling. The task is now placed instead into a
> temporary list inside which voluntary sleeps are accounted as RCU-tasks
> quiescent states. This would disarm the deadlock initially reported
> against PID namespace exit.
>
> Signed-off-by: Frederic Weisbecker
> ---
> include/linux/rcupdate.h | 2 --
> kernel/pid_namespace.c | 17 -
> kernel/rcu/tasks.h | 16 +++-
> 3 files changed, 3 insertions(+), 32 deletions(-)
LGTM. FWIW,
Reviewed-by: Oleg Nesterov
