On 2013/7/11 17:25, Michal Hocko wrote:
> On Thu 11-07-13 16:43:08, Li Zefan wrote:
> [...]
>>> @@ -248,6 +256,9 @@ void vmpressure(gfp_t gfp, struct mem_cgroup *memcg,
>>>
>>> if (scanned < vmpressure_win || work_pending(&vmpr->work))
>>> return;
>>> +
>>> + /* Reference will be released in vmpressure_work_fn */
>>> + vmpressure_pin_memcg(vmpr);
>>> schedule_work(&vmpr->work);
>>
>> Looks like a work can be queued right after the above work_pending()
>> returns 0, then we should do this:
>>
>> if (schedule_work(&vmpr->work))
>> vmpressure_pin_memcg(vmpr);
>
> But isn't this racy and the following would be safer?
Absolutely! Looks good to me.
> ---
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 0f2c909..8329262 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -6204,15 +6204,15 @@ struct mem_cgroup *vmpressure_to_mem_cgroup(struct vmpressure *vmpr)
> void vmpressure_pin_memcg(struct vmpressure *vmpr)
> {
> struct mem_cgroup *memcg = vmpressure_to_mem_cgroup(vmpr);
> - if (memcg != root_mem_cgroup)
> - css_get(&memcg->css);
> +
> + css_get(&memcg->css);
> }
>
> void vmpressure_unpin_memcg(struct vmpressure *vmpr)
> {
> struct mem_cgroup *memcg = vmpressure_to_mem_cgroup(vmpr);
> - if (memcg != root_mem_cgroup)
> - css_put(&memcg->css);
> +
> + css_put(&memcg->css);
> }
>
> static void __init mem_cgroup_soft_limit_tree_init(void)
> diff --git a/mm/vmpressure.c b/mm/vmpressure.c
> index 89865e5..3a955412 100644
> --- a/mm/vmpressure.c
> +++ b/mm/vmpressure.c
> @@ -259,7 +259,8 @@ void vmpressure(gfp_t gfp, struct mem_cgroup *memcg,
>
> /* Reference will be released in vmpressure_work_fn */
> vmpressure_pin_memcg(vmpr);
> - schedule_work(&vmpr->work);
> + if (!schedule_work(&vmpr->work))
> + vmpressure_unpin_memcg(vmpr);
> }
>
> /**
>
--
To unsubscribe from this list: send the line "unsubscribe cgroups" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
