Re: [dm-devel] [PATCH] dm-crypt: add the "high_priority" flag

[yangerkun <yangerkun@huaweicloud.com>]

在 2023/3/7 3:01, Mikulas Patocka 写道:
> 
> 
> On Mon, 6 Mar 2023, Mikulas Patocka wrote:
> 
>>> Hi,
>>>
>>> Thanks a lot for your review!
>>>
>>> It's ok to fix the softlockup, but for async write encrypt,
>>> kcryptd_crypt_write_io_submit will add bio to write_tree, and once we
>>> call cond_resched before every kcryptd_io_write, the write performance
>>> may be poor while we meet a high cpu usage scene.
>>
>> Hi
>>
>> To fix this problem, find the PID of the process "dmcrypt_write" and
>> change its priority to -20, for example "renice -n -20 -p 34748".
>>
>> This is the proper way how to fix it; locking up the process for one
>> second is not.
>>
>> We used to have high-priority workqueues by default, but it caused audio
>> playback skipping, so we had to revert it - see
>> f612b2132db529feac4f965f28a1b9258ea7c22b.
>>
>> Perhaps we should add an option to have high-priority kernel threads?
>>
>> Mikulas
> 
> Here I'm sending a patch that makes it optional to raise the priority
> of dm-crypt workqueues and the write thread. Test it, if it helps for you.

