Let's just rewind a bit. This thread exists because the kernel test robot failed
to compile pgd_none_or_clear_bad() (a core-mm function) for the arm architecture
after Anshuman changed the direct pgd dereference to pgdp_get(). The reason
compilation failed is because arm defines its own pgdp_get() override, but it is
broken (there is a typo).
Let's not rewind, because had you fully read and digested my reply, you
would have seen why this isn't a problem... but let me spell it out.
Code before Anshuman's change:
static inline int pgd_none_or_clear_bad(pgd_t *pgd)
{
if (pgd_none(*pgd))
return 1;
if (unlikely(pgd_bad(*pgd))) {
pgd_clear_bad(pgd);
return 1;
}
return 0;
}
This isn't a problem as the code stands. While there is a dereference
in C, that dereference is a simple struct copy, something that we use
everywhere in the kernel. However, that is as far as it goes, because
neither pgd_none() and pgd_bad() make use of their argument, and thus
the compiler will optimise it away, resulting in no actual access to
the page tables - _as_ _intended_.
Right. Are you saying you depend upon those loads being optimized away for
correctness or performance reasons?
If these are going to be converted to pgd_get(), then we need pgd_get()
to _also_ be optimised away,
OK, agreed.
So perhaps the best approach is to modify the existing default pxdp_get()
implementations to just do a C dereference. That will ensure that there are no
intended consequences, unlike moving to READ_ONCE() by default. Then riscv
(which I think is the only arch to actually use pxdp_get() currently?) will need
its own pxdp_get() overrides, which use READ_ONCE(). arm64 would also define its
own overrides in terms of READ_ONCE() to ensure single copy atomicity in the
presence of HW updates.
How does that sound to you?
and if e.g. this is the only place that
pgd_get() is going to be used, the suggestion I made in my previous
email is entirely reasonable, since we know that the result of pgd_get()
will not actually be used.
I guess you could do that as an arm-specific override, but I don't think it adds
anything over using my proposed reworked default? Your call.
As an aside, the kernel also dereferences p4d, pud, pmd and pte pointers in
various circumstances.
I already covered these in my previous reply.
And other changes in this series are also replacing those
direct dereferences with calls to similar helpers. The fact that these are all
folded (by a custom arm implementation if I've understood the below correctly)
just means that each dereference is returning what you would call the pmd from
the HW perspective, I think?
It'll "return" the first of each pair of level-1 page table entries,
which is pgd[0] or *p4d, *pud, *pmd - but all of these except *pmd
need to be optimised away, so throwing lots of READ_ONCE() around
this code without considering this is certainly the wrong approach.
Yep, got it.
The core-mm today
dereferences pgd pointers (and p4d, pud, pmd pointers) directly in its code. See
follow_pfnmap_start(),
Doesn't seem to exist at least not in 6.11.
Appologies, I'm on mm-unstable and that isn't upstream yet. See follow_pte() in
v6.11 or __apply_to_page_range(), or pgd_none_or_clear_bad() as per above.
Looking at follow_pte(), it's not a problem.
I think we wouldn't be having this conversation before:
commit a32618d28dbe6e9bf8ec508ccbc3561a7d7d32f0
Author: Russell King <rmk+kernel@xxxxxxxxxxxxxxxx>
Date: Tue Nov 22 17:30:28 2011 +0000
ARM: pgtable: switch to use pgtable-nopud.h
where:
-#define pgd_none(pgd) (0)
-#define pgd_bad(pgd) (0)
existed before this commit - and thus the dereference in things like:
pgd_none(*pgd)
wouldn't even be visible to beyond the preprocessor step.
