Re: [net][PATCH 1/5] rds: fix reordering with composite message notification

[Yanjun Zhu <yanjun.zhu@oracle.com>]

On 2019/7/10 13:32, Santosh Shilimkar wrote:
> RDS composite message(rdma + control) user notification needs to be
> triggered once the full message is delivered and such a fix was
> added as part of commit 941f8d55f6d61 ("RDS: RDMA: Fix the composite
> message user notification"). But rds_send_remove_from_sock is missing
> data part notify check and hence at times the user don't get
> notification which isn't desirable.
>
> One way is to fix the rds_send_remove_from_sock to check of that case
> but considering the ordering complexity with completion handler and
> rdma + control messages are always dispatched back to back in same send
> context, just delaying the signaled completion on rmda work request also
> gets the desired behaviour. i.e Notifying application only after
> RDMA + control message send completes. So patch updates the earlier
> fix with this approach. The delay signaling completions of rdma op
> till the control message send completes fix was done by Venkat
> Venkatsubra in downstream kernel.
>
> Reviewed-and-tested-by: Zhu Yanjun <yanjun.zhu@oracle.com>

Thanks. I am fine with this.

Zhu Yanjun

> Reviewed-by: Gerd Rausch <gerd.rausch@oracle.com>
> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@oracle.com>
> ---
>   net/rds/ib_send.c | 29 +++++++++++++----------------
>   net/rds/rdma.c    | 10 ----------
>   net/rds/rds.h     |  1 -
>   net/rds/send.c    |  4 +---
>   4 files changed, 14 insertions(+), 30 deletions(-)
>
> diff --git a/net/rds/ib_send.c b/net/rds/ib_send.c
> index 18f2341..dfe6237 100644
> --- a/net/rds/ib_send.c
> +++ b/net/rds/ib_send.c
> @@ -69,6 +69,16 @@ static void rds_ib_send_complete(struct rds_message *rm,
>   	complete(rm, notify_status);
>   }
>   
> +static void rds_ib_send_unmap_data(struct rds_ib_connection *ic,
> +				   struct rm_data_op *op,
> +				   int wc_status)
> +{
> +	if (op->op_nents)
> +		ib_dma_unmap_sg(ic->i_cm_id->device,
> +				op->op_sg, op->op_nents,
> +				DMA_TO_DEVICE);
> +}
> +
>   static void rds_ib_send_unmap_rdma(struct rds_ib_connection *ic,
>   				   struct rm_rdma_op *op,
>   				   int wc_status)
> @@ -129,21 +139,6 @@ static void rds_ib_send_unmap_atomic(struct rds_ib_connection *ic,
>   		rds_ib_stats_inc(s_ib_atomic_fadd);
>   }
>   
> -static void rds_ib_send_unmap_data(struct rds_ib_connection *ic,
> -				   struct rm_data_op *op,
> -				   int wc_status)
> -{
> -	struct rds_message *rm = container_of(op, struct rds_message, data);
> -
> -	if (op->op_nents)
> -		ib_dma_unmap_sg(ic->i_cm_id->device,
> -				op->op_sg, op->op_nents,
> -				DMA_TO_DEVICE);
> -
> -	if (rm->rdma.op_active && rm->data.op_notify)
> -		rds_ib_send_unmap_rdma(ic, &rm->rdma, wc_status);
> -}
> -
>   /*
>    * Unmap the resources associated with a struct send_work.
>    *
> @@ -902,7 +897,9 @@ int rds_ib_xmit_rdma(struct rds_connection *conn, struct rm_rdma_op *op)
>   		send->s_queued = jiffies;
>   		send->s_op = NULL;
>   
> -		nr_sig += rds_ib_set_wr_signal_state(ic, send, op->op_notify);
> +		if (!op->op_notify)
> +			nr_sig += rds_ib_set_wr_signal_state(ic, send,
> +							     op->op_notify);
>   
>   		send->s_wr.opcode = op->op_write ? IB_WR_RDMA_WRITE : IB_WR_RDMA_READ;
>   		send->s_rdma_wr.remote_addr = remote_addr;
> diff --git a/net/rds/rdma.c b/net/rds/rdma.c
> index b340ed4..916f5ec 100644
> --- a/net/rds/rdma.c
> +++ b/net/rds/rdma.c
> @@ -641,16 +641,6 @@ int rds_cmsg_rdma_args(struct rds_sock *rs, struct rds_message *rm,
>   		}
>   		op->op_notifier->n_user_token = args->user_token;
>   		op->op_notifier->n_status = RDS_RDMA_SUCCESS;
> -
> -		/* Enable rmda notification on data operation for composite
> -		 * rds messages and make sure notification is enabled only
> -		 * for the data operation which follows it so that application
> -		 * gets notified only after full message gets delivered.
> -		 */
> -		if (rm->data.op_sg) {
> -			rm->rdma.op_notify = 0;
> -			rm->data.op_notify = !!(args->flags & RDS_RDMA_NOTIFY_ME);
> -		}
>   	}
>   
>   	/* The cookie contains the R_Key of the remote memory region, and
> diff --git a/net/rds/rds.h b/net/rds/rds.h
> index 0d8f67c..f0066d1 100644
> --- a/net/rds/rds.h
> +++ b/net/rds/rds.h
> @@ -476,7 +476,6 @@ struct rds_message {
>   		} rdma;
>   		struct rm_data_op {
>   			unsigned int		op_active:1;
> -			unsigned int		op_notify:1;
>   			unsigned int		op_nents;
>   			unsigned int		op_count;
>   			unsigned int		op_dmasg;
> diff --git a/net/rds/send.c b/net/rds/send.c
> index 166dd57..031b1e9 100644
> --- a/net/rds/send.c
> +++ b/net/rds/send.c
> @@ -491,14 +491,12 @@ void rds_rdma_send_complete(struct rds_message *rm, int status)
>   	struct rm_rdma_op *ro;
>   	struct rds_notifier *notifier;
>   	unsigned long flags;
> -	unsigned int notify = 0;
>   
>   	spin_lock_irqsave(&rm->m_rs_lock, flags);
>   
> -	notify =  rm->rdma.op_notify | rm->data.op_notify;
>   	ro = &rm->rdma;
>   	if (test_bit(RDS_MSG_ON_SOCK, &rm->m_flags) &&
> -	    ro->op_active && notify && ro->op_notifier) {
> +	    ro->op_active && ro->op_notify && ro->op_notifier) {
>   		notifier = ro->op_notifier;
>   		rs = rm->m_rs;
>   		sock_hold(rds_rs_to_sk(rs));