Re: Regression with kernel 6.3 "kernel BUG at include/linux/swapops.h:472!"

[Helge Deller <deller@gmx.de>]

On 5/13/23 15:21, David Hildenbrand wrote:
>> Yes, makes sense.
>>
>>> [1] Total is 1 Gbyte, and running
>>>      | dd if=/dev/zero bs=896M count=1 | pv --rate-limit=1k >/dev/null
>>>      might not be the best style but does the trick: Wait for pv to
>>>      count up to a minute, then ^C it. If the host is still okay after
>>>      that, it's considered "good".
>>
>> Thanks for bisecting and coming up with a testcase!
>> The attached patch survives for me on my C3000 with 2GB RAM with this test:
>>     dd if=/dev/zero bs=1896M count=1 | pv
>> (well, the OOM-killer might jump in, but even that is survived).
>>
>> Could you try the patch below?
>
> Thanks for debugging! :)
>>
>> [PATCH] parisc: Fix encoding of swp_entry due to added SWP_EXCLUSIVE flag
>>
>> Fix the __swp_offset() and __swp_entry() macros due to commit 6d239fc78c0b
>> ("parisc/mm: support __HAVE_ARCH_PTE_SWP_EXCLUSIVE") which introduced the
>> SWP_EXCLUSIVE flag by reusing the _PAGE_ACCESSED flag.
>>
>> Reported-by: Christoph Biedl <linux-kernel.bfrz@manchmal.in-ulm.de>
>> Fixes: 6d239fc78c0b ("parisc/mm: support __HAVE_ARCH_PTE_SWP_EXCLUSIVE")
>>
>> diff --git a/arch/parisc/include/asm/pgtable.h b/arch/parisc/include/asm/pgtable.h
>> index e2950f5db7c9..522846be54b7 100644
>> --- a/arch/parisc/include/asm/pgtable.h
>> +++ b/arch/parisc/include/asm/pgtable.h
>> @@ -413,12 +413,12 @@ extern void paging_init (void);
>>    *   For the 64bit version, the offset is extended by 32bit.
>>    */
>>   #define __swp_type(x)                     ((x).val & 0x1f)
>> -#define __swp_offset(x)                   ( (((x).val >> 6) &  0x7) | \
>> -                      (((x).val >> 8) & ~0x7) )
>> +#define __swp_offset(x)                   ( (((x).val >> 5) &  0x7) | \
>> +                      (((x).val >> 10) << 3) )
>>   #define __swp_entry(type, offset)         ((swp_entry_t) { \
>>                           ((type) & 0x1f) | \
>> -                        ((offset &  0x7) << 6) | \
>> -                        ((offset & ~0x7) << 8) })
>> +                        ((offset &  0x7) << 5) | \
>> +                        ((offset & ~0x7) << 7) })
>>   #define __pte_to_swp_entry(pte)        ((swp_entry_t) { pte_val(pte) })
>>   #define __swp_entry_to_pte(x)        ((pte_t) { (x).val })
>
> This fix makes it work like the layout I documented.

Yes, and your layout looks good for me.

> What I originally tried doing was reusing one of the spare bits instead of reworking
> the layout. Apparently, I got the old layout wrong. :(

Don't worry! Your patch harmonizes parisc to the other platforms, which is good.

> So if I understood the layout right this time, maybe we can just use one of the two
> spare bits: _PAGE_HUGE (or alternatively, _PAGE_DIRTY_BIT)?

Yes, or keep what you suggested.

What I don't understand yet is the original code:
#define __swp_type(x)                     ((x).val & 0x1f)
#define __swp_offset(x)                   ( (((x).val >> 6) &  0x7) | \
                                           (((x).val >> 8) & ~0x7) )
#define __swp_entry(type, offset)         ((swp_entry_t) { (type) | \
                                             ((offset &  0x7) << 6) | \
                                             ((offset & ~0x7) << 8) })

Don't we loose one of the offset bits?
Mask 0x7 is 3 bits, but we shift by 6 and 8 (=2 bits difference), so I believe the second shift should be 9.
If it would be 9, then no &0x07 is needed and only one shift would be sufficient.

I don't know much in the swap pte area, but isn't the previous original code wrong?
Which bits of the swp_entry are used where?

Helge

> diff --git a/arch/parisc/include/asm/pgtable.h b/arch/parisc/include/asm/pgtable.h
> index e2950f5db7c9..ee9f08cd5938 100644
> --- a/arch/parisc/include/asm/pgtable.h
> +++ b/arch/parisc/include/asm/pgtable.h
> @@ -218,8 +218,8 @@ extern void __update_cache(pte_t pte);
>   #define _PAGE_KERNEL_RWX       (_PAGE_KERNEL_EXEC | _PAGE_WRITE)
>   #define _PAGE_KERNEL           (_PAGE_KERNEL_RO | _PAGE_WRITE)
>
> -/* We borrow bit 23 to store the exclusive marker in swap PTEs. */
> -#define _PAGE_SWP_EXCLUSIVE    _PAGE_ACCESSED
> +/* We borrow bit 21 to store the exclusive marker in swap PTEs. */
> +#define _PAGE_SWP_EXCLUSIVE    _PAGE_HUGE
>
>   /* The pgd/pmd contains a ptr (in phys addr space); since all pgds/pmds
>    * are page-aligned, we don't care about the PAGE_OFFSET bits, except
> @@ -405,7 +405,7 @@ extern void paging_init (void);
>    *
>    *                         1 1 1 1 1 1 1 1 1 2 2 2 2 2 2 2 2 2 2 3 3
>    *   0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
> - *   <---------------- offset -----------------> P E <ofs> < type ->
> + *   <---------------- offset ---------------> E P <ofs> 0 < type ->
>    *
>    *   E is the exclusive marker that is not stored in swap entries.
>    *   _PAGE_PRESENT (P) must be 0.
> diff --git a/lib/maple_tree.c b/lib/maple_tree.c
> index 110a36479dce..7510db355f48 100644
>
>