2024-04-11 09:15:15

by Li Lingfeng

[permalink] [raw]
Subject: [PATCH 6.6] Revert "dm-crypt, dm-verity: disable tasklets"

This reverts commit 5735a2671ffb70ea29ca83969fe01316ee2ed6fc which is
commit 0a9bab391e336489169b95cb0d4553d921302189 upstream.

Tasklet is thought to cause memory corruption [1], so it was disabled in
dm-crypt and dm-verity. However, memory corruption may not happen since
cc->io_queue is created without WQ_UNBOUND [2].
Revert commit 5735a2671ffb ("dm-crypt, dm-verity: disable tasklets") to
bring tasklet back.

[1] https://lore.kernel.org/all/[email protected]/T/
[2] https://lore.kernel.org/all/[email protected]/

Signed-off-by: Li Lingfeng <[email protected]>
---
drivers/md/dm-crypt.c | 38 +++++++++++++++++++++++++++++++++--
drivers/md/dm-verity-target.c | 26 ++++++++++++++++++++++--
drivers/md/dm-verity.h | 1 +
3 files changed, 61 insertions(+), 4 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index aa6bb5b4704b..a60d91d02e28 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -75,8 +75,10 @@ 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;

@@ -1775,6 +1777,7 @@ 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);
}

@@ -1785,6 +1788,13 @@ static void crypt_inc_pending(struct dm_crypt_io *io)

static void kcryptd_queue_read(struct dm_crypt_io *io);

+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.
@@ -1817,6 +1827,20 @@ static void crypt_dec_pending(struct dm_crypt_io *io)

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);
}

@@ -2291,6 +2315,11 @@ 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;
@@ -2302,10 +2331,15 @@ static void kcryptd_queue_crypt(struct dm_crypt_io *io)
* irqs_disabled(): the kernel may run some IO completion from the idle thread, but
* it is being executed with irqs disabled.
*/
- if (!(in_hardirq() || irqs_disabled())) {
- kcryptd_crypt(&io->work);
+ if (in_hardirq() || irqs_disabled()) {
+ io->in_tasklet = true;
+ tasklet_init(&io->tasklet, kcryptd_crypt_tasklet, (unsigned long)&io->work);
+ tasklet_schedule(&io->tasklet);
return;
}
+
+ kcryptd_crypt(&io->work);
+ return;
}

INIT_WORK(&io->work, kcryptd_crypt);
diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
index 49e4a35d7019..0bb126eadc0d 100644
--- a/drivers/md/dm-verity-target.c
+++ b/drivers/md/dm-verity-target.c
@@ -701,6 +701,23 @@ static void verity_work(struct work_struct *w)
verity_finish_io(io, errno_to_blk_status(verity_verify_io(io)));
}

+static void verity_tasklet(unsigned long data)
+{
+ struct dm_verity_io *io = (struct dm_verity_io *)data;
+ int err;
+
+ io->in_tasklet = true;
+ err = verity_verify_io(io);
+ if (err == -EAGAIN || err == -ENOMEM) {
+ /* fallback to retrying with work-queue */
+ INIT_WORK(&io->work, verity_work);
+ queue_work(io->v->verify_wq, &io->work);
+ return;
+ }
+
+ verity_finish_io(io, errno_to_blk_status(err));
+}
+
static void verity_end_io(struct bio *bio)
{
struct dm_verity_io *io = bio->bi_private;
@@ -713,8 +730,13 @@ static void verity_end_io(struct bio *bio)
return;
}

- INIT_WORK(&io->work, verity_work);
- queue_work(io->v->verify_wq, &io->work);
+ if (static_branch_unlikely(&use_tasklet_enabled) && io->v->use_tasklet) {
+ tasklet_init(&io->tasklet, verity_tasklet, (unsigned long)io);
+ tasklet_schedule(&io->tasklet);
+ } else {
+ INIT_WORK(&io->work, verity_work);
+ queue_work(io->v->verify_wq, &io->work);
+ }
}

