| Message ID | 20221005032257.80681-1-kch@nvidia.com |
|---|---|
| Headers | show |
| Series | block: add and use init tagset helper | expand |
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
On 10/5/22 02:47, Ulf Hansson wrote: > 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: >>> +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. Hi Chaitanya, Please consider to drop the entire patch series. In addition to the disadvantages mentioned above I'd like to mention the following disadvantages: * Replacing named member assignments with positional arguments in a function call makes code harder to read and harder to verify. * This patch series makes tree-wide changes without improving the code in a substantial way. Thanks, Bart.
On Tue, Oct 04, 2022 at 08:22:36PM -0700, Chaitanya Kulkarni wrote: > Hi, > > 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 repetation of > inialization code of the tag set in current block drivers and any future > ones. > > P.S. I'm aware of the EXPORT_SYMBOL_GPL() checkpatch warn just to make > get some feedback to so I can remove the RFC tag. > *If* there were commonalities at init and these could be broken up into common groups, each having their own set of calls, then we simplify and can abstract these. I say this without doing a complete review of the removals, but if there really isn't much of commonalities I tend to agree with Bart that open coding this is better. Luis
On Fri, Oct 07, 2022 at 11:26:13AM -0700, Luis Chamberlain wrote: > *If* there were commonalities at init and these could be broken up into > common groups, each having their own set of calls, then we simplify and > can abstract these. I say this without doing a complete review of the > removals, but if there really isn't much of commonalities I tend to > agree with Bart that open coding this is better. The commonality is that there are various required or optional fields to fill out. I actually have a WIP series to make the tag_set dynamically allocated and refcounted to fix some long standing life time issues. That creates a new alloc helper that will take a few mandatory arguments and would heavily clash with this series.