On Wed, Sep 09, 2020 at 07:03:39AM +0000, Zhang, Qiang wrote:
>
> When config preempt RCU, and then there are multiple levels node, the
> current task is preempted in rcu read critical region.
> the current task be add to "rnp->blkd_tasks" link list, and the
> "rnp->gp_tasks" may be assigned a value . these rnp is leaf node in RCU
> tree.
>
> But in "rcu_gp_fqs_loop" func, we check blocked readers in root node.
>
> static void rcu_gp_fqs_loop(void)
> {
> .....
> struct rcu_node *rnp = rcu_get_root();
> .....
> if (!READ_ONCE(rnp->qsmask) &&
> !rcu_preempt_blocked_readers_cgp(rnp))
> ------> rnp is root node
> break;
> ....
> }
>
> the root node's blkd_tasks never add task, the "rnp->gp_tasks" is never be
> assigned value, this check is invailed.
> Should we check leaf nodes like this
There are two cases:
1. There is only a single rcu_node structure, which is both root
and leaf. In this case, the current check is required: Both
->qsmask and the ->blkd_tasks list must be checked. Your
rcu_preempt_blocked_readers() would work in this case, but
the current code is a bit faster because it does not need
to acquire the ->lock nor does it need the loop overhead.
2. There are multiple levels. In this case, as you say, the root
rcu_node structure's ->blkd_tasks list will always be empty.
But also in this case, the root rcu_node structure's ->qsmask
cannot be zero until all the leaf rcu_node structures' ->qsmask
fields are zero and their ->blkd_tasks lists no longer have
tasks blocking the current grace period. This means that your
rcu_preempt_blocked_readers() function would never return
true in this case.
So the current code is fine.
Are you seeing failures on mainline kernels? If so, what is the failure
mode?
Thanx, Paul
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -1846,6 +1846,25 @@ static bool rcu_gp_init(void)
> return true;
> }
>
> +static bool rcu_preempt_blocked_readers(void)
> +{
> + struct rcu_node *rnp;
> + unsigned long flags;
> + bool ret = false;
> +
> + rcu_for_each_leaf_node(rnp) {
> + raw_spin_lock_irqsave_rcu_node(rnp, flags);
> + if (rcu_preempt_blocked_readers_cgp(rnp)) {
> + ret = true;
> + raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
> + break;
> + }
> + raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
> + }
> +
> + return ret;
> +}
> +
> /*
> * Helper function for swait_event_idle_exclusive() wakeup at
> force-quiescent-state
> * time.
> @@ -1864,7 +1883,7 @@ static bool rcu_gp_fqs_check_wake(int *gfp)
> return true;
>
> // The current grace period has completed.
> - if (!READ_ONCE(rnp->qsmask) && !rcu_preempt_blocked_readers_cgp(rnp))
> + if (!READ_ONCE(rnp->qsmask) && !rcu_preempt_blocked_readers())
> return true;
>
> return false;
> @@ -1927,7 +1946,7 @@ static void rcu_gp_fqs_loop(void)
> /* Locking provides needed memory barriers. */
> /* If grace period done, leave loop. */
> if (!READ_ONCE(rnp->qsmask) &&
> - !rcu_preempt_blocked_readers_cgp(rnp))
> + !rcu_preempt_blocked_readers())
> break;
> /* If time for quiescent-state forcing, do it. */
> if (!time_after(rcu_state.jiffies_force_qs, jiffies) ||
> --
>
>
> thanks
> Qiang
