On Wednesday, 15 September 2021 12:52:48 PM AEST Peter Xu wrote:

quoted

quoted

The flag introduced in this patch will be a preparation for more bits defined
in the future, e.g., for a new bit in flag to show whether to persist the
upcoming uffd-wp bit in pgtable entries.

That's kind of the problem. The patch itself looks correct to me however as
mentioned it is mostly reverting a previous cleanup and it's hard to tell why
that's justified without the subsequent patches. Perhaps it makes the usage of
zap_details a bit clearer, but a comment also would with less code.

I know you want to try and shrink the uffd-wp series but I think this patch
might be easier to review if it was included as part of that series.

I posted it because I think it's suitable to have it even without uffd-wp.

I tried to explain it above on two things this patch wanted to fix:

Firstly the comment is wrong; we've moved back and forth on changing the
zap_details flags but the comment is not changing along the way and it's not
matching the code right now.

Secondly I do think we should have a flag showing explicit willingness to skip
swap entries.  Yes, uffd-wp is the planned new one, but my point is anyone who
will introduce a new user of zap_details pointer could overlook this fact.  The
new flag helps us to make sure someone will at least read the flags and know
what'll happen with it.

For the 2nd reasoning, I also explicitly CCed Kirill too, so Kirill can provide
any comment if he disagrees.  For now, I still think we should keep having such
a flag otherwise it could be error-prone.

Could you buy-in above reasoning?

Kind of, I do think it makes the usage of details a bit clearer or at least
harder to miss. It is just that if that was the sole aim of this patch I think
there might be simpler (less code) ways of doing so.

Basically above is what I wanted to express in my commit message.  I hope that
can justify that this patch (even if extremly simple) can still be considered
as acceptable upstream even without uffd-wp series.

If you still insist on this patch not suitable for standalone merging and
especially if some other reviewer would think the same, I can move it back to
uffd-wp series for sure.  Then I'll repost this series with 4 patches only.

I won't insist, the code looks correct and it doesn't make things any less
clear so you can put my Reviewed-by on it and perhaps leave it to Andrew or
another reviewer to determine if this should be taken in this series or as part
of a future uffd-wp series.

 - Alistair

In all cases, thanks for looking at the series.