Re: [PATCH 0/1] memcg: kill start_kernel()->mm_init_owner(init_mm)

[Michal Hocko <mhocko@suse.cz>]

On Fri 18-04-14 21:12:59, Oleg Nesterov wrote:
> On 04/18, Oleg Nesterov wrote:
> >
> > OTOH, mm->owner is used by mm/memcontrol.c, so perhaps the prefix is fine?
> >
> > I do not even understand why do we have CONFIG_MM_OWNER, perhaps it should
> > die?
> 
> Seriously what do you think about the patch below? Afaics CONFIG_MM_OWNER
> is pointless.

Yes, I do not see any reason for a separate config when it has a single
user which cannot work without it. So feel free to add my
Acked-by: Michal Hocko <mhocko@suse.cz>

to the patch with your s-o-b and a simple changelog explaining there the
config option didn't have any user outside of MEMCG which cannot work
without it.

> And I just noticed start_kernel()->mm_init_owner(). It looks simply wrong
> even if this is harmless, or I missed something?
> 
> Could you review 1/1 as well?
> 
> Oleg.
> ---
> 
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 8967e20..de16272 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -406,7 +406,7 @@ struct mm_struct {
>  	spinlock_t			ioctx_lock;
>  	struct kioctx_table __rcu	*ioctx_table;
>  #endif
> -#ifdef CONFIG_MM_OWNER
> +#ifdef CONFIG_MEMCG
>  	/*
>  	 * "owner" points to a task that is regarded as the canonical
>  	 * user/owner of this mm. All of the following must be true in
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 82fe3da..570a325 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -2947,7 +2947,7 @@ static inline void inc_syscw(struct task_struct *tsk)
>  #define TASK_SIZE_OF(tsk)	TASK_SIZE
>  #endif
>  
> -#ifdef CONFIG_MM_OWNER
> +#ifdef CONFIG_MEMCG
>  extern void mm_update_next_owner(struct mm_struct *mm);
>  extern void mm_init_owner(struct mm_struct *mm, struct task_struct *p);
>  #else
> @@ -2958,7 +2958,7 @@ static inline void mm_update_next_owner(struct mm_struct *mm)
>  static inline void mm_init_owner(struct mm_struct *mm, struct task_struct *p)
>  {
>  }
> -#endif /* CONFIG_MM_OWNER */
> +#endif /* CONFIG_MEMCG */
>  
>  static inline unsigned long task_rlimit(const struct task_struct *tsk,
>  		unsigned int limit)
> diff --git a/init/Kconfig b/init/Kconfig
> index 765018c..143119e 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -933,7 +933,6 @@ config RESOURCE_COUNTERS
>  config MEMCG
>  	bool "Memory Resource Controller for Control Groups"
>  	depends on RESOURCE_COUNTERS
> -	select MM_OWNER
>  	select EVENTFD
>  	help
>  	  Provides a memory resource controller that manages both anonymous
> @@ -951,9 +950,6 @@ config MEMCG
>  	  disable memory resource controller and you can avoid overheads.
>  	  (and lose benefits of memory resource controller)
>  
> -	  This config option also selects MM_OWNER config option, which
> -	  could in turn add some fork/exit overhead.
> -
>  config MEMCG_SWAP
>  	bool "Memory Resource Controller Swap Extension"
>  	depends on MEMCG && SWAP
> @@ -1173,9 +1169,6 @@ config SCHED_AUTOGROUP
>  	  desktop applications.  Task group autogeneration is currently based
>  	  upon task session.
>  
> -config MM_OWNER
> -	bool
> -
>  config SYSFS_DEPRECATED
>  	bool "Enable deprecated sysfs features to support old userspace tools"
>  	depends on SYSFS
> diff --git a/kernel/exit.c b/kernel/exit.c
> index 429659c..e5c4668 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -313,7 +313,7 @@ kill_orphaned_pgrp(struct task_struct *tsk, struct task_struct *parent)
>  	}
>  }
>  
> -#ifdef CONFIG_MM_OWNER
> +#ifdef CONFIG_MEMCG
>  /*
>   * A task is exiting.   If it owned this mm, find a new owner for the mm.
>   */
> @@ -399,7 +399,7 @@ assign_new_owner:
>  	task_unlock(c);
>  	put_task_struct(c);
>  }
> -#endif /* CONFIG_MM_OWNER */
> +#endif /* CONFIG_MEMCG */
>  
>  /*
>   * Turn us into a lazy TLB process if we
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 54a8d26..ed75cf5 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1099,12 +1099,12 @@ static void rt_mutex_init_task(struct task_struct *p)
>  #endif
>  }
>  
> -#ifdef CONFIG_MM_OWNER
> +#ifdef CONFIG_MEMCG
>  void mm_init_owner(struct mm_struct *mm, struct task_struct *p)
>  {
>  	mm->owner = p;
>  }
> -#endif /* CONFIG_MM_OWNER */
> +#endif /* CONFIG_MEMCG */
>  
>  /*
>   * Initialize POSIX timer handling for a single task.
> 

-- 
Michal Hocko
SUSE Labs