| Message ID | 20230711061129.2706759-1-yangerkun@huaweicloud.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] dm-crypt: reexport sysfs of kcryptd workqueue | expand |
[resend after rebase and fixing dm-devel mailing list address] On Wed, Apr 10 2024 at 11:35P -0400, Mike Snitzer <snitzer@kernel.org> wrote: > On Sat, Mar 30 2024 at 4:05P -0400, > yangerkun <yangerkun@huaweicloud.com> wrote: > > > Hi, > > > > Ping for this patch! > > > > 在 2023/7/11 14:11, 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: > > > rewrite the commit msg > > > > > > v2->v3: > > > no logical change, just rebase to latest linux kernel > > > > > > diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c > > > index 1dc6227d353e..f4678eb71322 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 > > > */ > > > @@ -182,6 +184,7 @@ struct crypt_config { > > > struct crypto_aead **tfms_aead; > > > } cipher_tfm; > > > unsigned int tfms_count; > > > + int crypt_queue_id; > > > unsigned long cipher_flags; > > > /* > > > @@ -2735,6 +2738,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); > > > @@ -3371,12 +3377,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; > > > > I'm OK with adding WQ_SYSFS to export workqueue info. dm-crypt's > performance is very tighly coupled with its use of workqueues. So > allowing more visibility into workqueues makes sense. > > That said, I'm not loving that the sysfs entry will have a dynamic > name (made possible with ida) -- but I can live with it. > > However, I do think it is useful to always use WQ_SYSFS (even if > DM_CRYPT_SAME_CPU, and also for the IO wq). > > So I've made the use of ida common to all dm-crypt wq. I was able to > do this more cleanly by rebasing ontop of dm-6.10 (which now includes > recent dm-crypt patch to allow WQ_HIGHPRI to be configured), see: > https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-6.10&id=6bd1e0b331ddd7bb4d6b2abc8472a36602180aa5 > > Thanks, > Mike
在 2024/4/11 0:02, Mike Snitzer 写道: > [resend after rebase and fixing dm-devel mailing list address] > > On Wed, Apr 10 2024 at 11:35P -0400, > Mike Snitzer <snitzer@kernel.org> wrote: > >> On Sat, Mar 30 2024 at 4:05P -0400, >> yangerkun <yangerkun@huaweicloud.com> wrote: >> >>> Hi, >>> >>> Ping for this patch! >>> >>> 在 2023/7/11 14:11, 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: >>>> rewrite the commit msg >>>> >>>> v2->v3: >>>> no logical change, just rebase to latest linux kernel >>>> >>>> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c >>>> index 1dc6227d353e..f4678eb71322 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 >>>> */ >>>> @@ -182,6 +184,7 @@ struct crypt_config { >>>> struct crypto_aead **tfms_aead; >>>> } cipher_tfm; >>>> unsigned int tfms_count; >>>> + int crypt_queue_id; >>>> unsigned long cipher_flags; >>>> /* >>>> @@ -2735,6 +2738,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); >>>> @@ -3371,12 +3377,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; >>> >> >> I'm OK with adding WQ_SYSFS to export workqueue info. dm-crypt's >> performance is very tighly coupled with its use of workqueues. So >> allowing more visibility into workqueues makes sense. >> >> That said, I'm not loving that the sysfs entry will have a dynamic >> name (made possible with ida) -- but I can live with it. >> >> However, I do think it is useful to always use WQ_SYSFS (even if >> DM_CRYPT_SAME_CPU, and also for the IO wq). >> >> So I've made the use of ida common to all dm-crypt wq. I was able to >> do this more cleanly by rebasing ontop of dm-6.10 (which now includes >> recent dm-crypt patch to allow WQ_HIGHPRI to be configured), see: >> https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-6.10&id=6bd1e0b331ddd7bb4d6b2abc8472a36602180aa5 >> >> Thanks, >> Mike This version looks good to me. Thanks a lot!
diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c index 1dc6227d353e..f4678eb71322 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 */ @@ -182,6 +184,7 @@ struct crypt_config { struct crypto_aead **tfms_aead; } cipher_tfm; unsigned int tfms_count; + int crypt_queue_id; unsigned long cipher_flags; /* @@ -2735,6 +2738,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); @@ -3371,12 +3377,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;