From: Bart Van Assche <Bart.VanAssche@sandisk.com>
To: "paolo.valente@linaro.org" <paolo.valente@linaro.org>
CC: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
        "fchecconi@gmail.com" <fchecconi@gmail.com>,
        "linus.walleij@linaro.org" <linus.walleij@linaro.org>,
        "axboe@kernel.dk" <axboe@kernel.dk>,
        "avanzini.arianna@gmail.com" <avanzini.arianna@gmail.com>,
        "broonie@kernel.org" <broonie@kernel.org>,
        "tj@kernel.org" <tj@kernel.org>,
        "ulf.hansson@linaro.org" <ulf.hansson@linaro.org>
Subject: Re: [PATCH RFC 01/14] block, bfq: introduce the BFQ-v0 I/O
 scheduler as an extra scheduler
Thread-Topic: [PATCH RFC 01/14] block, bfq: introduce the BFQ-v0 I/O
 scheduler as an extra scheduler
Thread-Index: AQHSn/u/G4Q8LG/Xr0CSoUvMInNnOg==
Date: Sat, 18 Mar 2017 15:24:34 +0000
Message-ID: <1489850658.2339.1.camel@sandisk.com>
References: <20170304160131.57366-1-paolo.valente@linaro.org>
         <20170304160131.57366-2-paolo.valente@linaro.org>
         <1D08B61A9CF0974AA09887BE32D889DA0DA145@ULS-OP-MBXIP03.sdcorp.global.sandisk.com>
         <0B1F1CFD-C8AB-442D-B5C6-426D19B1FBA3@linaro.org>
In-Reply-To: <0B1F1CFD-C8AB-442D-B5C6-426D19B1FBA3@linaro.org>
Accept-Language: en-US, nl-NL
Content-Language: en-US
Content-Type: text/plain; charset="utf-8"
Content-ID: <86D0CE21ADBEC8478A4A96942153F79D@sandisk.com>
MIME-Version: 1.0
WDCIPOUTBOUND: EOP-TRUE
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Mar 2017 15:24:38.2020
 (UTC)
X-MS-Exchange-CrossTenant-Id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=b61c8803-16f3-4c35-9b17-6f65f441df86;Ip=[74.221.232.55];Helo=[sacsmgep15.sandisk.com]
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO2PR04MB827
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id v2IFSBsF019654
Content-Length: 1945
Lines: 49

On Sat, 2017-03-18 at 08:08 -0400, Paolo Valente wrote:
> > Il giorno 06 mar 2017, alle ore 14:40, Bart Van Assche <bart.vanassche@sandisk.com> ha scritto:
> > > +#define BFQ_BFQQ_FNS(name)						\
> > > +static void bfq_mark_bfqq_##name(struct bfq_queue *bfqq)		\
> > > +{									\
> > > +	(bfqq)->flags |= (1 << BFQ_BFQQ_FLAG_##name);			\
> > > +}									\
> > > +static void bfq_clear_bfqq_##name(struct bfq_queue *bfqq)		\
> > > +{									\
> > > +	(bfqq)->flags &= ~(1 << BFQ_BFQQ_FLAG_##name);			\
> > > +}									\
> > > +static int bfq_bfqq_##name(const struct bfq_queue *bfqq)		\
> > > +{									\
> > > +	return ((bfqq)->flags & (1 << BFQ_BFQQ_FLAG_##name)) != 0;	\
> > > +}
> > 
> > Are the bodies of the above functions duplicates of __set_bit(),
> > __clear_bit() and test_bit()?
> 
> Yes.  We wrapped them into functions, because writing mark_flag_name
> seemed more readable than writing the implementation of the marking of the
> flag.

Please do not open-code __set_bit(), __clear_bit() and test_bit() but use
these macros instead.

> > > +	} else
> > > +		/*
> > > +		 * Async queues get always the maximum possible
> > > +		 * budget, as for them we do not care about latency
> > > +		 * (in addition, their ability to dispatch is limited
> > > +		 * by the charging factor).
> > > +		 */
> > > +		budget = bfqd->bfq_max_budget;
> > > +
> > 
> > Please balance braces. Checkpatch should have warned about the use of "}
> > else" instead of "} else {".
> 
> No warning, I guess because the body of the else contains only a
> simple instruction.  Just to learn for the future: what's the
> rationale for adding braces here, but not imposing braces everywhere
> for single-instruction bodies?

It's a general style recommendation for all kernel code: if braces are used
for one side of an if-statement, also use braces for the other side, and
definitely if that other side consists of multiple lines due to a comment.

Bart.