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

[Mike Snitzer <snitzer@kernel.org>]

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