Re: [PATCH 1/6] blk-mq: figure out correct numa node for hw queue

[Ming Lei <ming.lei@redhat.com>]

On Tue, Mar 01, 2022 at 07:19:43PM +0000, John Garry wrote:
> On 28/02/2022 09:04, Ming Lei wrote:
> > The current code always uses default queue map and hw queue index
> > for figuring out the numa node for hw queue, this way isn't correct
> > because blk-mq supports three queue maps, and the correct queue map
> > should be used for the specified hw queue.
> > 
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> 
> Hi Ming,
> 
> Just some small comments to consider if you need to respin.
> 
> Thanks,
> John
> 
> >   block/blk-mq.c | 36 ++++++++++++++++++++++++++++++------
> >   1 file changed, 30 insertions(+), 6 deletions(-)
> > 
> > diff --git a/block/blk-mq.c b/block/blk-mq.c
> > index a05ce7725031..931add81813b 100644
> > --- a/block/blk-mq.c
> > +++ b/block/blk-mq.c
> > @@ -3107,15 +3107,41 @@ void blk_mq_free_rq_map(struct blk_mq_tags *tags)
> >   	blk_mq_free_tags(tags);
> >   }
> >    +static int
> 
> enum hctx_type?
> 
> > hctx_idx_to_type(struct blk_mq_tag_set *set,
> > +		unsigned int hctx_idx)
> > +{
> > +	int j;
> 
> super nit: normally use i

OK

> 
> > +
> > +	for (j = 0; j < set->nr_maps; j++) {
> > +		unsigned int start =  set->map[j].queue_offset;
> 
> nit: double whitespace intentional?

will fix it.

> 
> > +		unsigned int end = start + set->map[j].nr_queues;
> > +
> > +		if (hctx_idx >= start && hctx_idx < end)
> > +			break;
> > +	}
> > +
> > +	if (j >= set->nr_maps)
> > +		j = HCTX_TYPE_DEFAULT;
> > +
> > +	return j;
> > +}
> > +
> > +static int blk_mq_get_hctx_node(struct blk_mq_tag_set *set,
> > +		unsigned int hctx_idx)
> > +{
> > +	int type = hctx_idx_to_type(set, hctx_idx);
> > +
> > +	return blk_mq_hw_queue_to_node(&set->map[type], hctx_idx);
> > +}
> > +
> >   static struct blk_mq_tags *blk_mq_alloc_rq_map(struct blk_mq_tag_set *set,
> >   					       unsigned int hctx_idx,
> >   					       unsigned int nr_tags,
> >   					       unsigned int reserved_tags)
> >   {
> >   	struct blk_mq_tags *tags;
> > -	int node;
> > +	int node = blk_mq_get_hctx_node(set, hctx_idx);
> 
> nit: the code originally had reverse firtree ordering, which I suppose is
> not by mistake

What is reverse firtree ordering here? I don't know what is wrong
with the above one line change from patch style viewpoint, and
checkpatch complains nothing here.


Thanks,
Ming