Re: [PATCH] x86/mm: fix return value of p[um]dp_set_access_flags

[Dave Hansen <dave.hansen@intel.com>]

On 9/19/19 1:25 AM, Wei Yang wrote:
> Function p[um]dp_set_access_flags is used with update_mmu_cache_p[um]d
> and the return value from p[um]dp_set_access_flags indicates whether it
> is necessary to do the cache update.

If this change is correct, why was it not applied to
ptep_set_access_flags()?  That function has the same form.

BTW, I rather dislike the 'dirty' variable name.  It seems to be
indicating whether the fault was a write fault or not and whether we
*expect* the dirty bit to be set in 'entry'.

> From current code logic, only when changed && dirty, related page table
> entry would be updated. It is not necessary to update cache when the
> real page table entry is not changed.

This logic doesn't really hold up, though.

If we are only writing accessed and/or dirty bits, then we *never* need
to flush.  The flush might avoid a stale TLB entry causing an extra page
walk by the hardware, but it's probably not ever worth the cost of the
flush.

Unless there's something weird happening with paravirt, I can't ever see
a good reason to flush the TLB when just setting accessed/dirty bits on
bare-metal.

This seems like a place where a debugging check to validate that only
accessed/dirty bits are only being set would be a good idea.