From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753241AbbFIOkN (ORCPT ); Tue, 9 Jun 2015 10:40:13 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53936 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751383AbbFIOkI (ORCPT ); Tue, 9 Jun 2015 10:40:08 -0400 From: Jeff Moyer To: Tejun Heo Cc: axboe@kernel.dk, linux-kernel@vger.kernel.org, cgroups@vger.kernel.org, vgoyal@redhat.com, avanzini.arianna@gmail.com Subject: Re: [PATCH 7/8] cfq-iosched: fold cfq_find_alloc_queue() into cfq_get_queue() References: <1433753973-23684-1-git-send-email-tj@kernel.org> <1433753973-23684-8-git-send-email-tj@kernel.org> X-PGP-KeyID: 1F78E1B4 X-PGP-CertKey: F6FE 280D 8293 F72C 65FD 5A58 1FF8 A7CA 1F78 E1B4 X-PCLoadLetter: What the f**k does that mean? Date: Tue, 09 Jun 2015 10:40:02 -0400 In-Reply-To: <1433753973-23684-8-git-send-email-tj@kernel.org> (Tejun Heo's message of "Mon, 8 Jun 2015 17:59:32 +0900") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Tejun Heo writes: > cfq_find_alloc_queue() checks whether a queue actually needs to be > allocated, which is unnecessary as its sole caller, cfq_get_queue(), > only calls it if so. Also, the oom queue fallback logic is scattered > between cfq_get_queue() and cfq_find_alloc_queue(). There really > isn't much going on in the latter and things can be made simpler by > folding it into cfq_get_queue(). > > This patch collapses cfq_find_alloc_queue() into cfq_get_queue(). The > change is fairly straight-forward with one exception - async_cfqq is > now initialized to NULL and the "!is_sync" test in the last if > conditional is replaced with "async_cfqq" test. This is because gcc > (5.1.1) gets confused for some reason and warns that async_cfqq may be > used uninitialized otherwise. Oh well, the code isn't necessarily > worse this way. > > This patch doesn't cause any functional difference. The resulting code (introduced by the last patch, I know) is not ideal: rcu_read_lock(); cfqg = cfq_lookup_create_cfqg(cfqd, bio_blkcg(bio)); if (!cfqg) { cfqq = &cfqd->oom_cfqq; goto out; } if (!is_sync) { if (!ioprio_valid(cic->ioprio)) { struct task_struct *tsk = current; ioprio = task_nice_ioprio(tsk); ioprio_class = task_nice_ioclass(tsk); } async_cfqq = cfq_async_queue_prio(cfqd, ioprio_class, ioprio); cfqq = *async_cfqq; if (cfqq) goto out; } As you mentioned, we don't need to lookup the cfqg for the async queue. What's more is we could fallback to the oom_cfqq even if we had an existing async cfqq. I'm guessing you structured the code this way to make the error path cleaner. I don't think it's a big deal, as it should be a rare occurrence, so... Reviewed-by: Jeff Moyer