Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 24 Sep 2020 13:39:05 -0700
From:   Roman Gushchin <guro@fb.com>
To:     Johannes Weiner <hannes@cmpxchg.org>
CC:     Andrew Morton <akpm@linux-foundation.org>,
        Shakeel Butt <shakeelb@google.com>,
        Michal Hocko <mhocko@kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-mm@kvack.org>,
        <kernel-team@fb.com>
Subject: Re: [PATCH v1 3/4] mm: introduce page memcg flags
Message-ID: <20200924203905.GD1899519@carbon.dhcp.thefacebook.com>
References: <20200922203700.2879671-1-guro@fb.com>
 <20200922203700.2879671-4-guro@fb.com>
 <20200924200122.GC329853@cmpxchg.org>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200924200122.GC329853@cmpxchg.org>
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
Received: from carbon.dhcp.thefacebook.com (2620:10d:c090:400::5:a448) by MWHPR04CA0063.namprd04.prod.outlook.com (2603:10b6:300:6c::25) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3391.14 via Frontend Transport; Thu, 24 Sep 2020 20:39:08 +0000
X-Originating-IP: [2620:10d:c090:400::5:a448]
X-MS-PublicTrafficType: Email
X-MS-Office365-Filtering-Correlation-Id: 9b07ff72-627c-4c15-184f-08d860c9e380
X-MS-TrafficTypeDiagnostic: BYAPR15MB2438:
X-MS-Exchange-Transport-Forked: True
X-Microsoft-Antispam-PRVS: <BYAPR15MB2438DEB27F92EEDBDFABEB07BE390@BYAPR15MB2438.namprd15.prod.outlook.com>
X-FB-Source: Internal
X-MS-Oob-TLC-OOBClassifiers: OLM:10000;
X-MS-Exchange-SenderADCheck: 1
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info: NZryXtDGI2qtTGJ5Qwc6cGns84uVKeXZVZ0aG3kOo6My5qF4pTkh0y7ypHzkKaalLHk+y5qQM0LvFR1QD9XdhTejlX7pRicdtWc0gl0vECETFZveiw9ICqVVQc3PsUdMzuUk2U4mKgkISvvELrdbBpscFHaCK+PSdu6yx7N35swAnuaxoNnZ+GNfVNKAgwmW3DTP2XSqqWg3sVNQnUkunXNIgLf8PlzFY8kD4Znvmq6IC8JMwhlBIBKOyKpog1/2v6YiRpzXEmgPOT8zw6HMkKixrDODW4MbF+nFsx1bmUwdLgGvwezi/9y7tFlAHpseWhFst6hu6O+cT6qHxP6UH9tF8N/98CZlh76MB2Lg1Mex4sKgO1RmIHec6HkgrILN
X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:BYAPR15MB4136.namprd15.prod.outlook.com;PTR:;CAT:NONE;SFS:(346002)(376002)(39860400002)(396003)(366004)(136003)(316002)(33656002)(1076003)(478600001)(6916009)(8676002)(66946007)(55016002)(5660300002)(2906002)(66556008)(9686003)(8936002)(6506007)(186003)(83380400001)(54906003)(86362001)(52116002)(16526019)(7696005)(4326008)(66476007)(6666004);DIR:OUT;SFP:1102;
X-MS-Exchange-AntiSpam-MessageData: xPbC2dIkzIJby1jpJ94v8bpsdzpdTS9aqeufic1QmSrJ3P6qmR47nK26wkUyyyk0v+Cj8lFv0n3XPLdTguNiidF8lHA+OL/gLSOeyughtcF+KSUayR+0pOt+FxJR+nkCa3tONRRPcljJxnn0SuTiqFw4frRmqdw2U8zESA4Pnj7LaumxhtTvFQzdLOt3ippGkIrKgeviD+d7OZxiqZTVYMHvgda2onOVSA0t/KUbHPF/dOvKoLdpiTN8in05sPIlTglTpnuL5kRPNG0HP2O7Q+W5Q534OGBXlFVZa6HRCQagox35skoLbmKwF2sFDJrfFtzLBFgnuhL0igDsFS7XQJ4MtTe5iZyyPeCmmW+M9oZVIWRMabjUFMNfPJ+zUqSsmU33Bx/lwlfb9LJgnZcaOk7lroNTGvV3ILVp0tku6BxAleTRHSGjOv4Bj5Er8MO6ELYPe0UWzwJNjDxKll0kn1duBvdYPdbptmwsFTdLSHkEuGy+/PNHPghl8TY3BQHcDGZDB83p4Qc6dqS3/81AocXBSOinTBrala8wOt3ZXtRmhNI6Ts7UC0yGtHXmf1rMqd1bmoYSfBePwcQkMffn+7B1DLcGgICOYhYQBdC8TuZI6FI/MSxzZmxRruSkTGZms+9nRTkNPUwp5pnBGuysru/OgAB9WAchNekYV+rKHzg=
X-MS-Exchange-CrossTenant-Network-Message-Id: 9b07ff72-627c-4c15-184f-08d860c9e380
X-MS-Exchange-CrossTenant-AuthSource: BYAPR15MB4136.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Sep 2020 20:39:09.8498
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: L71WkWl5slczURkkgJnM9Lz3s0RtrA3qmvbG60NwMbgjBQWEo57oPiKG2MTkJoYE
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR15MB2438
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.235,18.0.687
 definitions=2020-09-24_15:2020-09-24,2020-09-24 signatures=0
