[PATCH 0/1] alpha: Fix pte_swp_exclusive on alpha

[PATCH 0/1] alpha: Fix pte_swp_exclusive on alpha

[Magnus Lindholm]

First noticed that swapoff fails to unmount/free a previously used swap
file or partition on alpha. swapoff fails to writeback exclusive swap
pages and gets stuck in an infinite loop trying to do so. This was
introduced in commit a172d5128706028ac07b8db709728379ecc72f6e. This
patch provides a fix for pte_swp_exclusive. Verified on UP2000 alpha.

[PATCH 1/1] alpha: Fix pte_swp_exclusive on alpha

[Magnus Lindholm]

Function pte_swp_exclusive() checks if _PAGE_SWP_EXCLUSIVE bit is set in
PTE but returns lower 32-bits only. Shift bits right by 32 to return upper
32-bits of PTE which contain the _PAGE_SWP_EXCLUSIVE bit. On alpha this is
bit 39 but on most other architectures this bit already resides somewhere
in the first 32-bits and hence a shift is not necessary on those archs.

---
 arch/alpha/include/asm/pgtable.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/alpha/include/asm/pgtable.h b/arch/alpha/include/asm/pgtable.h
index 02e8817a8921..a96d652629fd 100644
--- a/arch/alpha/include/asm/pgtable.h
+++ b/arch/alpha/include/asm/pgtable.h
@@ -336,7 +336,7 @@ extern inline pte_t mk_swap_pte(unsigned long type, unsigned long offset)
 
 static inline int pte_swp_exclusive(pte_t pte)
 {
-	return pte_val(pte) & _PAGE_SWP_EXCLUSIVE;
+	return (pte_val(pte) & _PAGE_SWP_EXCLUSIVE)>>32;
 }
 
 static inline pte_t pte_swp_mkexclusive(pte_t pte)
-- 
2.48.1

Re: [PATCH 1/1] alpha: Fix pte_swp_exclusive on alpha

[Al Viro]

On Sun, Feb 16, 2025 at 06:04:53PM +0100, Magnus Lindholm wrote:
> Function pte_swp_exclusive() checks if _PAGE_SWP_EXCLUSIVE bit is set in
> PTE but returns lower 32-bits only. Shift bits right by 32 to return upper
> 32-bits of PTE which contain the _PAGE_SWP_EXCLUSIVE bit. On alpha this is
> bit 39 but on most other architectures this bit already resides somewhere
> in the first 32-bits and hence a shift is not necessary on those archs.

Just make it return bool and be done with that - all users are either
if (pte_swp_exclusive(...)) or if (!pte_swp_exclusive(...)) or assignments
to bool variable.

No need to shift anything - compiler probably will figure out that
	if ((int)((x & (1UL<<39)>>32)))
is equivalent to
	if (x & (1UL<<39))
but why bother with such convolutions in the first place?

Seriously, just make it

bool pte_swp_exclusive(pte_t pte)
{
	return pte_val(pte) & _PAGE_SWP_EXCLUSIVE;
}

and that's it - conversion from arithmetical types to bool will do the right thing.

Re: [PATCH 1/1] alpha: Fix pte_swp_exclusive on alpha

[Al Viro]

On Sun, Feb 16, 2025 at 05:17:41PM +0000, Al Viro wrote:
> On Sun, Feb 16, 2025 at 06:04:53PM +0100, Magnus Lindholm wrote:
> > Function pte_swp_exclusive() checks if _PAGE_SWP_EXCLUSIVE bit is set in
> > PTE but returns lower 32-bits only. Shift bits right by 32 to return upper
> > 32-bits of PTE which contain the _PAGE_SWP_EXCLUSIVE bit. On alpha this is
> > bit 39 but on most other architectures this bit already resides somewhere
> > in the first 32-bits and hence a shift is not necessary on those archs.
> 
> Just make it return bool and be done with that - all users are either
> if (pte_swp_exclusive(...)) or if (!pte_swp_exclusive(...)) or assignments
> to bool variable.
> 
> No need to shift anything - compiler probably will figure out that
> 	if ((int)((x & (1UL<<39)>>32)))
> is equivalent to
> 	if (x & (1UL<<39))
> but why bother with such convolutions in the first place?
> 
> Seriously, just make it
> 
> bool pte_swp_exclusive(pte_t pte)
> {
> 	return pte_val(pte) & _PAGE_SWP_EXCLUSIVE;
> }
> 
> and that's it - conversion from arithmetical types to bool will do the right thing.

