On 07.06.23 23:12, Edgecombe, Rick P wrote:
On Wed, 2023-06-07 at 19:11 +0200, Janusz Krzysztofik wrote:On Wednesday, 7 June 2023 17:31:24 CEST Dave Hansen wrote:On 6/7/23 08:23, Janusz Krzysztofik wrote:Extend bitmask used by pgprot_modify() for selecting bits to be preserved with _PAGE_PAT bit. However, since that bit can be reused as _PAGE_PSE, and the _PAGE_CHG_MASK symbol, primarly used by pte_modify(), is likely intentionally defined with that bit not set, keep that symbol unchanged.I'm really having a hard time parsing what that last sentence is saying. Could you try again, please?OK, but then I need to get my doubts addressed by someone first, otherwise I'm not able to provide a better justification from my heart. The issue needs to be fixed by including _PAGE_PAT bit into a bitmask used by pgprot_modify() for selecting bits to be preserved. We can either do that internally to pgprot_modify() (my initial proposal, which my poorly worded paragraph was still trying to describe and justify), or by making _PAGE_PAT a part of _PAGE_CHG_MASK, as suggested by Borislav and reflected in my v2 changelog. But for the latter, I think we need to make sure that we don't break other users of _PAGE_CHG_MASK. Maybe Borislav can confirm that's the case. Since _PAGE_PAT is the same as _PAGE_PSE, _HPAGE_CHG_MASK -- a huge pmds' counterpart of _PAGE_CHG_MASK, introduced by commit c489f1257b8c ("thp: add pmd_modify"), defined as (_PAGE_CHG_MASK | _PAGE_PSE) -- will no longer differ from _PAGE_CHG_MASK as soon as we add _PAGE_PAT bit to the latter. If such modification of _PAGE_CHG_MASK was irrelevant to its users then one may ask why a new symbol was introduced instead of reusing the existing one with that otherwise irrelevant bit (_PAGE_PSE in that case) added. I've initially assumed that keeping _PAGE_CHG_MASK without _PAGE_PSE (vel _PAGE_PAT) included into it was intentional for some reason. Maybe Johannes Weiner, the author of that patch (adding him to Cc:), could shed more light on that.So since _PAGE_PSE is actually the same value as _PAGE_PAT, you don't actually need to have _PAGE_PSE in _HPAGE_CHG_MASK in order to get functional correctness. Is that right? I think it is still a little hidden (even before this) and I wonder about separating out the common bits into, like, _COMMON_PAGE_CHG_MASK or something. Then setting specific PAGE and HPAGE bits (like _PAGE_PAT, _PAGE_PSE and _PAGE_PAT_LARGE) in their specific define. Would it be more readable that way?
I'd go that route. I don't think we should rely on _PAGE_PSE == _PAGE_PAT here. Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
