Re: [PATCH] mm/mempolicy: Fix redundant check and refine lock protection scope in init_nodemask_of_mempolicy

[David Hildenbrand <david@redhat.com>]

On 21.11.24 13:24, Zhen Ni wrote:
> 1.Removing redundant checks for current->mempolicy, with a more concise
> check order.
> 
> 2.Using READ_ONCE(current->mempolicy) for safe, single access to
> current->mempolicy to prevent potential race conditions.
> 
> 3.Optimizing the scope of task_lock(current). The lock now only protects
> the critical section where mempolicy is accessed, reducing the duration
> the lock is held. This enhances performance by limiting the scope of the
> lock to only what is necessary.
> 
> Signed-off-by: Zhen Ni <zhen.ni@easystack.cn>
> ---
>   mm/mempolicy.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index b646fab3e45e..8bff8830b7e6 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -2132,11 +2132,14 @@ bool init_nodemask_of_mempolicy(nodemask_t *mask)
>   {
>   	struct mempolicy *mempolicy;
>   
> -	if (!(mask && current->mempolicy))
 > +	if (!mask)> +		return false;
> +
> +	mempolicy = READ_ONCE(current->mempolicy);
> +	if (!mempolicy)
>   		return false;
>   
>   	task_lock(current);
> -	mempolicy = current->mempolicy;
>   	switch (mempolicy->mode) {
>   	case MPOL_PREFERRED:
>   	case MPOL_PREFERRED_MANY:

(a) current->mempolicy can only change under the task_lock(), see 
do_set_mempolicy().

(b) current->mempolicy cannot usually NULL once it was none-NULL. There 
are two exceptions:

copy_process() might set it to NULL when creating the process, before 
the task gets scheduled.

do_exit() might set it to NULL via mpol_put_task_policy().

I don't think init_nodemask_of_mempolicy() can be called while our task 
is exiting ...


Now, if you read current->mempolicy outside of task_lock() to the 
*dereference* it when it might already have changed+was freed, that's a 
BUG you're introducing.


So, regarding your (1): which redundant "check" ? There is a single "not 
NULL" chech. Reagrding your (2), I think you are introducing a BUG and 
regrding your (3) please share performance numbers, or realize that youa 
re making something up ;)


The only improvement we could make here is converting:
	if (!(mask && current->mempolicy))
into a more readable
	if (!mask || !current->mapping)

And maybe adding a comment that while we can race with someone changing 
current->mapping, we cannot race with someone changing it to NULL.

-- 
Cheers,

David / dhildenb