Re: [PATCH 06/10] powerpc/mm/slice: implement slice_check_range_fits

Le 07/03/2018 à 08:16, Nicholas Piggin a écrit :

On Wed, 7 Mar 2018 07:12:23 +0100
Christophe LEROY [off-list ref] wrote:

quoted

Le 07/03/2018 à 00:12, Nicholas Piggin a écrit :

quoted

On Tue, 6 Mar 2018 15:41:00 +0100
Christophe LEROY [off-list ref] wrote:
   

quoted

Le 06/03/2018 à 14:25, Nicholas Piggin a écrit :

quoted

quoted

quoted

quoted

@@ -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.

quoted

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.

Yes good point.

Christophe

I think I kept this one but fixed all your other suggestions in
the v2 series.

Thanks,
Nick