Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <0b122e73-22c3-e3a3-3aaf-d2b8361c196d@redhat.com>
Date:   Tue, 6 Dec 2022 10:16:08 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.0
Content-Language: en-US
To:     Peter Xu <peterx@redhat.com>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        Andrew Morton <akpm@linux-foundation.org>,
        Yang Shi <shy828301@gmail.com>,
        Hugh Dickins <hughd@google.com>,
        Andrea Arcangeli <aarcange@redhat.com>
References: <20221205150857.167583-1-david@redhat.com> <Y45u+0c4Hu2snEO2@x1n>
From:   David Hildenbrand <david@redhat.com>
Organization: Red Hat
Subject: Re: [PATCH v1] mm/swap: fix SWP_PFN_BITS with
 CONFIG_PHYS_ADDR_T_64BIT on 32bit
In-Reply-To: <Y45u+0c4Hu2snEO2@x1n>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

> 
> 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