linux-kernel - Re: [PATCH RFC 10/14] block, bfq: add Early Queue Merge (EQM)

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <7b7c0d4c-c10d-1e69-4ea0-1ad05a4100a2@kernel.dk>
Date:   Wed, 15 Mar 2017 10:30:33 -0600
From:   Jens Axboe <axboe@...nel.dk>
To:     Paolo Valente <paolo.valente@...aro.org>
Cc:     Tejun Heo <tj@...nel.org>, Fabio Checconi <fchecconi@...il.com>,
        Arianna Avanzini <avanzini.arianna@...il.com>,
        linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
        ulf.hansson@...aro.org, linus.walleij@...aro.org,
        broonie@...nel.org, Mauro Andreolini <mauro.andreolini@...more.it>
Subject: Re: [PATCH RFC 10/14] block, bfq: add Early Queue Merge (EQM)

On 03/15/2017 09:47 AM, Jens Axboe wrote:
> I think you understood me correctly. Currently I think the putting of
> the io context is somewhat of a mess. You have seemingly random places
> where you have to use special unlock functions, to ensure that you
> notice that some caller deeper down has set ->ioc_to_put. I took a quick
> look at it, and by far most of the cases can return an io_context to
> free quite easily. You can mark these functions __must_check to ensure
> that we don't drop an io_context, inadvertently. That's already a win
> over the random ->ioc_to_put store. And you can then get rid of
> bfq_unlock_put_ioc and it's irq variant as well.
> 
> The places where you are already returning a value, like off dispatch
> for instance, you can just pass in a pointer to an io_context pointer.
> 
> If you get this right, it'll be a lot less fragile and hacky than your
> current approach.

Even just looking a little closer, you also find cases where you
potentially twice store ->ioc_to_put. That kind of mixup can't happen if
you return it properly.

In __bfq_dispatch_request(), for instance. You call bfq_select_queue(),
and that in turn calls bfq_bfqq_expire(), which calls
__bfq_bfqq_expire() which can set ->ioc_to_put. But later on,
__bfq_dispatch_request() calls bfq_dispatch_rq_from_bfqq(), which in
turn calls bfq_bfqq_expire() that can also set ->ioc_to_put. There's no
"magic" bfq_unlock_and_put_ioc() in-between those. Maybe the former call
never sets ->ioc_to_put if it returns with bfqq == NULL? Hard to tell.

Or __bfq_insert_request(), it calls bfq_add_request(), which may set
->ioc_to_put through bfq_bfqq_handle_idle_busy_switch() ->
bfq_bfqq_expire(). And then from calling bfq_rq_enqueued() ->
bfq_bfqq_expire().

There might be more, but I think the above is plenty of evidence that
the current ->ioc_to_put solution is a bad hack, fragile, and already
has bugs.

How often do you expect this putting of the io_context to happen? If
it's not a very frequent occurence, maybe using a deferred workqueue to
put it IS the right solution. As it currently stands, the code doesn't
really work, and it's fragile. It can't be cleaned up without
refactoring, since the call paths are all extremely intermingled.

-- 
Jens Axboe