[net] net/smc: Clear memory when release and reuse buffer

Commit Message

Tony Lu Nov. 25, 2021, 12:28 p.m. UTC

Currently, buffers are clear when smc create connections and reuse
buffer. It will slow down the speed of establishing new connection. In
most cases, the applications hope to establish connections as quickly as
possible.

This patch moves memset() from connection creation path to release and
buffer unuse path, this trades off between speed of establishing and
release.

Test environments:
- CPU Intel Xeon Platinum 8 core, mem 32 GiB, nic Mellanox CX4
- socket sndbuf / rcvbuf: 16384 / 131072 bytes
- w/o first round, 5 rounds, avg, 100 conns batch per round
- smc_buf_create() use bpftrace kprobe, introduces extra latency

Latency benchmarks for smc_buf_create():
  w/o patch : 19040.0 ns
  w/  patch :  1932.6 ns
  ratio :        10.2% (-89.8%)

Latency benchmarks for socket create and connect:
  w/o patch :   143.3 us
  w/  patch :   102.2 us
  ratio :        71.3% (-28.7%)

The latency of establishing connections is reduced by 28.7%.

Signed-off-by: Tony Lu <[e-mail]>
Reviewed-by: Wen Gu <[e-mail]>
---
 net/smc/smc_core.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

Comments

Jakub Kicinski Nov. 26, 2021, 7:28 p.m. UTC | #1

On Thu, 25 Nov 2021 20:28:59 +0800 Tony Lu wrote:
> Currently, buffers are clear when smc create connections and reuse
> buffer. It will slow down the speed of establishing new connection. In
> most cases, the applications hope to establish connections as quickly as
> possible.
> 
> This patch moves memset() from connection creation path to release and
> buffer unuse path, this trades off between speed of establishing and
> release.
> 
> Test environments:
> - CPU Intel Xeon Platinum 8 core, mem 32 GiB, nic Mellanox CX4
> - socket sndbuf / rcvbuf: 16384 / 131072 bytes
> - w/o first round, 5 rounds, avg, 100 conns batch per round
> - smc_buf_create() use bpftrace kprobe, introduces extra latency
> 
> Latency benchmarks for smc_buf_create():
>   w/o patch : 19040.0 ns
>   w/  patch :  1932.6 ns
>   ratio :        10.2% (-89.8%)
> 
> Latency benchmarks for socket create and connect:
>   w/o patch :   143.3 us
>   w/  patch :   102.2 us
>   ratio :        71.3% (-28.7%)
> 
> The latency of establishing connections is reduced by 28.7%.
> 
> Signed-off-by: Tony Lu <[e-mail]>
> Reviewed-by: Wen Gu <[e-mail]>

The tag in the subject seems incorrect, we tag things as [PATCH net] 
if they are fixes, and as [PATCH net-next] if they are new features,
code refactoring or performance improvements.

Is this a fix for a regression? In which case we need a Fixes tag to
indicate where it was introduced. Otherwise it needs to be tagged as
[PATCH net-next].

I'm assuming Karsten will take it via his tree, otherwise you'll need
to repost.

Karsten Graul Nov. 28, 2021, 9:42 p.m. UTC | #2

On 26/11/2021 20:28, Jakub Kicinski wrote:
> The tag in the subject seems incorrect, we tag things as [PATCH net] 
> if they are fixes, and as [PATCH net-next] if they are new features,
> code refactoring or performance improvements.
> 
> Is this a fix for a regression? In which case we need a Fixes tag to
> indicate where it was introduced. Otherwise it needs to be tagged as
> [PATCH net-next].
> 
> I'm assuming Karsten will take it via his tree, otherwise you'll need
> to repost.
> 

We are testing this change atm and will submit it via our tree.
Very nice change, I like it!

Tony Lu Nov. 30, 2021, 2:52 a.m. UTC | #3

On Fri, Nov 26, 2021 at 11:28:55AM -0800, Jakub Kicinski wrote:
> On Thu, 25 Nov 2021 20:28:59 +0800 Tony Lu wrote:
> > Currently, buffers are clear when smc create connections and reuse
> > buffer. It will slow down the speed of establishing new connection. In
> > most cases, the applications hope to establish connections as quickly as
> > possible.
> > 
> > This patch moves memset() from connection creation path to release and
> > buffer unuse path, this trades off between speed of establishing and
> > release.
> > 
> > Test environments:
> > - CPU Intel Xeon Platinum 8 core, mem 32 GiB, nic Mellanox CX4
> > - socket sndbuf / rcvbuf: 16384 / 131072 bytes
> > - w/o first round, 5 rounds, avg, 100 conns batch per round
> > - smc_buf_create() use bpftrace kprobe, introduces extra latency
> > 
> > Latency benchmarks for smc_buf_create():
> >   w/o patch : 19040.0 ns
> >   w/  patch :  1932.6 ns
> >   ratio :        10.2% (-89.8%)
> > 
> > Latency benchmarks for socket create and connect:
> >   w/o patch :   143.3 us
> >   w/  patch :   102.2 us
> >   ratio :        71.3% (-28.7%)
> > 
> > The latency of establishing connections is reduced by 28.7%.
> > 
> > Signed-off-by: Tony Lu <[e-mail]>
> > Reviewed-by: Wen Gu <[e-mail]>
> 
> The tag in the subject seems incorrect, we tag things as [PATCH net] 
> if they are fixes, and as [PATCH net-next] if they are new features,
> code refactoring or performance improvements.
> 
> Is this a fix for a regression? In which case we need a Fixes tag to
> indicate where it was introduced. Otherwise it needs to be tagged as
> [PATCH net-next].
> 
> I'm assuming Karsten will take it via his tree, otherwise you'll need
> to repost.

Sorry for the unclear tag. This patch introduces a performance
improvement. It should be with net-next.

I will fix it and send v2. Thank you.

Thanks,
Tony Lu

Karsten Graul Dec. 2, 2021, 2:23 p.m. UTC | #4

On 30/11/2021 03:52, Tony Lu wrote:
> Sorry for the unclear tag. This patch introduces a performance
> improvement. It should be with net-next.
> 
> I will fix it and send v2. Thank you.

Will you now send a v2 to net-next, or should I pick your v1 and 
submit it via our tree?

Tony Lu Dec. 3, 2021, 3:31 a.m. UTC | #5

On Thu, Dec 02, 2021 at 03:23:07PM +0100, Karsten Graul wrote:
> On 30/11/2021 03:52, Tony Lu wrote:
> > Sorry for the unclear tag. This patch introduces a performance
> > improvement. It should be with net-next.
> > 
> > I will fix it and send v2. Thank you.
> 
> Will you now send a v2 to net-next, or should I pick your v1 and 
> submit it via our tree?

Sorry about my unclear reply in the previous mail. It's nice to pick v1
to your tree. If v2 is needed, I will send it out soon. Thank you.

Thanks,
Tony Lu

Tony Lu Dec. 3, 2021, 3:47 a.m. UTC | #6

