Re: [dm-devel] [PATCH v2] dm-crypt: reexport sysfs of kcryptd workqueue

[Mike Snitzer <snitzer@kernel.org>]

[top-posting because of all the previous top-posting]

Hi,

I certainly would like the ability to allow control over the
workqueues using WQ_SYSFS.  But with Tejun's latest WQ_UNBOUND changes
(just merged during 6.5 merge window) I think we'd do well to audit
the flags we're using.

Tejun offered this note in his summary patch header for his 6.5 changes:
"Alasdair Kergon, Mike Snitzer, DM folks
---------------------------------------

I ran fio on top of dm-crypt to compare performance of different
configurations. It mostly behaved as expected but please let me know if
anything doens't look right. Also, DM_CRYPT_SAME_CPU can now be implemented
by applying strict "cpu" scope on the unbound workqueue and it would make
sense to add WQ_SYSFS to the kcryptd workqueue so that users can tune the
settings on the fly."

Anyway, I'd welcome you rebasing your patch ontop of Linus's latest
linux.git.  Then we (Mikulas, you, and/or I) can take a closer look at
addressing Tejun's DM_CRYPT_SAME_CPU suggestion.

Thanks,
Mike

On Mon, Jun 26 2023 at  4:43P -0400,
yangerkun <yangerkun@huaweicloud.com> wrote:

> Hi, Mike,
> 
> Sorry for the noise. This patch has been proposed for a long time. I wonder
> does there any suggestion for the patch. Looking forward to your reply!
> 
> Thanks,
> Yang Erkun.
> 
> 在 2023/3/25 9:01, yangerkun 写道:
> > Ping...
> > 
> > 在 2023/3/1 11:29, yangerkun 写道:
> > > From: yangerkun <yangerkun@huawei.com>
> > > 
> > > Once there is a heavy IO load, so many encrypt/decrypt work will occupy
> > > all of the cpu, which may lead the poor performance for other service.
> > > So the idea like 'a2b8b2d97567 ("dm crypt: export sysfs of kcryptd
> > > workqueue")' said seems necessary. We can export "kcryptd" workqueue
> > > sysfs, the entry like cpumask/max_active and so on can help us to limit
> > > the usage for encrypt/decrypt work.
> > > 
> > > However, that commit does not consider the reload table will call .ctr
> > > before .dtr, so the reload for dm-crypt will fail since the same sysfs
> > > problem, and then we revert that commit('48b0777cd93d ("Revert "dm
> > > crypt: export sysfs of kcryptd workqueue"")').
> > > 
> > > Actually, what we should do is give a unique name once we try reload
> > > table, we can use ida to fix the problem.
> > > 
> > > Signed-off-by: yangerkun <yangerkun@huawei.com>
> > > ---
> > >   drivers/md/dm-crypt.c | 28 +++++++++++++++++++++++-----
> > >   1 file changed, 23 insertions(+), 5 deletions(-)
> > > 
> > > v1->v2:
> > > rewritten the commit msg
> > > 
> > > diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> > > index 40cb1719ae4d..948d1e11d064 100644
> > > --- a/drivers/md/dm-crypt.c
> > > +++ b/drivers/md/dm-crypt.c
> > > @@ -47,6 +47,8 @@
> > >   #define DM_MSG_PREFIX "crypt"
> > > +static DEFINE_IDA(crypt_queue_ida);
> > > +
> > >   /*
> > >    * context holding the current state of a multi-part conversion
> > >    */
> > > @@ -180,6 +182,7 @@ struct crypt_config {
> > >           struct crypto_aead **tfms_aead;
> > >       } cipher_tfm;
> > >       unsigned int tfms_count;
> > > +    int crypt_queue_id;
> > >       unsigned long cipher_flags;
> > >       /*
> > > @@ -2704,6 +2707,9 @@ static void crypt_dtr(struct dm_target *ti)
> > >       if (cc->crypt_queue)
> > >           destroy_workqueue(cc->crypt_queue);
> > > +    if (cc->crypt_queue_id)
> > > +        ida_free(&crypt_queue_ida, cc->crypt_queue_id);
> > > +
> > >       crypt_free_tfms(cc);
> > >       bioset_exit(&cc->bs);
> > > @@ -3340,12 +3346,24 @@ static int crypt_ctr(struct dm_target *ti,
> > > unsigned int argc, char **argv)
> > >       }
> > >       if (test_bit(DM_CRYPT_SAME_CPU, &cc->flags))
> > > -        cc->crypt_queue = alloc_workqueue("kcryptd/%s",
> > > WQ_CPU_INTENSIVE | WQ_MEM_RECLAIM,
> > > +        cc->crypt_queue = alloc_workqueue("kcryptd-%s",
> > > WQ_CPU_INTENSIVE | WQ_MEM_RECLAIM,
> > >                             1, devname);
> > > -    else
> > > -        cc->crypt_queue = alloc_workqueue("kcryptd/%s",
> > > -                          WQ_CPU_INTENSIVE | WQ_MEM_RECLAIM |
> > > WQ_UNBOUND,
> > > -                          num_online_cpus(), devname);
> > > +    else {
> > > +        int id = ida_alloc_min(&crypt_queue_ida, 1, GFP_KERNEL);
> > > +
> > > +        if (id < 0) {
> > > +            ti->error = "Couldn't get kcryptd queue id";
> > > +            ret = id;
> > > +            goto bad;
> > > +        }
> > > +
> > > +        cc->crypt_queue_id = id;
> > > +        cc->crypt_queue = alloc_workqueue("kcryptd-%s-%d",
> > > +                          WQ_CPU_INTENSIVE | WQ_MEM_RECLAIM |
> > > +                          WQ_UNBOUND | WQ_SYSFS,
> > > +                          num_online_cpus(), devname, id);
> > > +    }
> > > +
> > >       if (!cc->crypt_queue) {
> > >           ti->error = "Couldn't create kcryptd queue";
> > >           goto bad;
> > 
> 

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel