lkml.org 
[lkml]   [2021]   [Oct]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] blk-mq: Fix blk_mq_tagset_busy_iter() for shared tags
From
Date
On 13/10/2021 15:29, Ming Lei wrote:
>> As I understand, Kashyap mentioned no throughput regression with my series,
>> but just higher cpu usage in blk_mq_find_and_get_req().
>>
>> I'll see if I can see such a thing in my setup.
>>
>> But could it be that since we only have a single sets of requests per
>> tagset, and not a set of requests per HW queue, there is more contention on
>> the common set of requests in the refcount_inc_not_zero() call ***, below:
>>
>> static struct request *blk_mq_find_and_get_req(struct blk_mq_tags *tags,
>> unsigned int bitnr)
>> {
>> ...
>>
>> rq = tags->rqs[bitnr];
>> if (... || !refcount_inc_not_zero(&rq->ref)) ***
>> ...
>> }
> Kashyap's log shows that contention on tags->lock is increased, that
> should be caused by nr_hw_queues iterating.

If the lock contention increases on tags->lock then I am not totally
surprised. For shared sbitmap, each HW queue had its own tags (and tags
lock). Now with shared tags, we have a single lock over the tagset, and
so we would have more contention. That's on the basis that we have many
parallel callers to blk_mq_queue_tag_busy_iter().

> blk_mq_find_and_get_req()
> will be run nr_hw_queue times compared with pre-shared-sbitmap, since it
> is done before checking rq->mq_hctx.

Isn't shared sitmap older than blk_mq_find_and_get_req()?

Anyway, for 5.14 shared sbitmap support, we iter nr_hw_queue times. And
now, for shared tags, we still do that. I don't see what's changed in
that regard.

>
>> But I wonder why this function is even called often...
>>
>>>> There is also blk_mq_all_tag_iter():
>>>>
>>>> void blk_mq_all_tag_iter(struct blk_mq_tags *tags, busy_tag_iter_fn *fn,
>>>> void *priv)
>>>> {
>>>> __blk_mq_all_tag_iter(tags, fn, priv, BT_TAG_ITER_STATIC_RQS);
>>>> }
>>>>
>>>> But then the only user is blk_mq_hctx_has_requests():
>>>>
>>>> static bool blk_mq_hctx_has_requests(struct blk_mq_hw_ctx *hctx)
>>>> {
>>>> struct blk_mq_tags *tags = hctx->sched_tags ?
>>>> hctx->sched_tags : hctx->tags;
>>>> struct rq_iter_data data = {
>>>> .hctx = hctx,
>>>> };
>>>>
>>>> blk_mq_all_tag_iter(tags, blk_mq_has_request, &data);
>>>> return data.has_rq;
>>>> }
>>> This above one only iterates over the specified hctx/tags, it won't be
>>> affected.
>>>
>>>> But, again like bt_iter(), blk_mq_has_request() will check the hctx matches:
>>> Not see what matters wrt. checking hctx.
>> I'm just saying that something like the following would be broken for shared
>> tags:
>>
>> static bool blk_mq_has_request(struct request *rq, void *data, bool
>> reserved)
>> {
>> struct rq_iter_data *iter_data = data;
>>
>> iter_data->has_rq = true;
>> return true;
>> }
>>
>> static bool blk_mq_hctx_has_requests(struct blk_mq_hw_ctx *hctx)
>> {
>> struct rq_iter_data data = {
>> };
>>
>> blk_mq_all_tag_iter(tags, blk_mq_has_request, &data);
>> return data.has_rq;
>> }
>>
>> As it ignores that we want to check for a specific hctx.
> No, that isn't what I meant, follows the change I suggested:

I didn't mean that this was your suggestion. I am just saying that we
need to be careful iter'ing tags for shared tags now, as in that example.

>
>
> diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
> index 72a2724a4eee..2a2ad6dfcc33 100644
> --- a/block/blk-mq-tag.c
> +++ b/block/blk-mq-tag.c
> @@ -232,8 +232,9 @@ static bool bt_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data)
> if (!rq)
> return true;
>
> - if (rq->q == hctx->queue && rq->mq_hctx == hctx)
> - ret = iter_data->fn(hctx, rq, iter_data->data, reserved);
> + if (rq->q == hctx->queue && (rq->mq_hctx == hctx ||
> + blk_mq_is_shared_tags(hctx->flags)))
> + ret = iter_data->fn(rq->mq_hctx, rq, iter_data->data, reserved);
> blk_mq_put_rq_ref(rq);
> return ret;
> }
> @@ -460,6 +461,9 @@ void blk_mq_queue_tag_busy_iter(struct request_queue *q, busy_iter_fn *fn,
> if (tags->nr_reserved_tags)
> bt_for_each(hctx, &tags->breserved_tags, fn, priv, true);
> bt_for_each(hctx, &tags->bitmap_tags, fn, priv, false);
> +
> + if (blk_mq_is_shared_tags(hctx->flags))
> + break;
> }
> blk_queue_exit(q);
> }
>

I suppose that is ok, and means that we iter once.

However, I have to ask, where is the big user of
blk_mq_queue_tag_busy_iter() coming from? I saw this from Kashyap's mail:

> 1.31% 1.31% kworker/57:1H-k [kernel.vmlinux]
> native_queued_spin_lock_slowpath
> ret_from_fork
> kthread
> worker_thread
> process_one_work
> blk_mq_timeout_work
> blk_mq_queue_tag_busy_iter
> bt_iter
> blk_mq_find_and_get_req
> _raw_spin_lock_irqsave
> native_queued_spin_lock_slowpath

How or why blk_mq_timeout_work()?

Thanks,
john

\
 
 \ /
  Last update: 2021-10-13 17:11    [W:0.073 / U:0.180 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site