From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-f172.google.com (mail-pl1-f172.google.com [209.85.214.172]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id C52D122339 for ; Mon, 27 Jan 2025 02:59:07 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.214.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737946749; cv=none; b=DvIcN8ST3WXp+NDEGv9axno87+AsJSC+y9dx8nKdArShEs7vAgZjLWr03b+T9n74UTPp1XhZTdlOkH5ZHtbeGp1AbZhad6EZYoeCQnEleGB1bJaswQUn3suP8s8894w6DjrLDgB8wBfCkuYNh5+J6imrRPY+mYRmg0JJt5P6oAU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737946749; c=relaxed/simple; bh=J5gg+ygzIjMDdrcFZ4Wcp34PCbVnoLct7UCtiO6HpJA=; h=Date:From:To:cc:Subject:In-Reply-To:Message-ID:References: MIME-Version:Content-Type; b=F8fGu0W2Gq6Ydy27mVZvsE1eKQCLCeAnhQLkWQanJW9GthNaFPxUSWCZRqQpLSbe6DhU12ynzxnVoAcfFf8S+egq8Mj/V61dV/bBDHRR7Sf7Jp4gvsveBEAlKvO/6jomv/bFn0W6TdQun7x/ZaWwFNaoA+JW1uMEg1SlC6IC6IM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=EEkvU0Ii; arc=none smtp.client-ip=209.85.214.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="EEkvU0Ii" Received: by mail-pl1-f172.google.com with SMTP id d9443c01a7336-21628b3fe7dso66774935ad.3 for ; Sun, 26 Jan 2025 18:59:07 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1737946747; x=1738551547; darn=vger.kernel.org; h=mime-version:references:message-id:in-reply-to:subject:cc:to:from :date:from:to:cc:subject:date:message-id:reply-to; bh=gY7whz075gF2oDUVkEoQRi0JP9AZVP1TZQbx76u1kos=; b=EEkvU0Iif3Dx9u3xruy1rmclES5h2vE5UakacT/HQ3YLydv/WqfB5MnMBa0/xrKjOV +cSMfz9N50ltmOfIUofO3DHqrojQ5eW1dTNxDaQv55ijtfPXjAA8GtPET9I2N2Z4HPOE uFyByCZE+q/exjgy2qHim/bPEcrakH8gEvs1id/3/C4yDmwrKjpNTFMI3UgBAvJZ3Nej IWcKa/9JyxOOyhqopvWTDmC7q2ZH2r1N6fu8VeSjanyZBQ/EsG34KgZShqWG5763KmTX ni8ZtuoUYg66MyosQH/4j5KWIfAIX0J1kHOVbvTH+1dW48MW/NeCLQnFPLEfxS5Bt+oZ hB7w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1737946747; x=1738551547; h=mime-version:references:message-id:in-reply-to:subject:cc:to:from :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=gY7whz075gF2oDUVkEoQRi0JP9AZVP1TZQbx76u1kos=; b=ehIa5ETd2FBwH72B0Ymx4g4ph6627Si46MaB9PNqdwFtP4dOQyUWirdYT8214I4JY6 8qdLgHjkgQe1vJztgHDXfWffDXpIa2se4RJYWc/Q39UVGA3Go3/UKKQdsx3URrWoAIyz KyWtzFRBdJCdldxtiZQFEIQGNY6ibA3Hw5XheqleW8oQyoVWg1X/A9AHnbrA2u4v1F5S k6AWDt/W4mjrC04ULdW/hRCXg5POLr3yMnfxV0ypEDGlcwI8y5Mm+V06Xqt2l5CwKHZa mECtmWx7VP8+1QCt8Pw/ux42vfljRb4dYZlqeMRaPwXYfcm+oFnhSUIss8KslLdSYic1 fRrA== X-Forwarded-Encrypted: i=1; AJvYcCUjiYX09vFyC3IWK9y+1MeojUaq+vTsuoqyYr77Sgk+yal2Vd6xTLiSPTYbo3AvHorbzqn7KdM2FY/KoC8=@vger.kernel.org X-Gm-Message-State: AOJu0Yxf1b450fXsfk/K6xmRicUQTZzm2Y41H2WXaEFzyfpE6LeOwGex B0ISi2sMchfeb8HDY7mOS128NOcY0p3FpV1yMKN8yMkRjcvyimdJpoQb+5GIZg== X-Gm-Gg: ASbGncuN9XMQU9t78zOcF0RHQqyk0t7ZD6Do7k6QOpQDn4aVw3gRJv/IaveytW5/EFb QOb119KeCzBCygshsmJEIo2RNJUWJG1bv2Oj/TiUXfVcAO6VCDvMn57oZA4qN3zppRXgmpF7/s3 ZDgss+MEvNhz21YFhcosOv9a2n6/pGBA/Tavlwrs+JmGAN+cpyxS/iZMpg969Whx+geBW0SaqLq qpIcTlZEoPFqh3ufIQFUE1N/uxE+6fhC5ElqLR8OsT2hVkcNpnzy072XUtgrLf6vQ3jb/bvr1YZ 7EgMSgIbjVTqblGuuthdKxAzbyb4xJbVKQTkwP4ckmDeZkOe7y2zYlFUM8AaSY82 X-Google-Smtp-Source: AGHT+IEtis62Qgxn0OUJHhskPq+vJz2+MtDmdCQrrgYXMhkRJPuAp/K3UaUnvwTAxqlINGYQ2m8HrA== X-Received: by 2002:a17:902:e5c6:b0:216:5e6e:68b4 with SMTP id d9443c01a7336-21c355f0a3bmr611437575ad.46.1737946746849; Sun, 26 Jan 2025 18:59:06 -0800 (PST) Received: from darker.attlocal.net (172-10-233-147.lightspeed.sntcca.sbcglobal.net. [172.10.233.147]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-21da424eb96sm52650665ad.222.2025.01.26.18.59.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 26 Jan 2025 18:59:05 -0800 (PST) Date: Sun, 26 Jan 2025 18:59:04 -0800 (PST) From: Hugh Dickins To: Johannes Weiner cc: Hugh Dickins , Andrew Morton , Michal Hocko , Roman Gushchin , Shakeel Butt , Muchun Song , linux-mm@kvack.org, cgroups@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] mm: memcontrol: move memsw charge callbacks to v1 In-Reply-To: <20250124155420.GA1222@cmpxchg.org> Message-ID: <9f162aae-00fb-dafd-848f-52214836789d@google.com> References: <20250124054132.45643-1-hannes@cmpxchg.org> <20250124155420.GA1222@cmpxchg.org> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII On Fri, 24 Jan 2025, Johannes Weiner wrote: > On Thu, Jan 23, 2025 at 10:53:04PM -0800, Hugh Dickins wrote: > > On Fri, 24 Jan 2025, Johannes Weiner wrote: > > > > > The interweaving of two entirely different swap accounting strategies > > > has been one of the more confusing parts of the memcg code. Split out > > > the v1 code to clarify the implementation and a handful of callsites, > > > and to avoid building the v1 bits when !CONFIG_MEMCG_V1. > > > > > > text data bss dec hex filename > > > 39253 6446 4160 49859 c2c3 mm/memcontrol.o.old > > > 38877 6382 4160 49419 c10b mm/memcontrol.o > > > > > > Signed-off-by: Johannes Weiner > > > > I'm not really looking at this, but want to chime in that I found the > > memcg1 swap stuff in mm/memcontrol.c, not in mm/memcontrol-v1.c, very > > misleading when I was doing the folio_unqueue_deferred_split() business: > > so, without looking into the details of it, strongly approve of the > > direction you're taking here - thank you. > > Thanks, I'm glad to hear that! > > > But thought you could go even further, given that > > static inline bool do_memsw_account(void) > > { > > return !cgroup_subsys_on_dfl(memory_cgrp_subsys); > > } > > > > I thought that amounted to do_memsw_account iff memcg_v1; > > but I never did grasp cgroup_subsys_on_dfl very well, > > so ignore me if I'm making no sense to you. > > Yes, technically we should be able to move all the code guarded by > this check to v1 proper in some form. > > [ It's a runtime check for whether the memory controller is attached > to a cgroup1 or a cgroup2 mount. You can still mount the v1 > controller when !CONFIG_MEMCG_V1, but in that case it won't have any > memory control files, so whether we update the memsw counter or not, > the results of it won't be visible. ] > > But memcg1_swapout()/swapin() are special in that they are completely > separate, v1-specific memcg entry points. The same is not true for the > other occurrences: Information overload! Thank you for going to the trouble of explaining those other cases, appreciated, but by "thought you could go even further", all I had meant was that the do_memsw_account() checks in memcg1_swap*() looked redundant to me; and possibly some other do_memsw_account() checks. I'll say no more, I don't want to expose my memcg2 ignorance further, and I don't deserve another reply. Hugh > > - mem_cgroup_margin(): > - mem_cgroup_get_max(): > > The v1 part is about half the function in both cases. We could > split that out into a v1 subfunction, but IMO at a relatively > high cost to the readability of the v1 control flow. > > - drain_stock: > - try_charge_memcg: > - uncharge_batch: > - mem_cgroup_replace_folio: > - __mem_cgroup_try_charge_swap: > - __mem_cgroup_uncharge_swap: > - mem_cgroup_get_nr_swap_pages: > - mem_cgroup_swap_full: > > The majority of the code applies to v2 or both versions, and > the v1 checks either cause an early return or guard the update > to the memsw page_counter. > > So not much to farm out code-wise. And the test uses a static > branch, so not much overhead to be cut either.