Re: [RFC PATCH 01/21] block: add and use init tagset helper
- To: Chaitanya Kulkarni <kch@nvidia.com>, Damien Le Moal <damien.lemoal@opensource.wdc.com>
- Cc: linux-block@vger.kernel.org, linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org, linux-remoteproc@vger.kernel.org, linux-mmc@vger.kernel.org, linux-mtd@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-nvme@lists.infradead.org, linux-s390@vger.kernel.org, linux-scsi@vger.kernel.org, axboe@kernel.dk, efremov@linux.com, josef@toxicpanda.com, idryomov@gmail.com, dongsheng.yang@easystack.cn, haris.iqbal@ionos.com, jinpu.wang@ionos.com, mst@redhat.com, jasowang@redhat.com, pbonzini@redhat.com, stefanha@redhat.com, ohad@wizery.com, andersson@kernel.org, baolin.wang@linux.alibaba.com, richard@nod.at, miquel.raynal@bootlin.com, vigneshr@ti.com, marcan@marcan.st, sven@svenpeter.dev, alyssa@rosenzweig.io, kbusch@kernel.org, hch@lst.de, sagi@grimberg.me, sth@linux.ibm.com, hoeppner@linux.ibm.com, hca@linux.ibm.com, gor@linux.ibm.com, agordeev@linux.ibm.com, borntraeger@linux.ibm.com, svens@linux.ibm.com, jejb@linux.ibm.com, martin.petersen@oracle.com, hare@suse.de, bhelgaas@google.com, john.garry@huawei.com, mcgrof@kernel.org, christophe.jaillet@wanadoo.fr, vaibhavgupta40@gmail.com, wsa+renesas@sang-engineering.com, johannes.thumshirn@wdc.com, bvanassche@acm.org, ming.lei@redhat.com, shinichiro.kawasaki@wdc.com, vincent.fu@samsung.com, christoph.boehmwalder@linbit.com, joel@jms.id.au, vincent.whitchurch@axis.com, nbd@other.debian.org, ceph-devel@vger.kernel.org, virtualization@lists.linux-foundation.org, asahi@lists.linux.dev
- Subject: Re: [RFC PATCH 01/21] block: add and use init tagset helper
- From: Ulf Hansson <ulf.hansson@linaro.org>
- Date: Wed, 5 Oct 2022 11:47:34 +0200
- Message-id: <[🔎] CAPDyKFpBpiydQn+=24CqtaH_qa3tQfN2gQSiUrHCjnLSuy4=Kg@mail.gmail.com>
- In-reply-to: <[🔎] 6fee2d7a-7fd1-73ee-2911-87a4ed3e8769@opensource.wdc.com>
- References: <[🔎] 20221005032257.80681-1-kch@nvidia.com> <20221005032257.80681-2-kch@nvidia.com> <[🔎] 6fee2d7a-7fd1-73ee-2911-87a4ed3e8769@opensource.wdc.com>
On Wed, 5 Oct 2022 at 07:11, Damien Le Moal
<damien.lemoal@opensource.wdc.com> wrote:
>
> On 10/5/22 12:22, Chaitanya Kulkarni wrote:
> > Add and use the helper to initialize the common fields of the tag_set
> > such as blk_mq_ops, number of h/w queues, queue depth, command size,
> > numa_node, timeout, BLK_MQ_F_XXX flags, driver data. This initialization
> > is spread all over the block drivers. This avoids the code repetation of
> > the inialization code of the tag set in current block drivers and any
>
> s/inialization/initialization
> s/repetation/repetition
>
> > future ones.
> >
> > Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
> > ---
> > block/blk-mq.c | 20 ++++++++++++++++++++
> > drivers/block/null_blk/main.c | 10 +++-------
> > include/linux/blk-mq.h | 5 +++++
> > 3 files changed, 28 insertions(+), 7 deletions(-)
> >
> > diff --git a/block/blk-mq.c b/block/blk-mq.c
> > index 8070b6c10e8d..e3a8dd81bbe2 100644
> > --- a/block/blk-mq.c
> > +++ b/block/blk-mq.c
> > @@ -4341,6 +4341,26 @@ static int blk_mq_alloc_tag_set_tags(struct blk_mq_tag_set *set,
> > return blk_mq_realloc_tag_set_tags(set, 0, new_nr_hw_queues);
> > }
> >
> > +void blk_mq_init_tag_set(struct blk_mq_tag_set *set,
> > + const struct blk_mq_ops *ops, unsigned int nr_hw_queues,
> > + unsigned int queue_depth, unsigned int cmd_size, int numa_node,
> > + unsigned int timeout, unsigned int flags, void *driver_data)
>
> That is an awful lot of arguments... I would be tempted to say pack all
> these into a struct but then that would kind of negate this patchset goal.
> Using a function with that many arguments will be error prone, and hard to
> review... Not a fan.
I completely agree.
But there is also another problem going down this route. If/when we
realize that there is another parameter needed in the blk_mq_tag_set.
Today that's quite easy to add (assuming the parameter can be
optional), without changing the blk_mq_init_tag_set() interface.
>
> > +{
> > + if (!set)
> > + return;
> > +
> > + set->ops = ops;
> > + set->nr_hw_queues = nr_hw_queues;
> > + set->queue_depth = queue_depth;
> > + set->cmd_size = cmd_size;
> > + set->numa_node = numa_node;
> > + set->timeout = timeout;
> > + set->flags = flags;
> > + set->driver_data = driver_data;
> > +}
> > +
>
[...]
Kind regards
Uffe
Reply to: