[PATCH 0/2] net: fix memcg overhead caused by sk->sk_forward_alloc size

[PATCH 0/2] net: fix memcg overhead caused by sk->sk_forward_alloc size

[Cathy Zhang]

Dear Reviewers,

memcg charge overhead is observed while we benchmark memcached with
memtier by running containers. It's caused by commit 4890b686f408 ("net:
Keep sk->sk_forward_alloc as small as possible"), which aims to reduce
system memory pressure, but makes the per-socket forward allocated
memory too small. The impact of this change is to trigger more
frequently memory allocation during TCP connection lifecycle and leads
to memcg charge overhead finally.

To avoid memcg charge overhead mentioned above, this series defines 64KB
as reclaim threshold when uncharging per-socket memory. It reduces the
frequency of memory allocation and charging during TCP connection, and
it's much less than the original 2MB per-socket reserved memory before
commit 4890b686f408 ("net: keep sk->sk_forward_alloc as small as
possibile"). Run memcached/memtier test with the 64KB reclaim threshold,
RPS gains around 2.01x.

This series also provides a new ABI /proc/sys/net/core/reclaim_threshold
with flexibility to tune the reclaim threshold according to system
running status.

This series is based on the latest tip/master tree.

Thanks for your time to help review and your feedback will be greatly!

Cathy Zhang (2):
  net: Keep sk->sk_forward_alloc as a proper size
  net: Add sysctl_reclaim_threshold

 Documentation/admin-guide/sysctl/net.rst | 12 ++++++++++++
 include/net/sock.h                       | 24 +++++++++++++++++++++++-
 net/core/sysctl_net_core.c               | 14 ++++++++++++++
 3 files changed, 49 insertions(+), 1 deletion(-)

2.34.1

[PATCH 1/2] net: Keep sk->sk_forward_alloc as a proper size

[Cathy Zhang]

Before commit 4890b686f408 ("net: keep sk->sk_forward_alloc as small as
possible"), each TCP can forward allocate up to 2 MB of memory and
tcp_memory_allocated might hit tcp memory limitation quite soon. To
reduce the memory pressure, that commit keeps sk->sk_forward_alloc as
small as possible, which will be less than 1 page size if SO_RESERVE_MEM
is not specified.

However, with commit 4890b686f408 ("net: keep sk->sk_forward_alloc as
small as possible"), memcg charge hot paths are observed while system is
stressed with a large amount of connections. That is because
sk->sk_forward_alloc is too small and it's always less than
sk->truesize, network handlers like tcp_rcv_established() should jump to
slow path more frequently to increase sk->sk_forward_alloc. Each memory
allocation will trigger memcg charge, then perf top shows the following
contention paths on the busy system.

    16.77%  [kernel]            [k] page_counter_try_charge
    16.56%  [kernel]            [k] page_counter_cancel
    15.65%  [kernel]            [k] try_charge_memcg

In order to avoid the memcg overhead and performance penalty,
sk->sk_forward_alloc should be kept with a proper size instead of as
small as possible. Keep memory up to 64KB from reclaims when uncharging
sk_buff memory, which is closer to the maximum size of sk_buff. It will
help reduce the frequency of allocating memory during TCP connection.
The original reclaim threshold for reserved memory per-socket is 2MB, so
the extraneous memory reserved now is about 32 times less than before
commit 4890b686f408 ("net: keep sk->sk_forward_alloc as small as
possible").

Run memcached with memtier_benchamrk to verify the optimization fix. 8
server-client pairs are created with bridge network on localhost, server
and client of the same pair share 28 logical CPUs.

Results (Average for 5 run)
RPS (with/without patch)	+2.01x

Fixes: 4890b686f408 ("net: keep sk->sk_forward_alloc as small as possible")

Signed-off-by: Cathy Zhang <cathy.zhang@intel.com>
Signed-off-by: Lizhen You <lizhen.you@intel.com>
Tested-by: Long Tao <tao.long@intel.com>
Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Reviewed-by: Tim Chen <tim.c.chen@linux.intel.com>
Reviewed-by: Suresh Srinivas <suresh.srinivas@intel.com>
---
 include/net/sock.h | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/include/net/sock.h b/include/net/sock.h
index 8b7ed7167243..6d2960479a80 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1657,12 +1657,33 @@ static inline void sk_mem_charge(struct sock *sk, int size)
 	sk->sk_forward_alloc -= size;
 }
 
+/* The following macro controls memory reclaiming in sk_mem_uncharge().
+ */
+#define SK_RECLAIM_THRESHOLD	(1 << 16)
 static inline void sk_mem_uncharge(struct sock *sk, int size)
 {
+	int reclaimable;
+
 	if (!sk_has_account(sk))
 		return;
 	sk->sk_forward_alloc += size;
-	sk_mem_reclaim(sk);
+
+	reclaimable = sk->sk_forward_alloc - sk_unused_reserved_mem(sk);
+
+	/* Reclaim memory to reduce memory pressure when multiple sockets
+	 * run in parallel. However, if we reclaim all pages and keep
+	 * sk->sk_forward_alloc as small as possible, it will cause
+	 * paths like tcp_rcv_established() going to the slow path with
+	 * much higher rate for forwarded memory expansion, which leads
+	 * to contention hot points and performance drop.
+	 *
+	 * In order to avoid the above issue, it's necessary to keep
+	 * sk->sk_forward_alloc with a proper size while doing reclaim.
+	 */
+	if (reclaimable > SK_RECLAIM_THRESHOLD) {
+		reclaimable -= SK_RECLAIM_THRESHOLD;
+		__sk_mem_reclaim(sk, reclaimable);
+	}
 }
 
 /*
-- 
2.34.1

[PATCH 2/2] net: Add sysctl_reclaim_threshold

[Cathy Zhang]

Add a new ABI /proc/sys/net/core/reclaim_threshold which allows to
change the size of reserved memory from reclaiming in sk_mem_uncharge.
It allows to keep sk->sk_forward_alloc as small as possible when
system is under memory pressure, it also allows to change it larger to
avoid memcg charge overhead and improve performance when system is not
under memory pressure. The original reclaim threshold for reserved
memory per-socket is 2MB, it's selected as the max value, while the
default value is 64KB which is closer to the maximum size of sk_buff.

Issue the following command as root to change the default value:

	echo 16384 > /proc/sys/net/core/reclaim_threshold

Signed-off-by: Cathy Zhang <cathy.zhang@intel.com>
Signed-off-by: Lizhen You <lizhen.you@intel.com>
Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Reviewed-by: Tim Chen <tim.c.chen@linux.intel.com>
Reviewed-by: Suresh Srinivas <suresh.srinivas@intel.com>
---
 Documentation/admin-guide/sysctl/net.rst | 12 ++++++++++++
 include/net/sock.h                       | 11 ++++++-----
 net/core/sysctl_net_core.c               | 14 ++++++++++++++
 3 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/Documentation/admin-guide/sysctl/net.rst b/Documentation/admin-guide/sysctl/net.rst
index 466c560b0c30..2981278af3d9 100644
--- a/Documentation/admin-guide/sysctl/net.rst
+++ b/Documentation/admin-guide/sysctl/net.rst
@@ -413,6 +413,18 @@ historical importance.
 
 Default: 0
 
+reclaim_threshold
+------------------------
+
+The threshold indicates when it can start to reclaim memory during a TCP
+connection lifecycle. If the per-socket forward allocated memory is beyond the
+threshold, it will reclaim the part exceeding this value. It could help keep
+per-socket forward allocated memory with a proper size to improve performance
+and make system away from memory pressure meanwhile. The threshold value is
+allowed to be changed in [4096, 2097152].
+
+Default: 64 KB
+
 2. /proc/sys/net/unix - Parameters for Unix domain sockets
 ----------------------------------------------------------
 
diff --git a/include/net/sock.h b/include/net/sock.h
index 6d2960479a80..bd8162ed2056 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -89,6 +89,8 @@ void SOCK_DEBUG(const struct sock *sk, const char *msg, ...)
 }
 #endif
 
+extern unsigned int sysctl_reclaim_threshold __read_mostly;
+
 /* This is the per-socket lock.  The spinlock provides a synchronization
  * between user contexts and software interrupt processing, whereas the
  * mini-semaphore synchronizes multiple users amongst themselves.
@@ -1657,12 +1659,10 @@ static inline void sk_mem_charge(struct sock *sk, int size)
 	sk->sk_forward_alloc -= size;
 }
 
-/* The following macro controls memory reclaiming in sk_mem_uncharge().
- */
-#define SK_RECLAIM_THRESHOLD	(1 << 16)
 static inline void sk_mem_uncharge(struct sock *sk, int size)
 {
 	int reclaimable;
+	int reclaim_threshold;
 
 	if (!sk_has_account(sk))
 		return;
@@ -1680,8 +1680,9 @@ static inline void sk_mem_uncharge(struct sock *sk, int size)
 	 * In order to avoid the above issue, it's necessary to keep
 	 * sk->sk_forward_alloc with a proper size while doing reclaim.
 	 */
-	if (reclaimable > SK_RECLAIM_THRESHOLD) {
-		reclaimable -= SK_RECLAIM_THRESHOLD;
+	reclaim_threshold = READ_ONCE(sysctl_reclaim_threshold);
+	if (reclaimable > reclaim_threshold) {
+		reclaimable -= reclaim_threshold;
 		__sk_mem_reclaim(sk, reclaimable);
 	}
 }
diff --git a/net/core/sysctl_net_core.c b/net/core/sysctl_net_core.c
index 782273bb93c2..82aee37769ba 100644
--- a/net/core/sysctl_net_core.c
+++ b/net/core/sysctl_net_core.c
@@ -46,6 +46,11 @@ EXPORT_SYMBOL(sysctl_fb_tunnels_only_for_init_net);
 int sysctl_devconf_inherit_init_net __read_mostly;
 EXPORT_SYMBOL(sysctl_devconf_inherit_init_net);
 
+static unsigned int min_reclaim = PAGE_SIZE;
+static unsigned int max_reclaim = 2 * 1024 * 1024;
+unsigned int sysctl_reclaim_threshold __read_mostly = 64 * 1024;
+EXPORT_SYMBOL(sysctl_reclaim_threshold);
+
 #if IS_ENABLED(CONFIG_NET_FLOW_LIMIT) || IS_ENABLED(CONFIG_RPS)
 static void dump_cpumask(void *buffer, size_t *lenp, loff_t *ppos,
 			 struct cpumask *mask)
@@ -407,6 +412,15 @@ static struct ctl_table net_core_table[] = {
 		.proc_handler	= proc_dointvec_minmax,
 		.extra1		= &min_rcvbuf,
 	},
+	{
+		.procname	= "reclaim_threshold",
+		.data		= &sysctl_reclaim_threshold,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_douintvec_minmax,
+		.extra1		= &min_reclaim,
+		.extra2		= &max_reclaim,
+	},
 	{
 		.procname	= "dev_weight",
 		.data		= &weight_p,
-- 
2.34.1

Re: [PATCH 2/2] net: Add sysctl_reclaim_threshold

[kernel test robot]

Hi Cathy,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]
[also build test ERROR on net/main horms-ipvs/master linus/master v6.3 next-20230505]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Cathy-Zhang/net-Keep-sk-sk_forward_alloc-as-a-proper-size/20230506-123121
base:   net-next/main
patch link:    https://lore.kernel.org/r/20230506042958.15051-3-cathy.zhang%40intel.com
patch subject: [PATCH 2/2] net: Add sysctl_reclaim_threshold
config: mips-randconfig-r033-20230430 (https://download.01.org/0day-ci/archive/20230506/202305061521.bAXb1ha9-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project b0fb98227c90adf2536c9ad644a74d5e92961111)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install mips cross compiling tool for clang build
        # apt-get install binutils-mips-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/24beb0b9f6d299a34b853699e5bcaa28a74cad5f
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Cathy-Zhang/net-Keep-sk-sk_forward_alloc-as-a-proper-size/20230506-123121
        git checkout 24beb0b9f6d299a34b853699e5bcaa28a74cad5f
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=mips olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305061521.bAXb1ha9-lkp@intel.com/

All errors (new ones prefixed by >>):

>> ld.lld: error: undefined symbol: sysctl_reclaim_threshold
   >>> referenced by sock.c
   >>>               net/core/sock.o:(sock_rfree) in archive vmlinux.a
   >>> referenced by sock.c
   >>>               net/core/sock.o:(sock_rfree) in archive vmlinux.a
   >>> referenced by filter.c
   >>>               net/core/filter.o:(bpf_msg_pop_data) in archive vmlinux.a
   >>> referenced 15 more times

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 2/2] net: Add sysctl_reclaim_threshold

[kernel test robot]

Hi Cathy,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]
[also build test ERROR on net/main horms-ipvs/master linus/master v6.3 next-20230505]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Cathy-Zhang/net-Keep-sk-sk_forward_alloc-as-a-proper-size/20230506-123121
base:   net-next/main
patch link:    https://lore.kernel.org/r/20230506042958.15051-3-cathy.zhang%40intel.com
patch subject: [PATCH 2/2] net: Add sysctl_reclaim_threshold
config: s390-randconfig-r034-20230504 (https://download.01.org/0day-ci/archive/20230506/202305061623.xkekJbaH-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project b0fb98227c90adf2536c9ad644a74d5e92961111)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install s390 cross compiling tool for clang build
        # apt-get install binutils-s390x-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/24beb0b9f6d299a34b853699e5bcaa28a74cad5f
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Cathy-Zhang/net-Keep-sk-sk_forward_alloc-as-a-proper-size/20230506-123121
        git checkout 24beb0b9f6d299a34b853699e5bcaa28a74cad5f
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305061623.xkekJbaH-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

WARNING: modpost: vmlinux.o: section mismatch in reference: __next_mem_range (section: .text) -> memblock (section: .meminit.data)
WARNING: modpost: vmlinux.o: section mismatch in reference: __next_mem_range (section: .text) -> memblock (section: .meminit.data)
ERROR: modpost: "devm_platform_ioremap_resource" [drivers/char/xillybus/xillybus_of.ko] undefined!
>> ERROR: modpost: "sysctl_reclaim_threshold" [net/sctp/sctp.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

RE: [PATCH 2/2] net: Add sysctl_reclaim_threshold

[Zhang, Cathy]

Yep, the patch is wrongly applied to tip git, sorry for that! Let me rebase it to net git and re-send then.

> -----Original Message-----
> From: lkp <lkp@intel.com>
> Sent: Saturday, May 6, 2023 4:04 PM
> To: Zhang, Cathy <cathy.zhang@intel.com>; edumazet@google.com;
> davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com
> Cc: llvm@lists.linux.dev; oe-kbuild-all@lists.linux.dev; Brandeburg, Jesse
> <jesse.brandeburg@intel.com>; Srinivas, Suresh
> <suresh.srinivas@intel.com>; Chen, Tim C <tim.c.chen@intel.com>; You,
> Lizhen <lizhen.you@intel.com>; Zhang, Cathy <cathy.zhang@intel.com>;
> eric.dumazet@gmail.com; netdev@vger.kernel.org
> Subject: Re: [PATCH 2/2] net: Add sysctl_reclaim_threshold
> 
> Hi Cathy,
> 
> kernel test robot noticed the following build errors:
> 
> [auto build test ERROR on net-next/main] [also build test ERROR on net/main
> horms-ipvs/master linus/master v6.3 next-20230505] [If your patch is
> applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Cathy-Zhang/net-Keep-
> sk-sk_forward_alloc-as-a-proper-size/20230506-123121
> base:   net-next/main
> patch link:    https://lore.kernel.org/r/20230506042958.15051-3-
> cathy.zhang%40intel.com
> patch subject: [PATCH 2/2] net: Add sysctl_reclaim_threshold
> config: mips-randconfig-r033-20230430 (https://download.01.org/0day-
> ci/archive/20230506/202305061521.bAXb1ha9-lkp@intel.com/config)
> compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project
> b0fb98227c90adf2536c9ad644a74d5e92961111)
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-
> tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # install mips cross compiling tool for clang build
>         # apt-get install binutils-mips-linux-gnu
>         # https://github.com/intel-lab-
> lkp/linux/commit/24beb0b9f6d299a34b853699e5bcaa28a74cad5f
>         git remote add linux-review https://github.com/intel-lab-lkp/linux
>         git fetch --no-tags linux-review Cathy-Zhang/net-Keep-sk-
> sk_forward_alloc-as-a-proper-size/20230506-123121
>         git checkout 24beb0b9f6d299a34b853699e5bcaa28a74cad5f
>         # save the config file
>         mkdir build_dir && cp config build_dir/.config
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross
> W=1 O=build_dir ARCH=mips olddefconfig
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross
> W=1 O=build_dir ARCH=mips SHELL=/bin/bash
> 
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <lkp@intel.com>
> | Link:
> | https://lore.kernel.org/oe-kbuild-all/202305061521.bAXb1ha9-lkp@intel.
> | com/
> 
> All errors (new ones prefixed by >>):
> 
> >> ld.lld: error: undefined symbol: sysctl_reclaim_threshold
>    >>> referenced by sock.c
>    >>>               net/core/sock.o:(sock_rfree) in archive vmlinux.a
>    >>> referenced by sock.c
>    >>>               net/core/sock.o:(sock_rfree) in archive vmlinux.a
>    >>> referenced by filter.c
>    >>>               net/core/filter.o:(bpf_msg_pop_data) in archive vmlinux.a
>    >>> referenced 15 more times
> 
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests

RE: [PATCH 1/2] net: Keep sk->sk_forward_alloc as a proper size

[Chen, Tim C]

+
+	/* Reclaim memory to reduce memory pressure when multiple sockets

Improper comment style.
	/* 
	  * comment
	  */


+	 * run in parallel. However, if we reclaim all pages and keep
+	 * sk->sk_forward_alloc as small as possible, it will cause
+	 * paths like tcp_rcv_established() going to the slow path with
+	 * much higher rate for forwarded memory expansion, which leads
+	 * to contention hot points and performance drop.
+	 *
+	 * In order to avoid the above issue, it's necessary to keep
+	 * sk->sk_forward_alloc with a proper size while doing reclaim.
+	 */
+	if (reclaimable > SK_RECLAIM_THRESHOLD) {
+		reclaimable -= SK_RECLAIM_THRESHOLD;
+		__sk_mem_reclaim(sk, reclaimable);
+	}
 }
 
 /*
--
2.34.1

RE: [PATCH 1/2] net: Keep sk->sk_forward_alloc as a proper size

[Chen, Tim C]

Never mind. networking code has its own comment style according to
https://www.kernel.org/doc/html/v4.10/process/coding-style.html

Tim

-----Original Message-----
From: Chen, Tim C 
Sent: Monday, May 8, 2023 4:17 PM
To: Zhang, Cathy <cathy.zhang@intel.com>; edumazet@google.com; davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com
Cc: Brandeburg, Jesse <jesse.brandeburg@intel.com>; Srinivas, Suresh <suresh.srinivas@intel.com>; You, Lizhen <Lizhen.You@intel.com>; eric.dumazet@gmail.com; netdev@vger.kernel.org
Subject: RE: [PATCH 1/2] net: Keep sk->sk_forward_alloc as a proper size

+
+	/* Reclaim memory to reduce memory pressure when multiple sockets

Improper comment style.
	/* 
	  * comment
	  */


+	 * run in parallel. However, if we reclaim all pages and keep
+	 * sk->sk_forward_alloc as small as possible, it will cause
+	 * paths like tcp_rcv_established() going to the slow path with
+	 * much higher rate for forwarded memory expansion, which leads
+	 * to contention hot points and performance drop.
+	 *
+	 * In order to avoid the above issue, it's necessary to keep
+	 * sk->sk_forward_alloc with a proper size while doing reclaim.
+	 */
+	if (reclaimable > SK_RECLAIM_THRESHOLD) {
+		reclaimable -= SK_RECLAIM_THRESHOLD;
+		__sk_mem_reclaim(sk, reclaimable);
+	}
 }
 
 /*
--
2.34.1