| Message ID | f53ff8a9-da8e-da35-cdf8-3bd260d9af33@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
| Series | [resend] dm-crypt: add the "high_priority" flag | expand |
On 4/8/24 9:36 PM, Mikulas Patocka wrote: > 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 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 It is pity that we need an optional flag here leaving decision to the user (I would prefer a magic workqueue setting that will self-tune automagically.) I guess people will set it and forgot about it (until some problem reappears) - as we can store persistent performance flags for LUKS2, so this one will be a new option there. ... > @@ -3399,18 +3401,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); Just one nitpicking though: test_bit(...) ? WQ_HIGHPRI : 0 looks more clear/readable to me (but I guess test_bit should always return 0/1 only). Milan
Hey Milan, Hope things are good for you! On Tue, Apr 09 2024 at 9:04P -0400, Milan Broz <gmazyland@gmail.com> wrote: > On 4/8/24 9:36 PM, Mikulas Patocka wrote: > > 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 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 > > It is pity that we need an optional flag here leaving decision to the user > (I would prefer a magic workqueue setting that will self-tune automagically.) > > I guess people will set it and forgot about it (until some problem reappears) > - as we can store persistent performance flags for LUKS2, so this one will > be a new option there. > > ... > > > @@ -3399,18 +3401,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); > > Just one nitpicking though: > > test_bit(...) ? WQ_HIGHPRI : 0 > > looks more clear/readable to me (but I guess test_bit should always return 0/1 only). I agree, but I actually cleaned stuff up in further with the following incremental patch: diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c index fad4b920008f..eabc576d8d28 100644 --- a/drivers/md/dm-crypt.c +++ b/drivers/md/dm-crypt.c @@ -3234,6 +3234,7 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv) const char *devname = dm_table_device_name(ti->table); int key_size; unsigned int align_mask; + unsigned int common_wq_flags; unsigned long long tmpll; int ret; size_t iv_size_padding, additional_req_size; @@ -3401,23 +3402,25 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv) } ret = -ENOMEM; - cc->io_queue = alloc_workqueue("kcryptd_io/%s", WQ_MEM_RECLAIM | - test_bit(DM_CRYPT_HIGH_PRIORITY, &cc->flags) * WQ_HIGHPRI, - 1, devname); + common_wq_flags = WQ_MEM_RECLAIM; + if (test_bit(DM_CRYPT_HIGH_PRIORITY, &cc->flags)) + common_wq_flags |= WQ_HIGHPRI; + + cc->io_queue = alloc_workqueue("kcryptd_io/%s", common_wq_flags, 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 | - test_bit(DM_CRYPT_HIGH_PRIORITY, &cc->flags) * WQ_HIGHPRI, + if (test_bit(DM_CRYPT_SAME_CPU, &cc->flags)) { + cc->crypt_queue = alloc_workqueue("kcryptd/%s", + common_wq_flags | WQ_CPU_INTENSIVE, 1, devname); - else + } else { cc->crypt_queue = alloc_workqueue("kcryptd/%s", - WQ_CPU_INTENSIVE | WQ_MEM_RECLAIM | WQ_UNBOUND | - test_bit(DM_CRYPT_HIGH_PRIORITY, &cc->flags) * WQ_HIGHPRI, + common_wq_flags | WQ_CPU_INTENSIVE | WQ_UNBOUND, num_online_cpus(), devname); + } if (!cc->crypt_queue) { ti->error = "Couldn't create kcryptd queue"; goto bad; The end result is here (also revised commit header): https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-6.10&id=fdca2c9cb999e781fbb3171541c709bf0e43fbda Doing it this way made sense given the following commit I staged (to reintroduce the use of WQ_SYSFS, will send mail on that next). Thanks, Mike
Index: linux-2.6/drivers/md/dm-crypt.c =================================================================== --- linux-2.6.orig/drivers/md/dm-crypt.c 2024-04-08 19:54:41.000000000 +0200 +++ linux-2.6/drivers/md/dm-crypt.c 2024-04-08 19:54:58.000000000 +0200 @@ -137,9 +137,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 */ @@ -3134,7 +3134,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; @@ -3161,6 +3161,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); @@ -3399,18 +3401,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"; @@ -3427,6 +3433,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; @@ -3547,6 +3555,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); @@ -3560,6 +3569,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)) @@ -3579,6 +3590,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) ? @@ -3706,7 +3718,7 @@ static void crypt_io_hints(struct dm_tar static struct target_type crypt_target = { .name = "crypt", - .version = {1, 25, 0}, + .version = {1, 26, 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 2024-04-08 19:54:41.000000000 +0200 +++ linux-2.6/Documentation/admin-guide/device-mapper/dm-crypt.rst 2024-04-08 19:54:41.000000000 +0200 @@ -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
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 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(-)