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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 18819C00140 for ; Fri, 19 Aug 2022 01:21:49 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 7092B8D0002; Thu, 18 Aug 2022 21:21:49 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 691448D0001; Thu, 18 Aug 2022 21:21:49 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 559458D0002; Thu, 18 Aug 2022 21:21:49 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 426258D0001 for ; Thu, 18 Aug 2022 21:21:49 -0400 (EDT) Received: from smtpin04.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 1820B1C6153 for ; Fri, 19 Aug 2022 01:21:49 +0000 (UTC) X-FDA: 79814590338.04.FA598FF Received: from mail-vk1-f182.google.com (mail-vk1-f182.google.com [209.85.221.182]) by imf02.hostedemail.com (Postfix) with ESMTP id AA1B080007 for ; Fri, 19 Aug 2022 01:21:48 +0000 (UTC) Received: by mail-vk1-f182.google.com with SMTP id bj43so1616428vkb.4 for ; Thu, 18 Aug 2022 18:21:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc; bh=x+WlY4BSKVFKBcgHLYvbuY4NBBdZIkpvhjCiWa6AJbU=; b=N90AWt3Kudge/Xp8O33r141kO5yVImla6A9uZyKqvZPOMvytwEj/oktBy1IoB9eFl3 VBIq/P1WeQRqWDOxOYfCJmiPO4GFp3SL2IFEhjQztwiURDCcWrdRgxPYF9xZFmBMYPmz d55S+BnqKMFHdz8Q58f9IOFjUADp5CjiLVCGapRXEOjF3qoc8DwldBJ1lAtbD6kvIknB ElUHGCzzeVNdp8dga4j4nit27hiVv6OA340TyNRAbm9+K1DCarcbA+0M85jBT2IRFn0x V4Ad7YqriucrHuTaTGRzby26Rg8iyFBqqz7FlmJI8VQuw2LYVXpZMoh92iPqJFFZjzm0 VBNA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc; bh=x+WlY4BSKVFKBcgHLYvbuY4NBBdZIkpvhjCiWa6AJbU=; b=uVAJIyrT6hnudczkVtuIRW1MzJzIeQGVP60YP+0VcbFDNJrKdUUdhZmajCuofVv7pk AyTuW3Y1LbJv4WoSgh5ENalugH734Ctcc8dZ3YQTvdDgqiSoV/z1dsq7hK51FSDMpHqU C22RgLd+Q4wC99FwGbQu40BMqeIr0TTPmmOt6RcC0jqDmgF0d41BZTn7CSyAyx4pyPCY ts2ewxTQi9sCiEGzwLj3RK7kfDkYo4WZdj0xbJCv1aD8p6CWcqskNx9PkTLZq7iIsj2H fpJPCpeDa5x6FB3cxdaDFdmqnsHup/LIcOy8wRzxm97HDOozPGKPln2oyHt7pMNhpg5H kqEg== X-Gm-Message-State: ACgBeo0vHzpBqpQrYoFe1jZsdA4r+gybVpvh5oh2MF3nQlnd9mjorzMN snQsjcrn0mhyxhI8883m+fcvw6bJ8sD3eV1vpzo= X-Google-Smtp-Source: AA6agR6PAzXuqdzrwQGMaEaIMftD+9yhgUz9H2/0i37ltB8Htymm8nErBu3DcAgs9eYvMuk8SL+EAz+wMTXPxBqlv7k= X-Received: by 2002:a1f:251:0:b0:380:d262:4f4f with SMTP id 78-20020a1f0251000000b00380d2624f4fmr2314419vkc.5.1660872108064; Thu, 18 Aug 2022 18:21:48 -0700 (PDT) MIME-Version: 1.0 References: <20220818143118.17733-1-laoar.shao@gmail.com> <20220818143118.17733-11-laoar.shao@gmail.com> In-Reply-To: From: Yafang Shao Date: Fri, 19 Aug 2022 09:21:11 +0800 Message-ID: Subject: Re: [PATCH bpf-next v2 10/12] mm, memcg: Add new helper get_obj_cgroup_from_cgroup To: Shakeel Butt Cc: Alexei Starovoitov , Daniel Borkmann , Andrii Nakryiko , Martin KaFai Lau , Song Liu , Yonghong Song , John Fastabend , KP Singh , Stanislav Fomichev , Hao Luo , jolsa@kernel.org, Johannes Weiner , Michal Hocko , Roman Gushchin , Muchun Song , Andrew Morton , Tejun Heo , Zefan Li , Cgroups , netdev , bpf , Linux MM Content-Type: text/plain; charset="UTF-8" ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1660872108; a=rsa-sha256; cv=none; b=aT3LH+6uZJrREb01uGtiaPsr4aN0eqoBD73LsXLiSR8dJo6qPGtBrLNwkEUuriMr9bkbCJ HwQumdG583gJki6hJ0A7jM07oReUMNDadPUsqiTrmL+yyjEFsXwY+QTXtx8ef5r/oxFaQF APFS6zT8e9K1iSCwf6J83TOCKGXwFV4= ARC-Authentication-Results: i=1; imf02.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=N90AWt3K; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf02.hostedemail.com: domain of laoar.shao@gmail.com designates 209.85.221.182 as permitted sender) smtp.mailfrom=laoar.shao@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1660872108; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=x+WlY4BSKVFKBcgHLYvbuY4NBBdZIkpvhjCiWa6AJbU=; b=78s0OTR0J/BPB1bCBFVBY1qTdOn82Q9jUS1MUQhYMRMGAnrlFb7E2P4wLhOM7C0nhsTPWk 6OBPD+koVhv/ZbkNxUX0pky3Dx773+dbZ+R2UFvmgFRuQAGY0CmFR15A8fhk8yCZW4xKwN F7Cwe7zgVOEr6oKT9vIFtlRHA183/Xg= Authentication-Results: imf02.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=N90AWt3K; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf02.hostedemail.com: domain of laoar.shao@gmail.com designates 209.85.221.182 as permitted sender) smtp.mailfrom=laoar.shao@gmail.com X-Rspam-User: X-Stat-Signature: ncj33qasa7zrqhrej63s6nomgmdpjyie X-Rspamd-Queue-Id: AA1B080007 X-Rspamd-Server: rspam03 X-HE-Tag: 1660872108-462259 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, Aug 19, 2022 at 4:38 AM Shakeel Butt wrote: > > On Thu, Aug 18, 2022 at 7:32 AM Yafang Shao wrote: > > > > We want to open a cgroup directory and pass the fd into kernel, and then > > in the kernel we can charge the allocated memory into the open cgroup if it > > has valid memory subsystem. In the bpf subsystem, the opened cgroup will > > be store as a struct obj_cgroup pointer, so a new helper > > get_obj_cgroup_from_cgroup() is introduced. > > > > It also add a comment on why the helper __get_obj_cgroup_from_memcg() > > must be protected by rcu read lock. > > > > Signed-off-by: Yafang Shao > > --- > > include/linux/memcontrol.h | 1 + > > mm/memcontrol.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 48 insertions(+) > > > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > > index 2f0a611..901a921 100644 > > --- a/include/linux/memcontrol.h > > +++ b/include/linux/memcontrol.h > > @@ -1713,6 +1713,7 @@ static inline void set_shrinker_bit(struct mem_cgroup *memcg, > > int __memcg_kmem_charge_page(struct page *page, gfp_t gfp, int order); > > void __memcg_kmem_uncharge_page(struct page *page, int order); > > > > +struct obj_cgroup *get_obj_cgroup_from_cgroup(struct cgroup *cgrp); > > struct obj_cgroup *get_obj_cgroup_from_current(void); > > struct obj_cgroup *get_obj_cgroup_from_page(struct page *page); > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > index 618c366..0409cc4 100644 > > --- a/mm/memcontrol.c > > +++ b/mm/memcontrol.c > > @@ -2895,6 +2895,14 @@ struct mem_cgroup *mem_cgroup_from_obj(void *p) > > return page_memcg_check(folio_page(folio, 0)); > > } > > > > +/* > > + * Pls. note that the memg->objcg can be freed after it is deferenced, > > + * that can happen when the memcg is changed from online to offline. > > + * So this helper must be protected by read rcu lock. > > + * > > + * After obj_cgroup_tryget(), it is safe to use the objcg outside of the rcu > > + * read-side critical section. > > + */ > > The comment is too verbose. My suggestion would be to add > WARN_ON_ONCE(!rcu_read_lock_held()) in the function and if you want to > add a comment, just say that the caller should have a reference on > memcg. > Sure, I will change it. > > static struct obj_cgroup *__get_obj_cgroup_from_memcg(struct mem_cgroup *memcg) > > { > > struct obj_cgroup *objcg = NULL; > > @@ -2908,6 +2916,45 @@ static struct obj_cgroup *__get_obj_cgroup_from_memcg(struct mem_cgroup *memcg) > > return objcg; > > } > > > > +static struct obj_cgroup *get_obj_cgroup_from_memcg(struct mem_cgroup *memcg) > > +{ > > + struct obj_cgroup *objcg; > > + > > + if (memcg_kmem_bypass()) > > + return NULL; > > + > > + rcu_read_lock(); > > + objcg = __get_obj_cgroup_from_memcg(memcg); > > + rcu_read_unlock(); > > + return objcg; > > +} > > + > > +struct obj_cgroup *get_obj_cgroup_from_cgroup(struct cgroup *cgrp) > > Since this function is exposed to other files, it would be nice to > have a comment similar to get_mem_cgroup_from_mm() for this function. > I know other get_obj_cgroup variants do not have such a comment yet > but let's start with this. > Sure, I will add it. > > +{ > > + struct cgroup_subsys_state *css; > > + struct mem_cgroup *memcg; > > + struct obj_cgroup *objcg; > > + > > + rcu_read_lock(); > > + css = rcu_dereference(cgrp->subsys[memory_cgrp_id]); > > + if (!css || !css_tryget_online(css)) { > > Any reason to use css_tryget_online() instead of css_tryget()? Because in this case, the cgroup is from an opened cgroup dir, which should be online. But considering this is a generic helper, which may be used by others, it would be more reasonable to use css_tryget(). I will change it. -- Regards Yafang