Re: [v2 PATCH 1/2] mm: mempolicy: make the behavior consistent when MPOL_MF_MOVE* and MPOL_MF_STRICT were specified

[Yang Shi <yang.shi@linux.alibaba.com>]

On 7/17/19 3:55 AM, Vlastimil Babka wrote:
> On 7/16/19 7:18 PM, Yang Shi wrote:
>>> I think after your patch, you miss putback_movable_pages() in cases
>>> where some were queued, and later the walk returned -EIO. The previous
>>> code doesn't miss it, but it's also not obvious due to the multiple if
>>> (!err) checks. I would rewrite it some thing like this:
>>>
>>> if (ret < 0) {
>>>       putback_movable_pages(&pagelist);
>>>       err = ret;
>>>       goto mmap_out; // a new label above up_write()
>>> }
>> Yes, the old code had putback_movable_pages called when !err. But, I
>> think that is for error handling of mbind_range() if I understand it
>> correctly since if queue_pages_range() returns -EIO (only MPOL_MF_STRICT
>> was specified and there was misplaced page) that page list should be
>> empty . The old code should checked whether that list is empty or not.
> Hm I guess you're right, returning with EIO means nothing was queued.
>> So, in the new code I just removed that.
>>
>>> The rest can have reduced identation now.
>> Yes, the goto does eliminate the extra indentation.
>>
>>>> +	else {
>>>> +		err = mbind_range(mm, start, end, new);
>>>>    
>>>> -		if (nr_failed && (flags & MPOL_MF_STRICT))
>>>> -			err = -EIO;
>>>> -	} else
>>>> -		putback_movable_pages(&pagelist);
>>>> +		if (!err) {
>>>> +			int nr_failed = 0;
>>>> +
>>>> +			if (!list_empty(&pagelist)) {
>>>> +				WARN_ON_ONCE(flags & MPOL_MF_LAZY);
>>>> +				nr_failed = migrate_pages(&pagelist, new_page,
>>>> +					NULL, start, MIGRATE_SYNC,
>>>> +					MR_MEMPOLICY_MBIND);
>>>> +				if (nr_failed)
>>>> +					putback_movable_pages(&pagelist);
>>>> +			}
>>>> +
>>>> +			if ((ret > 0) ||
>>>> +			    (nr_failed && (flags & MPOL_MF_STRICT)))
>>>> +				err = -EIO;
>>>> +		} else
>>>> +			putback_movable_pages(&pagelist);
>>> While at it, IIRC the kernel style says that when the 'if' part uses
>>> '{ }' then the 'else' part should as well, and it shouldn't be mixed.
>> Really? The old code doesn't have '{ }' for else, and checkpatch doesn't
>> report any error or warning.
> Checkpatch probably doesn't catch it, nor did the reviewers of the older
> code. But coding-style.rst says:
>
> Do not unnecessarily use braces where a single statement will do.
>
> ...
>
> This does not apply if only one branch of a conditional statement is a
> single
> statement; in the latter case use braces in both branches:
>
> .. code-block:: c
>
>          if (condition) {
>                  do_this();
>                  do_that();
>          } else {
>                  otherwise();
>          }

Thanks. Good to know this. Anyway, with the "goto" suggested above, we 
don't need that "else" anymore and we could save some change of lines.

>
>
> Thanks,
> Vlastimil