From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-14.4 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C36BFC35641 for ; Sat, 22 Feb 2020 01:49:51 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 768C6206EF for ; Sat, 22 Feb 2020 01:49:51 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="YxwOEI3a" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 768C6206EF Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 16F076B0006; Fri, 21 Feb 2020 20:49:51 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 120536B0007; Fri, 21 Feb 2020 20:49:51 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 00E016B0008; Fri, 21 Feb 2020 20:49:50 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0209.hostedemail.com [216.40.44.209]) by kanga.kvack.org (Postfix) with ESMTP id DEBE86B0006 for ; Fri, 21 Feb 2020 20:49:50 -0500 (EST) Received: from smtpin13.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 97AA1181AEF1D for ; Sat, 22 Feb 2020 01:49:50 +0000 (UTC) X-FDA: 76516081740.13.blade17_1bafcb3686e43 X-HE-Tag: blade17_1bafcb3686e43 X-Filterd-Recvd-Size: 8038 Received: from mail-oi1-f196.google.com (mail-oi1-f196.google.com [209.85.167.196]) by imf39.hostedemail.com (Postfix) with ESMTP for ; Sat, 22 Feb 2020 01:49:50 +0000 (UTC) Received: by mail-oi1-f196.google.com with SMTP id a22so3530537oid.13 for ; Fri, 21 Feb 2020 17:49:50 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=9RB5FaQbzQgbIL5nfWOlaYI9euOnM0n/xrO01cC3y4A=; b=YxwOEI3aPWiqbDzeypVY5RciIfiOmAgVFyUfidQdPNEy5hAeU3MlnuS2lvPsg8ZQQt 32TeqEWSjAXgXzGwqYOhopacZHXWMkyaIr7PG4raOs9Igi6tjJMPuZJHtP9+e5bHTvZr akNuht7bh9C+DsU3ARsOhZR/zPYpTHAzFteAqblFeEciTjkN9zn/tTMDoZMOZXomimyN y2judLthdCsztfodgjm5kPbweqH3d0A9dNEFIFUYS5miqGt/oF4srFdf5FopXqPk6uQU fKjvOoMg2Yfzhqg0Uy/In1FqidDB6nO5RcNxOyS5HjUPGk1sN2QehJkSQ7oz6VO9nbpc WS2g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=9RB5FaQbzQgbIL5nfWOlaYI9euOnM0n/xrO01cC3y4A=; b=DX2dA0k22ceq0ZacgOrarMYnaMcpUU08IQT1bnIF8OrI+c5nLOEGvdPZYxB1/dnGwf 53M3Dr64tmYg7yc/2QJ1uhsrqTyPxcJ03vulAHQwH/Dw3P3ZhgbB64cC8F4jLvWXDGaR pry3D5IH2gN7e0lRk3ePYC0z0JHaa3S97iQmnw7dc0mEMLczBjx6d3EC2rdaTODpDqGN Tp1gutNrqlZFVX8EqJ/5BUuFgE7vzWNpju/iZpV2A8RNySVBU5Ksl6NPT8ypb6658Vg5 g5ugaX8tj8h6XdscqLxwUbq9mMP2JlwR6Kt4ixiWK2e0Y/Szb4YizP2Qdmul+a2Xda1l 7hhw== X-Gm-Message-State: APjAAAWTIYmO/WUDXL7nzpcImTX7KeJSQaV4L2mMIyVdlmMdWmgcPbpm +h3l6sHQd8YlozjDMc+KwwctCGNJarHv6d62uRWGyg== X-Google-Smtp-Source: APXvYqw5MkBVBvtCzvavj3S5PGsEm5xQdu1+qp/GKgHgauqtCmnPRt4yxKhpuy6RL+FIJanxP8jq+BEk6+fHVbZu2rg= X-Received: by 2002:aca:4183:: with SMTP id o125mr4361233oia.125.1582336189111; Fri, 21 Feb 2020 17:49:49 -0800 (PST) MIME-Version: 1.0 References: <20200221195919.186576-1-shakeelb@google.com> <20200222011046.GB459391@carbon.DHCP.thefacebook.com> In-Reply-To: <20200222011046.GB459391@carbon.DHCP.thefacebook.com> From: Shakeel Butt Date: Fri, 21 Feb 2020 17:49:37 -0800 Message-ID: Subject: Re: [PATCH] memcg: css_tryget_online cleanups To: Roman Gushchin Cc: Johannes Weiner , Michal Hocko , Andrew Morton , Linux MM , Cgroups , LKML Content-Type: text/plain; charset="UTF-8" X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Fri, Feb 21, 2020 at 5:10 PM Roman Gushchin wrote: > > On Fri, Feb 21, 2020 at 11:59:19AM -0800, Shakeel Butt wrote: > > Currently multiple locations in memcg code, css_tryget_online() is being > > used. However it doesn't matter whether the cgroup is online for the > > callers. Online used to matter when we had reparenting on offlining and > > we needed a way to prevent new ones from showing up. > > > > The failure case for couple of these css_tryget_online usage is to > > fallback to root_mem_cgroup which kind of make bypassing the memcg > > limits possible for some workloads. For example creating an inotify > > group in a subcontainer and then deleting that container after moving the > > process to a different container will make all the event objects > > allocated for that group to the root_mem_cgroup. So, using > > css_tryget_online() is dangerous for such cases. > > > > Two locations still use the online version. The swapin of offlined > > memcg's pages and the memcg kmem cache creation. The kmem cache indeed > > needs the online version as the kernel does the reparenting of memcg > > kmem caches. For the swapin case, it has been left for later as the > > fallback is not really that concerning. > > > > Signed-off-by: Shakeel Butt > > Hello, Shakeel! > > > --- > > mm/memcontrol.c | 14 +++++++++----- > > 1 file changed, 9 insertions(+), 5 deletions(-) > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > index 63bb6a2aab81..75fa8123909e 100644 > > --- a/mm/memcontrol.c > > +++ b/mm/memcontrol.c > > @@ -656,7 +656,7 @@ __mem_cgroup_largest_soft_limit_node(struct mem_cgroup_tree_per_node *mctz) > > */ > > __mem_cgroup_remove_exceeded(mz, mctz); > > if (!soft_limit_excess(mz->memcg) || > > - !css_tryget_online(&mz->memcg->css)) > > + !css_tryget(&mz->memcg->css)) > > Looks good. > > > goto retry; > > done: > > return mz; > > @@ -962,7 +962,8 @@ struct mem_cgroup *get_mem_cgroup_from_page(struct page *page) > > return NULL; > > > > rcu_read_lock(); > > - if (!memcg || !css_tryget_online(&memcg->css)) > > + /* Page should not get uncharged and freed memcg under us. */ > > + if (!memcg || WARN_ON(!css_tryget(&memcg->css))) > > I'm slightly worried about this WARN_ON(). > As I understand the idea is that the caller must own the page and make > sure that page->memcg remains intact. Yes you are correct. > Do we really need this? There are no current such users, maybe just the warning in the comment is enough and use css_get(). I don't have any strong opinion. I will at least convert the warning to once and wait for comments from others. > > Also, I'd go with WARN_ON_ONCE() to limit the dmesg flow in the case > if something will go wrong. > > > memcg = root_mem_cgroup; > > rcu_read_unlock(); > > return memcg; > > @@ -975,10 +976,13 @@ EXPORT_SYMBOL(get_mem_cgroup_from_page); > > static __always_inline struct mem_cgroup *get_mem_cgroup_from_current(void) > > { > > if (unlikely(current->active_memcg)) { > > - struct mem_cgroup *memcg = root_mem_cgroup; > > + struct mem_cgroup *memcg; > > > > rcu_read_lock(); > > - if (css_tryget_online(¤t->active_memcg->css)) > > + /* current->active_memcg must hold a ref. */ > > Hm, does it? > memalloc_use_memcg() isn't touching the memcg's reference counter. > And if it does hold a reference, why can't we just do css_get()? The callers of the memalloc_use_memcg() should already have the refcnt of the memcg elevated. I should add that to the comment description of memalloc_use_memcg(). > > > + if (WARN_ON(!css_tryget(¤t->active_memcg->css))) > > + memcg = root_mem_cgroup; > > Btw, if css_tryget() fails here, what does it mean? > I'd s/WARN_ON/WARN_ON_ONCE too. > If css_tryget() fails, it means someone is using memalloc_use_memcg() without holding the reference to the memcg. Converting to once makes sense. > > + else > > memcg = current->active_memcg; > > rcu_read_unlock(); > > return memcg; > > @@ -6703,7 +6707,7 @@ void mem_cgroup_sk_alloc(struct sock *sk) > > goto out; > > if (!cgroup_subsys_on_dfl(memory_cgrp_subsys) && !memcg->tcpmem_active) > > goto out; > > - if (css_tryget_online(&memcg->css)) > > + if (css_tryget(&memcg->css)) > > So it can be offline, right? Makes sense. > Actually we got the memcg from the current just few lines above within rcu lock. memcg can not go offline here, right? > > sk->sk_memcg = memcg; > > out: > > rcu_read_unlock(); > > -- > > 2.25.0.265.gbab2e86ba0-goog > > > > Overall I have to admit it all is quite tricky. I had a patchset doing > a similar cleanup (but not only in the mm code), but dropped it after > Tejun showed me some edge cases, when it would cause a regression. > > So I really think it's a valuable work, but we need to be careful here. > Totally agreed. Shakeel