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=-3.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=no 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 E0068C4727D for ; Fri, 2 Oct 2020 14:24:11 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 47C58207DE for ; Fri, 2 Oct 2020 14:24:11 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=cmpxchg-org.20150623.gappssmtp.com header.i=@cmpxchg-org.20150623.gappssmtp.com header.b="tZEYQ2oi" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 47C58207DE Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=cmpxchg.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id C3AF96B005D; Fri, 2 Oct 2020 10:24:10 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id BEA426B0072; Fri, 2 Oct 2020 10:24:10 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id AB1F26B0073; Fri, 2 Oct 2020 10:24:10 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0130.hostedemail.com [216.40.44.130]) by kanga.kvack.org (Postfix) with ESMTP id 6B5F96B005D for ; Fri, 2 Oct 2020 10:24:10 -0400 (EDT) Received: from smtpin16.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id DA6E5181AE875 for ; Fri, 2 Oct 2020 14:24:09 +0000 (UTC) X-FDA: 77327205018.16.pipe74_440d008271a4 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin16.hostedemail.com (Postfix) with ESMTP id B2B84100E5A52 for ; Fri, 2 Oct 2020 14:24:09 +0000 (UTC) X-HE-Tag: pipe74_440d008271a4 X-Filterd-Recvd-Size: 6176 Received: from mail-qk1-f193.google.com (mail-qk1-f193.google.com [209.85.222.193]) by imf35.hostedemail.com (Postfix) with ESMTP for ; Fri, 2 Oct 2020 14:24:08 +0000 (UTC) Received: by mail-qk1-f193.google.com with SMTP id f142so1456789qke.13 for ; Fri, 02 Oct 2020 07:24:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cmpxchg-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=J6aAVCPaMvKSHOZjnjEf6/gbikFRYlnB3sj1/Tf0Wm0=; b=tZEYQ2oiZPhVz0jxDWbJSFolyVeR1WXMHqMWkqBm4CC21X68tD3whGHoER1ydI+9rN RC53o8XjkPPaCJV81P5x1wvk+Sv0noq1kwwbWmizk6nVyRHxqDkeGxtkjQeTDIHmdsif uIqHYeNLVsujCrIJsM4IM29P0kNyKBbtBf/O7jZ2xYIo1KKOVjyxhgxVVbzrMTXpBCfz SkeLUWU6XlfNW4Bgd+pASbn21mhPKCqZk0fWZUtvtazY3g/ld4s1noL7/1gls/YefNbo +DCuYMVde8SOoCb1XEKCty0rGezCgAJzZtaPJ+urg2sQ+XkJjh+Juo5GQGE9aw+2EODo 3CeQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=J6aAVCPaMvKSHOZjnjEf6/gbikFRYlnB3sj1/Tf0Wm0=; b=ZuCjuLbikBx3uvJ/znoq4AKcN8RVJrOW5Q4hf6HfX0p63SrgB7uy4H7bllSztaYeBV zPPjw6efkE+7Al0p2t2yFA6UVgfBtfUEF05NdhvsyxSE4j+WDt72BdF4pug2uSmmXb/X eIg0ftKWUz+SJFFNg515T3wWXoe3wm6B8HbhSfRoqyG+t9yxWTJaIpIpFz4QQ4N/t9V0 snzVZ+NFbLqoxckXnCR5R0vqMlKWV11Lv3ZGpNX077FB80wLy4NgQd9LJK1MQoQoL7L8 XQ1tbSjEOoU0uTb1h59e4lUM5YuV3SlD6hj50bxT/x0NXQwFxtsWxuL+ts97y54IXmiJ wgdQ== X-Gm-Message-State: AOAM530XveuydviXepdRa22XvrFzLN6henDW7J4jxhVFnKDiC1Ml2zab hHkom9C+V82aKFat0070G++oZA== X-Google-Smtp-Source: ABdhPJyRuEXfi/Ldsn3i78m/8J1WyOXKLYmIY0cIc1CstIXxKppiLlcAewJ2ISLpYUnLTRHe5FyHLw== X-Received: by 2002:a05:620a:656:: with SMTP id a22mr2259439qka.63.1601648648098; Fri, 02 Oct 2020 07:24:08 -0700 (PDT) Received: from localhost ([2620:10d:c091:480::1:d668]) by smtp.gmail.com with ESMTPSA id y30sm1176545qth.7.2020.10.02.07.24.06 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 02 Oct 2020 07:24:07 -0700 (PDT) Date: Fri, 2 Oct 2020 10:22:25 -0400 From: Johannes Weiner To: Roman Gushchin Cc: Andrew Morton , Shakeel Butt , Michal Hocko , linux-kernel@vger.kernel.org, linux-mm@kvack.org, kernel-team@fb.com Subject: Re: [PATCH v4 1/4] mm: memcontrol: use helpers to access page's memcg data Message-ID: <20201002142225.GB525590@cmpxchg.org> References: <20201001002710.748166-1-guro@fb.com> <20201001002710.748166-2-guro@fb.com> <20201001134638.GE469663@cmpxchg.org> <20201001182739.GA2706729@carbon.DHCP.thefacebook.com> <20201001185950.GB500308@cmpxchg.org> <20201001205143.GE2706729@carbon.DHCP.thefacebook.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201001205143.GE2706729@carbon.DHCP.thefacebook.com> 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 Thu, Oct 01, 2020 at 01:51:43PM -0700, Roman Gushchin wrote: > On Thu, Oct 01, 2020 at 02:59:50PM -0400, Johannes Weiner wrote: > > On Thu, Oct 01, 2020 at 11:27:39AM -0700, Roman Gushchin wrote: > > > On Thu, Oct 01, 2020 at 09:46:38AM -0400, Johannes Weiner wrote: > > > > On Wed, Sep 30, 2020 at 05:27:07PM -0700, Roman Gushchin wrote: > > > > > +/* > > > > > + * set_page_memcg - associate a page with a memory cgroup > > > > > + * @page: a pointer to the page struct > > > > > + * @memcg: a pointer to the memory cgroup > > > > > + * > > > > > + * Associates a page with a memory cgroup. > > > > > + */ > > > > > +static inline void set_page_memcg(struct page *page, struct mem_cgroup *memcg) > > > > > +{ > > > > > + VM_BUG_ON_PAGE(PageSlab(page), page); > > > > > + > > > > > + /* > > > > > + * Please, refer to page_memcg()'s description for the page and memcg > > > > > + * binding stability requirements. > > > > > + */ > > > > > + page->memcg_data = (unsigned long)memcg; > > > > > +} > > > > > > > > Please delete and inline this as per previous feedback, thanks. > > > > > > Why it's better? > > > It's ok for set_page_memcg(), but obviously worse for set_page_objcgs(): > > > it was nice to have all bit magic in one place, in few helper functions. > > > And now it spills into several places. What's the win? > > > > set_page_objcgs() is a worthwhile abstraction because it includes the > > synchronization primitives that make it safe to use wrt > > page_objcgs(). They encapsulate the cmpxchg and the READ_ONCE(). > > > > set_page_memcg() doesn't do any synchronization and relies fully on > > the contextual locking. The name implies that it includes things to > > make it safe wrt page_memcg(), which isn't true at all. It's a long > > and misleading name for '='. > > > > Btw, I really don't mind having this discussion, but please don't send > > revisions that silently ignore feedback you don't agree with. > > I'm not ignoring: I thought you was looking to remove clear_page_* functions, > but it wasn't clear you want eliminate set_page_memcg() function. Please, when > you asking for some "style" changes, please, provide some rationale, it's way > less obvious than you think, what exactly you don't like in the proposed version. https://lore.kernel.org/lkml/20200930210124.GB469663@cmpxchg.org/ mentions and provides a rationale for inlining the setting and clearing (unnecessary abstraction/encapsulation). This is a code base shared by many different developers. It's routine to be asked for style changes that make the new code blend in with its surroundings, follow precedents and use idiomatic constructs, or avoid anti-patterns that are known to cause maintenance burden or make it harder to hack or debug the code. If I'm being too terse and something isn't clear, please just ask for clarification.