Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
From:   "Zhang, Qiang1" <qiang1.zhang@intel.com>
To:     "paulmck@kernel.org" <paulmck@kernel.org>
CC:     "frederic@kernel.org" <frederic@kernel.org>,
        "rcu@vger.kernel.org" <rcu@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH] rcu: Dump all rcuc kthreads status for CPUs that not
 report quiescent state
Thread-Topic: [PATCH] rcu: Dump all rcuc kthreads status for CPUs that not
 report quiescent state
Thread-Index: AQHYTwpGNYsEpnO3pUOLiRwx3W6tm6zxsXSAgAApSfA=
Date:   Sat, 16 Apr 2022 03:08:47 +0000
Message-ID: <PH0PR11MB5880C3206B4BB782188844BDDAF19@PH0PR11MB5880.namprd11.prod.outlook.com>
References: <20220413074411.2369623-1-qiang1.zhang@intel.com>
 <20220416002025.GO4285@paulmck-ThinkPad-P17-Gen-1>
In-Reply-To: <20220416002025.GO4285@paulmck-ThinkPad-P17-Gen-1>
Accept-Language: en-US
Content-Language: en-US
dlp-product: dlpe-windows
dlp-version: 11.6.401.20
dlp-reaction: no-action
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?ST2bELaMbK7ui0BF3HIHNdewKtG/VDFmHZyy1FaFds/DgLJB/uVyz+O1/WWe?=
 =?us-ascii?Q?hkfCi4cY4hrePJYG9lHrSaBgXcD3IxTlf6U2u/6uB1ND3Bhf4+/A2j7AW9Tq?=
 =?us-ascii?Q?PDR276KsabF8Yfqz0QDrSXG0cwTzXIJHCvnT9s5QwkAu9PzLL884+DXYTZY3?=
 =?us-ascii?Q?N4guC3koJCf2zVXr4iK0GhDWLwk1PA5etWpmmvDyiVzGDIBaRClpjf7xYurj?=
 =?us-ascii?Q?5IR2U8QFCvv09r2Fyj8OhwO38aeUa/Daewv3HhG0G7wBzO1WTKpHQro7Onkb?=
 =?us-ascii?Q?8xIxLDDjNNZ9szwTvJ32/QZejttt0LZ/Bs10KaDjwR8xa5/LwcZ0Rfd7gDfJ?=
 =?us-ascii?Q?Y/7i5aWXmMx556NqKFGRFjltMkj9wlJmWf14h/G0sPV9JlVl3RoQHSJPN6Vj?=
 =?us-ascii?Q?8Huzc6HPJlBigI8QLrgW7c0RPW9PhWLj14nWTucO5YeMjcoE/3ZxEbf/6l2h?=
 =?us-ascii?Q?7iEWgKLgqQ9hcsj3ABbPHDWGDF2fL5kMSJB1bLyNQvlEQUNJzDTmRwtfL0Wl?=
 =?us-ascii?Q?q2F2Xy/1FXrosegCX3gVeFQe7BrMdrBlO6X0FTfqmg9VODpOvpBb27p48KgN?=
 =?us-ascii?Q?JeMj1aWo9s3tFa/autHMclkLFxgBA2ET2e3E0mERoQ3PFxhCyT2c4D6sl1kf?=
 =?us-ascii?Q?ShpJfFY6x7TPiRPOjAQLKK9wfFZ5yLZmzbTqZU9foijGkjoqeTO/FlmE0T6s?=
 =?us-ascii?Q?1fzlII3N5kpJXyJaIuG8BiEzXj+GglSzX1oK02y9YlCKZYUzNnXctRthIkT8?=
 =?us-ascii?Q?MUnCqwxGvQVvP3n5Qsi2BJjnbidSBf3owMmRbLdDLP2JMluFkFEi62KiolOW?=
 =?us-ascii?Q?0QTshKFkrHEHpFKH5UKacaNmKC+YakibftHsFITrKoM0PCExoS97CDyMDQMm?=
 =?us-ascii?Q?UTrU6Mu5hcUMT7RkULsjaqivxyBRo+vtOzRS5/81mq3ulwg72+O/F5EDLUzl?=
 =?us-ascii?Q?uDHAxlFyRU1S9Fr8Opyk2n43mSUm+BWwKzjcGK/7s1ULmDkANJFcNY6Y9bLa?=
 =?us-ascii?Q?oBFloWuXLjhZXeonGpSP0LWRJDatFxT1TIYr/2DmU/PjyoBRZO0JiIn5+HdH?=
 =?us-ascii?Q?V7Aa70pxwaLLenzcs8QTnb+e/DG5+VvxIrqDw2rEzrAsen4xG4qP4yBPebme?=
 =?us-ascii?Q?+SMMEEQooPUh7+F6A0fGDpAXfevDMcz2OTwWcCiARsnAx8kjvlSmjx0ItJWW?=
 =?us-ascii?Q?+NKqpTTKsLraizwTFqZNsqq15fWtJD94ZyHWOCtxyM2GqWijZc/Pia6YsTTj?=
 =?us-ascii?Q?fGLg6jE8dB/CQ2TOeTg8ngklyVw5iLADYEPxI5yfFC/5NdJNj5KzNptgQmjS?=
 =?us-ascii?Q?Xyt82LUt1NmrP470cJOKwmS6rotjSAY6oRn0sUdRhz2/bfF0VjlevkUXfLmc?=
 =?us-ascii?Q?556O6eshZ/z/DNaDD0N/Uw9g5w1CBgINExm4s3yvkufUSPA0PSZzCKcI0/2J?=
 =?us-ascii?Q?t1d+PJVvvVuKzHtBuT9BaATn98cgcYWBFCV8PLTfFdtQsH1kzgwawq8Ec2Hz?=
 =?us-ascii?Q?MM7O0s/rngIWni7oPcgUhOOy8EnbjNXNUu/h4ZVc/OLxaZs7X7tIg500QZDN?=
 =?us-ascii?Q?9STdt86titMkghcp8akUa9voqWBIL4knJu29yThEmFA7A/ReKqrfOviIrGOW?=
 =?us-ascii?Q?wEoyNq65LK3gGh6f9e/NTu3Vwe04Puww6vcpVMAYvCBQLYLA5yaUsNvqhv9j?=
 =?us-ascii?Q?5KJH/uiFRPDzgjbWIXmrbtfo6jKR2pqSPeaS8ZonT3w1fmH47jecB+hUdRrw?=
 =?us-ascii?Q?ltxaPSBdAw=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB5880.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2b0c47d3-0bb0-4bd9-7f1f-08da1f566cb3
