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

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

[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;
-- 
2.39.2

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

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

[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

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

[yangerkun]

在 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!