Re: [PATCH v3 5/5] mm/memcg: Protect memcg_stock with a local_lock_t

[Michal Hocko <mhocko-IBi9RG/b67k@public.gmane.org>]

On Thu 17-02-22 10:48:02, Sebastian Andrzej Siewior wrote:
[...]
> @@ -2266,7 +2273,6 @@ static void drain_all_stock(struct mem_cgroup *root_memcg)
>  	 * as well as workers from this path always operate on the local
>  	 * per-cpu data. CPU up doesn't touch memcg_stock at all.
>  	 */
> -	curcpu = get_cpu();

Could you make this a separate patch?

>  	for_each_online_cpu(cpu) {
>  		struct memcg_stock_pcp *stock = &per_cpu(memcg_stock, cpu);
>  		struct mem_cgroup *memcg;
> @@ -2282,14 +2288,9 @@ static void drain_all_stock(struct mem_cgroup *root_memcg)
>  		rcu_read_unlock();
>  
>  		if (flush &&
> -		    !test_and_set_bit(FLUSHING_CACHED_CHARGE, &stock->flags)) {
> -			if (cpu == curcpu)
> -				drain_local_stock(&stock->work);
> -			else
> -				schedule_work_on(cpu, &stock->work);
> -		}
> +		    !test_and_set_bit(FLUSHING_CACHED_CHARGE, &stock->flags))
> +			schedule_work_on(cpu, &stock->work);

Maybe I am missing but on !PREEMPT kernels there is nothing really
guaranteeing that the worker runs so there should be cond_resched after
the mutex is unlocked. I do not think we want to rely on callers to be
aware of this subtlety.

An alternative would be to split out __drain_local_stock which doesn't
do local_lock.

-- 
Michal Hocko
SUSE Labs