linux-kernel - Re: [PATCH] block, bfq: remove batches of confusing ifdefs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Sat, 2 Dec 2017 18:22:33 +0100
From:   Paolo Valente <paolo.valente@...aro.org>
To:     Jens Axboe <axboe@...nel.dk>
Cc:     linux-block <linux-block@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Ulf Hansson <ulf.hansson@...aro.org>, broonie@...nel.org,
        linus.walleij@...aro.org, lucmiccio@...il.com,
        bfq-iosched@...glegroups.com
Subject: Re: [PATCH] block, bfq: remove batches of confusing ifdefs


> Il giorno 02 dic 2017, alle ore 17:06, Jens Axboe <axboe@...nel.dk> ha scritto:
> 
> On 12/02/2017 03:04 AM, Paolo Valente wrote:
>> 
>>> Il giorno 30 nov 2017, alle ore 22:21, Jens Axboe <axboe@...nel.dk> ha scritto:
>>> 
>>> On 11/28/2017 02:37 AM, Paolo Valente wrote:
>>>> Commit a33801e8b473 ("block, bfq: move debug blkio stats behind
>>>> CONFIG_DEBUG_BLK_CGROUP") introduced two batches of confusing ifdefs:
>>>> one reported in [1], plus a similar one in another function. This
>>>> commit removes both batches, in the way suggested in [1].
>>> 
>>> Some comments below.
>>> 
>>>> +static inline void bfq_update_dispatch_stats(struct request *rq,
>>>> +					     spinlock_t *queue_lock,
>>>> +					     struct bfq_queue *in_serv_queue,
>>>> +					     bool idle_timer_disabled)
>>>> +{
>>> 
>>> Don't pass in the queue lock. The normal convention is to pass in the
>>> queue, thus making this:
>>> 
>>> static void bfq_update_dispatch_stats(struct request_queue *q,
>>> 				      struct request *rq,
>>> 				      struct bfq_queue *in_serv_queue,
>>> 				      bool idle_timer_disabled)
>>> 
>> 
>> Ok, thanks.  One question, just to try to learn, if you have time and
>> patience for a brief explanation.  Was this convention originated by
>> some rationale?  My concern is that bfq_update_dispatch_stats works on
>> no field of q but the lock, and this fact would have been made
>> explicit by passing only that exact field.
> 
> When you just pass in a lock, nobody knows what that lock is without
> looking at the caller. If you pass in the queue, it's apparent
> what is being locked.
> 

Got it, thanks a lot.

>>> which also gets rid of the inline. In general, never inline anything.
>>> The compiler should figure it out for you. This function is way too big
>>> to inline, plus the cost of what it's doing completely dwarfes function
>>> call overhead.
>>> 
>> 
>> Actually, I did so because of Linus' suggestion in [1]: "So for
>> example, the functions that can go away should obviously be inline
>> functions so that you don't end up having the compiler generate the
>> arguments and the call to an empty function body ..."
>> 
>> Maybe I misinterpreted his suggestion, and he meant that the function
>> should be designed in such a way to be (almost) certainly considered
>> inline by the compiler?
> 
> You can do that for the empty version, don't do it for the non-empty
> version. That will go away, the other one will not.
> 

Of course, thanks, and sorry for the silly question.

I'll make and submit a new patch according to your comments.

Paolo

> -- 
> Jens Axboe