Re: [PATCH RFC] m68k: skip kernel premption if interrupts were disabled

[Michael Schmitz <schmitzmic@gmail.com>]

Hi Finn,

thanks - I'm pretty sure tried that early on but botched it by excessive 
locking (i.e., keeping preemption disabled when calling 
get_zeroed_page()!).

So I don't think you do use too much locking here.

I'm running stress tests for a while now, without any trouble so far. 
Need to add a few other stressors back in, and repeat all that on a 
slower ARAnyM instance but I'm quite confident you found the solution.

Geert: with this data race fixed, it does appear my RFC patch is no 
longer needed. Finn or I probably ought to prepare a new RFC patch to go 
on top of your preemption patch. There is no commit ID to use in a 
Fixes: tag for that one, correct?

Cheers,

     Michael

On 26/03/24 22:58, Finn Thain wrote:
> On Tue, 26 Mar 2024, Michael Schmitz wrote:
>
>> see below (hopefully not whitespace-damaged).
>>
> Following your approach, I added mutual exclusion to the other candidate
> in the same file, namely get_pointer_table(). Together with your patch,
> that seems to be sufficient to make stress-ng happy. I don't know the real
> extent of the data race here so I've probably used too much locking in
> this experiment (?)
>
> diff --git a/arch/m68k/mm/motorola.c b/arch/m68k/mm/motorola.c
> index 068ad7c9bb5c..d1ad55ee1b60 100644
> --- a/arch/m68k/mm/motorola.c
> +++ b/arch/m68k/mm/motorola.c
> @@ -140,10 +140,14 @@ void __init init_pointer_table(void *table, int type)
>   
>   void *get_pointer_table(int type)
>   {
> -	ptable_desc *dp = ptable_list[type].next;
> -	unsigned int mask = list_empty(&ptable_list[type]) ? 0 : PD_MARKBITS(dp);
> +	ptable_desc *dp;
> +	unsigned int mask;
>   	unsigned int tmp, off;
>   
> +	preempt_disable();
> +
> +	dp = ptable_list[type].next;
> +	mask = list_empty(&ptable_list[type]) ? 0 : PD_MARKBITS(dp);
>   	/*
>   	 * For a pointer table for a user process address space, a
>   	 * table is taken from a page allocated for the purpose.  Each
> @@ -154,8 +158,13 @@ void *get_pointer_table(int type)
>   		void *page;
>   		ptable_desc *new;
>   
> -		if (!(page = (void *)get_zeroed_page(GFP_KERNEL)))
> +		sched_preempt_enable_no_resched();
> +
> +		if (!(page = (void *)get_zeroed_page(GFP_KERNEL))) {
>   			return NULL;
> +		}
> +
> +		preempt_disable();
>   
>   		if (type == TABLE_PTE) {
>   			/*
> @@ -171,6 +180,7 @@ void *get_pointer_table(int type)
>   		PD_MARKBITS(new) = ptable_mask(type) - 1;
>   		list_add_tail(new, dp);
>   
> +		sched_preempt_enable_no_resched();
>   		return (pmd_t *)page;
>   	}
>   
> @@ -181,6 +191,7 @@ void *get_pointer_table(int type)
>   		/* move to end of list */
>   		list_move_tail(dp, &ptable_list[type]);
>   	}
> +	sched_preempt_enable_no_resched();
>   	return page_address(PD_PAGE(dp)) + off;
>   }
>