From: yangerkun <yangerkun@huaweicloud.com>
To: Mike Snitzer <snitzer@kernel.org>
Cc: mpatocka@redhat.com, agk@redhat.com, tj@kernel.org,
dm-devel@lists.linux.dev, yangerkun@huawei.com,
yukuai3@huawei.com, Milan Broz <gmazyland@gmail.com>
Subject: Re: [PATCH v3] dm-crypt: reexport sysfs of kcryptd workqueue
Date: Thu, 11 Apr 2024 10:17:58 +0800 [thread overview]
Message-ID: <cfd693de-449e-a505-39e5-0a7931c3a5b2@huaweicloud.com> (raw)
In-Reply-To: <Zha4M0ru46QrKYgq@redhat.com>
在 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!
prev parent reply other threads:[~2024-04-11 2:18 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-11 6:11 [dm-devel] [PATCH v3] dm-crypt: reexport sysfs of kcryptd workqueue yangerkun
[not found] ` <45a09f8d-d167-c78c-7ac8-3ed27db18653@huaweicloud.com>
[not found] ` <Zhaxz1ZkGzbfW0RQ@redhat.com>
2024-04-10 16:02 ` Mike Snitzer
2024-04-11 2:17 ` yangerkun [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cfd693de-449e-a505-39e5-0a7931c3a5b2@huaweicloud.com \
--to=yangerkun@huaweicloud.com \
--cc=agk@redhat.com \
--cc=dm-devel@lists.linux.dev \
--cc=gmazyland@gmail.com \
--cc=mpatocka@redhat.com \
--cc=snitzer@kernel.org \
--cc=tj@kernel.org \
--cc=yangerkun@huawei.com \
--cc=yukuai3@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).