Re: [PATCH v2] memcg: Fix memcg_kmem_bypass() for remote memcg charging

[Roman Gushchin <guro@fb.com>]

On Thu, May 14, 2020 at 09:16:29AM +0800, Zefan Li wrote:
> On 2020/5/14 0:11, Roman Gushchin wrote:
> > On Wed, May 13, 2020 at 07:47:49PM +0800, Zefan Li wrote:
> >> While trying to use remote memcg charging in an out-of-tree kernel module
> >> I found it's not working, because the current thread is a workqueue thread.
> >>
> >> As we will probably encounter this issue in the future as the users of
> >> memalloc_use_memcg() grow, it's better we fix it now.
> >>
> >> Signed-off-by: Zefan Li <lizefan@huawei.com>
> >> ---
> >>
> >> v2: add a comment as sugguested by Michal. and add changelog to explain why
> >> upstream kernel needs this fix.
> >>
> >> ---
> >>
> >>  mm/memcontrol.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> >> index a3b97f1..43a12ed 100644
> >> --- a/mm/memcontrol.c
> >> +++ b/mm/memcontrol.c
> >> @@ -2802,6 +2802,9 @@ static void memcg_schedule_kmem_cache_create(struct mem_cgroup *memcg,
> >>  
> >>  static inline bool memcg_kmem_bypass(void)
> >>  {
> >> +	/* Allow remote memcg charging in kthread contexts. */
> >> +	if (unlikely(current->active_memcg))
> >> +		return false;
> >>  	if (in_interrupt() || !current->mm || (current->flags & PF_KTHREAD))
> >>  		return true;
> > 
> > Shakeel is right about interrupts. How about something like this?
> > 
> > static inline bool memcg_kmem_bypass(void)
> > {
> > 	if (in_interrupt())
> > 		return true;
> > 
> > 	if ((!current->mm || current->flags & PF_KTHREAD) && !current->active_memcg)
> > 		return true;
> > 
> > 	return false;
> > }
> > 
> 
> I thought the user should ensure not do this, but now I think it makes sense to just bypass
> the interrupt case.

I think now it's mostly a legacy of the opt-out kernel memory accounting.
Actually we can relax this requirement by forcibly overcommit the memory cgroup
if the allocation is happening from the irq context, and punish it afterwards.
Idk how much we wanna this, hopefully nobody is allocating large non-temporarily
objects from an irq.

Will you send a v3?

Thanks!