[PATCH net 1/2] rds: ib: Increment i_fastreg_wrs before bailing out

[PATCH net 1/2] rds: ib: Increment i_fastreg_wrs before bailing out

[Håkon Bugge]

We need to increment i_fastreg_wrs before we bail out from
rds_ib_post_reg_frmr().

Fixes: 1659185fb4d0 ("RDS: IB: Support Fastreg MR (FRMR) memory registration mode")
Fixes: 3a2886cca703 ("net/rds: Keep track of and wait for FRWR segments in use upon shutdown")
Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
---
 net/rds/ib_frmr.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/net/rds/ib_frmr.c b/net/rds/ib_frmr.c
index 28c1b00221780..7e3b04a83904d 100644
--- a/net/rds/ib_frmr.c
+++ b/net/rds/ib_frmr.c
@@ -133,12 +133,15 @@ static int rds_ib_post_reg_frmr(struct rds_ib_mr *ibmr)
 
 	ret = ib_map_mr_sg_zbva(frmr->mr, ibmr->sg, ibmr->sg_dma_len,
 				&off, PAGE_SIZE);
-	if (unlikely(ret != ibmr->sg_dma_len))
-		return ret < 0 ? ret : -EINVAL;
+	if (unlikely(ret != ibmr->sg_dma_len)) {
+		ret = ret < 0 ? ret : -EINVAL;
+		goto out_inc;
+	}
 
-	if (cmpxchg(&frmr->fr_state,
-		    FRMR_IS_FREE, FRMR_IS_INUSE) != FRMR_IS_FREE)
-		return -EBUSY;
+	if (cmpxchg(&frmr->fr_state, FRMR_IS_FREE, FRMR_IS_INUSE) != FRMR_IS_FREE) {
+		ret = -EBUSY;
+		goto out_inc;
+	}
 
 	atomic_inc(&ibmr->ic->i_fastreg_inuse_count);
 
@@ -178,9 +181,11 @@ static int rds_ib_post_reg_frmr(struct rds_ib_mr *ibmr)
 	 * being accessed while registration is still pending.
 	 */
 	wait_event(frmr->fr_reg_done, !frmr->fr_reg);
-
 out:
+	return ret;
 
+out_inc:
+	atomic_inc(&ibmr->ic->i_fastreg_wrs);
 	return ret;
 }
 
-- 
2.43.5

[PATCH net 2/2] rds: ib: Remove unused extern definition

[Håkon Bugge]

Fixes: 2cb2912d6563 ("RDS: IB: add Fastreg MR (FRMR) detection support")
Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
---
 net/rds/ib_mr.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/rds/ib_mr.h b/net/rds/ib_mr.h
index ea5e9aee4959e..5884de8c6f45b 100644
--- a/net/rds/ib_mr.h
+++ b/net/rds/ib_mr.h
@@ -108,7 +108,6 @@ struct rds_ib_mr_pool {
 };
 
 extern struct workqueue_struct *rds_ib_mr_wq;
-extern bool prefer_frmr;
 
 struct rds_ib_mr_pool *rds_ib_create_mr_pool(struct rds_ib_device *rds_dev,
 					     int npages);
-- 
2.43.5

Re: [PATCH net 2/2] rds: ib: Remove unused extern definition

[Greg KH]

On Wed, Sep 03, 2025 at 06:31:37PM +0200, Håkon Bugge wrote:
> Fixes: 2cb2912d6563 ("RDS: IB: add Fastreg MR (FRMR) detection support")
> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
> ---
>  net/rds/ib_mr.h | 1 -
>  1 file changed, 1 deletion(-)

I know I can't take patches without any changelog text, but maybe other
maintainer are more lax :)

good luck!

greg k-h

Re: [PATCH net 2/2] rds: ib: Remove unused extern definition

[Haakon Bugge]

> On 3 Sep 2025, at 18:38, Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> On Wed, Sep 03, 2025 at 06:31:37PM +0200, Håkon Bugge wrote:
>> Fixes: 2cb2912d6563 ("RDS: IB: add Fastreg MR (FRMR) detection support")
>> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
>> ---
>> net/rds/ib_mr.h | 1 -
>> 1 file changed, 1 deletion(-)
> 
> I know I can't take patches without any changelog text, but maybe other
> maintainer are more lax :)

You mean the empty commit message? Did pass checkpatch.pl --strict though.


Thxs, Håkon

Re: [PATCH net 2/2] rds: ib: Remove unused extern definition

[Greg KH]

