From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Return-Path: Content-Type: text/plain; charset=windows-1252 Mime-Version: 1.0 (Mac OS X Mail 9.3 \(3124\)) Subject: Re: [PATCH RFC 10/14] block, bfq: add Early Queue Merge (EQM) From: Paolo Valente In-Reply-To: <7b7c0d4c-c10d-1e69-4ea0-1ad05a4100a2@kernel.dk> Date: Wed, 15 Mar 2017 17:59:17 +0100 Cc: Tejun Heo , Fabio Checconi , Arianna Avanzini , linux-block@vger.kernel.org, Linux-Kernal , Ulf Hansson , Linus Walleij , broonie@kernel.org, Mauro Andreolini Message-Id: <2086B143-E8E3-43AD-B074-771378579081@linaro.org> References: <20170304160131.57366-1-paolo.valente@linaro.org> <20170304160131.57366-11-paolo.valente@linaro.org> <1D2EDF98-59BA-4A98-8251-9BEC4AC752C4@linaro.org> <7b7c0d4c-c10d-1e69-4ea0-1ad05a4100a2@kernel.dk> To: Jens Axboe List-ID: > Il giorno 15 mar 2017, alle ore 17:30, Jens Axboe ha = scritto: >=20 > 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. >>=20 >> 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. >>=20 >> If you get this right, it'll be a lot less fragile and hacky than = your >> current approach. >=20 > 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. >=20 > 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 =3D=3D NULL? Hard to = tell. >=20 > 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(). >=20 I have checked that. Basically, since a queue can't be expired twice, then it should never happen that ioc_to_put is set twice before being used. Yet, I do agree that using a shared field and exploiting collateral effects makes code very complex and fragile (maybe even buggy if my speculative check is wrong). Just, it has been the best solution I found, to avoid deferred work as you asked. In fact, I still find quite heavy the alternative of passing a pointer to an ioc forth and back across seven or eight nested functions. > 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. >=20 > How often do you expect this putting of the io_context to happen? Unfortunately often, as it must be done also every time the in-service queue is reset. But, in this respect, are we sure that we do need to grab a reference to the ioc when we set a queue in service (as done in cfq, and copied into bfq)? I mean, we have the hook exit_ioc for controlling the disappearing of an ioc. Am I missing something here too? Thanks, Paolo > 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. >=20 > --=20 > Jens Axboe >=20 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753541AbdCOQ7k (ORCPT ); Wed, 15 Mar 2017 12:59:40 -0400 Received: from mail-wm0-f49.google.com ([74.125.82.49]:36806 "EHLO mail-wm0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751480AbdCOQ7h (ORCPT ); Wed, 15 Mar 2017 12:59:37 -0400 Content-Type: text/plain; charset=windows-1252 Mime-Version: 1.0 (Mac OS X Mail 9.3 \(3124\)) Subject: Re: [PATCH RFC 10/14] block, bfq: add Early Queue Merge (EQM) From: Paolo Valente In-Reply-To: <7b7c0d4c-c10d-1e69-4ea0-1ad05a4100a2@kernel.dk> Date: Wed, 15 Mar 2017 17:59:17 +0100 Cc: Tejun Heo , Fabio Checconi , Arianna Avanzini , linux-block@vger.kernel.org, Linux-Kernal , Ulf Hansson , Linus Walleij , broonie@kernel.org, Mauro Andreolini Message-Id: <2086B143-E8E3-43AD-B074-771378579081@linaro.org> References: <20170304160131.57366-1-paolo.valente@linaro.org> <20170304160131.57366-11-paolo.valente@linaro.org> <1D2EDF98-59BA-4A98-8251-9BEC4AC752C4@linaro.org> <7b7c0d4c-c10d-1e69-4ea0-1ad05a4100a2@kernel.dk> To: Jens Axboe X-Mailer: Apple Mail (2.3124) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id v2FGxsHV008071 > Il giorno 15 mar 2017, alle ore 17:30, Jens Axboe ha scritto: > > 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(). > I have checked that. Basically, since a queue can't be expired twice, then it should never happen that ioc_to_put is set twice before being used. Yet, I do agree that using a shared field and exploiting collateral effects makes code very complex and fragile (maybe even buggy if my speculative check is wrong). Just, it has been the best solution I found, to avoid deferred work as you asked. In fact, I still find quite heavy the alternative of passing a pointer to an ioc forth and back across seven or eight nested functions. > 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? Unfortunately often, as it must be done also every time the in-service queue is reset. But, in this respect, are we sure that we do need to grab a reference to the ioc when we set a queue in service (as done in cfq, and copied into bfq)? I mean, we have the hook exit_ioc for controlling the disappearing of an ioc. Am I missing something here too? Thanks, Paolo > 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 >