X-Proofpoint-Spam-Details: rule=fb_default_notspam policy=fb_default score=0 mlxscore=0
 lowpriorityscore=0 priorityscore=1501 impostorscore=0 adultscore=0
 malwarescore=0 spamscore=0 mlxlogscore=936 suspectscore=1 bulkscore=0
 clxscore=1015 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2006250000 definitions=main-2009240151
X-FB-Internal: deliver
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Sep 24, 2020 at 04:01:22PM -0400, Johannes Weiner wrote:
> On Tue, Sep 22, 2020 at 01:36:59PM -0700, Roman Gushchin wrote:
> > The lowest bit in page->memcg_data is used to distinguish between
> > struct memory_cgroup pointer and a pointer to a objcgs array.
> > All checks and modifications of this bit are open-coded.
> > 
> > Let's formalize it using page memcg flags, defined in page_memcg_flags
> > enum and replace all open-coded accesses with test_bit()/__set_bit().
> > 
> > Few additional flags might be added later. Flags are intended to be
> > mutually exclusive.
> > 
> > Signed-off-by: Roman Gushchin <guro@fb.com>
> > ---
> >  include/linux/memcontrol.h | 29 +++++++++++++++++++----------
> >  1 file changed, 19 insertions(+), 10 deletions(-)
> > 
> > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> > index ab3ea3e90583..9a49f1e1c0c7 100644
> > --- a/include/linux/memcontrol.h
> > +++ b/include/linux/memcontrol.h
> > @@ -343,6 +343,11 @@ struct mem_cgroup {
> >  
> >  extern struct mem_cgroup *root_mem_cgroup;
> >  
> > +enum page_memcg_flags {
> > +	/* page->memcg_data is a pointer to an objcgs vector */
> > +	PG_MEMCG_OBJ_CGROUPS,
> 
> How about enum memcg_data_flags and PGMEMCG_OBJCG?

Honestly I prefer the original names. I'm ok with enum memcg_data_flags,
if you prefer it. PGMEMCG_OBJCG looks bulky with too many letters
without a separator, also we use object cgroups (plural) everywhere,
like OBJCGS vs OBJCG. PG_MEMCG_OBJCGS works for me.

> 
> > @@ -371,13 +376,7 @@ static inline struct mem_cgroup *page_mem_cgroup_check(struct page *page)
> >  {
> >  	unsigned long memcg_data = page->memcg_data;
> >  
> > -	/*
> > -	 * The lowest bit set means that memcg isn't a valid
> > -	 * memcg pointer, but a obj_cgroups pointer.
> > -	 * In this case the page is shared and doesn't belong
> > -	 * to any specific memory cgroup.
> > -	 */
> > -	if (memcg_data & 0x1UL)
> > +	if (test_bit(PG_MEMCG_OBJ_CGROUPS, &memcg_data))
> >  		return NULL;
> >  
> >  	return (struct mem_cgroup *)memcg_data;
> > @@ -422,7 +421,13 @@ static inline void clear_page_mem_cgroup(struct page *page)
> >   */
> >  static inline struct obj_cgroup **page_obj_cgroups(struct page *page)
> >  {
> > -	return (struct obj_cgroup **)(page->memcg_data & ~0x1UL);
> > +	unsigned long memcg_data = page->memcg_data;
> > +
> > +	VM_BUG_ON_PAGE(memcg_data && !test_bit(PG_MEMCG_OBJ_CGROUPS,
> > +					       &memcg_data), page);
> > +	__clear_bit(PG_MEMCG_OBJ_CGROUPS, &memcg_data);
> 
> The flag names make sense to me, but this shouldn't be using test_bit,
> __clear_bit, __set_bit etc. on local variables. It suggests that it's
> modifying some shared/global state, when it's just masking out a bit
> during a read. We usually just open-code the bitwise ops for that.

It will be way more bulky otherwise, all those memcg_data & (1UL << PG_MEMCG_OBJ_CGROUPS) etc.
I don't see why these bitops helpers can't be used on local variables.
Is the preference to not use them this way documented anywhere?

Thanks!