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
