linux-kernel - Re: [RFC PATCH 01/21] block: add and use init tagset helper

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPDyKFpBpiydQn+=24CqtaH_qa3tQfN2gQSiUrHCjnLSuy4=Kg@mail.gmail.com>
Date:   Wed, 5 Oct 2022 11:47:34 +0200
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Chaitanya Kulkarni <kch@...dia.com>,
        Damien Le Moal <damien.lemoal@...nsource.wdc.com>
Cc:     linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-omap@...r.kernel.org, linux-remoteproc@...r.kernel.org,
        linux-mmc@...r.kernel.org, linux-mtd@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-nvme@...ts.infradead.org, linux-s390@...r.kernel.org,
        linux-scsi@...r.kernel.org, axboe@...nel.dk, efremov@...ux.com,
        josef@...icpanda.com, idryomov@...il.com,
        dongsheng.yang@...ystack.cn, haris.iqbal@...os.com,
        jinpu.wang@...os.com, mst@...hat.com, jasowang@...hat.com,
        pbonzini@...hat.com, stefanha@...hat.com, ohad@...ery.com,
        andersson@...nel.org, baolin.wang@...ux.alibaba.com,
        richard@....at, miquel.raynal@...tlin.com, vigneshr@...com,
        marcan@...can.st, sven@...npeter.dev, alyssa@...enzweig.io,
        kbusch@...nel.org, hch@....de, sagi@...mberg.me, sth@...ux.ibm.com,
        hoeppner@...ux.ibm.com, hca@...ux.ibm.com, gor@...ux.ibm.com,
        agordeev@...ux.ibm.com, borntraeger@...ux.ibm.com,
        svens@...ux.ibm.com, jejb@...ux.ibm.com,
        martin.petersen@...cle.com, hare@...e.de, bhelgaas@...gle.com,
        john.garry@...wei.com, mcgrof@...nel.org,
        christophe.jaillet@...adoo.fr, vaibhavgupta40@...il.com,
        wsa+renesas@...g-engineering.com, johannes.thumshirn@....com,
        bvanassche@....org, ming.lei@...hat.com,
        shinichiro.kawasaki@....com, vincent.fu@...sung.com,
        christoph.boehmwalder@...bit.com, joel@....id.au,
        vincent.whitchurch@...s.com, nbd@...er.debian.org,
        ceph-devel@...r.kernel.org,
        virtualization@...ts.linux-foundation.org, asahi@...ts.linux.dev
Subject: Re: [RFC PATCH 01/21] block: add and use init tagset helper

On Wed, 5 Oct 2022 at 07:11, Damien Le Moal
<damien.lemoal@...nsource.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@...dia.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