On 4/27/23 15:31, Lorenzo Stoakes wrote:
...
bool file_backed = !vma_is_anonymous(vma);
would lead to a slightly better reading experience below.
Well you see, I'm not so sure about that, because vma_is_anonymous() checks
vm_ops == NULL not vm_file == NULL which can be the case for a special
mapping like VDSO that is not in fact file-backed :) the horror, the
horror.
Yes, you are right. It looks like vma_anon is a better name here,
after all.
...
...and now we call it again. I think once should be enough, though.
Right, this was intentional (I think I mentioned it in the revision
notes?), because there is a conundrum here - the invocation from
vma_wants_writenotify() needs to check this _first_ before performing the
_other_ checks in vma_needs_dirty_tracking(), but external calls need all
the checks. It'd be ugly to pass a boolean to see if we should check this
or not, and it's hardly an egregious duplication for the _computer_
(something likely in a cache line != NULL) which aids readability and
reduces duplication for the _reader_ of the code for a path that is
inherently slow (likely going to fault in pages etc.)
I think it'd be confusing to have yet another split into
vma_can_track_dirty() or whatever because then suddenly for the check to be
meaningful you have to _always_ check 2 things.
Other options like passing an output parameter or returning something other
than boolean are equally distasteful.
Agreed. (And yes, I overlooked that discussion in the version notes.)
Also, with the exception of that double call to
vm_ops_needs_writenotify(), these changes to mmap.c are code cleanup
that has the same behavior as before. As such, it's better to separate
them out from this patch whose goal is very much to change behavior.
It's not really cleanup, it's separating out some of the logic explicitly
to be used in this new context, without which the separation would not be
useful, so I feel it's a bit over the top to turn a small single patch into
two simply to avoid this.
Sure, OK.
Thanks for the review, I will respin with the suggestions (other than ones
I don't quite agree with as explained above) and a clearer description in
line with Mika's suggestions.
Hopefully we can move closer to this actually getting some
reviewed/acked-by tags soon :)
thanks,
--
John Hubbard
NVIDIA
