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@xxxxxxxxxx>
> > ---
>
> 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
