From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf0-x231.google.com (mail-pf0-x231.google.com [IPv6:2607:f8b0:400e:c00::231]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 3zx4h62cNszF1Td for ; Wed, 7 Mar 2018 18:16:37 +1100 (AEDT) Received: by mail-pf0-x231.google.com with SMTP id 68so613010pfx.3 for ; Tue, 06 Mar 2018 23:16:37 -0800 (PST) Date: Wed, 7 Mar 2018 17:16:21 +1000 From: Nicholas Piggin To: Christophe LEROY Cc: linuxppc-dev@lists.ozlabs.org, "Aneesh Kumar K . V" Subject: Re: [PATCH 06/10] powerpc/mm/slice: implement slice_check_range_fits Message-ID: <20180307171621.00ec8814@roar.ozlabs.ibm.com> In-Reply-To: References: <20180306132507.10649-1-npiggin@gmail.com> <20180306132507.10649-7-npiggin@gmail.com> <20180307091232.4fb8d3c2@roar.ozlabs.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Wed, 7 Mar 2018 07:12:23 +0100 Christophe LEROY wrote: > Le 07/03/2018 à 00:12, Nicholas Piggin a écrit : > > On Tue, 6 Mar 2018 15:41:00 +0100 > > Christophe LEROY wrote: > > > >> Le 06/03/2018 à 14:25, Nicholas Piggin a écrit : > >>> @@ -596,10 +601,11 @@ unsigned long slice_get_unmapped_area(unsigned long addr, unsigned long len, > >>> slice_or_mask(&potential_mask, &good_mask); > >>> slice_print_mask(" potential", &potential_mask); > >>> > >>> - if ((addr != 0 || fixed) && > >>> - slice_check_fit(mm, &mask, &potential_mask)) { > >>> - slice_dbg(" fits potential !\n"); > >>> - goto convert; > >>> + if (addr || fixed) { > >>> + if (slice_check_range_fits(mm, &potential_mask, addr, len)) { > >>> + slice_dbg(" fits potential !\n"); > >>> + goto convert; > >>> + } > >> > >> Why not keep the original structure and just replacing slice_check_fit() > >> by slice_check_range_fits() ? > >> > >> I believe cleanups should not be mixed with real feature changes. If > >> needed, you should have a cleanup patch up front the serie. > > > > For code that is already changing, I think minor cleanups are okay if > > they're very simple. Maybe this is getting to the point of needing > > another patch. You've made valid points for a lot of other unnecessary > > cleanups though, so I'll fix all of those. > > Ok, that's not a big point, but I like when patches really modifies > only the lines they need to modify. Fair point, and in the end I agree mostly they should do that. But I don't think entirely if you can make the code slightly better as you go (again, so long as the change is obvious). I think having extra patches for trivial cleanups is not that great either. > Why do we need a double if ? > > Why not just the following ? With proper alignment of the second line > with the open parenthese, it fits in one line > > if ((addr != 0 || fixed) && > - slice_check_fit(mm, &mask, &potential_mask)) { > + slice_check_range_fits(mm, &potential_mask, addr, len)) { > slice_dbg(" fits potential !\n"); > goto convert; For this case the main motivation was to make this test match the form of the same test (with different mask) above here. Doing the same thing with different coding styles annoys me. I think I kept this one but fixed all your other suggestions in the v2 series. Thanks, Nick