* [PATCH resend] dm-crypt: add the "high_priority" flag
@ 2024-04-08 19:36 Mikulas Patocka
2024-04-09 13:04 ` Milan Broz
0 siblings, 1 reply; 3+ messages in thread
From: Mikulas Patocka @ 2024-04-08 19:36 UTC (permalink / raw
To: Mike Snitzer
Cc: dm-devel, Milan Broz, yangerkun, Bart Van Assche,
Alasdair G. Kergon, Laurence Oberman
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(-)
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH resend] dm-crypt: add the "high_priority" flag
2024-04-08 19:36 [PATCH resend] dm-crypt: add the "high_priority" flag Mikulas Patocka
@ 2024-04-09 13:04 ` Milan Broz
2024-04-10 15:33 ` Mike Snitzer
0 siblings, 1 reply; 3+ messages in thread
From: Milan Broz @ 2024-04-09 13:04 UTC (permalink / raw
To: Mikulas Patocka, Mike Snitzer
Cc: dm-devel, yangerkun, Bart Van Assche, Alasdair G. Kergon,
Laurence Oberman
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH resend] dm-crypt: add the "high_priority" flag
2024-04-09 13:04 ` Milan Broz
@ 2024-04-10 15:33 ` Mike Snitzer
0 siblings, 0 replies; 3+ messages in thread
From: Mike Snitzer @ 2024-04-10 15:33 UTC (permalink / raw
To: Milan Broz
Cc: Mikulas Patocka, dm-devel, yangerkun, Bart Van Assche,
Alasdair G. Kergon, Laurence Oberman
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-04-10 15:33 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-08 19:36 [PATCH resend] dm-crypt: add the "high_priority" flag Mikulas Patocka
2024-04-09 13:04 ` Milan Broz
2024-04-10 15:33 ` Mike Snitzer
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.