Re: [PATCH] block/laptop_mode: Convert timers to use timer_setup()

[Kees Cook]

On Thu, Oct 5, 2017 at 7:56 AM, Jens Axboe <axboe@xxxxxxxxx> wrote:
> On 10/04/2017 06:49 PM, Kees Cook wrote:
>> 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.
>>
>> Cc: Jens Axboe <axboe@xxxxxxxxx>
>> Cc: Michal Hocko <mhocko@xxxxxxxx>
>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>> Cc: Jan Kara <jack@xxxxxxx>
>> Cc: Johannes Weiner <hannes@xxxxxxxxxxx>
>> Cc: Nicholas Piggin <npiggin@xxxxxxxxx>
>> Cc: Vladimir Davydov <vdavydov.dev@xxxxxxxxx>
>> Cc: Matthew Wilcox <mawilcox@xxxxxxxxxxxxx>
>> Cc: Jeff Layton <jlayton@xxxxxxxxxx>
>> Cc: linux-block@xxxxxxxxxxxxxxx
>> Cc: linux-mm@xxxxxxxxx
>> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
>> ---
>> This requires commit 686fef928bba ("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.

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().

> 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

-- 
Kees Cook
Pixel Security