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 01/14] block, bfq: introduce the BFQ-v0 I/O scheduler as an extra scheduler From: Paolo Valente In-Reply-To: <9ddb9586-d97e-e7b2-0081-216521f7be76@kernel.dk> Date: Sat, 18 Mar 2017 08:41:52 -0400 Cc: Tejun Heo , Fabio Checconi , Arianna Avanzini , linux-block@vger.kernel.org, Linux-Kernal , Ulf Hansson , Linus Walleij , broonie@kernel.org Message-Id: <08CE1658-04B0-48F4-BF3D-A6DB0F032905@linaro.org> References: <20170304160131.57366-1-paolo.valente@linaro.org> <20170304160131.57366-2-paolo.valente@linaro.org> <9ddb9586-d97e-e7b2-0081-216521f7be76@kernel.dk> To: Jens Axboe List-ID: > Il giorno 07 mar 2017, alle ore 18:22, Jens Axboe ha = scritto: >=20 >> +/** >> + * bfq_entity_of - get an entity from a node. >> + * @node: the node field of the entity. >> + * >> + * Convert a node pointer to the relative entity. This is used only >> + * to simplify the logic of some functions and not as the generic >> + * conversion mechanism because, e.g., in the tree walking = functions, >> + * the check for a %NULL value would be redundant. >> + */ >> +static struct bfq_entity *bfq_entity_of(struct rb_node *node) >> +{ >> + struct bfq_entity *entity =3D NULL; >> + >> + if (node) >> + entity =3D rb_entry(node, struct bfq_entity, rb_node); >> + >> + return entity; >> +} >=20 > Get rid of pointless wrappers like this, just use rb_entry() in the > caller. It's harmful to the readability of the code to have to lookup > things like this, if it's a list_entry or rb_entry() in the caller you > know exactly what is going on immediately. >=20 Ok. Just a quick request for help about this. The code seems to become quite ugly with a verbatim replacement of this function with its body (and there are several occurrences of it), so there is probably = something I'm missing. For example, given the following loop: for (; entity ; entity =3D bfq_entity_of(rb_first(active))) bfq_reparent_leaf_entity(bfqd, entity); the only non-cryptic, but heavier solution I see is: for (; entity ; ) { bfq_reparent_leaf_entity(bfqd, entity); if (rb_first(active)) entity =3D rb_entry(rb_first(active), struct = bfq_entity, rb_node); else entity =3D NULL; } Am I missing something? Or are these extra lines of code a reasonable price to pay for increased transparency? Thanks, Paolo > --=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 S1751594AbdCRMmH (ORCPT ); Sat, 18 Mar 2017 08:42:07 -0400 Received: from mail-qk0-f177.google.com ([209.85.220.177]:35757 "EHLO mail-qk0-f177.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751397AbdCRMlz (ORCPT ); Sat, 18 Mar 2017 08:41:55 -0400 Content-Type: text/plain; charset=windows-1252 Mime-Version: 1.0 (Mac OS X Mail 9.3 \(3124\)) Subject: Re: [PATCH RFC 01/14] block, bfq: introduce the BFQ-v0 I/O scheduler as an extra scheduler From: Paolo Valente In-Reply-To: <9ddb9586-d97e-e7b2-0081-216521f7be76@kernel.dk> Date: Sat, 18 Mar 2017 08:41:52 -0400 Cc: Tejun Heo , Fabio Checconi , Arianna Avanzini , linux-block@vger.kernel.org, Linux-Kernal , Ulf Hansson , Linus Walleij , broonie@kernel.org Message-Id: <08CE1658-04B0-48F4-BF3D-A6DB0F032905@linaro.org> References: <20170304160131.57366-1-paolo.valente@linaro.org> <20170304160131.57366-2-paolo.valente@linaro.org> <9ddb9586-d97e-e7b2-0081-216521f7be76@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 v2ICgALF005414 > Il giorno 07 mar 2017, alle ore 18:22, Jens Axboe ha scritto: > >> +/** >> + * bfq_entity_of - get an entity from a node. >> + * @node: the node field of the entity. >> + * >> + * Convert a node pointer to the relative entity. This is used only >> + * to simplify the logic of some functions and not as the generic >> + * conversion mechanism because, e.g., in the tree walking functions, >> + * the check for a %NULL value would be redundant. >> + */ >> +static struct bfq_entity *bfq_entity_of(struct rb_node *node) >> +{ >> + struct bfq_entity *entity = NULL; >> + >> + if (node) >> + entity = rb_entry(node, struct bfq_entity, rb_node); >> + >> + return entity; >> +} > > Get rid of pointless wrappers like this, just use rb_entry() in the > caller. It's harmful to the readability of the code to have to lookup > things like this, if it's a list_entry or rb_entry() in the caller you > know exactly what is going on immediately. > Ok. Just a quick request for help about this. The code seems to become quite ugly with a verbatim replacement of this function with its body (and there are several occurrences of it), so there is probably something I'm missing. For example, given the following loop: for (; entity ; entity = bfq_entity_of(rb_first(active))) bfq_reparent_leaf_entity(bfqd, entity); the only non-cryptic, but heavier solution I see is: for (; entity ; ) { bfq_reparent_leaf_entity(bfqd, entity); if (rb_first(active)) entity = rb_entry(rb_first(active), struct bfq_entity, rb_node); else entity = NULL; } Am I missing something? Or are these extra lines of code a reasonable price to pay for increased transparency? Thanks, Paolo > -- > Jens Axboe >