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

[Yang Shi]

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