Re: [PATCH v1] mm/swap: fix SWP_PFN_BITS with CONFIG_PHYS_ADDR_T_64BIT on 32bit

[David Hildenbrand]

> Thanks for debugging this one.

Sure!

> > ---
> >
> > This makes my x86 PAE case work as expected again. Only cross compiled
> > on other architectures.
>
> IIUC it's not about PAE but !SPARSEMEM, as PAE actually has it defined when
> with sparsemem:
>
> #ifdef CONFIG_X86_32
> # ifdef CONFIG_X86_PAE
> #  define SECTION_SIZE_BITS	29
> #  define MAX_PHYSMEM_BITS	36
> # else
> #  define SECTION_SIZE_BITS	26
> #  define MAX_PHYSMEM_BITS	32
> # endif
> #else /* CONFIG_X86_32 */
> # define SECTION_SIZE_BITS	27 /* matt - 128 is convenient right now */
> # define MAX_PHYSMEM_BITS	(pgtable_l5_enabled() ? 52 : 46)
> #endif

Indeed, I'll extend the description accordingly.

> One trivial comment below.
>
> > ---
> >   include/linux/swapops.h | 8 +++++---
> >   1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/linux/swapops.h b/include/linux/swapops.h
> > index 86b95ccb81bb..4bb7a20f3fa5 100644
> > --- a/include/linux/swapops.h
> > +++ b/include/linux/swapops.h
> > @@ -32,11 +32,13 @@
> >    * store PFN, we only need SWP_PFN_BITS bits.  Each of the pfn swap entries
> >    * can use the extra bits to store other information besides PFN.
> >    */
> > -#ifdef MAX_PHYSMEM_BITS
> > +#if defined(MAX_PHYSMEM_BITS)
> >   #define SWP_PFN_BITS			(MAX_PHYSMEM_BITS - PAGE_SHIFT)
> > -#else  /* MAX_PHYSMEM_BITS */
> > +#elif !defined(CONFIG_64BIT) && defined(CONFIG_PHYS_ADDR_T_64BIT)
> > +#define SWP_PFN_BITS			SWP_TYPE_SHIFT
>
> Can we add a comment showing where SWP_TYPE_SHIFT comes from?  It should be
> a min value comes from either the limitation of phys address width, or from
> definition of swp_entry_t (which is unsigned long).
>
> Or I'd rather make this then the code explains better on itself, and the
> change should be smaller too:
>
>   #ifdef MAX_PHYSMEM_BITS
>   #define SWP_PFN_BITS                   (MAX_PHYSMEM_BITS - PAGE_SHIFT)
>   #else  /* MAX_PHYSMEM_BITS */
> -#define SWP_PFN_BITS                   (BITS_PER_LONG - PAGE_SHIFT)
> +#define SWP_PFN_BITS                   MIN((sizeof(phys_addr_t) * 8) - \
> +                                           PAGE_SHIFT, SWP_TYPE_SHIFT)
>   #endif /* MAX_PHYSMEM_BITS */
>   #define SWP_PFN_MASK                   (BIT(SWP_PFN_BITS) - 1)
>                                   
> What do you think?

Sure, if we can make the compiler happy:

./include/linux/swapops.h: In function 'swp_offset_pfn':
./include/linux/swapops.h:38:41: error: implicit declaration of function 
'MIN' [-Werror=implicit-function-declaration]
   38 | #define SWP_PFN_BITS 
MIN((sizeof(phys_addr_t) * 8) - \
      |                                         ^~~
./include/vdso/bits.h:7:44: note: in definition of macro 'BIT'
    7 | #define BIT(nr)                 (UL(1) << (nr))
      |                                            ^~
./include/linux/swapops.h:41:46: note: in expansion of macro 'SWP_PFN_BITS'
   41 | #define SWP_PFN_MASK                    (BIT(SWP_PFN_BITS) - 1)
      |                                              ^~~~~~~~~~~~
./include/linux/swapops.h:119:36: note: in expansion of macro 'SWP_PFN_MASK'
  119 |         return swp_offset(entry) & SWP_PFN_MASK;
      |                                    ^~~~~~~~~~~~


using "min" instead gives plenty of warnings. min_t() seems to work:

diff --git a/include/linux/swapops.h b/include/linux/swapops.h
index 27ade4f22abb..088f25aa0e98 100644
--- a/include/linux/swapops.h
+++ b/include/linux/swapops.h
@@ -34,8 +34,10 @@
  */
 #ifdef MAX_PHYSMEM_BITS
 #define SWP_PFN_BITS                   (MAX_PHYSMEM_BITS - PAGE_SHIFT)
-#else  /* MAX_PHYSMEM_BITS */
-#define SWP_PFN_BITS                   (BITS_PER_LONG - PAGE_SHIFT)
+#else /* MAX_PHYSMEM_BITS */
+#define SWP_PFN_BITS                   min_t(phys_addr_t, \
+                                             (sizeof(phys_addr_t) * 8) - \
+                                             PAGE_SHIFT, SWP_TYPE_SHIFT)
 #endif /* MAX_PHYSMEM_BITS */
 #define SWP_PFN_MASK                   (BIT(SWP_PFN_BITS) - 1)



I'm currently cross-compiling that and will give it a churn.

Thanks!

--
Thanks,

David / dhildenb