linux-kernel - Re: [PATCH] rcu: Start grace period even for single callback

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150611150715.GI3913@linux.vnet.ibm.com>
Date:	Thu, 11 Jun 2015 08:07:16 -0700
From:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:	Kishan Kumar <kishank@...eaurora.org>
Cc:	josh@...htriplett.org, rostedt@...dmis.org,
	mathieu.desnoyers@...icios.com, laijs@...fujitsu.com,
	linux-kernel@...r.kernel.org, kaushalk@...eaurora.org,
	Mohammed Khajapasha <mkhaja@...eaurora.org>,
	Vignesh Radhakrishnan <vigneshr@...eaurora.org>
Subject: Re: [PATCH] rcu: Start grace period even for single callback

On Thu, Jun 11, 2015 at 07:18:18PM +0530, Kishan Kumar wrote:
> When we queue a callback for RCU, it goes and sits
> on the nxttail of the per-cpu RCU data structure.
> Callback is represented via struct callback_head
> structure.
> 
> struct callback_head {
>         struct callback_head *next;
>         void (*func)(struct callback_head *head);
> };
> 
> In case of a single callback queued in the nxttail,
> the next field will be NULL. "next" happens to be
> the zeroeth element of struct callback_head.
> 
> The condition "if(*rdp->nxttail[RCU_NEXT_READY_TAIL])"
> in the function cpu_needs_another_gp(), essentially
> checks if any callback is queued.
> 
> Since *rdp->nxttail[RCU_NEXT_READY_TAIL] dereferences
> to the first element, the if condition will just turn
> out to be if(NULL) in case there is a single callback
> queued. This in turn causes cpu_needs_another_gp() to
> return false even though we need a grace period to
> process the single callback. This leads to writers
> waiting until a second call_back gets queued, which can
> cause undesirable effects like boot up delay upto 300
> seconds, etc.
> 
> Fix this by performing this check on the "func" field
> rather than the "next" field of the callback_head.
> 
> Signed-off-by: Kishan Kumar <kishank@...eaurora.org>
> Signed-off-by: Mohammed Khajapasha <mkhaja@...eaurora.org>
> Signed-off-by: Vignesh Radhakrishnan <vigneshr@...eaurora.org>

Hmmm...

Exactly what did you do to test the problem and verify your fix?

>From what I can see, if there is exactly one newly queued callback, we will
have the following:

o	rdp->nxttail[RCU_DONE_TAIL] == &rdp->nxtlist
o	rdp->nxttail[RCU_WAIT_TAIL] == &rdp->nxtlist
o	rdp->nxttail[RCU_NEXT_READY_TAIL] == &rdp->nxtlist
o	rdp->nxttail[RCU_NEXT_TAIL] == &rdp->nxtlist->next

And in this case, rdp->nxtlist will reference the first callback.
So *rdp->nxttail[RCU_NEXT_READY_TAIL] will be non-NULL in this case,
as required.

So what am I missing here?

Also, what version of the kernel are you using?  Are you posting callbacks
during early boot?  (Such callbacks won't be invoked until after the
scheduler starts running.)  What kernel configuration are you using?

							Thanx, Paul

> ---
>  kernel/rcu/tree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index fc3abf1..394a4fa 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -324,7 +324,7 @@ cpu_needs_another_gp(struct rcu_state *rsp, struct rcu_data *rdp)
>  		return 1;  /* Yes, a no-CBs CPU needs one. */
>  	if (!rdp->nxttail[RCU_NEXT_TAIL])
>  		return 0;  /* No, this is a no-CBs (or offline) CPU. */
> -	if (*rdp->nxttail[RCU_NEXT_READY_TAIL])
> +	if (((struct callback_head *)rdp->nxttail[RCU_NEXT_READY_TAIL])->func)
>  		return 1;  /* Yes, this CPU has newly registered callbacks. */
>  	for (i = RCU_WAIT_TAIL; i < RCU_NEXT_TAIL; i++)
>  		if (rdp->nxttail[i - 1] != rdp->nxttail[i] &&
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation.
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/