On Wed, Sep 03, 2025 at 04:51:01PM +0000, Haakon Bugge wrote:
> 
> 
> > On 3 Sep 2025, at 18:38, Greg KH <gregkh@linuxfoundation.org> wrote:
> > 
> > On Wed, Sep 03, 2025 at 06:31:37PM +0200, Håkon Bugge wrote:
> >> Fixes: 2cb2912d6563 ("RDS: IB: add Fastreg MR (FRMR) detection support")
> >> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
> >> ---
> >> net/rds/ib_mr.h | 1 -
> >> 1 file changed, 1 deletion(-)
> > 
> > I know I can't take patches without any changelog text, but maybe other
> > maintainer are more lax :)
> 
> You mean the empty commit message? Did pass checkpatch.pl --strict though.

checkpatch is a hint.  What would you want to see if you were a reviewer?

Re: [PATCH net 2/2] rds: ib: Remove unused extern definition

[Haakon Bugge]

> On 3 Sep 2025, at 18:56, Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> On Wed, Sep 03, 2025 at 04:51:01PM +0000, Haakon Bugge wrote:
>> 
>> 
>>> On 3 Sep 2025, at 18:38, Greg KH <gregkh@linuxfoundation.org> wrote:
>>> 
>>> On Wed, Sep 03, 2025 at 06:31:37PM +0200, Håkon Bugge wrote:
>>>> Fixes: 2cb2912d6563 ("RDS: IB: add Fastreg MR (FRMR) detection support")
>>>> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
>>>> ---
>>>> net/rds/ib_mr.h | 1 -
>>>> 1 file changed, 1 deletion(-)
>>> 
>>> I know I can't take patches without any changelog text, but maybe other
>>> maintainer are more lax :)
>> 
>> You mean the empty commit message? Did pass checkpatch.pl --strict though.
> 
> checkpatch is a hint.  What would you want to see if you were a reviewer?

NP. Let me send a v2 with a commit message that also explains the history here.


Thxs, Håkon

Re: [PATCH net 1/2] rds: ib: Increment i_fastreg_wrs before bailing out

[Paolo Abeni]

On 9/3/25 6:31 PM, Håkon Bugge wrote:
> We need to increment i_fastreg_wrs before we bail out from
> rds_ib_post_reg_frmr().

Elaborating a bit more on the `why` could help the review.

> 
> Fixes: 1659185fb4d0 ("RDS: IB: Support Fastreg MR (FRMR) memory registration mode")
> Fixes: 3a2886cca703 ("net/rds: Keep track of and wait for FRWR segments in use upon shutdown")
> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>

[...]
@@ -178,9 +181,11 @@ static int rds_ib_post_reg_frmr(struct rds_ib_mr *ibmr)
>  	 * being accessed while registration is still pending.
>  	 */
>  	wait_event(frmr->fr_reg_done, !frmr->fr_reg);
> -
>  out:
> +	return ret;
>  
> +out_inc:
> +	atomic_inc(&ibmr->ic->i_fastreg_wrs);

The existing error path on ib_post_send() is left untouched. I think it
would be cleaner and less error prone to let it use the above label, too.

/P

Re: [PATCH net 1/2] rds: ib: Increment i_fastreg_wrs before bailing out

[Haakon Bugge]

> On 9 Sep 2025, at 12:54, Paolo Abeni <pabeni@redhat.com> wrote:
> 
> On 9/3/25 6:31 PM, Håkon Bugge wrote:
>> We need to increment i_fastreg_wrs before we bail out from
>> rds_ib_post_reg_frmr().
> 
> Elaborating a bit more on the `why` could help the review.

Yes, it's lean. Let me expand on it.

>> Fixes: 1659185fb4d0 ("RDS: IB: Support Fastreg MR (FRMR) memory registration mode")
>> Fixes: 3a2886cca703 ("net/rds: Keep track of and wait for FRWR segments in use upon shutdown")
>> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
> 
> [...]
> @@ -178,9 +181,11 @@ static int rds_ib_post_reg_frmr(struct rds_ib_mr *ibmr)
>> * being accessed while registration is still pending.
>> */
>> wait_event(frmr->fr_reg_done, !frmr->fr_reg);
>> -
>> out:
>> + return ret;
>> 
>> +out_inc:
>> + atomic_inc(&ibmr->ic->i_fastreg_wrs);
> 
> The existing error path on ib_post_send() is left untouched. I think it
> would be cleaner and less error prone to let it use the above label, too.

I agree, overlooked that. I'll send a v3 tomorrow.

Thanks for the review, Håkon

> 
> /P
>