Re: [PATCH v2] mm/memcontrol: fix stuck FLUSHING_CACHED_CHARGE bit on isolated cpus

[Michal Hocko]

On Fri 28-08-26 13:50:36, Rik van Riel wrote:
> When drain_all_stock() sets FLUSHING_CACHED_CHARGE before checking
> isolation, schedule_drain_work() can drop the work in a separate RCU
> critical section, and housekeeping_update()'s synchronize_rcu() can race
> that second check, leaving the flag set.
> 
> drain_local_stock() only clears the bit for work that ran, so the flag
> remains set and the stock is never drained again.
> 
> Have schedule_drain_work() return whether the work was queued, and clear
> FLUSHING_CACHED_CHARGE in drain_all_stock() when the remote CPU is
> isolated, so future drains can retry.
> 
> Fixes: 6a792697a53a ("memcg: do not drain charge pcp caches on remote isolated cpus")
> Cc: stable@xxxxxxxxxxxxxxx
> Suggested-by: Michal Hocko <mhocko@xxxxxxxx>
> Suggested-by: Shakeel Butt <shakeel.butt@xxxxxxxxx>
> Signed-off-by: Rik van Riel <riel@xxxxxxxxxxx>

Acked-by: Michal Hocko <mhocko@xxxxxxxx>

Thanks

> ---
> v2: use the approach suggested by Michal and Shakeel
> 
> Link: https://lore.kernel.org/all/cover.1787890328.git.riel@xxxxxxxxxxx/
> 
>  mm/memcontrol.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 1271d390b617..309398e943ca 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2306,7 +2306,7 @@ static bool is_memcg_drain_needed(struct memcg_stock_pcp *stock,
>  	return flush;
>  }
>  
> -static void schedule_drain_work(int cpu, struct work_struct *work)
> +static bool schedule_drain_work(int cpu, struct work_struct *work)
>  {
>  	/*
>  	 * Protect housekeeping cpumask read and work enqueue together
> @@ -2315,8 +2315,11 @@ static void schedule_drain_work(int cpu, struct work_struct *work)
>  	 * pending work on newly isolated CPUs.
>  	 */
>  	guard(rcu)();
> -	if (!cpu_is_isolated(cpu))
> -		queue_work_on(cpu, memcg_wq, work);
> +	if (cpu_is_isolated(cpu))
> +		return false;
> +
> +	queue_work_on(cpu, memcg_wq, work);
> +	return true;
>  }
>  
>  /*
> @@ -2348,8 +2351,9 @@ void drain_all_stock(struct mem_cgroup *root_memcg)
>  				      &memcg_st->flags)) {
>  			if (cpu == curcpu)
>  				drain_local_memcg_stock(&memcg_st->work);
> -			else
> -				schedule_drain_work(cpu, &memcg_st->work);
> +			else if (!schedule_drain_work(cpu, &memcg_st->work))
> +				clear_bit(FLUSHING_CACHED_CHARGE,
> +					  &memcg_st->flags);
>  		}
>  
>  		if (!test_bit(FLUSHING_CACHED_CHARGE, &obj_st->flags) &&
> @@ -2358,8 +2362,9 @@ void drain_all_stock(struct mem_cgroup *root_memcg)
>  				      &obj_st->flags)) {
>  			if (cpu == curcpu)
>  				drain_local_obj_stock(&obj_st->work);
> -			else
> -				schedule_drain_work(cpu, &obj_st->work);
> +			else if (!schedule_drain_work(cpu, &obj_st->work))
> +				clear_bit(FLUSHING_CACHED_CHARGE,
> +					  &obj_st->flags);
>  		}
>  	}
>  	migrate_enable();
> -- 
> 2.55.0
> 
> 

-- 
Michal Hocko
SUSE Labs