FWIW,

sed -i -e '/pte_swp_exclusive/s/int/bool/' `git grep -l pte_swp_exclusive arch/`

will do the right thing - check and you'll see.

Re: [PATCH 1/1] alpha: Fix pte_swp_exclusive on alpha

[Al Viro]

On Sun, Feb 16, 2025 at 05:17:41PM +0000, Al Viro wrote:
> On Sun, Feb 16, 2025 at 06:04:53PM +0100, Magnus Lindholm wrote:
> > Function pte_swp_exclusive() checks if _PAGE_SWP_EXCLUSIVE bit is set in
> > PTE but returns lower 32-bits only. Shift bits right by 32 to return upper
> > 32-bits of PTE which contain the _PAGE_SWP_EXCLUSIVE bit. On alpha this is
> > bit 39 but on most other architectures this bit already resides somewhere
> > in the first 32-bits and hence a shift is not necessary on those archs.
> 
> Just make it return bool and be done with that - all users are either
> if (pte_swp_exclusive(...)) or if (!pte_swp_exclusive(...)) or assignments
> to bool variable.
> 
> No need to shift anything - compiler probably will figure out that
> 	if ((int)((x & (1UL<<39)>>32)))

Sorry,

	if ((int)(((x & (1UL<<39))>>32))

> is equivalent to
> 	if (x & (1UL<<39))
> but why bother with such convolutions in the first place?
> 
> Seriously, just make it
> 
> bool pte_swp_exclusive(pte_t pte)
> {
> 	return pte_val(pte) & _PAGE_SWP_EXCLUSIVE;
> }
> 
> and that's it - conversion from arithmetical types to bool will do the right thing.
> 

Re: [PATCH 1/1] alpha: Fix pte_swp_exclusive on alpha

[Magnus Lindholm]

Hi,

Making pte_swp_exclusive return bool is a neat solution! As Al pointed out,
it will better reflect how pte_swp_exclusive is actually used in the code.
I assume we would want this for all architectures implementing
pte_swp_exclusive? This implies that this change will have a wider
impact and not be an alpha specific fix.
I can prepare and post a v2 of this patch using this approach.

Magnus

On Sun, Feb 16, 2025 at 6:17 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Sun, Feb 16, 2025 at 06:04:53PM +0100, Magnus Lindholm wrote:
> > Function pte_swp_exclusive() checks if _PAGE_SWP_EXCLUSIVE bit is set in
> > PTE but returns lower 32-bits only. Shift bits right by 32 to return upper
> > 32-bits of PTE which contain the _PAGE_SWP_EXCLUSIVE bit. On alpha this is
> > bit 39 but on most other architectures this bit already resides somewhere
> > in the first 32-bits and hence a shift is not necessary on those archs.
>
> Just make it return bool and be done with that - all users are either
> if (pte_swp_exclusive(...)) or if (!pte_swp_exclusive(...)) or assignments
> to bool variable.
>
> No need to shift anything - compiler probably will figure out that
>         if ((int)((x & (1UL<<39)>>32)))
> is equivalent to
>         if (x & (1UL<<39))
> but why bother with such convolutions in the first place?
>
> Seriously, just make it
>
> bool pte_swp_exclusive(pte_t pte)
> {
>         return pte_val(pte) & _PAGE_SWP_EXCLUSIVE;
> }
>
> and that's it - conversion from arithmetical types to bool will do the right thing.