Thanks, will test it!
> 
> Mikulas
> 
> 
> 
> From: Mikulas Patocka <mpatocka@redhat.com>
> 
> It was reported that dm-crypt performs badly when the system is loaded[1].
> So I'm adding an option "high_priority" that sets the workqueues and the
> write_thread to nice level -20. This used to be the default in the past,
> but it caused audio skipping, so it had to be reverted - see the commit
> f612b2132db529feac4f965f28a1b9258ea7c22b. This commit makes it optional,
> so that normal users won't be harmed by it.
> 
> [1] https://listman.redhat.com/archives/dm-devel/2023-February/053410.html
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> 
> ---
>   Documentation/admin-guide/device-mapper/dm-crypt.rst |    5 +++
>   drivers/md/dm-crypt.c                                |   28 +++++++++++++------
>   2 files changed, 25 insertions(+), 8 deletions(-)
> 
> Index: linux-2.6/drivers/md/dm-crypt.c
> ===================================================================
> --- linux-2.6.orig/drivers/md/dm-crypt.c
> +++ linux-2.6/drivers/md/dm-crypt.c
> @@ -133,9 +133,9 @@ struct iv_elephant_private {
>    * and encrypts / decrypts at the same time.
>    */
>   enum flags { DM_CRYPT_SUSPENDED, DM_CRYPT_KEY_VALID,
> -	     DM_CRYPT_SAME_CPU, DM_CRYPT_NO_OFFLOAD,
> -	     DM_CRYPT_NO_READ_WORKQUEUE, DM_CRYPT_NO_WRITE_WORKQUEUE,
> -	     DM_CRYPT_WRITE_INLINE };
> +	     DM_CRYPT_SAME_CPU, DM_CRYPT_HIGH_PRIORITY,
> +	     DM_CRYPT_NO_OFFLOAD, DM_CRYPT_NO_READ_WORKQUEUE,
> +	     DM_CRYPT_NO_WRITE_WORKQUEUE, DM_CRYPT_WRITE_INLINE };
>   
>   enum cipher_flags {
>   	CRYPT_MODE_INTEGRITY_AEAD,	/* Use authenticated mode for cipher */
> @@ -3087,7 +3087,7 @@ static int crypt_ctr_optional(struct dm_
>   	struct crypt_config *cc = ti->private;
>   	struct dm_arg_set as;
>   	static const struct dm_arg _args[] = {
> -		{0, 8, "Invalid number of feature args"},
> +		{0, 9, "Invalid number of feature args"},
>   	};
>   	unsigned int opt_params, val;
>   	const char *opt_string, *sval;
> @@ -3114,6 +3114,8 @@ static int crypt_ctr_optional(struct dm_
>   
>   		else if (!strcasecmp(opt_string, "same_cpu_crypt"))
>   			set_bit(DM_CRYPT_SAME_CPU, &cc->flags);
> +		else if (!strcasecmp(opt_string, "high_priority"))
> +			set_bit(DM_CRYPT_HIGH_PRIORITY, &cc->flags);
>   
>   		else if (!strcasecmp(opt_string, "submit_from_crypt_cpus"))
>   			set_bit(DM_CRYPT_NO_OFFLOAD, &cc->flags);
> @@ -3352,18 +3354,22 @@ static int crypt_ctr(struct dm_target *t
>   	}
>   
>   	ret = -ENOMEM;
> -	cc->io_queue = alloc_workqueue("kcryptd_io/%s", WQ_MEM_RECLAIM, 1, devname);
> +	cc->io_queue = alloc_workqueue("kcryptd_io/%s", WQ_MEM_RECLAIM |
> +				       test_bit(DM_CRYPT_HIGH_PRIORITY, &cc->flags) * WQ_HIGHPRI,
> +				       1, devname);
>   	if (!cc->io_queue) {
>   		ti->error = "Couldn't create kcryptd io queue";
>   		goto bad;
>   	}
>   
>   	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 |
> +						  test_bit(DM_CRYPT_HIGH_PRIORITY, &cc->flags) * WQ_HIGHPRI,
>   						  1, devname);
>   	else
>   		cc->crypt_queue = alloc_workqueue("kcryptd/%s",
> -						  WQ_CPU_INTENSIVE | WQ_MEM_RECLAIM | WQ_UNBOUND,
> +						  WQ_CPU_INTENSIVE | WQ_MEM_RECLAIM | WQ_UNBOUND |
> +						  test_bit(DM_CRYPT_HIGH_PRIORITY, &cc->flags) * WQ_HIGHPRI,
>   						  num_online_cpus(), devname);
>   	if (!cc->crypt_queue) {
>   		ti->error = "Couldn't create kcryptd queue";
> @@ -3380,6 +3386,8 @@ static int crypt_ctr(struct dm_target *t
>   		ti->error = "Couldn't spawn write thread";
>   		goto bad;
>   	}
> +	if (test_bit(DM_CRYPT_HIGH_PRIORITY, &cc->flags))
> +		set_user_nice(cc->write_thread, MIN_NICE);
>   
>   	ti->num_flush_bios = 1;
>   	ti->limit_swap_bios = true;
> @@ -3500,6 +3508,7 @@ static void crypt_status(struct dm_targe
>   
>   		num_feature_args += !!ti->num_discard_bios;
>   		num_feature_args += test_bit(DM_CRYPT_SAME_CPU, &cc->flags);
> +		num_feature_args += test_bit(DM_CRYPT_HIGH_PRIORITY, &cc->flags);
>   		num_feature_args += test_bit(DM_CRYPT_NO_OFFLOAD, &cc->flags);
>   		num_feature_args += test_bit(DM_CRYPT_NO_READ_WORKQUEUE, &cc->flags);
>   		num_feature_args += test_bit(DM_CRYPT_NO_WRITE_WORKQUEUE, &cc->flags);
> @@ -3513,6 +3522,8 @@ static void crypt_status(struct dm_targe
>   				DMEMIT(" allow_discards");
>   			if (test_bit(DM_CRYPT_SAME_CPU, &cc->flags))
>   				DMEMIT(" same_cpu_crypt");
> +			if (test_bit(DM_CRYPT_HIGH_PRIORITY, &cc->flags))
> +				DMEMIT(" high_priority");
>   			if (test_bit(DM_CRYPT_NO_OFFLOAD, &cc->flags))
>   				DMEMIT(" submit_from_crypt_cpus");
>   			if (test_bit(DM_CRYPT_NO_READ_WORKQUEUE, &cc->flags))
> @@ -3532,6 +3543,7 @@ static void crypt_status(struct dm_targe
>   		DMEMIT_TARGET_NAME_VERSION(ti->type);
>   		DMEMIT(",allow_discards=%c", ti->num_discard_bios ? 'y' : 'n');
>   		DMEMIT(",same_cpu_crypt=%c", test_bit(DM_CRYPT_SAME_CPU, &cc->flags) ? 'y' : 'n');
> +		DMEMIT(",high_priority=%c", test_bit(DM_CRYPT_HIGH_PRIORITY, &cc->flags) ? 'y' : 'n');
>   		DMEMIT(",submit_from_crypt_cpus=%c", test_bit(DM_CRYPT_NO_OFFLOAD, &cc->flags) ?
>   		       'y' : 'n');
>   		DMEMIT(",no_read_workqueue=%c", test_bit(DM_CRYPT_NO_READ_WORKQUEUE, &cc->flags) ?
> @@ -3659,7 +3671,7 @@ static void crypt_io_hints(struct dm_tar
>   
>   static struct target_type crypt_target = {
>   	.name   = "crypt",
> -	.version = {1, 24, 0},
> +	.version = {1, 25, 0},
>   	.module = THIS_MODULE,
>   	.ctr    = crypt_ctr,
>   	.dtr    = crypt_dtr,
> Index: linux-2.6/Documentation/admin-guide/device-mapper/dm-crypt.rst
> ===================================================================
> --- linux-2.6.orig/Documentation/admin-guide/device-mapper/dm-crypt.rst
> +++ linux-2.6/Documentation/admin-guide/device-mapper/dm-crypt.rst
> @@ -113,6 +113,11 @@ same_cpu_crypt
>       The default is to use an unbound workqueue so that encryption work
>       is automatically balanced between available CPUs.
>   
> +high_priority
> +    Set dm-crypt workqueues and the writer thread to high priority. This
> +    improves throughput and latency of dm-crypt while degrading general
> +    responsiveness of the system.
> +
>   submit_from_crypt_cpus
>       Disable offloading writes to a separate thread after encryption.
>       There are some situations where offloading write bios from the
> 

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