| Message ID | 20240130091300.2968534-8-tj@kernel.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
| Series | [1/8] workqueue: Update lock debugging code | expand |
On 2024-01-29 23:11:54 [-1000], Tejun Heo wrote: > --- a/drivers/md/dm-crypt.c > +++ b/drivers/md/dm-crypt.c > @@ -2263,9 +2232,8 @@ static void kcryptd_queue_crypt(struct dm_crypt_io *io) /* * in_hardirq(): Crypto API's skcipher_walk_first() refuses to work in hard IRQ context. * irqs_disabled(): the kernel may run some IO completion from the idle thread, but * it is being executed with irqs disabled. */ > * it is being executed with irqs disabled. > */ > if (in_hardirq() || irqs_disabled()) { > - io->in_tasklet = true; > - tasklet_init(&io->tasklet, kcryptd_crypt_tasklet, (unsigned long)&io->work); > - tasklet_schedule(&io->tasklet); > + INIT_WORK(&io->work, kcryptd_crypt); > + queue_work(system_bh_wq, &io->work); Why do we need the tasklet here in the first place? Couldn't we use workqueue? As per comment, the request originates in hardirq and then it is moved to tasklet. Couldn't it be moved to workqueue regardless? > return; > } > Sebastian
Hello, On Tue, Jan 30, 2024 at 11:46:45AM +0100, Sebastian Andrzej Siewior wrote: > On 2024-01-29 23:11:54 [-1000], Tejun Heo wrote: > > if (in_hardirq() || irqs_disabled()) { > > - io->in_tasklet = true; > > - tasklet_init(&io->tasklet, kcryptd_crypt_tasklet, (unsigned long)&io->work); > > - tasklet_schedule(&io->tasklet); > > + INIT_WORK(&io->work, kcryptd_crypt); > > + queue_work(system_bh_wq, &io->work); > > Why do we need the tasklet here in the first place? Couldn't we use > workqueue? As per comment, the request originates in hardirq and then it > is moved to tasklet. Couldn't it be moved to workqueue regardless? Yes, you can and if you replace that system_bh_wq with system_wq, or system_highpri_wq, everything should be fine in terms of correctness. However, that would mean that the work item now would always incur a scheduling latency which can be lengthy in certain circumstances. Whether that's an actual problem for the subsystem at hand, I have no idea. Thanks.
This seems OK. Reviewed-by: Mikulas Patocka <mpatocka@redhat.com> On Mon, 29 Jan 2024, Tejun Heo wrote: > The only generic interface to execute asynchronously in the BH context is > tasklet; however, it's marked deprecated and has some design flaws. To > replace tasklets, BH workqueue support was recently added. A BH workqueue > behaves similarly to regular workqueues except that the queued work items > are executed in the BH context. > > This patch converts dm-crypt from tasklet to BH workqueue. > > Like a regular workqueue, a BH workqueue allows freeing the currently > executing work item. Converting from tasklet to BH workqueue removes the > need for deferring bio_endio() again to a work item, which was buggy anyway. > > I tested this lightly with "--perf-no_read_workqueue > --perf-no_write_workqueue" + some code modifications, but would really > -appreciate if someone who knows the code base better could take a look. > > Signed-off-by: Tejun Heo <tj@kernel.org> > Link: http://lkml.kernel.org/r/82b964f0-c2c8-a2c6-5b1f-f3145dc2c8e5@redhat.com > Cc: Mikulas Patocka <mpatocka@redhat.com> > Cc: Linus Torvalds <torvalds@linux-foundation.org> > Cc: Alasdair Kergon <agk@redhat.com> > Cc: Mike Snitzer <snitzer@kernel.org> > Cc: dm-devel@lists.linux.dev > --- > drivers/md/dm-crypt.c | 36 ++---------------------------------- > 1 file changed, 2 insertions(+), 34 deletions(-) > > diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c > index 855b482cbff1..619c762d4072 100644 > --- a/drivers/md/dm-crypt.c > +++ b/drivers/md/dm-crypt.c > @@ -73,11 +73,8 @@ struct dm_crypt_io { > struct bio *base_bio; > u8 *integrity_metadata; > bool integrity_metadata_from_pool:1; > - bool in_tasklet:1; > > struct work_struct work; > - struct tasklet_struct tasklet; > - > struct convert_context ctx; > > atomic_t io_pending; > @@ -1762,7 +1759,6 @@ static void crypt_io_init(struct dm_crypt_io *io, struct crypt_config *cc, > io->ctx.r.req = NULL; > io->integrity_metadata = NULL; > io->integrity_metadata_from_pool = false; > - io->in_tasklet = false; > atomic_set(&io->io_pending, 0); > } > > @@ -1771,13 +1767,6 @@ static void crypt_inc_pending(struct dm_crypt_io *io) > atomic_inc(&io->io_pending); > } > > -static void kcryptd_io_bio_endio(struct work_struct *work) > -{ > - struct dm_crypt_io *io = container_of(work, struct dm_crypt_io, work); > - > - bio_endio(io->base_bio); > -} > - > /* > * One of the bios was finished. Check for completion of > * the whole request and correctly clean up the buffer. > @@ -1800,21 +1789,6 @@ static void crypt_dec_pending(struct dm_crypt_io *io) > kfree(io->integrity_metadata); > > base_bio->bi_status = error; > - > - /* > - * If we are running this function from our tasklet, > - * we can't call bio_endio() here, because it will call > - * clone_endio() from dm.c, which in turn will > - * free the current struct dm_crypt_io structure with > - * our tasklet. In this case we need to delay bio_endio() > - * execution to after the tasklet is done and dequeued. > - */ > - if (io->in_tasklet) { > - INIT_WORK(&io->work, kcryptd_io_bio_endio); > - queue_work(cc->io_queue, &io->work); > - return; > - } > - > bio_endio(base_bio); > } > > @@ -2246,11 +2220,6 @@ static void kcryptd_crypt(struct work_struct *work) > kcryptd_crypt_write_convert(io); > } > > -static void kcryptd_crypt_tasklet(unsigned long work) > -{ > - kcryptd_crypt((struct work_struct *)work); > -} > - > static void kcryptd_queue_crypt(struct dm_crypt_io *io) > { > struct crypt_config *cc = io->cc; > @@ -2263,9 +2232,8 @@ static void kcryptd_queue_crypt(struct dm_crypt_io *io) > * it is being executed with irqs disabled. > */ > if (in_hardirq() || irqs_disabled()) { > - io->in_tasklet = true; > - tasklet_init(&io->tasklet, kcryptd_crypt_tasklet, (unsigned long)&io->work); > - tasklet_schedule(&io->tasklet); > + INIT_WORK(&io->work, kcryptd_crypt); > + queue_work(system_bh_wq, &io->work); > return; > } > > -- > 2.43.0 >
diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c index 855b482cbff1..619c762d4072 100644 --- a/drivers/md/dm-crypt.c +++ b/drivers/md/dm-crypt.c @@ -73,11 +73,8 @@ struct dm_crypt_io { struct bio *base_bio; u8 *integrity_metadata; bool integrity_metadata_from_pool:1; - bool in_tasklet:1; struct work_struct work; - struct tasklet_struct tasklet; - struct convert_context ctx; atomic_t io_pending; @@ -1762,7 +1759,6 @@ static void crypt_io_init(struct dm_crypt_io *io, struct crypt_config *cc, io->ctx.r.req = NULL; io->integrity_metadata = NULL; io->integrity_metadata_from_pool = false; - io->in_tasklet = false; atomic_set(&io->io_pending, 0); } @@ -1771,13 +1767,6 @@ static void crypt_inc_pending(struct dm_crypt_io *io) atomic_inc(&io->io_pending); } -static void kcryptd_io_bio_endio(struct work_struct *work) -{ - struct dm_crypt_io *io = container_of(work, struct dm_crypt_io, work); - - bio_endio(io->base_bio); -} - /* * One of the bios was finished. Check for completion of * the whole request and correctly clean up the buffer. @@ -1800,21 +1789,6 @@ static void crypt_dec_pending(struct dm_crypt_io *io) kfree(io->integrity_metadata); base_bio->bi_status = error; - - /* - * If we are running this function from our tasklet, - * we can't call bio_endio() here, because it will call - * clone_endio() from dm.c, which in turn will - * free the current struct dm_crypt_io structure with - * our tasklet. In this case we need to delay bio_endio() - * execution to after the tasklet is done and dequeued. - */ - if (io->in_tasklet) { - INIT_WORK(&io->work, kcryptd_io_bio_endio); - queue_work(cc->io_queue, &io->work); - return; - } - bio_endio(base_bio); } @@ -2246,11 +2220,6 @@ static void kcryptd_crypt(struct work_struct *work) kcryptd_crypt_write_convert(io); } -static void kcryptd_crypt_tasklet(unsigned long work) -{ - kcryptd_crypt((struct work_struct *)work); -} - static void kcryptd_queue_crypt(struct dm_crypt_io *io) { struct crypt_config *cc = io->cc; @@ -2263,9 +2232,8 @@ static void kcryptd_queue_crypt(struct dm_crypt_io *io) * it is being executed with irqs disabled. */ if (in_hardirq() || irqs_disabled()) { - io->in_tasklet = true; - tasklet_init(&io->tasklet, kcryptd_crypt_tasklet, (unsigned long)&io->work); - tasklet_schedule(&io->tasklet); + INIT_WORK(&io->work, kcryptd_crypt); + queue_work(system_bh_wq, &io->work); return; }
The only generic interface to execute asynchronously in the BH context is tasklet; however, it's marked deprecated and has some design flaws. To replace tasklets, BH workqueue support was recently added. A BH workqueue behaves similarly to regular workqueues except that the queued work items are executed in the BH context. This patch converts dm-crypt from tasklet to BH workqueue. Like a regular workqueue, a BH workqueue allows freeing the currently executing work item. Converting from tasklet to BH workqueue removes the need for deferring bio_endio() again to a work item, which was buggy anyway. I tested this lightly with "--perf-no_read_workqueue --perf-no_write_workqueue" + some code modifications, but would really -appreciate if someone who knows the code base better could take a look. Signed-off-by: Tejun Heo <tj@kernel.org> Link: http://lkml.kernel.org/r/82b964f0-c2c8-a2c6-5b1f-f3145dc2c8e5@redhat.com Cc: Mikulas Patocka <mpatocka@redhat.com> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Alasdair Kergon <agk@redhat.com> Cc: Mike Snitzer <snitzer@kernel.org> Cc: dm-devel@lists.linux.dev --- drivers/md/dm-crypt.c | 36 ++---------------------------------- 1 file changed, 2 insertions(+), 34 deletions(-)