Date:   Thu, 5 Oct 2017 12:26:18 -0600
From:   Jens Axboe <axboe@...nel.dk>
To:     Kees Cook <keescook@...omium.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Michal Hocko <mhocko@...e.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Jan Kara <jack@...e.cz>, Johannes Weiner <hannes@...xchg.org>,
        Nicholas Piggin <npiggin@...il.com>,
        Vladimir Davydov <vdavydov.dev@...il.com>,
        Matthew Wilcox <mawilcox@...rosoft.com>,
        Jeff Layton <jlayton@...hat.com>, linux-block@...r.kernel.org,
        Linux-MM <linux-mm@...ck.org>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH] block/laptop_mode: Convert timers to use timer_setup()

On 10/05/2017 11:49 AM, Kees Cook wrote:
> On Thu, Oct 5, 2017 at 7:56 AM, Jens Axboe <axboe@...nel.dk> 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@...nel.dk>
>>> Cc: Michal Hocko <mhocko@...e.com>
>>> Cc: Andrew Morton <akpm@...ux-foundation.org>
>>> Cc: Jan Kara <jack@...e.cz>
>>> Cc: Johannes Weiner <hannes@...xchg.org>
>>> Cc: Nicholas Piggin <npiggin@...il.com>
>>> Cc: Vladimir Davydov <vdavydov.dev@...il.com>
>>> Cc: Matthew Wilcox <mawilcox@...rosoft.com>
>>> Cc: Jeff Layton <jlayton@...hat.com>
>>> Cc: linux-block@...r.kernel.org
>>> Cc: linux-mm@...ck.org
>>> Cc: Thomas Gleixner <tglx@...utronix.de>
>>> Signed-off-by: Kees Cook <keescook@...omium.org>
>>> ---
>>> 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().

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.

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

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

-- 
Jens Axboe

Powered by blists - more mailing lists