2022-10-27 15:33:05

by Steven Rostedt

[permalink] [raw]
Subject: Re: [RFC][PATCH v2 04/31] timers: block: Use del_timer_shutdown() before freeing timer

[
quilt mail --send still can't handle unicode characters.
Here's the patch again
]

From: "Steven Rostedt (Google)" <[email protected]>

Before a timer is freed, del_timer_shutdown() must be called.

Link: https://lore.kernel.org/all/[email protected]/

Cc: Philipp Reisner <[email protected]>
Cc: Lars Ellenberg <[email protected]>
Cc: "Christoph Böhmwalder" <[email protected]>
Cc: Jens Axboe <[email protected]>
Cc: [email protected]
Cc: Tejun Heo <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Steven Rostedt (Google) <[email protected]>
---
block/blk-iocost.c | 2 +-
block/blk-iolatency.c | 2 +-
block/blk-stat.c | 2 +-
block/blk-throttle.c | 2 +-
block/kyber-iosched.c | 2 +-
drivers/block/drbd/drbd_main.c | 2 +-
drivers/block/loop.c | 2 +-
drivers/block/sunvdc.c | 2 +-
8 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 495396425bad..e2d4bdd3d135 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -2814,7 +2814,7 @@ static void ioc_rqos_exit(struct rq_qos *rqos)
ioc->running = IOC_STOP;
spin_unlock_irq(&ioc->lock);

