Received-SPF: pass (google.com: domain of linux-kernel+bounces-98119-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <2d508574-c2b8-489b-a26d-71b1c36961cf@linux.dev>
Date: Sun, 10 Mar 2024 05:53:26 +0100
Precedence: bulk
MIME-Version: 1.0
Subject: Re: [PATCH] RDMA/siw: Reuse value read using READ_ONCE instead of
 re-reading it
To: linke li <lilinke99@qq.com>
Cc: Bernard Metzler <bmt@zurich.ibm.com>, Jason Gunthorpe <jgg@ziepe.ca>,
 Leon Romanovsky <leon@kernel.org>, linux-rdma@vger.kernel.org,
 linux-kernel@vger.kernel.org
References: <tencent_32C3AEB0599DF0A0010A862439636CDA2707@qq.com>
From: Zhu Yanjun <yanjun.zhu@linux.dev>
In-Reply-To: <tencent_32C3AEB0599DF0A0010A862439636CDA2707@qq.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

在 2024/3/9 13:27, linke li 写道:
> In siw_orqe_start_rx, the orqe's flag in the if condition is read using
> READ_ONCE, checked, and then re-read, voiding all guarantees of the
> checks. Reuse the value that was read by READ_ONCE to ensure the
> consistency of the flags throughout the function.
> 
> Signed-off-by: linke li <lilinke99@qq.com>
> ---
>   drivers/infiniband/sw/siw/siw_qp_rx.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/siw/siw_qp_rx.c b/drivers/infiniband/sw/siw/siw_qp_rx.c
> index ed4fc39718b4..f5f69de56882 100644
> --- a/drivers/infiniband/sw/siw/siw_qp_rx.c
> +++ b/drivers/infiniband/sw/siw/siw_qp_rx.c
> @@ -740,6 +740,7 @@ static int siw_orqe_start_rx(struct siw_qp *qp)
>   {
>   	struct siw_sqe *orqe;
>   	struct siw_wqe *wqe = NULL;
> +	u16 orqe_flags;
>   
>   	if (unlikely(!qp->attrs.orq_size))
>   		return -EPROTO;
> @@ -748,7 +749,8 @@ static int siw_orqe_start_rx(struct siw_qp *qp)
>   	smp_mb();
>   
>   	orqe = orq_get_current(qp);
> -	if (READ_ONCE(orqe->flags) & SIW_WQE_VALID) {

In this if test, READ_ONCE is needed to read orqe->flags. But in this 
commit, this READ_ONCE is moved to other places.

In a complicated environment, for example, this function is called many 
times at the same time and orqe->flags is changed at the same time, I am 
not sure if this will introduce risks or not.

if you need to ensure the consistency of the flags throughout the 
function, not sure if the following is better or not.

if (((orqe_flags=READ_ONCE(orqe->flags))) & SIW_WQE_VALID) {

Thanks,
Zhu Yanjun

> +	orqe_flags = READ_ONCE(orqe->flags);
> +	if (orqe_flags & SIW_WQE_VALID) {
>   		/* RRESP is a TAGGED RDMAP operation */
>   		wqe = rx_wqe(&qp->rx_tagged);
>   		wqe->sqe.id = orqe->id;
> @@ -756,7 +758,7 @@ static int siw_orqe_start_rx(struct siw_qp *qp)
>   		wqe->sqe.sge[0].laddr = orqe->sge[0].laddr;
>   		wqe->sqe.sge[0].lkey = orqe->sge[0].lkey;
>   		wqe->sqe.sge[0].length = orqe->sge[0].length;
> -		wqe->sqe.flags = orqe->flags;
> +		wqe->sqe.flags = orqe_flags;
>   		wqe->sqe.num_sge = 1;
>   		wqe->bytes = orqe->sge[0].length;
>   		wqe->processed = 0;