| Message ID | 20221005032257.80681-1-kch@nvidia.com |
|---|---|
| Headers | show |
| Series | block: add and use init tagset helper | expand |
On 10/4/22 22:11, Damien Le Moal 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. yes.. > Using a function with that many arguments will be error prone, and hard to > review... Not a fan. > Recent addition to the block layer code blk_rq_map_user_io() has 9 arguments:- int blk_rq_map_user_io(struct request *req, struct rq_map_data *map_data, void __user *ubuf, unsigned long buf_len, gfp_t gfp_mask, bool vec, int iov_count, bool check_iter_count, int rw) above function also has 9 arguments not more than what is present in the block tree. I can trim down the argument list probably by removing the numa_node as it is set to NUMA_NO_NODE for most of the drivers. -ck
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 10/5/22 10:22, Chaitanya Kulkarni wrote: > On 10/5/22 09:54, Bart Van Assche wrote: >> 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. >> > > Thanks for the feedback, will drop it. > > -ck > Actually I take that back. The problems listed here are:- * Long argument list, hard to verify (Damien,Bart):- solution is to make smaller like other function bio_alloc_bioset() blk_next_bio() present in the tree. * Not future proof if tag_set gets new member that needs to be initialized increasing the argument list of the new API. (Ulf):- solution is to only use common and mandatory members which are necessary as new API args, so if new members gets added it will not affect the API, that also trims down the argument list. I will trim down the argument list with the most common arguments and keep it to max 4-5 mandatory arguments identical to what we have done this for blk_next_bio() and bio_alloc_bioset() [1] where mandatory arguments are part of the initialization API than repeating the code all the in the tree, that creates maintenance work of treewide patches. Also, instead of doing tree wide change in series I'll start small and gradually add more patches over time. This definitely adds a more value to the code where code is not repeated for mandatory arguments, which are way less than 9. -ck [1] 8c16567d867ed bio_alloc_bioset() 5 arguments. 0a3140ea0fae3 blk_next_bio() 5 arguments.
On 10/6/22 11:13, Chaitanya Kulkarni wrote: > I will trim down the argument list with the most common arguments > and keep it to max 4-5 mandatory arguments identical to what we > have done this for blk_next_bio() and bio_alloc_bioset() [1] > where mandatory arguments are part of the initialization API > than repeating the code all the in the tree, that creates > maintenance work of treewide patches. > > Also, instead of doing tree wide change in series I'll start small > and gradually add more patches over time. > > This definitely adds a more value to the code where code is not > repeated for mandatory arguments, which are way less than 9. Hmm ... I'm not convinced that the approach outlined above will result in a valuable patch series. I think my objections also apply to the approach outlined above. 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 10/6/22 12:40 PM, Bart Van Assche wrote: > On 10/6/22 11:13, Chaitanya Kulkarni wrote: >> I will trim down the argument list with the most common arguments >> and keep it to max 4-5 mandatory arguments identical to what we >> have done this for blk_next_bio() and bio_alloc_bioset() [1] >> where mandatory arguments are part of the initialization API >> than repeating the code all the in the tree, that creates >> maintenance work of treewide patches. >> >> Also, instead of doing tree wide change in series I'll start small >> and gradually add more patches over time. >> >> This definitely adds a more value to the code where code is not >> repeated for mandatory arguments, which are way less than 9. > > Hmm ... I'm not convinced that the approach outlined above will result > in a valuable patch series. I think my objections also apply to the > approach outlined above. I would have to agree, I don't think this series buys us anything really, and in several ways it actually makes it worse or creates more of a maintenance problem going forward.
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.