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

[Nicholas Piggin <npiggin@gmail.com>]

On Wed, 7 Mar 2018 07:12:23 +0100
Christophe LEROY <christophe.leroy@c-s.fr> wrote:

> Le 07/03/2018 à 00:12, Nicholas Piggin a écrit :
> > On Tue, 6 Mar 2018 15:41:00 +0100
> > Christophe LEROY <christophe.leroy@c-s.fr> 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