block/laptop_mode: Convert timers to use timer_setup()
Subsystems (3)
PATCH 1/1 block/laptop_mode: Convert timers to use timer_setup()
In preparation for unconditionally passing the struct timer_list pointer to all timer callbacks, switch to using the new timer_setup() and from_timer() to pass the timer pointer explicitly.
---
This requires commit 686fef928bba ("timer: Prepare to change timer callback argument type") in v4.14-rc3, but should be otherwise stand-alone.
--- block/blk-core.c | 10 +++++----- include/linux/writeback.h | 2 +- mm/page-writeback.c | 9 +++++---- 3 files changed, 11 insertions(+), 10 deletions(-)
block/blk-core.c +5 -5
@@ -803,9 +803,9 @@ static void blk_queue_usage_counter_release(struct percpu_ref *ref)wake_up_all (&q->mq_freeze_wq); } -static void blk_rq_timed_out_timer (unsigned long data) +static void blk_rq_timed_out_timer (struct timer_list *t) { -struct request_queue *q = (struct request_queue *)data; +struct request_queue *q =from_timer (q, t, timeout);kblockd_schedule_work (&q->timeout_work); }
@@ -841,9 +841,9 @@ struct request_queue *blk_alloc_queue_node(gfp_t gfp_mask, int node_id) q->backing_dev_info->name ="block" ; q->node = node_id; -setup_timer (&q->backing_dev_info->laptop_mode_wb_timer, - laptop_mode_timer_fn, (unsigned long ) q); -setup_timer (&q->timeout, blk_rq_timed_out_timer, (unsigned long ) q); +timer_setup (&q->backing_dev_info->laptop_mode_wb_timer, + laptop_mode_timer_fn,0 ); +timer_setup (&q->timeout, blk_rq_timed_out_timer,0 );INIT_LIST_HEAD (&q->queue_head);INIT_LIST_HEAD (&q->timeout_list);INIT_LIST_HEAD (&q->icq_list);
include/linux/writeback.h +1 -1
@@ -329,7 +329,7 @@ static inline void cgroup_writeback_umount(void)void laptop_io_completion (struct backing_dev_info *info);void laptop_sync_completion (void );void laptop_mode_sync (struct work_struct *work); -void laptop_mode_timer_fn (unsigned long data); +void laptop_mode_timer_fn (struct timer_list *t);#else static inline void laptop_sync_completion (void ) { }#endif
mm/page-writeback.c +5 -4
@@ -1977,9 +1977,10 @@ int dirty_writeback_centisecs_handler(struct ctl_table *table, int write, }#ifdef CONFIG_BLOCK -void laptop_mode_timer_fn (unsigned long data) +void laptop_mode_timer_fn (struct timer_list *t) { -struct request_queue *q = (struct request_queue *)data; +struct backing_dev_info *backing_dev_info = +from_timer (backing_dev_info, t, laptop_mode_wb_timer);int nr_pages =global_node_page_state (NR_FILE_DIRTY ) +global_node_page_state (NR_UNSTABLE_NFS );struct bdi_writeback *wb;
@@ -1988,11 +1989,11 @@ void laptop_mode_timer_fn(unsigned long data)* We want to write everything out, not just down to the dirty * threshold */ -if (!bdi_has_dirty_io (q->backing_dev_info)) +if (!bdi_has_dirty_io (backing_dev_info))return ;rcu_read_lock (); -list_for_each_entry_rcu (wb, &q->backing_dev_info->wb_list, bdi_node) +list_for_each_entry_rcu (wb, &backing_dev_info->wb_list, bdi_node)if (wb_has_dirty_io (wb))wb_start_writeback (wb, nr_pages, true,WB_REASON_LAPTOP_TIMER );
Signature · 6 lines
2.7.4 -- Kees Cook Pixel Security
Jens Axboe
maintainerQuoted 17 lines · Kees Cook
In preparation for unconditionally passing the struct
timer_listpointer to all timer callbacks, switch to using the newtimer_setup()andfrom_timer()to pass the timer pointer explicitly.Cc Jens Axboe <[email protected]>Cc Michal Hocko <[email protected]>Cc Andrew Morton <[email protected]>Cc Jan Kara <[email protected]>Cc Johannes Weiner <[email protected]>Cc Nicholas Piggin <[email protected]>Cc Vladimir Davydov <[email protected]>Cc Matthew Wilcox <[email protected]>Cc Jeff Layton <[email protected]>Cc Thomas Gleixner <[email protected]>Signed-off-by Kees Cook <[email protected]>
Quoted 4 lines
---
This requires commit686fef928bba("timer: Prepare to change timer callback argument type") in v4.14-rc3, but should be otherwise stand-alone.
My only complaint about this is the use of a from_timer() macro instead of just using container_of() at the call sites to actually show that is happening. I'm generally opposed to obfuscation like that. It just means you have to look up what is going on, instead of it being readily apparent to the reader/reviewer.
I guess I do have a a second complaint as well - that it landed in -rc3, which is rather late considering subsystem trees are usually forked earlier than that. Had this been in -rc1, I would have had an easier time applying the block bits for 4.15.
Signature · 1 line
Jens Axboe
Quoted 23 lines · Jens Axboe
Quoted 17 lines · Kees Cook
In preparation for unconditionally passing the struct
timer_listpointer to all timer callbacks, switch to using the newtimer_setup()andfrom_timer()to pass the timer pointer explicitly.Cc Jens Axboe <[email protected]>Cc Michal Hocko <[email protected]>Cc Andrew Morton <[email protected]>Cc Jan Kara <[email protected]>Cc Johannes Weiner <[email protected]>Cc Nicholas Piggin <[email protected]>Cc Vladimir Davydov <[email protected]>Cc Matthew Wilcox <[email protected]>Cc Jeff Layton <[email protected]>Cc Thomas Gleixner <[email protected]>Signed-off-by Kees Cook <[email protected]>Quoted 4 lines
---
This requires commit686fef928bba("timer: Prepare to change timer callback argument type") in v4.14-rc3, but should be otherwise stand-alone.
Quoted 5 lines
My only complaint about this is the use of a
from_timer()macro instead of just usingcontainer_of()at the call sites to actually show that is happening. I'm generally opposed to obfuscation like that. It just means you have to look up what is going on, instead of it being readily apparent to the reader/reviewer.
Yeah, this got discussed a bit with tglx and hch. Ultimately, this seems to be the least bad of several options. Specifically with regard to container_of(), it just gets to be huge, and makes things harder to read (almost always requires a line break, needlessly repeats the variable type definition, etc). Since there is precedent of both using wrappers on container_of() and for adding from_foo() helpers, I chose the resulting from_timer().
Quoted 4 lines
I guess I do have a a second complaint as well - that it landed in -rc3, which is rather late considering subsystem trees are usually forked earlier than that. Had this been in -rc1, I would have had an easier time applying the block bits for 4.15.
Yes, totally true. tglx and I ended up meeting face-to-face at the Kernel Recipes conference and we solved some outstanding design issues with the conversion. The timing meant the new API went into -rc3, which seemed better than missing an entire release cycle, or carrying deltas against maintainer trees that would drift. (This is actually my second massive refactoring of these changes...)
If you don't want to deal with the -rc3 issue, would you want these changes to get carried in the timer tree instead?
Thanks!
-Kees
Signature · 2 lines
Kees Cook Pixel Security
Jens Axboe
maintainerQuoted 30 lines · Kees Cook
Quoted 23 lines · Jens Axboe
Quoted 17 lines · Kees Cook
In preparation for unconditionally passing the struct
timer_listpointer to all timer callbacks, switch to using the newtimer_setup()andfrom_timer()to pass the timer pointer explicitly.Cc Jens Axboe <[email protected]>Cc Michal Hocko <[email protected]>Cc Andrew Morton <[email protected]>Cc Jan Kara <[email protected]>Cc Johannes Weiner <[email protected]>Cc Nicholas Piggin <[email protected]>Cc Vladimir Davydov <[email protected]>Cc Matthew Wilcox <[email protected]>Cc Jeff Layton <[email protected]>Cc Thomas Gleixner <[email protected]>Signed-off-by Kees Cook <[email protected]>Quoted 4 lines
---
This requires commit686fef928bba("timer: Prepare to change timer callback argument type") in v4.14-rc3, but should be otherwise stand-alone.Quoted 5 lines
My only complaint about this is the use of a
from_timer()macro instead of just usingcontainer_of()at the call sites to actually show that is happening. I'm generally opposed to obfuscation like that. It just means you have to look up what is going on, instead of it being readily apparent to the reader/reviewer.
Quoted 7 lines
Yeah, this got discussed a bit with tglx and hch. Ultimately, this seems to be the least bad of several options. Specifically with regard to
container_of(), it just gets to be huge, and makes things harder to read (almost always requires a line break, needlessly repeats the variable type definition, etc). Since there is precedent of both using wrappers oncontainer_of()and for addingfrom_foo()helpers, I chose the resultingfrom_timer().
It might make for a longer line, but at least it's a readable line. What does from_timer() do? Nobody knows, you have to find it and check. So I'd still argue that it's a hell of a lot more readable than some random function name.
Quoted 11 lines
Quoted 4 lines
I guess I do have a a second complaint as well - that it landed in -rc3, which is rather late considering subsystem trees are usually forked earlier than that. Had this been in -rc1, I would have had an easier time applying the block bits for 4.15.
Yes, totally true. tglx and I ended up meeting face-to-face at the Kernel Recipes conference and we solved some outstanding design issues with the conversion. The timing meant the new API went into -rc3, which seemed better than missing an entire release cycle, or carrying deltas against maintainer trees that would drift. (This is actually my second massive refactoring of these changes...)
Honestly, I think the change should have waited for 4.15 in that case. Why the rush? It wasn't ready for the merge window.
Quoted 2 lines
If you don't want to deal with the -rc3 issue, would you want these changes to get carried in the timer tree instead?
I can carry them, not a problem. Just a bit grumpy as this seems poorly handled.
Signature · 1 line
Jens Axboe
Quoted 2 lines · Jens Axboe
Honestly, I think the change should have waited for 4.15 in that case. Why the rush? It wasn't ready for the merge window.
My understanding from my discussion with tglx was that if the API change waiting for 4.15, then the conversions would have to wait until 4.16. With most conversions only depending on the new API, it seemed silly to wait another whole release when the work is just waiting to be merged.
But yes, timing was not ideal. I did try to get it in earlier, but I think tglx was busy with other concerns.
-Kees
Signature · 2 lines
Kees Cook Pixel Security
Jens Axboe
maintainerQuoted 9 lines · Kees Cook
Quoted 2 lines · Jens Axboe
Honestly, I think the change should have waited for 4.15 in that case. Why the rush? It wasn't ready for the merge window.
My understanding from my discussion with tglx was that if the API change waiting for 4.15, then the conversions would have to wait until 4.16. With most conversions only depending on the new API, it seemed silly to wait another whole release when the work is just waiting to be merged.
Right, it would have shifted everything a release. But that's how we do things! If something isn't ready _before_ the merge window, let alone talking -rc3 time, then it gets to wait unless it's fixing a regression introduced in the merge window. Or if you can argue that it's a really critical fix, sure, it can be squeezed in.
I'm puzzled why anyone would think that this is any different. What I'm hearing here is that "I didn't want to wait, and my change is more important than others", as an argument for why this should be treated any differently.
I'm not saying the change is bad, it looks trivial, from_timer() discussion aside. But it's not a critical fix, by any stretch of the imagination, and neither are the driver conversions. With this additionally causing extra problems because of the timing, that's just further proof that it should have waited.
Quoted 2 lines
But yes, timing was not ideal. I did try to get it in earlier, but I think tglx was busy with other concerns.
So it should have waited...
Signature · 1 line
Jens Axboe
Quoted 10 lines · Jens Axboe
Quoted 6 lines · Kees Cook
Yes, totally true. tglx and I ended up meeting face-to-face at the Kernel Recipes conference and we solved some outstanding design issues with the conversion. The timing meant the new API went into -rc3, which seemed better than missing an entire release cycle, or carrying deltas against maintainer trees that would drift. (This is actually my second massive refactoring of these changes...)
Honestly, I think the change should have waited for 4.15 in that case. Why the rush? It wasn't ready for the merge window.
Come on. You know very well that a prerequisite for global changes which is not yet used in Linus tree can get merged post merge windew in order to avoid massive inter maintainer tree dependencies. We've done that before.
There are only a few options we have for such global changes:
- Delay everything for a full release and keep hunting the ever changing code and the new users of old interfaces, which is a pain in the butt. I know what I'm talking about.
- Apply everything in a central tree, which is prone to merge conflicts in next when maintainer trees contain changes in the same area. So getting it through the maintainers is the best option for this kind of stuff.
- Create a separate branch for other maintainers to pull, which I did often enough in the past to avoid merge dependencies. I decided not to offer that branch this time, because it would be been necessary to pull it into a gazillion of trees. So we decided that Linus tree as a dependency is good enough.
Just for the record:
Last time I did this for block you did not complain, that you got something based on -rc3 from me because you wanted to have these changes in your tree so you could apply the depending multi-queue patches. tip irq/for-block exists for a reason.
So what's the difference to pull -rc3 this time? Nothing, just the fact that you are not interested in the change.
So please stop this hypocritical ranting right here.
Thanks,
tglx
Jens Axboe
maintainerQuoted 9 lines · Thomas Gleixner
Quoted 8 lines · Jens Axboe
Quoted 6 lines · Kees Cook
Yes, totally true. tglx and I ended up meeting face-to-face at the Kernel Recipes conference and we solved some outstanding design issues with the conversion. The timing meant the new API went into -rc3, which seemed better than missing an entire release cycle, or carrying deltas against maintainer trees that would drift. (This is actually my second massive refactoring of these changes...)
Quoted 6 lines
Quoted 2 lines
Honestly, I think the change should have waited for 4.15 in that case. Why the rush? It wasn't ready for the merge window.
Come on. You know very well that a prerequisite for global changes which is not yet used in Linus tree can get merged post merge windew in order to avoid massive inter maintainer tree dependencies. We've done that before.
My point is that doing it this late makes things harder than they should have been. If this was in for -rc1, it would have made things a lot easier. Or even -rc2. I try and wait to fork off the block tree for as long as I can, -rc2 is generally where that happens.
Quoted 5 lines
There are only a few options we have for such global changes:
- Delay everything for a full release and keep hunting the ever changing code and the new users of old interfaces, which is a pain in the butt. I know what I'm talking about.
Not disagreeing with that it's a pain in the butt. The timing is the main reason why that is the case, though.
Quoted 4 lines
- Apply everything in a central tree, which is prone to merge conflicts in next when maintainer trees contain changes in the same area. So getting it through the maintainers is the best option for this kind of stuff.
Don't disagree with that either.
Quoted 13 lines
- Create a separate branch for other maintainers to pull, which I did often enough in the past to avoid merge dependencies. I decided not to offer that branch this time, because it would be been necessary to pull it into a gazillion of trees. So we decided that Linus tree as a dependency is good enough.
Just for the record:
Last time I did this for block you did not complain, that you got something based on -rc3 from me because you wanted to have these changes in your tree so you could apply the depending multi-queue patches. tip irq/for-block exists for a reason.
Quoted 4 lines
So what's the difference to pull -rc3 this time? Nothing, just the fact that you are not interested in the change.
So please stop this hypocritical ranting right here.
I'm not judging this based on whether I find it interesting or not, but rather if it's something that's generally important to get in. Maybe it is, but I don't see any justification for that at all. So just looking at the isolated change, it does not strike me as something that's important enough to warrant special treatment (and the pain associated with that). I don't care about the core change, it's obviously trivial. Expecting maintainers to pick up this dependency asap mid cycle is what sucks.
Please stop accusing me of being hypocritical. I'm questionning the timing of the change, that should be possible without someone resorting to ad hominem attacks.
Signature · 1 line
Jens Axboe
Jens Axboe
maintainerQuoted 13 lines · Jens Axboe
Quoted 12 lines · Thomas Gleixner
Quoted 10 lines · Jens Axboe
Quoted 6 lines · Kees Cook
Yes, totally true. tglx and I ended up meeting face-to-face at the Kernel Recipes conference and we solved some outstanding design issues with the conversion. The timing meant the new API went into -rc3, which seemed better than missing an entire release cycle, or carrying deltas against maintainer trees that would drift. (This is actually my second massive refactoring of these changes...)
Honestly, I think the change should have waited for 4.15 in that case. Why the rush? It wasn't ready for the merge window.
Quoted 8 lines
Quoted 3 lines
Come on. You know very well that a prerequisite for global changes which is not yet used in Linus tree can get merged post merge windew in order to avoid massive inter maintainer tree dependencies. We've done that before.
My point is that doing it this late makes things harder than they should have been. If this was in for -rc1, it would have made things a lot easier. Or even -rc2. I try and wait to fork off the block tree for as long as I can, -rc2 is generally where that happens.
Timing of the change aside, this patch doesn't even apply to the 4.15 block tree:
checking file block/blk-core.c
checking file include/linux/writeback.h
Hunk #1 succeeded at 309 (offset -20 lines).
checking file mm/page-writeback.c
Hunk #1 FAILED at 1977.
Hunk #2 FAILED at 1988.
2 out of 2 hunks FAILED
Signature · 1 line
Jens Axboe
Jens,
Quoted 9 lines · Jens Axboe
Quoted 3 lines · Thomas Gleixner
Come on. You know very well that a prerequisite for global changes which is not yet used in Linus tree can get merged post merge windew in order to avoid massive inter maintainer tree dependencies. We've done that before.
My point is that doing it this late makes things harder than they should have been. If this was in for -rc1, it would have made things a lot easier. Or even -rc2. I try and wait to fork off the block tree for as long as I can, -rc2 is generally where that happens.
Well, yes. I know it's about habits. There is no real technical reason not to merge -rc3 or later into your devel/next branch. I actually do that for various reasons, one being that I prefer to have halfways testable branches, which is often not the case when they are based of rc1, which is especially true in this 4.14 cycle. The other is to pick up stuff which went into Linus tree via a urgent branch or even got applied from mail directly.
Quoted 8 lines
I'm not judging this based on whether I find it interesting or not, but rather if it's something that's generally important to get in. Maybe it is, but I don't see any justification for that at all. So just looking at the isolated change, it does not strike me as something that's important enough to warrant special treatment (and the pain associated with that). I don't care about the core change, it's obviously trivial. Expecting maintainers to pick up this dependency asap mid cycle is what sucks.
I'm really not getting the 'pain' point.
'git merge linus' is not really a pain and it does not break workflows assumed that you do that on a branch which has immutable state. If you want to keep your branches open for rebasing due to some wreckage in the middle of it, that's a different story.
Quoted 3 lines
Please stop accusing me of being hypocritical. I'm questionning the timing of the change, that should be possible without someone resorting to ad hominem attacks.
Well, it seemed hypocritical to me for a hopefully understandable reason. I didn't want to attack or offend you in any way.
I just know from repeated experience how painful it is to do full tree overhauls and sit on large patch queues for a long time. There is some point where you need to get things going and I really appreciate the work of people doing that. Refactoring the kernel to get rid of legacy burdens and in this case to address a popular attack vector is definitely useful for everybody and should be supported. We tried to make it easy by pushing this to Linus and I really did not expect that merging Linus -rc3 into a devel/next branch is a painful work to do.
As Kees said already, we can set that particular patch aside and push it along with the rest of ignored ones around 15-rc1 time so we can remove the old interfaces. Though we hopefully wont end up with a gazillion of ignored or considered too painful ones.
Thanks,
tglx
Jens Axboe
maintainerQuoted 13 lines · Thomas Gleixner
Jens,
Quoted 9 lines · Jens Axboe
Quoted 3 lines · Thomas Gleixner
Come on. You know very well that a prerequisite for global changes which is not yet used in Linus tree can get merged post merge windew in order to avoid massive inter maintainer tree dependencies. We've done that before.
My point is that doing it this late makes things harder than they should have been. If this was in for -rc1, it would have made things a lot easier. Or even -rc2. I try and wait to fork off the block tree for as long as I can, -rc2 is generally where that happens.
Quoted 7 lines
Well, yes. I know it's about habits. There is no real technical reason not to merge -rc3 or later into your devel/next branch. I actually do that for various reasons, one being that I prefer to have halfways testable branches, which is often not the case when they are based of rc1, which is especially true in this 4.14 cycle. The other is to pick up stuff which went into Linus tree via a urgent branch or even got applied from mail directly.
Yes, it's not impossible, I just usually prefer not to. For this case, I just setup a for-4.15/timer, that is the current block branch with -rc3 pulled in. I applied the two patches for floppy and amiflop, I'm assuming Kees will respin the writeback/laptop version and I can shove that in there too.
Quoted 9 lines
Quoted 8 lines
I'm not judging this based on whether I find it interesting or not, but rather if it's something that's generally important to get in. Maybe it is, but I don't see any justification for that at all. So just looking at the isolated change, it does not strike me as something that's important enough to warrant special treatment (and the pain associated with that). I don't care about the core change, it's obviously trivial. Expecting maintainers to pick up this dependency asap mid cycle is what sucks.
Quoted 6 lines
I'm really not getting the 'pain' point.
'git merge linus' is not really a pain and it does not break workflows assumed that you do that on a branch which has immutable state. If you want to keep your branches open for rebasing due to some wreckage in the middle of it, that's a different story.
I try never to rebase the development branches. It'll happen very rarely, but only for cases where the screwup has been short and I'm assuming/hoping no one pulled it yet.
Quoted 16 lines
Quoted 3 lines
Please stop accusing me of being hypocritical. I'm questionning the timing of the change, that should be possible without someone resorting to ad hominem attacks.
Well, it seemed hypocritical to me for a hopefully understandable reason. I didn't want to attack or offend you in any way.
I just know from repeated experience how painful it is to do full tree overhauls and sit on large patch queues for a long time. There is some point where you need to get things going and I really appreciate the work of people doing that. Refactoring the kernel to get rid of legacy burdens and in this case to address a popular attack vector is definitely useful for everybody and should be supported. We tried to make it easy by pushing this to Linus and I really did not expect that merging Linus -rc3 into a devel/next branch is a painful work to do.
Quoted 4 lines
As Kees said already, we can set that particular patch aside and push it along with the rest of ignored ones around 15-rc1 time so we can remove the old interfaces. Though we hopefully wont end up with a gazillion of ignored or considered too painful ones.
No worries, we'll get over it. The new branch is setup, so as soon as the patches are funneled in, hopefully it can be ignored/forgotten until the merge window opens.
Signature · 1 line
Jens Axboe
Quoted 5 lines · Jens Axboe
Yes, it's not impossible, I just usually prefer not to. For this case, I just setup a for-4.15/timer, that is the current block branch with -rc3 pulled in. I applied the two patches for floppy and amiflop, I'm assuming Kees will respin the writeback/laptop version and I can shove that in there too.
Thanks for setting this up! I'll respin and send it out again.
-Kees
Signature · 2 lines
Kees Cook Pixel Security