X-MS-Exchange-CrossTenant-originalarrivaltime: 16 Apr 2022 03:08:47.7072
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 60TjPUw/v1/i67pvgvJ+8yQhFDQVZpd2Beg1X4BuwZDOfLZVn5rIDDccyc8G0NkdByxE6SqIbwtRxAVj49s7sQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR11MB3717
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
	DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,RDNS_NONE,SPF_HELO_NONE,T_SCC_BODY_TEXT_LINE
	autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, Apr 13, 2022 at 03:44:11PM +0800, Zqiang wrote:
> If the rcutree.use_softirq is configured, when RCU Stall event=20
> happened, dump status of all rcuc kthreads who due to starvation=20
> prevented grace period ends on CPUs that not report quiescent state.
>=20
> Signed-off-by: Zqiang <qiang1.zhang@intel.com>

>This could be a good improvement!  But a few questions and comments below.
>
>							Thanx, Paul

> ---
>  kernel/rcu/tree_stall.h | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>=20
> diff --git a/kernel/rcu/tree_stall.h b/kernel/rcu/tree_stall.h index=20
> d7956c03edbd..e6ecc32cfe23 100644
> --- a/kernel/rcu/tree_stall.h
> +++ b/kernel/rcu/tree_stall.h
> @@ -348,6 +348,7 @@ static int rcu_print_task_stall(struct rcu_node=20
> *rnp, unsigned long flags)  }  #endif /* #else #ifdef=20
> CONFIG_PREEMPT_RCU */
> =20
> +static void rcuc_kthread_dump(struct rcu_data *rdp);
>  /*
>   * Dump stacks of all tasks running on stalled CPUs.  First try using
>   * NMIs, but fall back to manual remote stack tracing on=20
> architectures @@ -368,6 +369,7 @@ static void rcu_dump_cpu_stacks(void)
>  					pr_err("Offline CPU %d blocking current GP.\n", cpu);
>  				else if (!trigger_single_cpu_backtrace(cpu))
>  					dump_cpu_task(cpu);
> +				rcuc_kthread_dump(per_cpu_ptr(&rcu_data, cpu));

>Was this addition inspired by the chasing of a bug?  If so, please let me =
know exactly what information was (or would have been) the most helpful.
>
>For example, the starvation information might be more compactly represente=
d in the information printed by print_cpu_stall_info().
>Unless the stack trace was quite valuable, it might be best to omit it.
>After all, RCU CPU stall warnings are already infamously heavy on the text=
 output.

Is it better to modify it like this?


diff --git a/kernel/rcu/tree_stall.h b/kernel/rcu/tree_stall.h
index d7956c03edbd..37444ff00787 100644
--- a/kernel/rcu/tree_stall.h
+++ b/kernel/rcu/tree_stall.h
@@ -465,27 +465,38 @@ static void print_cpu_stall_info(int cpu)
               falsepositive ? " (false positive?)" : "");
 }

-static void rcuc_kthread_dump(struct rcu_data *rdp)
+static void rcuc_kthread_dump(void)
 {
        int cpu;
        unsigned long j;
+       unsigned long flags
+       struct rcu_node *rnp;
        struct task_struct *rcuc;
+       struct rcu_data *rdp;

-       rcuc =3D rdp->rcu_cpu_kthread_task;
-       if (!rcuc)
-               return;
+       rcu_for_each_leaf_node(rnp) {
+               raw_spin_lock_irqsave_rcu_node(rnp, flags);
+               for_each_leaf_node_possible_cpu(rnp, cpu)
+                       if (rnp->qsmask & leaf_node_cpu_bit(rnp, cpu)) {
+                               rdp =3D per_cpu_ptr(&rcu_data, cpu);
+                               rcuc =3D rdp->rcu_cpu_kthread_task;
+                               if (!rcuc)
+                                       return;

-       cpu =3D task_cpu(rcuc);
-       if (cpu_is_offline(cpu) || idle_cpu(cpu))
-               return;
+                               cpu =3D task_cpu(rcuc);
+                               if (cpu_is_offline(cpu) || idle_cpu(cpu))
+                                       return;

-       if (!rcu_is_rcuc_kthread_starving(rdp, &j))
-               return;
+                               if (!rcu_is_rcuc_kthread_starving(rdp, &j))
+                                       return;

-       pr_err("%s kthread starved for %ld jiffies\n", rcuc->comm, j);
-       sched_show_task(rcuc);
-       if (!trigger_single_cpu_backtrace(cpu))
-               dump_cpu_task(cpu);
+                               pr_err("%s kthread starved for %ld jiffies\=
n", rcuc->comm, j);
+                               sched_show_task(rcuc);
+                               if (!trigger_single_cpu_backtrace(cpu))
+                                       dump_cpu_task(cpu);
+                       }
+               raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
+       }
 }

/* Complain about starvation of grace-period kthread.  */
@@ -595,6 +606,9 @@ static void print_other_cpu_stall(unsigned long gp_seq,=
 unsigned long gps)
               smp_processor_id(), (long)(jiffies - gps),
               (long)rcu_seq_current(&rcu_state.gp_seq), totqlen, rcu_state=