/*
diff --git a/drivers/md/dm-verity.h b/drivers/md/dm-verity.h
index db93a91169d5..7e495cc375b0 100644
--- a/drivers/md/dm-verity.h
+++ b/drivers/md/dm-verity.h
@@ -87,6 +87,7 @@ struct dm_verity_io {
bool in_tasklet;

struct work_struct work;
+ struct tasklet_struct tasklet;

char *recheck_buffer;

--
2.31.1



2024-04-11 09:52:56

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH 6.6] Revert "dm-crypt, dm-verity: disable tasklets"

On Thu, Apr 11, 2024 at 05:15:39PM +0800, Li Lingfeng wrote:
> This reverts commit 5735a2671ffb70ea29ca83969fe01316ee2ed6fc which is
> commit 0a9bab391e336489169b95cb0d4553d921302189 upstream.
>
> Tasklet is thought to cause memory corruption [1], so it was disabled in
> dm-crypt and dm-verity. However, memory corruption may not happen since
> cc->io_queue is created without WQ_UNBOUND [2].
> Revert commit 5735a2671ffb ("dm-crypt, dm-verity: disable tasklets") to
> bring tasklet back.
>
> [1] https://lore.kernel.org/all/[email protected]/T/
> [2] https://lore.kernel.org/all/[email protected]/
>
> Signed-off-by: Li Lingfeng <[email protected]>
> ---
> drivers/md/dm-crypt.c | 38 +++++++++++++++++++++++++++++++++--
> drivers/md/dm-verity-target.c | 26 ++++++++++++++++++++++--
> drivers/md/dm-verity.h | 1 +
> 3 files changed, 61 insertions(+), 4 deletions(-)

What is the commit id of this in Linus's tree? And why only 6.6.y?

thanks,

greg k-h

2024-04-11 12:18:20

by Li Lingfeng

[permalink] [raw]
Subject: Re: [PATCH 6.6] Revert "dm-crypt, dm-verity: disable tasklets"

There is no need to revert it in Linus's tree since the expanded
workqueue can work like tasklet.
https://lore.kernel.org/lkml/[email protected]/

I'm not sure if the problem described in [1] could be triggered by a
scenario I haven't considered. So I only send the revert patch to 6.6
mainly for comments.
Of course, I think the other branches need it too.

Thanks

在 2024/4/11 17:52, Greg KH 写道:
> On Thu, Apr 11, 2024 at 05:15:39PM +0800, Li Lingfeng wrote:
>> This reverts commit 5735a2671ffb70ea29ca83969fe01316ee2ed6fc which is
>> commit 0a9bab391e336489169b95cb0d4553d921302189 upstream.
>>
>> Tasklet is thought to cause memory corruption [1], so it was disabled in
>> dm-crypt and dm-verity. However, memory corruption may not happen since
>> cc->io_queue is created without WQ_UNBOUND [2].
>> Revert commit 5735a2671ffb ("dm-crypt, dm-verity: disable tasklets") to
>> bring tasklet back.
>>
>> [1] https://lore.kernel.org/all/[email protected]/T/
>> [2] https://lore.kernel.org/all/[email protected]/
>>
>> Signed-off-by: Li Lingfeng <[email protected]>
>> ---
>> drivers/md/dm-crypt.c | 38 +++++++++++++++++++++++++++++++++--
>> drivers/md/dm-verity-target.c | 26 ++++++++++++++++++++++--
>> drivers/md/dm-verity.h | 1 +
>> 3 files changed, 61 insertions(+), 4 deletions(-)
> What is the commit id of this in Linus's tree? And why only 6.6.y?
>
> thanks,
>
> greg k-h
>

2024-04-11 12:46:33

by Mikulas Patocka

[permalink] [raw]
Subject: Re: [PATCH 6.6] Revert "dm-crypt, dm-verity: disable tasklets"

Hi

I NACK this.


On Thu, 11 Apr 2024, Li Lingfeng wrote:

> This reverts commit 5735a2671ffb70ea29ca83969fe01316ee2ed6fc which is
> commit 0a9bab391e336489169b95cb0d4553d921302189 upstream.
>
> Tasklet is thought to cause memory corruption [1], so it was disabled in
> dm-crypt and dm-verity. However, memory corruption may not happen since
> cc->io_queue is created without WQ_UNBOUND [2].
> Revert commit 5735a2671ffb ("dm-crypt, dm-verity: disable tasklets") to
> bring tasklet back.
>
> [1] https://lore.kernel.org/all/[email protected]/T/
> [2] https://lore.kernel.org/all/[email protected]/

Regarding [2] - if you add mdelay, you can make the race condition less
reproducible, but it is not a proper fix and the race condition stays
there unfixed.

Workqueues and ksoftirqd may be scheduled arbitrarily, there is no
guarantee that they will be executed in a particular order, even if they
are executed on the same CPU.

Mikulas

> Signed-off-by: Li Lingfeng <[email protected]>
> ---
> drivers/md/dm-crypt.c | 38 +++++++++++++++++++++++++++++++++--
> drivers/md/dm-verity-target.c | 26 ++++++++++++++++++++++--
> drivers/md/dm-verity.h | 1 +
> 3 files changed, 61 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index aa6bb5b4704b..a60d91d02e28 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -75,8 +75,10 @@ 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;
>
> @@ -1775,6 +1777,7 @@ 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);
> }
>
> @@ -1785,6 +1788,13 @@ static void crypt_inc_pending(struct dm_crypt_io *io)
>
> static void kcryptd_queue_read(struct dm_crypt_io *io);
>
> +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.
> @@ -1817,6 +1827,20 @@ static void crypt_dec_pending(struct dm_crypt_io *io)
>
> 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);
> }
>
> @@ -2291,6 +2315,11 @@ 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;
> @@ -2302,10 +2331,15 @@ static void kcryptd_queue_crypt(struct dm_crypt_io *io)
> * irqs_disabled(): the kernel may run some IO completion from the idle thread, but
> * it is being executed with irqs disabled.
> */
> - if (!(in_hardirq() || irqs_disabled())) {
> - kcryptd_crypt(&io->work);
> + if (in_hardirq() || irqs_disabled()) {
> + io->in_tasklet = true;
> + tasklet_init(&io->tasklet, kcryptd_crypt_tasklet, (unsigned long)&io->work);
> + tasklet_schedule(&io->tasklet);
> return;
> }
> +
> + kcryptd_crypt(&io->work);
> + return;
> }
>
> INIT_WORK(&io->work, kcryptd_crypt);
> diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
> index 49e4a35d7019..0bb126eadc0d 100644
> --- a/drivers/md/dm-verity-target.c
> +++ b/drivers/md/dm-verity-target.c
> @@ -701,6 +701,23 @@ static void verity_work(struct work_struct *w)
> verity_finish_io(io, errno_to_blk_status(verity_verify_io(io)));
> }
>
> +static void verity_tasklet(unsigned long data)
> +{
> + struct dm_verity_io *io = (struct dm_verity_io *)data;
> + int err;
> +
> + io->in_tasklet = true;
> + err = verity_verify_io(io);
> + if (err == -EAGAIN || err == -ENOMEM) {
> + /* fallback to retrying with work-queue */
> + INIT_WORK(&io->work, verity_work);
> + queue_work(io->v->verify_wq, &io->work);
> + return;
> + }
> +
> + verity_finish_io(io, errno_to_blk_status(err));
> +}
> +
> static void verity_end_io(struct bio *bio)
> {
> struct dm_verity_io *io = bio->bi_private;
> @@ -713,8 +730,13 @@ static void verity_end_io(struct bio *bio)
> return;
> }
>
> - INIT_WORK(&io->work, verity_work);
> - queue_work(io->v->verify_wq, &io->work);
> + if (static_branch_unlikely(&use_tasklet_enabled) && io->v->use_tasklet) {
> + tasklet_init(&io->tasklet, verity_tasklet, (unsigned long)io);
> + tasklet_schedule(&io->tasklet);
> + } else {
> + INIT_WORK(&io->work, verity_work);
> + queue_work(io->v->verify_wq, &io->work);
> + }
> }
>
> /*
> diff --git a/drivers/md/dm-verity.h b/drivers/md/dm-verity.h
> index db93a91169d5..7e495cc375b0 100644
> --- a/drivers/md/dm-verity.h
> +++ b/drivers/md/dm-verity.h
> @@ -87,6 +87,7 @@ struct dm_verity_io {
> bool in_tasklet;
>
> struct work_struct work;
> + struct tasklet_struct tasklet;
>
> char *recheck_buffer;
>
> --
> 2.31.1
>


2024-04-12 02:12:07

by Li Lingfeng

[permalink] [raw]
Subject: Re: [PATCH 6.6] Revert "dm-crypt, dm-verity: disable tasklets"

Hi

I'm having difficulty understanding "Workqueues and ksoftirqd may be
scheduled arbitrarily".
This is my understanding:
kcryptd_queue_crypt
 tasklet_schedule
  __tasklet_schedule
   __tasklet_schedule_common
    raise_softirq_irqoff
     wakeup_softirqd
      wake_up_process // ksoftirqd

run_ksoftirqd
 __do_softirq
  softirq_handle_begin
   __local_bh_disable_ip // Turn off preemption
<---------- [1] ---------->
  tasklet_action // h->action
   tasklet_action_common
    tasklet_trylock
     kcryptd_crypt_tasklet // t->func(t->data)
     ...
      queue_work(cc->io_queue, &io->work)
<---------- [2] ---------->
    tasklet_unlock

// workqueue process
kcryptd_io_bio_endio
 ...
 // free tasklet_struct

Since preemption has been turned off at [1], I'm confused about how the
CPU can be scheduled out to do work first at [2].
Would you mind explaining it to me?

Thanks

在 2024/4/11 20:37, Mikulas Patocka 写道:
> Hi
>
> I NACK this.
>
>
> On Thu, 11 Apr 2024, Li Lingfeng wrote:
>
>> This reverts commit 5735a2671ffb70ea29ca83969fe01316ee2ed6fc which is
>> commit 0a9bab391e336489169b95cb0d4553d921302189 upstream.
>>
>> Tasklet is thought to cause memory corruption [1], so it was disabled in
>> dm-crypt and dm-verity. However, memory corruption may not happen since
>> cc->io_queue is created without WQ_UNBOUND [2].
>> Revert commit 5735a2671ffb ("dm-crypt, dm-verity: disable tasklets") to
>> bring tasklet back.
>>
>> [1] https://lore.kernel.org/all/[email protected]/T/
>> [2] https://lore.kernel.org/all/[email protected]/
> Regarding [2] - if you add mdelay, you can make the race condition less
> reproducible, but it is not a proper fix and the race condition stays
> there unfixed.
>
> Workqueues and ksoftirqd may be scheduled arbitrarily, there is no
> guarantee that they will be executed in a particular order, even if they
> are executed on the same CPU.
>
> Mikulas
>
>> Signed-off-by: Li Lingfeng <[email protected]>
>> ---
>> drivers/md/dm-crypt.c | 38 +++++++++++++++++++++++++++++++++--
>> drivers/md/dm-verity-target.c | 26 ++++++++++++++++++++++--
>> drivers/md/dm-verity.h | 1 +
>> 3 files changed, 61 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
>> index aa6bb5b4704b..a60d91d02e28 100644
>> --- a/drivers/md/dm-crypt.c
>> +++ b/drivers/md/dm-crypt.c
>> @@ -75,8 +75,10 @@ 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;
>>
>> @@ -1775,6 +1777,7 @@ 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);
>> }
>>
>> @@ -1785,6 +1788,13 @@ static void crypt_inc_pending(struct dm_crypt_io *io)
>>
>> static void kcryptd_queue_read(struct dm_crypt_io *io);
>>
>> +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.
>> @@ -1817,6 +1827,20 @@ static void crypt_dec_pending(struct dm_crypt_io *io)
>>
>> 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);
>> }
>>
>> @@ -2291,6 +2315,11 @@ 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;
>> @@ -2302,10 +2331,15 @@ static void kcryptd_queue_crypt(struct dm_crypt_io *io)
>> * irqs_disabled(): the kernel may run some IO completion from the idle thread, but
>> * it is being executed with irqs disabled.
>> */
>> - if (!(in_hardirq() || irqs_disabled())) {
>> - kcryptd_crypt(&io->work);
>> + if (in_hardirq() || irqs_disabled()) {
>> + io->in_tasklet = true;
>> + tasklet_init(&io->tasklet, kcryptd_crypt_tasklet, (unsigned long)&io->work);
>> + tasklet_schedule(&io->tasklet);
>> return;
>> }
>> +
>> + kcryptd_crypt(&io->work);
>> + return;
>> }
>>
>> INIT_WORK(&io->work, kcryptd_crypt);
>> diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
>> index 49e4a35d7019..0bb126eadc0d 100644
>> --- a/drivers/md/dm-verity-target.c
>> +++ b/drivers/md/dm-verity-target.c
>> @@ -701,6 +701,23 @@ static void verity_work(struct work_struct *w)
>> verity_finish_io(io, errno_to_blk_status(verity_verify_io(io)));
>> }
>>
>> +static void verity_tasklet(unsigned long data)
>> +{
>> + struct dm_verity_io *io = (struct dm_verity_io *)data;
>> + int err;
>> +
>> + io->in_tasklet = true;
>> + err = verity_verify_io(io);
>> + if (err == -EAGAIN || err == -ENOMEM) {
>> + /* fallback to retrying with work-queue */
>> + INIT_WORK(&io->work, verity_work);
>> + queue_work(io->v->verify_wq, &io->work);
>> + return;
>> + }
>> +
>> + verity_finish_io(io, errno_to_blk_status(err));
>> +}
>> +
>> static void verity_end_io(struct bio *bio)
>> {
>> struct dm_verity_io *io = bio->bi_private;
>> @@ -713,8 +730,13 @@ static void verity_end_io(struct bio *bio)
>> return;
>> }
>>
>> - INIT_WORK(&io->work, verity_work);
>> - queue_work(io->v->verify_wq, &io->work);
>> + if (static_branch_unlikely(&use_tasklet_enabled) && io->v->use_tasklet) {
>> + tasklet_init(&io->tasklet, verity_tasklet, (unsigned long)io);
>> + tasklet_schedule(&io->tasklet);
>> + } else {
>> + INIT_WORK(&io->work, verity_work);
>> + queue_work(io->v->verify_wq, &io->work);
>> + }
>> }
>>
>> /*
>> diff --git a/drivers/md/dm-verity.h b/drivers/md/dm-verity.h
>> index db93a91169d5..7e495cc375b0 100644
>> --- a/drivers/md/dm-verity.h
>> +++ b/drivers/md/dm-verity.h
>> @@ -87,6 +87,7 @@ struct dm_verity_io {
>> bool in_tasklet;
>>
>> struct work_struct work;
>> + struct tasklet_struct tasklet;
>>
>> char *recheck_buffer;
>>
>> --
>> 2.31.1
>>

2024-04-15 15:12:06

by Mikulas Patocka

[permalink] [raw]
Subject: Re: [PATCH 6.6] Revert "dm-crypt, dm-verity: disable tasklets"



On Fri, 12 Apr 2024, Li Lingfeng wrote:

> Hi
>
> I'm having difficulty understanding "Workqueues and ksoftirqd may be scheduled
> arbitrarily".
> This is my understanding:
> kcryptd_queue_crypt
> ?tasklet_schedule
> ? __tasklet_schedule
> ?? __tasklet_schedule_common
> ??? raise_softirq_irqoff
> ???? wakeup_softirqd
> ????? wake_up_process // ksoftirqd
>
> run_ksoftirqd
> ?__do_softirq
> ? softirq_handle_begin
> ?? __local_bh_disable_ip // Turn off preemption
> <---------- [1] ---------->
> ? tasklet_action // h->action
> ?? tasklet_action_common
> ??? tasklet_trylock
> ???? kcryptd_crypt_tasklet // t->func(t->data)
> ???? ...
> ????? queue_work(cc->io_queue, &io->work)
> <---------- [2] ---------->
> ??? tasklet_unlock
>
> // workqueue process
> kcryptd_io_bio_endio
> ?...
> ?// free tasklet_struct
>
> Since preemption has been turned off at [1], I'm confused about how the CPU
> can be scheduled out to do work first at [2].
> Would you mind explaining it to me?
>
> Thanks

Yes, you are right that scheduling is disabled when ksoftirqd processes a
softirq task.

But the upstream kernel switched to bh workqueues anyway, so there is no
need to submit a different solution to the stable kernels.

Mikulas