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 49A24C5320E for ; Tue, 27 Aug 2024 04:45:35 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id A3FC76B007B; Tue, 27 Aug 2024 00:45:34 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 9EF7F6B0082; Tue, 27 Aug 2024 00:45:34 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 8B6CD6B0083; Tue, 27 Aug 2024 00:45:34 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 6C08D6B007B for ; Tue, 27 Aug 2024 00:45:34 -0400 (EDT) Received: from smtpin03.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id DE5B080445 for ; Tue, 27 Aug 2024 04:45:33 +0000 (UTC) X-FDA: 82496786946.03.FC2796D Received: from out-177.mta1.migadu.com (out-177.mta1.migadu.com [95.215.58.177]) by imf27.hostedemail.com (Postfix) with ESMTP id 0CEEF40009 for ; Tue, 27 Aug 2024 04:45:31 +0000 (UTC) Authentication-Results: imf27.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=ezfFWOer; spf=pass (imf27.hostedemail.com: domain of roman.gushchin@linux.dev designates 95.215.58.177 as permitted sender) smtp.mailfrom=roman.gushchin@linux.dev; dmarc=pass (policy=none) header.from=linux.dev ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1724733845; 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=DXh+mPkqGMQUdQjrnpg7XgHV33cM/bi2yysc+Jzmln0=; b=qndMxqW6go23NTvfEfXTDrkqzq6W1MDxYO8FN5mj6Pn+te/keJK12AQiw+OmkAhu2C45iw pdRyrq3NftTAIws/DlO5fVrKe6lI8gmClVepnfF2ZndEk/WgNnIMMIEfFMSWGwUV0avgf6 a+qV9upHfpwi6cmqMsKZUpCrDq4wNbM= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1724733845; a=rsa-sha256; cv=none; b=RNEHHm8m2xRfuubk2yoF0qTN4RdqQrbG+N/NRB8v72dZNdKeyrr+bJD715qg84j+52nCzX 8+8tA25PIQgf7K5SUm4apjWZtxM4ded24E0HBpmvS1uMZ9I6yQwYkuDuJh9oN0zvtXHpY8 iLBCbe3KCIMjL6otEXHgdZ+cdma+iJM= ARC-Authentication-Results: i=1; imf27.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=ezfFWOer; spf=pass (imf27.hostedemail.com: domain of roman.gushchin@linux.dev designates 95.215.58.177 as permitted sender) smtp.mailfrom=roman.gushchin@linux.dev; dmarc=pass (policy=none) header.from=linux.dev Date: Tue, 27 Aug 2024 03:06:32 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1724728000; h=from:from:reply-to:subject:subject: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=DXh+mPkqGMQUdQjrnpg7XgHV33cM/bi2yysc+Jzmln0=; b=ezfFWOer/4J1f3LXiOgSI2x7/WD2K4Tj/axgZi0hvYcXS9HJrGHaEtqPnOWYpSKguR1ol4 VMU24i7iXa71hoYOhRN3u+eArz/j46HMgVWxB+C20b5sDgk++ZP4Hh60kAJsARl6SmRo5J XONw1klJ3MhxuiP1eaTEltshraeknKc= X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Roman Gushchin To: Shakeel Butt Cc: Andrew Morton , Johannes Weiner , Michal Hocko , Muchun Song , Vlastimil Babka , David Rientjes , Hyeonggon Yoo <42.hyeyoo@gmail.com>, Eric Dumazet , "David S . Miller" , Jakub Kicinski , Paolo Abeni , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Meta kernel team , cgroups@vger.kernel.org, netdev@vger.kernel.org Subject: Re: [PATCH v1] memcg: add charging of already allocated slab objects Message-ID: References: <20240826232908.4076417-1-shakeel.butt@linux.dev> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240826232908.4076417-1-shakeel.butt@linux.dev> X-Migadu-Flow: FLOW_OUT X-Stat-Signature: 4yphmizb1nz91xtuso1ncyon5z8hyr8p X-Rspamd-Queue-Id: 0CEEF40009 X-Rspam-User: X-Rspamd-Server: rspam08 X-HE-Tag: 1724733931-641047 X-HE-Meta: U2FsdGVkX19sjUz+XxIjVH9YRKUmjvvZn6SsLSi6ggdmadE+AaUhjB1fNTVsM0VoaTr36WPLs79JYkit4cNKoWu4YWnMvF4891ExxSjL7z0CmrE9xI+Pq3UNlm+b8h8j2my6MkPxAZJPAXO+bUMosbyxentHzff1YWKtQjt+FsT7p8vXnazCeX2+Zs6lrVxIihzGXw2dHBQamLt6+lX/3KScq27Gsi72H6BPkipW3Bf/VJdDJZt0j2E3v4BrtR29MNnv3NRu9BBphIP12rNHVNRxqyh4gWYE+9fRDqAW1GJBFGuHV099F7UpVReoBzLdsTPGf63DdquKhiHkQ47K19g89c7ChVPv4IKeLI4w5RIWy5i0qXKscZn3T4dzXXpj5G1uzhUP6CrxWZTm9m3u5RE4QiAi86zj7uzCi1DCE0On4gjFfR9Z36lKyTBd8fNc6k4b+Hr5tF6XXDKg3d04rPZMbHOp+Izx2aLtKelPZkcUt/ZKlZ0EWERQ15fsq+SX24ppgzE1z9JcCtBW3f70XVqbMrizT3osQUUfi6nCc0lQ2EtPsR1OtRmEEIn/MO+ZKfAV8m9M1dNrNaOku7X6WodsJCsKAASUNtAo+9803XL/UMj8txXX8vaJWaggZ9Hqpu8f+rUcmVQZeYXmnAt91ppBaVotdxRJEJ23DKpHXLsGp5aApnGm1XXf03arXB9d+XwI2HqTK3OlxqgSABw3aMu6uEW0UaPPZUnyYqmtHVqYp0e9yDUgDWzpYteN26eWp3BxW8I9aHf4pRMnuzHiZ/qMNYk+hKvK+JxHb1YPcFc0xTAvPVBXX458iHwvFuEd1MIb4LGGGY7PtBQfc0gI8c5FegQrhNQhlpW448OnjWzVyVTrqYrucuLFTdbM9KX6dZoASBOmIMrm0RstCGpRhmQNuHmtFzBcO8qH44/s917pC870vnY4hhvNBrm55edIMehULDCvHQU4xKWgVXM Gp+lajwr 3NnWyaiXlcvQuK+lw5LcgvN3lq49C89tc9UOf8Fc8ofdsl/xHZuuDyHUgyl6npYfbJ82r+pq+BXZBId7mvVG0mp8kwsQVOyKEkCggp+h8YcrBVmkbzgOKPbYzOxzrCq/kFNQ5JCDEx7WAlph2F2u8HZgIM+U93turdImo5ERId4Nw3QlPEqvLKgqG3wvu8wKB4mAXuu2DgOnpwc/TQavy91+ZJVUgiimap+QLpksWLv3++JRN1pJp7hFaK8ZVCE9Bv55q 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: List-Subscribe: List-Unsubscribe: On Mon, Aug 26, 2024 at 04:29:08PM -0700, Shakeel Butt wrote: > At the moment, the slab objects are charged to the memcg at the > allocation time. However there are cases where slab objects are > allocated at the time where the right target memcg to charge it to is > not known. One such case is the network sockets for the incoming > connection which are allocated in the softirq context. > > Couple hundred thousand connections are very normal on large loaded > server and almost all of those sockets underlying those connections get > allocated in the softirq context and thus not charged to any memcg. > However later at the accept() time we know the right target memcg to > charge. Let's add new API to charge already allocated objects, so we can > have better accounting of the memory usage. > > To measure the performance impact of this change, tcp_crr is used from > the neper [1] performance suite. Basically it is a network ping pong > test with new connection for each ping pong. > > The server and the client are run inside 3 level of cgroup hierarchy > using the following commands: > > Server: > $ tcp_crr -6 > > Client: > $ tcp_crr -6 -c -H ${server_ip} > > If the client and server run on different machines with 50 GBPS NIC, > there is no visible impact of the change. > > For the same machine experiment with v6.11-rc5 as base. > > base (throughput) with-patch > tcp_crr 14545 (+- 80) 14463 (+- 56) > > It seems like the performance impact is within the noise. > > Link: https://github.com/google/neper [1] > Signed-off-by: Shakeel Butt Hi Shakeel, I like the idea and performance numbers look good. However some comments on the implementation: > --- > > Changes since the RFC: > - Added check for already charged slab objects. > - Added performance results from neper's tcp_crr > > include/linux/slab.h | 1 + > mm/slub.c | 54 +++++++++++++++++++++++++++++++++ > net/ipv4/inet_connection_sock.c | 5 +-- > 3 files changed, 58 insertions(+), 2 deletions(-) > > diff --git a/include/linux/slab.h b/include/linux/slab.h > index eb2bf4629157..05cfab107c72 100644 > --- a/include/linux/slab.h > +++ b/include/linux/slab.h > @@ -547,6 +547,7 @@ void *kmem_cache_alloc_lru_noprof(struct kmem_cache *s, struct list_lru *lru, > gfp_t gfpflags) __assume_slab_alignment __malloc; > #define kmem_cache_alloc_lru(...) alloc_hooks(kmem_cache_alloc_lru_noprof(__VA_ARGS__)) > > +bool kmem_cache_charge(void *objp, gfp_t gfpflags); > void kmem_cache_free(struct kmem_cache *s, void *objp); > > kmem_buckets *kmem_buckets_create(const char *name, slab_flags_t flags, > diff --git a/mm/slub.c b/mm/slub.c > index c9d8a2497fd6..580683597b5c 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -2185,6 +2185,16 @@ void memcg_slab_free_hook(struct kmem_cache *s, struct slab *slab, void **p, > > __memcg_slab_free_hook(s, slab, p, objects, obj_exts); > } > + > +static __fastpath_inline > +bool memcg_slab_post_charge(struct kmem_cache *s, void *p, gfp_t flags) > +{ > + if (likely(!memcg_kmem_online())) > + return true; We do have this check in kmem_cache_charge(), why do we need to check it again? > + > + return __memcg_slab_post_alloc_hook(s, NULL, flags, 1, &p); > +} > + > #else /* CONFIG_MEMCG */ > static inline bool memcg_slab_post_alloc_hook(struct kmem_cache *s, > struct list_lru *lru, > @@ -2198,6 +2208,13 @@ static inline void memcg_slab_free_hook(struct kmem_cache *s, struct slab *slab, > void **p, int objects) > { > } > + > +static inline bool memcg_slab_post_charge(struct kmem_cache *s, > + void *p, > + gfp_t flags) > +{ > + return true; > +} > #endif /* CONFIG_MEMCG */ > > /* > @@ -4062,6 +4079,43 @@ void *kmem_cache_alloc_lru_noprof(struct kmem_cache *s, struct list_lru *lru, > } > EXPORT_SYMBOL(kmem_cache_alloc_lru_noprof); > > +#define KMALLOC_TYPE (SLAB_KMALLOC | SLAB_CACHE_DMA | \ > + SLAB_ACCOUNT | SLAB_RECLAIM_ACCOUNT) > + > +bool kmem_cache_charge(void *objp, gfp_t gfpflags) > +{ > + struct slabobj_ext *slab_exts; > + struct kmem_cache *s; > + struct folio *folio; > + struct slab *slab; > + unsigned long off; > + > + if (!memcg_kmem_online()) > + return true; > + > + folio = virt_to_folio(objp); > + if (unlikely(!folio_test_slab(folio))) > + return false; Does it handle the case of a too-big-to-be-a-slab-object allocation? I think it's better to handle it properly. Also, why return false here? > + > + slab = folio_slab(folio); > + s = slab->slab_cache; > + > + /* Ignore KMALLOC_NORMAL cache to avoid circular dependency. */ > + if ((s->flags & KMALLOC_TYPE) == SLAB_KMALLOC) > + return true; And true here? It seems to be a bit inconsistent. Also, if we have this check here, it means your function won't handle kmallocs at all? Because !KMALLOC_NORMAL allocations won't get here. > + > + /* Ignore already charged objects. */ > + slab_exts = slab_obj_exts(slab); > + if (slab_exts) { > + off = obj_to_index(s, slab, objp); > + if (unlikely(slab_exts[off].objcg)) > + return true; > + } > + > + return memcg_slab_post_charge(s, objp, gfpflags); > +} > +EXPORT_SYMBOL(kmem_cache_charge); > + > /** > * kmem_cache_alloc_node - Allocate an object on the specified node > * @s: The cache to allocate from. > diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c > index 64d07b842e73..3c13ca8c11fb 100644 > --- a/net/ipv4/inet_connection_sock.c > +++ b/net/ipv4/inet_connection_sock.c > @@ -715,6 +715,7 @@ struct sock *inet_csk_accept(struct sock *sk, struct proto_accept_arg *arg) > release_sock(sk); > if (newsk && mem_cgroup_sockets_enabled) { > int amt = 0; > + gfp_t gfp = GFP_KERNEL | __GFP_NOFAIL; > > /* atomically get the memory usage, set and charge the > * newsk->sk_memcg. > @@ -731,8 +732,8 @@ struct sock *inet_csk_accept(struct sock *sk, struct proto_accept_arg *arg) > } > > if (amt) > - mem_cgroup_charge_skmem(newsk->sk_memcg, amt, > - GFP_KERNEL | __GFP_NOFAIL); > + mem_cgroup_charge_skmem(newsk->sk_memcg, amt, gfp); > + kmem_cache_charge(newsk, gfp); Wait, so we assume that newsk->sk_memcg === current memcg? Or we're ok with them being different? Thanks!