.n_online_cpus);
        if (ndetected) {
+               if (!use_softirq)
+                       rcuc_kthread_dump();
+
                rcu_dump_cpu_stacks();

                /* Complain about tasks blocking the grace period. */
@@ -660,7 +674,7 @@ static void print_cpu_stall(unsigned long gps)
        rcu_check_gp_kthread_starvation();

        if (!use_softirq)
-               rcuc_kthread_dump(rdp);
+               rcuc_kthread_dump();

        rcu_dump_cpu_stacks();

Thanks
Zqiang


>  			}
>  		raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
>  	}
> @@ -471,6 +473,9 @@ static void rcuc_kthread_dump(struct rcu_data *rdp)
>  	unsigned long j;
>  	struct task_struct *rcuc;
> =20
> +	if (use_softirq)
> +		return;
> +
>  	rcuc =3D rdp->rcu_cpu_kthread_task;
>  	if (!rcuc)

>The checks for use_softirq on the one hand and the checks for non-NULL
>rdp->rcu_cpu_kthread_task are a bit "interesting".  Not your fault or
>problem, of course!

Yes the ' if (!use_softirq)' is redundant,  the if rcu_cpu_kthread_task  Is=
 completely enough

>
>  		return;
> @@ -659,9 +664,6 @@ static void print_cpu_stall(unsigned long gps)
>  	rcu_check_gp_kthread_expired_fqs_timer();
>  	rcu_check_gp_kthread_starvation();
> =20
> -	if (!use_softirq)
>
>In particular, I am wondering if this "if" is redundant.

Yes the ' if (!use_softirq)' is also redundant

>
> -		rcuc_kthread_dump(rdp);
> -
>  	rcu_dump_cpu_stacks();
> =20
>  	raw_spin_lock_irqsave_rcu_node(rnp, flags);
> --
> 2.25.1
>=20