netdev - Re: [RFC PATCH net-next v4 10/11] net/smc: adapt cursor update when sndbuf and peer DMB are merged

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 21 Mar 2024 09:12:19 +0100
From: Jan Karcher <jaka@...ux.ibm.com>
To: Wen Gu <guwen@...ux.alibaba.com>, wintera@...ux.ibm.com,
        twinkler@...ux.ibm.com, hca@...ux.ibm.com, gor@...ux.ibm.com,
        agordeev@...ux.ibm.com, davem@...emloft.net, edumazet@...gle.com,
        kuba@...nel.org, pabeni@...hat.com, wenjia@...ux.ibm.com
Cc: borntraeger@...ux.ibm.com, svens@...ux.ibm.com, alibuda@...ux.alibaba.com,
        tonylu@...ux.alibaba.com, linux-kernel@...r.kernel.org,
        linux-s390@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [RFC PATCH net-next v4 10/11] net/smc: adapt cursor update when
 sndbuf and peer DMB are merged



On 17/03/2024 11:05, Wen Gu wrote:
> Since ghost sndbuf shares the same physical memory with peer DMB,
> the cursor update processing needs to be adapted to ensure that the
> data to be consumed won't be overwritten.
> 
> So in this case, the fin_curs and sndbuf_space that were originally
> updated after sending the CDC message should be modified to not be
> update until the peer updates cons_curs.
> 
> Signed-off-by: Wen Gu <guwen@...ux.alibaba.com>
> ---
>   net/smc/smc_cdc.c | 52 +++++++++++++++++++++++++++++++++++++----------
>   1 file changed, 41 insertions(+), 11 deletions(-)
> 
> diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
> index 3c06625ceb20..bf5b214ec15a 100644
> --- a/net/smc/smc_cdc.c
> +++ b/net/smc/smc_cdc.c
> @@ -18,6 +18,7 @@
>   #include "smc_tx.h"
>   #include "smc_rx.h"
>   #include "smc_close.h"
> +#include "smc_ism.h"
>   
>   /********************************** send *************************************/
>   
> @@ -255,17 +256,25 @@ int smcd_cdc_msg_send(struct smc_connection *conn)
>   		return rc;
>   	smc_curs_copy(&conn->rx_curs_confirmed, &curs, conn);
>   	conn->local_rx_ctrl.prod_flags.cons_curs_upd_req = 0;
> -	/* Calculate transmitted data and increment free send buffer space */
> -	diff = smc_curs_diff(conn->sndbuf_desc->len, &conn->tx_curs_fin,
> -			     &conn->tx_curs_sent);
> -	/* increased by confirmed number of bytes */
> -	smp_mb__before_atomic();
> -	atomic_add(diff, &conn->sndbuf_space);
> -	/* guarantee 0 <= sndbuf_space <= sndbuf_desc->len */
> -	smp_mb__after_atomic();
> -	smc_curs_copy(&conn->tx_curs_fin, &conn->tx_curs_sent, conn);
> +	if (!smc_ism_support_dmb_nocopy(conn->lgr->smcd)) {
> +		/* Ghost sndbuf shares the same memory region with
> +		 * peer DMB, so don't update the tx_curs_fin and
> +		 * sndbuf_space until peer has consumed the data.
> +		 */

Hi Wen Gu,

move this comment above the if please. Two consecutive multiline 
comments are difficult to read here.

> +		/* Calculate transmitted data and increment free
> +		 * send buffer space
> +		 */
> +		diff = smc_curs_diff(conn->sndbuf_desc->len, &conn->tx_curs_fin,
> +				     &conn->tx_curs_sent);
> +		/* increased by confirmed number of bytes */
> +		smp_mb__before_atomic();
> +		atomic_add(diff, &conn->sndbuf_space);
> +		/* guarantee 0 <= sndbuf_space <= sndbuf_desc->len */
> +		smp_mb__after_atomic();
> +		smc_curs_copy(&conn->tx_curs_fin, &conn->tx_curs_sent, conn);
>   
> -	smc_tx_sndbuf_nonfull(smc);
> +		smc_tx_sndbuf_nonfull(smc);
> +	}
>   	return rc;
>   }
>   
> @@ -323,7 +332,7 @@ static void smc_cdc_msg_recv_action(struct smc_sock *smc,
>   {
>   	union smc_host_cursor cons_old, prod_old;
>   	struct smc_connection *conn = &smc->conn;
> -	int diff_cons, diff_prod;
> +	int diff_cons, diff_prod, diff_tx;
>   
>   	smc_curs_copy(&prod_old, &conn->local_rx_ctrl.prod, conn);
>   	smc_curs_copy(&cons_old, &conn->local_rx_ctrl.cons, conn);
> @@ -339,6 +348,27 @@ static void smc_cdc_msg_recv_action(struct smc_sock *smc,
>   		atomic_add(diff_cons, &conn->peer_rmbe_space);
>   		/* guarantee 0 <= peer_rmbe_space <= peer_rmbe_size */
>   		smp_mb__after_atomic();
> +
> +		if (conn->lgr->is_smcd &&
> +		    smc_ism_support_dmb_nocopy(conn->lgr->smcd)) {
> +			/* Ghost sndbuf shares the same memory region with
> +			 * peer RMB, so update tx_curs_fin and sndbuf_space
> +			 * when peer has consumed the data.
> +			 */

Same as above.

Thanks
- Jan

> +			/* calculate peer rmb consumed data */
> +			diff_tx = smc_curs_diff(conn->sndbuf_desc->len,
> +						&conn->tx_curs_fin,
> +						&conn->local_rx_ctrl.cons);
> +			/* increase local sndbuf space and fin_curs */
> +			smp_mb__before_atomic();
> +			atomic_add(diff_tx, &conn->sndbuf_space);
> +			/* guarantee 0 <= sndbuf_space <= sndbuf_desc->len */
> +			smp_mb__after_atomic();
> +			smc_curs_copy(&conn->tx_curs_fin,
> +				      &conn->local_rx_ctrl.cons, conn);
> +
> +			smc_tx_sndbuf_nonfull(smc);
> +		}
>   	}
>   
>   	diff_prod = smc_curs_diff(conn->rmb_desc->len, &prod_old,