- del_timer_sync(&ioc->timer);
+ del_timer_shutdown(&ioc->timer);
free_percpu(ioc->pcpu_stat);
kfree(ioc);
}
diff --git a/block/blk-iolatency.c b/block/blk-iolatency.c
index 571fa95aafe9..7b61f09afedd 100644
--- a/block/blk-iolatency.c
+++ b/block/blk-iolatency.c
@@ -645,7 +645,7 @@ static void blkcg_iolatency_exit(struct rq_qos *rqos)
{
struct blk_iolatency *blkiolat = BLKIOLATENCY(rqos);

- del_timer_sync(&blkiolat->timer);
+ del_timer_shutdown(&blkiolat->timer);
flush_work(&blkiolat->enable_work);
blkcg_deactivate_policy(rqos->q, &blkcg_policy_iolatency);
kfree(blkiolat);
diff --git a/block/blk-stat.c b/block/blk-stat.c
index 2ea01b5c1aca..de51db302c44 100644
--- a/block/blk-stat.c
+++ b/block/blk-stat.c
@@ -165,7 +165,7 @@ void blk_stat_remove_callback(struct request_queue *q,
blk_queue_flag_clear(QUEUE_FLAG_STATS, q);
spin_unlock_irqrestore(&q->stats->lock, flags);

- del_timer_sync(&cb->timer);
+ del_timer_shutdown(&cb->timer);
}

static void blk_stat_free_callback_rcu(struct rcu_head *head)
diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 847721dc2b2b..95af99f24137 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -490,7 +490,7 @@ static void throtl_pd_free(struct blkg_policy_data *pd)
{
struct throtl_grp *tg = pd_to_tg(pd);

- del_timer_sync(&tg->service_queue.pending_timer);
+ del_timer_shutdown(&tg->service_queue.pending_timer);
blkg_rwstat_exit(&tg->stat_bytes);
blkg_rwstat_exit(&tg->stat_ios);
kfree(tg);
diff --git a/block/kyber-iosched.c b/block/kyber-iosched.c
index b05357bced99..59a444a47ba3 100644
--- a/block/kyber-iosched.c
+++ b/block/kyber-iosched.c
@@ -434,7 +434,7 @@ static void kyber_exit_sched(struct elevator_queue *e)
struct kyber_queue_data *kqd = e->elevator_data;
int i;

- del_timer_sync(&kqd->timer);
+ del_timer_shutdown(&kqd->timer);
blk_stat_disable_accounting(kqd->q);

for (i = 0; i < KYBER_NUM_DOMAINS; i++)
diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c
index f3e4db16fd07..3f574f3769c3 100644
--- a/drivers/block/drbd/drbd_main.c
+++ b/drivers/block/drbd/drbd_main.c
@@ -2184,7 +2184,7 @@ void drbd_destroy_device(struct kref *kref)
struct drbd_resource *resource = device->resource;
struct drbd_peer_device *peer_device, *tmp_peer_device;

- del_timer_sync(&device->request_timer);
+ del_timer_shutdown(&device->request_timer);

/* paranoia asserts */
D_ASSERT(device, device->open_cnt == 0);
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index ad92192c7d61..d134a5fd4ae7 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1755,7 +1755,7 @@ static void lo_free_disk(struct gendisk *disk)
if (lo->workqueue)
destroy_workqueue(lo->workqueue);
loop_free_idle_workers(lo, true);
- del_timer_sync(&lo->timer);
+ del_timer_shutdown(&lo->timer);
mutex_destroy(&lo->lo_mutex);
kfree(lo);
}
diff --git a/drivers/block/sunvdc.c b/drivers/block/sunvdc.c
index fb855da971ee..9868937a9602 100644
--- a/drivers/block/sunvdc.c
+++ b/drivers/block/sunvdc.c
@@ -1067,7 +1067,7 @@ static void vdc_port_remove(struct vio_dev *vdev)

flush_work(&port->ldc_reset_work);
cancel_delayed_work_sync(&port->ldc_reset_timer_work);
- del_timer_sync(&port->vio.timer);
+ del_timer_shutdown(&port->vio.timer);

del_gendisk(port->disk);
put_disk(port->disk);
--
2.35.1


2022-10-28 08:29:39

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [RFC][PATCH v2 04/31] timers: block: Use del_timer_shutdown() before freeing timer

This is just a single patch out of apparently 31, which claims that
something that doesn't even exist in mainline must be used without any
explanation. How do you expect anyone to be able to review it?

2022-10-28 11:06:34

by Steven Rostedt

[permalink] [raw]
Subject: Re: [RFC][PATCH v2 04/31] timers: block: Use del_timer_shutdown() before freeing timer

On Fri, 28 Oct 2022 01:26:03 -0700
Christoph Hellwig <[email protected]> wrote:

> This is just a single patch out of apparently 31, which claims that
> something that doesn't even exist in mainline must be used without any
> explanation. How do you expect anyone to be able to review it?

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

Only the first patch is relevant to you. I guess the Cc list would have
been too big to Cc everyone that was Cc'd in the series.

It not being in mainline is the reason I marked it RFC. As it's more of an
FYI than a pull it in request.

-- Steve

2022-10-28 13:59:12

by Jens Axboe

[permalink] [raw]
Subject: Re: [RFC][PATCH v2 04/31] timers: block: Use del_timer_shutdown() before freeing timer

On 10/28/22 4:24 AM, Steven Rostedt wrote:
> On Fri, 28 Oct 2022 01:26:03 -0700
> Christoph Hellwig <[email protected]> wrote:
>
>> This is just a single patch out of apparently 31, which claims that
>> something that doesn't even exist in mainline must be used without any
>> explanation. How do you expect anyone to be able to review it?
>
> https://lore.kernel.org/all/[email protected]/
>
> Only the first patch is relevant to you. I guess the Cc list would have
> been too big to Cc everyone that was Cc'd in the series.

No it's not, because how on earth would anyone know what the change does
if you only see the simple s/name/newname change? The patch is useless
by itself.

--
Jens Axboe



2022-10-28 14:35:35

by Steven Rostedt

[permalink] [raw]
Subject: Re: [RFC][PATCH v2 04/31] timers: block: Use del_timer_shutdown() before freeing timer

On Fri, 28 Oct 2022 07:56:50 -0600
Jens Axboe <[email protected]> wrote:

> On 10/28/22 4:24 AM, Steven Rostedt wrote:
> > On Fri, 28 Oct 2022 01:26:03 -0700
> > Christoph Hellwig <[email protected]> wrote:
> >
> >> This is just a single patch out of apparently 31, which claims that
> >> something that doesn't even exist in mainline must be used without any
> >> explanation. How do you expect anyone to be able to review it?
> >
> > https://lore.kernel.org/all/[email protected]/
> >
> > Only the first patch is relevant to you. I guess the Cc list would have
> > been too big to Cc everyone that was Cc'd in the series.
>
> No it's not, because how on earth would anyone know what the change does
> if you only see the simple s/name/newname change? The patch is useless
> by itself.
>

I meant this as the first patch:

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

Which was what the link above was suppose to point to.

It's the only patch relevant to the rest of the series, as the rest is just
converting over to the shutdown API, and the last patch changes
DEBUG_OBJECTS_TIMERS to catch if this was done properly.

That is, patch 01/31 and the patch you were Cc'd on is relevant, and for
those that want to look deeper, see patch 31 as well.

But if I included the Cc list for patch 01 for all those Cc'd in the
entire series, it would be a huge Cc list, so I avoided doing so.

Also, this is still RFC as the changes may still change. That is, this
patch set is a heads up to what is to come. Ideally, I'd like to get just
the API possibly in the kernel before the merge window without anyone using
it. Then I can ask all the sub systems to pull in these individual patches
as well.

-- Steve

2022-10-28 14:38:34

by Jens Axboe

[permalink] [raw]
Subject: Re: [RFC][PATCH v2 04/31] timers: block: Use del_timer_shutdown() before freeing timer

On 10/28/22 8:06 AM, Steven Rostedt wrote:
> On Fri, 28 Oct 2022 07:56:50 -0600
> Jens Axboe <[email protected]> wrote:
>
>> On 10/28/22 4:24 AM, Steven Rostedt wrote:
>>> On Fri, 28 Oct 2022 01:26:03 -0700
>>> Christoph Hellwig <[email protected]> wrote:
>>>
>>>> This is just a single patch out of apparently 31, which claims that
>>>> something that doesn't even exist in mainline must be used without any
>>>> explanation. How do you expect anyone to be able to review it?
>>>
>>> https://lore.kernel.org/all/[email protected]/
>>>
>>> Only the first patch is relevant to you. I guess the Cc list would have
>>> been too big to Cc everyone that was Cc'd in the series.
>>
>> No it's not, because how on earth would anyone know what the change does
>> if you only see the simple s/name/newname change? The patch is useless
>> by itself.
>>
>
> I meant this as the first patch:
>
> https://lore.kernel.org/all/[email protected]/
>
> Which was what the link above was suppose to point to.
>
> It's the only patch relevant to the rest of the series, as the rest is just
> converting over to the shutdown API, and the last patch changes
> DEBUG_OBJECTS_TIMERS to catch if this was done properly.
>
> That is, patch 01/31 and the patch you were Cc'd on is relevant, and for
> those that want to look deeper, see patch 31 as well.

So we got half of what was needed to make any kind of sense of judgement
on the patch.

> But if I included the Cc list for patch 01 for all those Cc'd in the
> entire series, it would be a huge Cc list, so I avoided doing so.

And my point is that just CC'ing the relevant list for patch 4/31 is
useless. Do we need to see the whole series? No. Does everyone need to
see patch 1/31? Yes, very much so. Without that, 4/31 means nothing.

This is pretty common for tree wide changes. The relevant lists need
to see the full context, patch 4/31 by itself is useless and may as well
not be sent at this point then.

--
Jens Axboe



2022-10-28 14:39:09

by Steven Rostedt

[permalink] [raw]
Subject: Re: [RFC][PATCH v2 04/31] timers: block: Use del_timer_shutdown() before freeing timer

On Fri, 28 Oct 2022 08:11:27 -0600
Jens Axboe <[email protected]> wrote:

> This is pretty common for tree wide changes. The relevant lists need
> to see the full context, patch 4/31 by itself is useless and may as well
> not be sent at this point then.

Ah, I didn't think about just including the mailing lists. The Cc lists
were auto-generated, and I didn't think about just taking out the lists.

Will do that for v2.

Thanks,

-- Steve