[PATCH] blkcg: not hold blkcg lock when deactivating policy.

[PATCH] blkcg: not hold blkcg lock when deactivating policy.

[Jiang Biao]

As described in the comment of blkcg_activate_policy(),
*Update of each blkg is protected by both queue and blkcg locks so
that holding either lock and testing blkcg_policy_enabled() is
always enough for dereferencing policy data.*
with queue lock held, there is no need to hold blkcg lock in
blkcg_deactivate_policy(). Similar case is in
blkcg_activate_policy(), which has removed holding of blkcg lock in
commit 4c55f4f9ad3001ac1fefdd8d8ca7641d18558e23.

Signed-off-by: Jiang Biao <jiang.biao2@zte.com.cn>
Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
CC: Tejun Heo <tj@kernel.org>
CC: Jens Axboe <axboe@kernel.dk>
---
 block/blk-cgroup.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index c2033a2..2b7f8d0 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1367,17 +1367,12 @@ void blkcg_deactivate_policy(struct request_queue *q,
 	__clear_bit(pol->plid, q->blkcg_pols);
 
 	list_for_each_entry(blkg, &q->blkg_list, q_node) {
-		/* grab blkcg lock too while removing @pd from @blkg */
-		spin_lock(&blkg->blkcg->lock);
-
 		if (blkg->pd[pol->plid]) {
 			if (pol->pd_offline_fn)
 				pol->pd_offline_fn(blkg->pd[pol->plid]);
 			pol->pd_free_fn(blkg->pd[pol->plid]);
 			blkg->pd[pol->plid] = NULL;
 		}
-
-		spin_unlock(&blkg->blkcg->lock);
 	}
 
 	spin_unlock_irq(q->queue_lock);
-- 
2.7.4

Re: [PATCH] blkcg: not hold blkcg lock when deactivating policy.

[Paolo Valente]

> Il giorno 17 apr 2018, alle ore 09:10, Jiang Biao =
<jiang.biao2@zte.com.cn> ha scritto:
>=20
> As described in the comment of blkcg_activate_policy(),
> *Update of each blkg is protected by both queue and blkcg locks so
> that holding either lock and testing blkcg_policy_enabled() is
> always enough for dereferencing policy data.*
> with queue lock held, there is no need to hold blkcg lock in
> blkcg_deactivate_policy(). Similar case is in
> blkcg_activate_policy(), which has removed holding of blkcg lock in
> commit 4c55f4f9ad3001ac1fefdd8d8ca7641d18558e23.
>=20

Hi,
by chance, did you check whether this may cause problems with bfq,
being the latter not protected by the queue lock as cfq?

Thanks,
Paolo

> Signed-off-by: Jiang Biao <jiang.biao2@zte.com.cn>
> Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
> CC: Tejun Heo <tj@kernel.org>
> CC: Jens Axboe <axboe@kernel.dk>
> ---
> block/blk-cgroup.c | 5 -----
> 1 file changed, 5 deletions(-)
>=20
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index c2033a2..2b7f8d0 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -1367,17 +1367,12 @@ void blkcg_deactivate_policy(struct =
request_queue *q,
> 	__clear_bit(pol->plid, q->blkcg_pols);
>=20
> 	list_for_each_entry(blkg, &q->blkg_list, q_node) {
> -		/* grab blkcg lock too while removing @pd from @blkg */
> -		spin_lock(&blkg->blkcg->lock);
> -
> 		if (blkg->pd[pol->plid]) {
> 			if (pol->pd_offline_fn)
> 				pol->pd_offline_fn(blkg->pd[pol->plid]);
> 			pol->pd_free_fn(blkg->pd[pol->plid]);
> 			blkg->pd[pol->plid] =3D NULL;
> 		}
> -
> -		spin_unlock(&blkg->blkcg->lock);
> 	}
>=20
> 	spin_unlock_irq(q->queue_lock);
> --=20
> 2.7.4
>=20

Re: [PATCH] blkcg: not hold blkcg lock when deactivating policy.

[Paolo Valente]

> Il giorno 17 apr 2018, alle ore 09:10, Jiang Biao <jiang.biao2@zte.com.cn> ha scritto:
> 
> As described in the comment of blkcg_activate_policy(),
> *Update of each blkg is protected by both queue and blkcg locks so
> that holding either lock and testing blkcg_policy_enabled() is
> always enough for dereferencing policy data.*
> with queue lock held, there is no need to hold blkcg lock in
> blkcg_deactivate_policy(). Similar case is in
> blkcg_activate_policy(), which has removed holding of blkcg lock in
> commit 4c55f4f9ad3001ac1fefdd8d8ca7641d18558e23.
> 

Hi,
by chance, did you check whether this may cause problems with bfq,
being the latter not protected by the queue lock as cfq?

Thanks,
Paolo

> Signed-off-by: Jiang Biao <jiang.biao2@zte.com.cn>
> Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
> CC: Tejun Heo <tj@kernel.org>
> CC: Jens Axboe <axboe@kernel.dk>
> ---
> block/blk-cgroup.c | 5 -----
> 1 file changed, 5 deletions(-)
> 
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index c2033a2..2b7f8d0 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -1367,17 +1367,12 @@ void blkcg_deactivate_policy(struct request_queue *q,
> 	__clear_bit(pol->plid, q->blkcg_pols);
> 
> 	list_for_each_entry(blkg, &q->blkg_list, q_node) {
> -		/* grab blkcg lock too while removing @pd from @blkg */
> -		spin_lock(&blkg->blkcg->lock);
> -
> 		if (blkg->pd[pol->plid]) {
> 			if (pol->pd_offline_fn)
> 				pol->pd_offline_fn(blkg->pd[pol->plid]);
> 			pol->pd_free_fn(blkg->pd[pol->plid]);
> 			blkg->pd[pol->plid] = NULL;
> 		}
> -
> -		spin_unlock(&blkg->blkcg->lock);
> 	}
> 
> 	spin_unlock_irq(q->queue_lock);
> -- 
> 2.7.4
> 

Re: [PATCH] blkcg: not hold blkcg lock when deactivating policy.

[jiang.biao2]

[-- Attachment #1.1: Type: text/plain, Size: 1286 bytes --]

Hi,
>> Il giorno 17 apr 2018, alle ore 09:10, Jiang Biao <jiang.biao2@zte.com.cn> ha scritto:
>>
>> As described in the comment of blkcg_activate_policy(),
>> *Update of each blkg is protected by both queue and blkcg locks so
>> that holding either lock and testing blkcg_policy_enabled() is
>> always enough for dereferencing policy data.*
>> with queue lock held, there is no need to hold blkcg lock in
>> blkcg_deactivate_policy(). Similar case is in
>> blkcg_activate_policy(), which has removed holding of blkcg lock in
>> commit 4c55f4f9ad3001ac1fefdd8d8ca7641d18558e23.
>>
>
> Hi,
> by chance, did you check whether this may cause problems with bfq,
> being the latter not protected by the queue lock as cfq?
Checked the bfq code, bfq seems never used blkcg lock derectly, and 
update of blkg in the common code is protected by both queue and 
blkcg locks, so IMHO this patch would not introduce any new problem
with bfq, even though bfq is not protected by queue lock.
On the other hand, the locks (queue lock/blkcg lock) used to protected
the update of blkg seems a bit too heavyweight, especially the queue lock 
which is used too widely may cause races with other contexts. I wonder
if there is any way to ease the case? e.g. add a new lock for blkg's own.:) 

Jiang,
Regards

Re: [PATCH] blkcg: not hold blkcg lock when deactivating policy.

[Paolo Valente]

> Il giorno 18 apr 2018, alle ore 11:18, jiang.biao2@zte.com.cn ha =
scritto:
>=20
> Hi,
>>> Il giorno 17 apr 2018, alle ore 09:10, Jiang Biao =
<jiang.biao2@zte.com.cn> ha scritto:
>>>=20
>>> As described in the comment of blkcg_activate_policy(),
>>> *Update of each blkg is protected by both queue and blkcg locks so
>>> that holding either lock and testing blkcg_policy_enabled() is
>>> always enough for dereferencing policy data.*
>>> with queue lock held, there is no need to hold blkcg lock in
>>> blkcg_deactivate_policy(). Similar case is in
>>> blkcg_activate_policy(), which has removed holding of blkcg lock in
>>> commit 4c55f4f9ad3001ac1fefdd8d8ca7641d18558e23.
>>>=20
>>=20
>> Hi,
>> by chance, did you check whether this may cause problems with bfq,
>> being the latter not protected by the queue lock as cfq?
> Checked the bfq code, bfq seems never used blkcg lock derectly, and=20
> update of blkg in the common code is protected by both queue and=20
> blkcg locks, so IMHO this patch would not introduce any new problem
> with bfq, even though bfq is not protected by queue lock.

Thank you very much for checking. Then:

Acked-by: Paolo Valente <paolo.valente@linaro.org>

Thanks,
Paolo

> On the other hand, the locks (queue lock/blkcg lock) used to protected
> the update of blkg seems a bit too heavyweight, especially the queue =
lock=20
> which is used too widely may cause races with other contexts. I wonder
> if there is any way to ease the case? e.g. add a new lock for blkg's =
own.:)=20
>=20
> Jiang,
> Regards

Re: [PATCH] blkcg: not hold blkcg lock when deactivating policy.

[Paolo Valente]

> Il giorno 18 apr 2018, alle ore 11:18, jiang.biao2@zte.com.cn ha scritto:
> 
> Hi,
>>> Il giorno 17 apr 2018, alle ore 09:10, Jiang Biao <jiang.biao2@zte.com.cn> ha scritto:
>>> 
>>> As described in the comment of blkcg_activate_policy(),
>>> *Update of each blkg is protected by both queue and blkcg locks so
>>> that holding either lock and testing blkcg_policy_enabled() is
>>> always enough for dereferencing policy data.*
>>> with queue lock held, there is no need to hold blkcg lock in
>>> blkcg_deactivate_policy(). Similar case is in
>>> blkcg_activate_policy(), which has removed holding of blkcg lock in
>>> commit 4c55f4f9ad3001ac1fefdd8d8ca7641d18558e23.
>>> 
>> 
>> Hi,
>> by chance, did you check whether this may cause problems with bfq,
>> being the latter not protected by the queue lock as cfq?
> Checked the bfq code, bfq seems never used blkcg lock derectly, and 
> update of blkg in the common code is protected by both queue and 
> blkcg locks, so IMHO this patch would not introduce any new problem
> with bfq, even though bfq is not protected by queue lock.

Thank you very much for checking. Then:

Acked-by: Paolo Valente <paolo.valente@linaro.org>

Thanks,
Paolo

> On the other hand, the locks (queue lock/blkcg lock) used to protected
> the update of blkg seems a bit too heavyweight, especially the queue lock 
> which is used too widely may cause races with other contexts. I wonder
> if there is any way to ease the case? e.g. add a new lock for blkg's own.:) 
> 
> Jiang,
> Regards

Re: [PATCH] blkcg: not hold blkcg lock when deactivating policy.

[Jens Axboe]

On 4/18/18 3:18 AM, jiang.biao2@zte.com.cn wrote:
> Hi,
>>> Il giorno 17 apr 2018, alle ore 09:10, Jiang Biao <jiang.biao2@zte.com.cn> ha scritto:
>>>
>>> As described in the comment of blkcg_activate_policy(),
>>> *Update of each blkg is protected by both queue and blkcg locks so
>>> that holding either lock and testing blkcg_policy_enabled() is
>>> always enough for dereferencing policy data.*
>>> with queue lock held, there is no need to hold blkcg lock in
>>> blkcg_deactivate_policy(). Similar case is in
>>> blkcg_activate_policy(), which has removed holding of blkcg lock in
>>> commit 4c55f4f9ad3001ac1fefdd8d8ca7641d18558e23.
>>>
>>
>> Hi,
>> by chance, did you check whether this may cause problems with bfq,
>> being the latter not protected by the queue lock as cfq?
> Checked the bfq code, bfq seems never used blkcg lock derectly, and 
> update of blkg in the common code is protected by both queue and 
> blkcg locks, so IMHO this patch would not introduce any new problem
> with bfq, even though bfq is not protected by queue lock.
> On the other hand, the locks (queue lock/blkcg lock) used to protected
> the update of blkg seems a bit too heavyweight, especially the queue lock 
> which is used too widely may cause races with other contexts. I wonder
> if there is any way to ease the case? e.g. add a new lock for blkg's own.:) 

It might make sense to lock it separately, but I would not worry
about it unless it shows up as hot in your testing.

I've applied your patch, thanks.

-- 
Jens Axboe

Re: [PATCH] blkcg: not hold blkcg lock when deactivating policy.

[jiang.biao2]

[-- Attachment #1.1: Type: text/plain, Size: 1407 bytes --]

>>> by chance, did you check whether this may cause problems with bfq,
>>> being the latter not protected by the queue lock as cfq?
>> Checked the bfq code, bfq seems never used blkcg lock derectly, and
>> update of blkg in the common code is protected by both queue and
>> blkcg locks, so IMHO this patch would not introduce any new problem
>> with bfq, even though bfq is not protected by queue lock.
>> On the other hand, the locks (queue lock/blkcg lock) used to protected
>> the update of blkg seems a bit too heavyweight, especially the queue lock
>> which is used too widely may cause races with other contexts. I wonder
>> if there is any way to ease the case? e.g. add a new lock for blkg's own.:)
>
> It might make sense to lock it separately, but I would not worry
> about it unless it shows up as hot in your testing.
Actually, we've met a triggering of nmi_watchdog, blocked at the queue lock
in blkcg_print_blkgs(), caused by the slow serial console and too many printks.
Related discussion is here,
https://bugzilla.kernel.org/show_bug.cgi?id=199003
Even though it's not caused by the queue lock directly, it would not happen
without using queue lock. The queue lock is big and used too widely, using it
would intensify the race, so we're trying to understand the locks using in blkg,
and maybe could improve the situation.

> I've applied your patch, thanks.
Thanks a lot. :)

Jiang,
Regards

Re: [PATCH] blkcg: not hold blkcg lock when deactivating policy.

[Jens Axboe]

On 4/18/18 6:54 PM, jiang.biao2@zte.com.cn wrote:
>>>> by chance, did you check whether this may cause problems with bfq,
>>>> being the latter not protected by the queue lock as cfq?
>>> Checked the bfq code, bfq seems never used blkcg lock derectly, and
>>> update of blkg in the common code is protected by both queue and
>>> blkcg locks, so IMHO this patch would not introduce any new problem
>>> with bfq, even though bfq is not protected by queue lock.
>>> On the other hand, the locks (queue lock/blkcg lock) used to protected
>>> the update of blkg seems a bit too heavyweight, especially the queue lock
>>> which is used too widely may cause races with other contexts. I wonder
>>> if there is any way to ease the case? e.g. add a new lock for blkg's own.:)
>>
>> It might make sense to lock it separately, but I would not worry
>> about it unless it shows up as hot in your testing.
> Actually, we've met a triggering of nmi_watchdog, blocked at the queue lock
> in blkcg_print_blkgs(), caused by the slow serial console and too many printks.
> Related discussion is here,
> https://bugzilla.kernel.org/show_bug.cgi?id=199003
> Even though it's not caused by the queue lock directly, it would not happen
> without using queue lock. The queue lock is big and used too widely, using it
> would intensify the race, so we're trying to understand the locks using in blkg,
> and maybe could improve the situation.

The queue lock is only used widely on non blk-mq, where it is the only
lock really. Doing serial IO under a spinlock is always going to suck,
regardless of how contended it is.

-- 
Jens Axboe

Re: [PATCH] blkcg: not hold blkcg lock when deactivating policy.

[jiang.biao2]

[-- Attachment #1.1: Type: text/plain, Size: 952 bytes --]

>>> It might make sense to lock it separately, but I would not worry
>>> about it unless it shows up as hot in your testing.
>> Actually, we've met a triggering of nmi_watchdog, blocked at the queue lock
>> in blkcg_print_blkgs(), caused by the slow serial console and too many printks.
>> Related discussion is here,
>> https://bugzilla.kernel.org/show_bug.cgi?id=199003
>> Even though it's not caused by the queue lock directly, it would not happen
>> without using queue lock. The queue lock is big and used too widely, using it
>> would intensify the race, so we're trying to understand the locks using in blkg,
>> and maybe could improve the situation.
> 
> The queue lock is only used widely on non blk-mq, where it is the only
> lock really. Doing serial IO under a spinlock is always going to suck,
> regardless of how contended it is.

It's not a problem of queue lock indeed, just want to dig deeper. :) 
Thanks for the reply.

Jiang,
Regards