| Message ID | 20190601074959.14036-9-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [01/16] uaccess: add untagged_addr definition for other arches | expand |
Both sparc64 and sh had this pattern, but now that I look at it more closely, I think your version is wrong, or at least nonoptimal. On Sat, Jun 1, 2019 at 12:50 AM Christoph Hellwig <hch@lst.de> wrote: > > +#define pgd_page(pgd) virt_to_page(__va(pgd_val(pgd))) Going through the virtual address is potentially very inefficient, and might in some cases just be wrong (ie it's definitely wrong for HIGHMEM style setups). It would likely be much better to go through the physical address and use "pfn_to_page()". I realize that we don't have a "pgd to physical", but neither do we really have a "pgd to virtual", and your "__va(pgd_val(x))" thing is not at allguaranteed to work. You're basically assuming that "pgd_val(x)" is the physical address, which is likely not entirely incorrect, but it should be checked by the architecture people. The pgd value could easily have high bits with meaning, which would also potentially screw up the __va(x) model. So I thgink this would be better done with #define pgd_page(pgd) pfn_to_page(pgd_pfn(pgd)) where that "pgd_pfn()" would need to be a new (but likely very trivial) function. That's what we do for pte_pfn(). IOW, it would likely end up something like #define pgd_to_pfn(pgd) (pgd_val(x) >> PFN_PGD_SHIFT) David? Linus
On Sat, Jun 01, 2019 at 09:28:54AM -0700, Linus Torvalds wrote: > Both sparc64 and sh had this pattern, but now that I look at it more > closely, I think your version is wrong, or at least nonoptimal. I bet it is. Then again these symbols are just required for the code to compile, as neither sparc64 nor sh actually use the particular variant of huge pages we need it for. Then again even actually dead code should better be not too buggy if it isn't just a stub. > So I thgink this would be better done with > > #define pgd_page(pgd) pfn_to_page(pgd_pfn(pgd)) > > where that "pgd_pfn()" would need to be a new (but likely very > trivial) function. That's what we do for pte_pfn(). > > IOW, it would likely end up something like > > #define pgd_to_pfn(pgd) (pgd_val(x) >> PFN_PGD_SHIFT) True. I guess it would be best if we could get most if not all architectures to use common versions of these macros so that we have the issue settled once.
diff --git a/arch/sparc/include/asm/pgtable_64.h b/arch/sparc/include/asm/pgtable_64.h index 22500c3be7a9..dcf970e82262 100644 --- a/arch/sparc/include/asm/pgtable_64.h +++ b/arch/sparc/include/asm/pgtable_64.h @@ -861,6 +861,7 @@ static inline unsigned long pud_page_vaddr(pud_t pud) #define pud_clear(pudp) (pud_val(*(pudp)) = 0UL) #define pgd_page_vaddr(pgd) \ ((unsigned long) __va(pgd_val(pgd))) +#define pgd_page(pgd) virt_to_page(__va(pgd_val(pgd))) #define pgd_present(pgd) (pgd_val(pgd) != 0U) #define pgd_clear(pgdp) (pgd_val(*(pgdp)) = 0UL)
sparc64 only had pgd_page_vaddr, but not pgd_page. Signed-off-by: Christoph Hellwig <hch@lst.de> --- arch/sparc/include/asm/pgtable_64.h | 1 + 1 file changed, 1 insertion(+)