Re: [PATCH 1/2] memcg: mm_update_next_owner: kill the "retry" logic

From: Michal Hocko
Date: Thu Jun 27 2024 - 05:02:31 EST


On Thu 27-06-24 10:29:42, Oleg Nesterov wrote:
> Michal, thanks for looking at this,
>
> On 06/27, Michal Hocko wrote:
> >
> > On Wed 26-06-24 17:29:24, Oleg Nesterov wrote:
> > > @@ -446,7 +463,6 @@ void mm_update_next_owner(struct mm_struct *mm)
> > > {
> > > struct task_struct *c, *g, *p = current;
> > >
> > > -retry:
> > > /*
> > > * If the exiting or execing task is not the owner, it's
> > > * someone else's problem.
> > > @@ -468,16 +484,16 @@ void mm_update_next_owner(struct mm_struct *mm)
> > > * Search in the children
> > > */
> > > list_for_each_entry(c, &p->children, sibling) {
> > > - if (c->mm == mm)
> > > - goto assign_new_owner;
> > > + if (c->mm == mm && try_to_set_owner(c, mm))
> > > + goto ret;
> >
> > You need to unlock tasklist_lock, right? Same for other goto ret.
>
> No. From the patch
>
> +/* drops tasklist_lock if succeeds */
> +static bool try_to_set_owner(struct task_struct *tsk, struct mm_struct *mm)
> +{
> + bool ret = false;
> +
> + task_lock(tsk);
> + if (likely(tsk->mm == mm)) {
> + /* tsk can't pass exit_mm/exec_mmap and exit */
> + read_unlock(&tasklist_lock);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> try_to_set_owner() drops tasklist right after it verifies that
> tsk->mm == mm under task_lock().

Yes, I am blind and the commend even explains that. I am not a propoment
of schemes where locks are released in a different function. But the
overall simplification here is worth that.

Acked-by: Michal Hocko <mhocko@xxxxxxxx>
--
Michal Hocko
SUSE Labs