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:5.131 / U:0.244 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site