On Fri, Nov 26, 2021 at 11:28:55AM -0800, Jakub Kicinski wrote:
> On Thu, 25 Nov 2021 20:28:59 +0800 Tony Lu wrote:
> > Currently, buffers are clear when smc create connections and reuse
> > buffer. It will slow down the speed of establishing new connection. In
> > most cases, the applications hope to establish connections as quickly as
> > possible.
> > 
> > This patch moves memset() from connection creation path to release and
> > buffer unuse path, this trades off between speed of establishing and
> > release.
> > 
> > Test environments:
> > - CPU Intel Xeon Platinum 8 core, mem 32 GiB, nic Mellanox CX4
> > - socket sndbuf / rcvbuf: 16384 / 131072 bytes
> > - w/o first round, 5 rounds, avg, 100 conns batch per round
> > - smc_buf_create() use bpftrace kprobe, introduces extra latency
> > 
> > Latency benchmarks for smc_buf_create():
> >   w/o patch : 19040.0 ns
> >   w/  patch :  1932.6 ns
> >   ratio :        10.2% (-89.8%)
> > 
> > Latency benchmarks for socket create and connect:
> >   w/o patch :   143.3 us
> >   w/  patch :   102.2 us
> >   ratio :        71.3% (-28.7%)
> > 
> > The latency of establishing connections is reduced by 28.7%.
> > 
> > Signed-off-by: Tony Lu <[e-mail]>
> > Reviewed-by: Wen Gu <[e-mail]>
> 
> The tag in the subject seems incorrect, we tag things as [PATCH net] 
> if they are fixes, and as [PATCH net-next] if they are new features,
> code refactoring or performance improvements.
> 
> Is this a fix for a regression? In which case we need a Fixes tag to
> indicate where it was introduced. Otherwise it needs to be tagged as
> [PATCH net-next].
> 
> I'm assuming Karsten will take it via his tree, otherwise you'll need
> to repost.

Sorry to miss Graul’s email, he said he would put it in his own tree. I
would send v2 out if he needed. Thank you.

Thanks,
Tony Lu

Karsten Graul Dec. 3, 2021, 7:23 a.m. UTC | #7

On 03/12/2021 04:31, Tony Lu wrote:
> On Thu, Dec 02, 2021 at 03:23:07PM +0100, Karsten Graul wrote:
>> On 30/11/2021 03:52, Tony Lu wrote:
>>> Sorry for the unclear tag. This patch introduces a performance
>>> improvement. It should be with net-next.
>>>
>>> I will fix it and send v2. Thank you.
>>
>> Will you now send a v2 to net-next, or should I pick your v1 and 
>> submit it via our tree?
> 
> Sorry about my unclear reply in the previous mail. It's nice to pick v1
> to your tree. If v2 is needed, I will send it out soon. Thank you.
> 
> Thanks,
> Tony Lu
> 

Okay, I pick it up now. Thank you.

Patch

diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index bb52c8b5f148..5f0bd547907d 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -1102,18 +1102,24 @@  static void smcr_buf_unuse(struct smc_buf_desc *rmb_desc,
 		smc_buf_free(lgr, true, rmb_desc);
 	} else {
 		rmb_desc->used = 0;
+		memset(rmb_desc->cpu_addr, 0, rmb_desc->len);
 	}
 }
 
 static void smc_buf_unuse(struct smc_connection *conn,
 			  struct smc_link_group *lgr)
 {
-	if (conn->sndbuf_desc)
+	if (conn->sndbuf_desc) {
 		conn->sndbuf_desc->used = 0;
-	if (conn->rmb_desc && lgr->is_smcd)
+		memset(conn->sndbuf_desc->cpu_addr, 0, conn->sndbuf_desc->len);
+	}
+	if (conn->rmb_desc && lgr->is_smcd) {
 		conn->rmb_desc->used = 0;
-	else if (conn->rmb_desc)
+		memset(conn->rmb_desc->cpu_addr, 0, conn->rmb_desc->len +
+		       sizeof(struct smcd_cdc_msg));
+	} else if (conn->rmb_desc) {
 		smcr_buf_unuse(conn->rmb_desc, lgr);
+	}
 }
 
 /* remove a finished connection from its link group */
@@ -2149,7 +2155,6 @@  static int __smc_buf_create(struct smc_sock *smc, bool is_smcd, bool is_rmb)
 		if (buf_desc) {
 			SMC_STAT_RMB_SIZE(smc, is_smcd, is_rmb, bufsize);
 			SMC_STAT_BUF_REUSE(smc, is_smcd, is_rmb);
-			memset(buf_desc->cpu_addr, 0, bufsize);
 			break; /* found reusable slot */
 		}