Re: [PATCH v6 1/4] block: Make fair tag sharing configurable

[Yu Kuai <yukuai1@xxxxxxxxxxxxxxx>]

Hi, Bart!

在 2023/12/04 12:13, Bart Van Assche 写道:
> On 12/1/23 23:21, Yu Kuai wrote:
> > 在 2023/12/01 3:31, Bart Van Assche 写道:
> > > +/*
> > > + * Enable or disable fair tag sharing for all request queues 
> > > associated with
> > > + * a tag set.
> > > + */
> > > +void blk_mq_update_fair_sharing(struct blk_mq_tag_set *set, bool 
> > > enable)
> > > +{
> > > +    const unsigned int DFTS_BIT = 
> > > ilog2(BLK_MQ_F_DISABLE_FAIR_TAG_SHARING);
> > > +    struct blk_mq_hw_ctx *hctx;
> > > +    struct request_queue *q;
> > > +    unsigned long i;
> > > +
> > > +    /*
> > > +     * Serialize against blk_mq_update_nr_hw_queues() and
> > > +     * blk_mq_realloc_hw_ctxs().
> > > +     */
> > > +    mutex_lock(&set->tag_list_lock);
> > I'm a litter confused about this comment, because
> > blk_mq_realloc_hw_ctxs() can be called from
> > blk_mq_update_nr_hw_queues().
> >
> > If you are talking about blk_mq_init_allocated_queue(), it looks like
> > just holding this lock is not enough?
>
> I added that comment because blk_mq_init_allocated_queue() calls
> blk_mq_realloc_hw_ctxs() before the request queue is added to
> set->tag_list. I will take a closer look at how
> blk_mq_init_allocated_queue() reads set->flags and will make sure
> that these reads are properly serialized against the changes made
> by blk_mq_update_fair_sharing().

Are you still intrested in this patchset? I really want this switch in
our product as well.

If so, how do you think about following changes, a new field in
blk_mq_tag_set will make synchronization much eaiser.

Thanks,
Kuai

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 6ab7f360ff2a..791306dcd656 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -3935,6 +3935,34 @@ static void blk_mq_map_swqueue(struct 
request_queue *q)
        }
 }

+static void queue_update_fair_tag_sharing(struct request_queue *q)
+{
+       struct blk_mq_hw_ctx *hctx;
+       unsigned long i;
+
+       queue_for_each_hw_ctx(q, hctx, i) {
+               if (q->tag_set->disable_fair_tag_sharing)
+                       hctx->flags |= BLK_MQ_F_DISABLE_FAIR_TAG_SHARING;
+               else
+                       hctx->flags &= ~BLK_MQ_F_DISABLE_FAIR_TAG_SHARING;
+       }
+
+}
+
+void blk_mq_update_fair_tag_sharing(struct blk_mq_tag_set *set)
+{
+       struct request_queue *q;
+
+       lockdep_assert_held(&set->tag_list_lock);
+
+       list_for_each_entry(q, &set->tag_list, tag_set_list) {
+               blk_mq_freeze_queue(q);
+               queue_update_tag_fair_share(q);
+               blk_mq_unfreeze_queue(q);
+       }
+}
+EXPORT_SYMBOL_GPL(blk_mq_update_tag_fair_share);
+
 /*
  * Caller needs to ensure that we're either frozen/quiesced, or that
  * the queue isn't live yet.
@@ -3989,6 +4017,7 @@ static void blk_mq_add_queue_tag_set(struct 
blk_mq_tag_set *set,
 {
        mutex_lock(&set->tag_list_lock);

+       queue_update_fair_tag_sharing(q);
        /*
         * Check to see if we're transitioning to shared (from 1 to 2 
queues).
         */

diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 958ed7e89b30..d76630ac45d8 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -506,6 +506,7 @@ struct blk_mq_tag_set {
        int                     numa_node;
        unsigned int            timeout;
        unsigned int            flags;
+       bool                    disable_fair_tag_sharing;
        void                    *driver_data;

        struct blk_mq_tags      **tags;

> Thanks,
>
> Bart.
>
> .