On Thu, Aug 08 2024 at 10:54, Peter Xu wrote:

On Thu, Aug 08, 2024 at 12:22:38AM +0200, Thomas Gleixner wrote:

quoted

On Wed, Aug 07 2024 at 15:48, Peter Xu wrote:

quoted

An entry should be reported as PUD leaf even if it's PROT_NONE, in which
case PRESENT bit isn't there. I hit bad pud without this when testing dax
1G on zapping a PROT_NONE PUD.

That does not qualify as a change log. What you hit is irrelevant unless
you explain the actual underlying problem. See Documentation/process/
including the TIP documentation.

Firstly, thanks a lot for the reviews.

I thought the commit message explained exactly what is the underlying
problem, no?

The problem is even if PROT_NONE, as long as the PSE bit is set on the PUD
it should be treated as a PUD leaf.

Sure. But why should it be treated so.

Currently, the code will return pud_leaf()==false for those PROT_NONE
PUD entries, and IMHO that is wrong.

Your humble opinion is fine, but hardly a technical argument.

This patch wants to make it right.  I still think that's mostly what I put
there in the commit message..

Would you please suggest something so I can try to make it better,
otherwise?  Or it'll be helpful too if you could point out which part of
the two documentations I should reference.

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog

  A good structure is to explain the context, the problem and the
  solution in separate paragraphs and this order

quoted

quoted

diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
index e39311a89bf4..a2a3bd4c1bda 100644
--- a/arch/x86/include/asm/pgtable.h
+++ b/arch/x86/include/asm/pgtable.h

@@ -1078,8 +1078,7 @@ static inline pmd_t *pud_pgtable(pud_t pud)
 #define pud_leaf pud_leaf
 static inline bool pud_leaf(pud_t pud)
 {
-	return (pud_val(pud) & (_PAGE_PSE | _PAGE_PRESENT)) ==
-		(_PAGE_PSE | _PAGE_PRESENT);
+	return pud_val(pud) & _PAGE_PSE;
 }

And the changelog does not explain why this change is not affecting any
existing user of pud_leaf().

That's what I want to do: I want to affect them..

Fine. Just the change log does not tell me what the actual problem is
("I hit something" does not qualify) and "should be reported" is not
helpful either as it does not explain anything

And IMHO it's mostly fine before because mprotect() is broken with 1g
anyway, and I guess nobody managed to populate any pud entry with PROT_NONE
on dax 1g before, and that's what this whole series is trying to fix.

Again your humble opinion matters, but technical facts and analysis
matter way more.

Thanks,

        tglx