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=-15.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable 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 50547C433E0 for ; Mon, 15 Feb 2021 10:30:05 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id F266064D73 for ; Mon, 15 Feb 2021 10:30:04 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229994AbhBOKaB (ORCPT ); Mon, 15 Feb 2021 05:30:01 -0500 Received: from mx2.suse.de ([195.135.220.15]:36324 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230171AbhBOK2f (ORCPT ); Mon, 15 Feb 2021 05:28:35 -0500 X-Virus-Scanned: by amavisd-new at test-mx.suse.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=susede1; t=1613384868; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=73NkWy0SYucsJms5v/JqP31gNuHokmiIPgdC8knJweg=; b=Tmnax6sJaIBzZXp3R9sNYEAdnpn1Ugf0Kem4uhGfk88hDPKs3PUvyOkp27ScBspmABWtSQ x1mqyWFbV/M8ApNz3GFqshMqfOREiaOw22s46pasXI8stgyvG/yWdbk5dqb+Zpje2rMnM/ 1s+WpacNOOfge1Mf709dhpF/q0ixbAE= Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id C5CFFAC32; Mon, 15 Feb 2021 10:27:48 +0000 (UTC) Date: Mon, 15 Feb 2021 11:27:47 +0100 From: Michal Hocko To: Muchun Song Cc: Johannes Weiner , Vladimir Davydov , Andrew Morton , Cgroups , Linux Memory Management List , LKML Subject: Re: [External] Re: [PATCH 3/4] mm: memcontrol: bail out early when id is zero Message-ID: References: <20210212170159.32153-1-songmuchun@bytedance.com> <20210212170159.32153-3-songmuchun@bytedance.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon 15-02-21 18:09:44, Muchun Song wrote: > On Mon, Feb 15, 2021 at 5:39 PM Michal Hocko wrote: > > > > On Sat 13-02-21 01:01:58, Muchun Song wrote: > > > The memcg ID cannot be zero, but we can pass zero to mem_cgroup_from_id, > > > so idr_find() is pointless and wastes CPU cycles. > > > > Is this possible at all to happen? If not why should we add a test for > > _all_ invocations? > > Yeah, this indeed can happen. If we allocate a new swap cache page > and charge it via mem_cgroup_charge, then the page will uncharge > the swap counter via mem_cgroup_uncharge_swap. When the swap > entry is indeed freed, we will call mem_cgroup_uncharge_swap again, > In this routine, we can pass zero to mem_cgroup_from_id. Right? If the above claim is correct, which I would need to double check then it should have been part of the changelog! Please think of your poor reviewers and the time they have to invest into the review. I would also like to see your waste of CPU cycles argument to be backed by something. Are we talking about cycles due to an additional function call? Is this really something we should even care about? > > > > > > Signed-off-by: Muchun Song > > > --- > > > mm/memcontrol.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > > index a3f26522765a..68ed4b297c13 100644 > > > --- a/mm/memcontrol.c > > > +++ b/mm/memcontrol.c > > > @@ -5173,6 +5173,9 @@ static inline void mem_cgroup_id_put(struct mem_cgroup *memcg) > > > struct mem_cgroup *mem_cgroup_from_id(unsigned short id) > > > { > > > WARN_ON_ONCE(!rcu_read_lock_held()); > > > + /* The memcg ID cannot be zero. */ > > > + if (id == 0) > > > + return NULL; > > > return idr_find(&mem_cgroup_idr, id); > > > } > > > > > > -- > > > 2.11.0 > > > > -- > > Michal Hocko > > SUSE Labs -- Michal Hocko SUSE Labs