1. Patchew
  2. linux
  3. View series

[PATCH net v3] rds: ib: Increment i_fastreg_wrs before bailing out

Håkon Bugge posted 1 patch 3 weeks, 1 day ago
There is a newer version of this series
net/rds/ib_frmr.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
[PATCH net v3] rds: ib: Increment i_fastreg_wrs before bailing out
Posted by Håkon Bugge 3 weeks, 1 day ago 
We need to increment i_fastreg_wrs before we bail out from
rds_ib_post_reg_frmr().

We have a fixed budget of how many FRWR operations that can be
outstanding using the dedicated QP used for memory registrations and
de-registrations. This budget is enforced by the atomic_t
i_fastreg_wrs. If we bail out early in rds_ib_post_reg_frmr(), we will
"leak" the possibility of posting an FRWR operation, and if that
accumulates, no FRWR operation can be carried out.

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")
Cc: stable@vger.kernel.org
Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>

---

v2 -> v3:
   * Amended commit message
   * Removed indentation of this section
   * Fixing error path from ib_post_send()

v1 -> v2: Added Cc: stable@vger.kernel.org
---
 net/rds/ib_frmr.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/net/rds/ib_frmr.c b/net/rds/ib_frmr.c
index 28c1b00221780..395a99b5a65ca 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);
 
@@ -166,11 +169,10 @@ static int rds_ib_post_reg_frmr(struct rds_ib_mr *ibmr)
 		/* Failure here can be because of -ENOMEM as well */
 		rds_transition_frwr_state(ibmr, FRMR_IS_INUSE, FRMR_IS_STALE);
 
-		atomic_inc(&ibmr->ic->i_fastreg_wrs);
 		if (printk_ratelimit())
 			pr_warn("RDS/IB: %s returned error(%d)\n",
 				__func__, ret);
-		goto out;
+		goto out_inc;
 	}
 
 	/* Wait for the registration to complete in order to prevent an invalid
@@ -178,9 +180,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
Re: [PATCH net v3] rds: ib: Increment i_fastreg_wrs before bailing out
Posted by kernel test robot 3 weeks ago 
Hi Håkon,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net/main]

url:    https://github.com/intel-lab-lkp/linux/commits/H-kon-Bugge/rds-ib-Increment-i_fastreg_wrs-before-bailing-out/20250910-190558
base:   net/main
patch link:    https://lore.kernel.org/r/20250910110501.350238-1-haakon.bugge%40oracle.com
patch subject: [PATCH net v3] rds: ib: Increment i_fastreg_wrs before bailing out
config: x86_64-buildonly-randconfig-001-20250911 (https://download.01.org/0day-ci/archive/20250911/202509110810.t8tVEIDs-lkp@intel.com/config)
compiler: gcc-14 (Debian 14.2.0-19) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250911/202509110810.t8tVEIDs-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202509110810.t8tVEIDs-lkp@intel.com/

All warnings (new ones prefixed by >>):

   net/rds/ib_frmr.c: In function 'rds_ib_post_reg_frmr':
>> net/rds/ib_frmr.c:183:1: warning: label 'out' defined but not used [-Wunused-label]
     183 | out:
         | ^~~


vim +/out +183 net/rds/ib_frmr.c

1659185fb4d002 Avinash Repaka    2016-03-01  122  
1659185fb4d002 Avinash Repaka    2016-03-01  123  static int rds_ib_post_reg_frmr(struct rds_ib_mr *ibmr)
1659185fb4d002 Avinash Repaka    2016-03-01  124  {
1659185fb4d002 Avinash Repaka    2016-03-01  125  	struct rds_ib_frmr *frmr = &ibmr->u.frmr;
1659185fb4d002 Avinash Repaka    2016-03-01  126  	struct ib_reg_wr reg_wr;
3e56c2f856d7ab Santosh Shilimkar 2016-12-04  127  	int ret, off = 0;
1659185fb4d002 Avinash Repaka    2016-03-01  128  
1659185fb4d002 Avinash Repaka    2016-03-01  129  	while (atomic_dec_return(&ibmr->ic->i_fastreg_wrs) <= 0) {
1659185fb4d002 Avinash Repaka    2016-03-01  130  		atomic_inc(&ibmr->ic->i_fastreg_wrs);
1659185fb4d002 Avinash Repaka    2016-03-01  131  		cpu_relax();
1659185fb4d002 Avinash Repaka    2016-03-01  132  	}
1659185fb4d002 Avinash Repaka    2016-03-01  133  
fb4b1373dcab08 Gerd Rausch       2021-08-17  134  	ret = ib_map_mr_sg_zbva(frmr->mr, ibmr->sg, ibmr->sg_dma_len,
3e56c2f856d7ab Santosh Shilimkar 2016-12-04  135  				&off, PAGE_SIZE);
cc67d6c0924d12 Håkon Bugge       2025-09-10  136  	if (unlikely(ret != ibmr->sg_dma_len)) {
cc67d6c0924d12 Håkon Bugge       2025-09-10  137  		ret = ret < 0 ? ret : -EINVAL;
cc67d6c0924d12 Håkon Bugge       2025-09-10  138  		goto out_inc;
cc67d6c0924d12 Håkon Bugge       2025-09-10  139  	}
1659185fb4d002 Avinash Repaka    2016-03-01  140  
cc67d6c0924d12 Håkon Bugge       2025-09-10  141  	if (cmpxchg(&frmr->fr_state, FRMR_IS_FREE, FRMR_IS_INUSE) != FRMR_IS_FREE) {
cc67d6c0924d12 Håkon Bugge       2025-09-10  142  		ret = -EBUSY;
cc67d6c0924d12 Håkon Bugge       2025-09-10  143  		goto out_inc;
cc67d6c0924d12 Håkon Bugge       2025-09-10  144  	}
3a2886cca703fd Gerd Rausch       2019-07-16  145  
3a2886cca703fd Gerd Rausch       2019-07-16  146  	atomic_inc(&ibmr->ic->i_fastreg_inuse_count);
3a2886cca703fd Gerd Rausch       2019-07-16  147  
1659185fb4d002 Avinash Repaka    2016-03-01  148  	/* Perform a WR for the fast_reg_mr. Each individual page
1659185fb4d002 Avinash Repaka    2016-03-01  149  	 * in the sg list is added to the fast reg page list and placed
1659185fb4d002 Avinash Repaka    2016-03-01  150  	 * inside the fast_reg_mr WR.  The key used is a rolling 8bit
1659185fb4d002 Avinash Repaka    2016-03-01  151  	 * counter, which should guarantee uniqueness.
1659185fb4d002 Avinash Repaka    2016-03-01  152  	 */
1659185fb4d002 Avinash Repaka    2016-03-01  153  	ib_update_fast_reg_key(frmr->mr, ibmr->remap_count++);
5f33141d2fc05a Gerd Rausch       2019-07-16  154  	frmr->fr_reg = true;
1659185fb4d002 Avinash Repaka    2016-03-01  155  
1659185fb4d002 Avinash Repaka    2016-03-01  156  	memset(&reg_wr, 0, sizeof(reg_wr));
1659185fb4d002 Avinash Repaka    2016-03-01  157  	reg_wr.wr.wr_id = (unsigned long)(void *)ibmr;
1659185fb4d002 Avinash Repaka    2016-03-01  158  	reg_wr.wr.opcode = IB_WR_REG_MR;
1659185fb4d002 Avinash Repaka    2016-03-01  159  	reg_wr.wr.num_sge = 0;
1659185fb4d002 Avinash Repaka    2016-03-01  160  	reg_wr.mr = frmr->mr;
1659185fb4d002 Avinash Repaka    2016-03-01  161  	reg_wr.key = frmr->mr->rkey;
1659185fb4d002 Avinash Repaka    2016-03-01  162  	reg_wr.access = IB_ACCESS_LOCAL_WRITE |
1659185fb4d002 Avinash Repaka    2016-03-01  163  			IB_ACCESS_REMOTE_READ |
1659185fb4d002 Avinash Repaka    2016-03-01  164  			IB_ACCESS_REMOTE_WRITE;
1659185fb4d002 Avinash Repaka    2016-03-01  165  	reg_wr.wr.send_flags = IB_SEND_SIGNALED;
1659185fb4d002 Avinash Repaka    2016-03-01  166  
f112d53b435692 Bart Van Assche   2018-07-18  167  	ret = ib_post_send(ibmr->ic->i_cm_id->qp, &reg_wr.wr, NULL);
1659185fb4d002 Avinash Repaka    2016-03-01  168  	if (unlikely(ret)) {
1659185fb4d002 Avinash Repaka    2016-03-01  169  		/* Failure here can be because of -ENOMEM as well */
3a2886cca703fd Gerd Rausch       2019-07-16  170  		rds_transition_frwr_state(ibmr, FRMR_IS_INUSE, FRMR_IS_STALE);
3a2886cca703fd Gerd Rausch       2019-07-16  171  
1659185fb4d002 Avinash Repaka    2016-03-01  172  		if (printk_ratelimit())
1659185fb4d002 Avinash Repaka    2016-03-01  173  			pr_warn("RDS/IB: %s returned error(%d)\n",
1659185fb4d002 Avinash Repaka    2016-03-01  174  				__func__, ret);
cc67d6c0924d12 Håkon Bugge       2025-09-10  175  		goto out_inc;
1659185fb4d002 Avinash Repaka    2016-03-01  176  	}
5f33141d2fc05a Gerd Rausch       2019-07-16  177  
5f33141d2fc05a Gerd Rausch       2019-07-16  178  	/* Wait for the registration to complete in order to prevent an invalid
5f33141d2fc05a Gerd Rausch       2019-07-16  179  	 * access error resulting from a race between the memory region already
5f33141d2fc05a Gerd Rausch       2019-07-16  180  	 * being accessed while registration is still pending.
5f33141d2fc05a Gerd Rausch       2019-07-16  181  	 */
5f33141d2fc05a Gerd Rausch       2019-07-16  182  	wait_event(frmr->fr_reg_done, !frmr->fr_reg);
5f33141d2fc05a Gerd Rausch       2019-07-16 @183  out:
cc67d6c0924d12 Håkon Bugge       2025-09-10  184  	return ret;
5f33141d2fc05a Gerd Rausch       2019-07-16  185  
cc67d6c0924d12 Håkon Bugge       2025-09-10  186  out_inc:
cc67d6c0924d12 Håkon Bugge       2025-09-10  187  	atomic_inc(&ibmr->ic->i_fastreg_wrs);
1659185fb4d002 Avinash Repaka    2016-03-01  188  	return ret;
1659185fb4d002 Avinash Repaka    2016-03-01  189  }
1659185fb4d002 Avinash Repaka    2016-03-01  190  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH net v3] rds: ib: Increment i_fastreg_wrs before bailing out
Posted by Allison Henderson 3 weeks, 1 day ago 
On Wed, 2025-09-10 at 13:04 +0200, Håkon Bugge wrote:
> We need to increment i_fastreg_wrs before we bail out from
> rds_ib_post_reg_frmr().
> 
> We have a fixed budget of how many FRWR operations that can be
> outstanding using the dedicated QP used for memory registrations and
> de-registrations. This budget is enforced by the atomic_t
> i_fastreg_wrs. If we bail out early in rds_ib_post_reg_frmr(), we will
> "leak" the possibility of posting an FRWR operation, and if that
> accumulates, no FRWR operation can be carried out.
Hi Håkon,

This sounds much clearer, thank you!

> 
> 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")
> Cc: stable@vger.kernel.org
> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
> 
> ---
> 
> v2 -> v3:
>    * Amended commit message
>    * Removed indentation of this section
>    * Fixing error path from ib_post_send()
> 
> v1 -> v2: Added Cc: stable@vger.kernel.org
> ---
>  net/rds/ib_frmr.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/net/rds/ib_frmr.c b/net/rds/ib_frmr.c
> index 28c1b00221780..395a99b5a65ca 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);
>  
> @@ -166,11 +169,10 @@ static int rds_ib_post_reg_frmr(struct rds_ib_mr *ibmr)
>  		/* Failure here can be because of -ENOMEM as well */
>  		rds_transition_frwr_state(ibmr, FRMR_IS_INUSE, FRMR_IS_STALE);
>  
> -		atomic_inc(&ibmr->ic->i_fastreg_wrs);
>  		if (printk_ratelimit())
>  			pr_warn("RDS/IB: %s returned error(%d)\n",
>  				__func__, ret);
> -		goto out;
Just one nit:  This was the only place the out label was used isnt it?  If so, lets go ahead and clear it out.  

> +		goto out_inc;
>  	}
>  
>  	/* Wait for the registration to complete in order to prevent an invalid
> @@ -178,9 +180,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:
With that fixed, you can go ahead and add my rvb:
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

Thank you!

> +	return ret;
>  
> +out_inc:
> +	atomic_inc(&ibmr->ic->i_fastreg_wrs);
>  	return ret;
>  }
>
Re: [PATCH net v3] rds: ib: Increment i_fastreg_wrs before bailing out
Posted by Haakon Bugge 3 weeks ago 
Hi Allison,


> On 10 Sep 2025, at 20:47, Allison Henderson <allison.henderson@oracle.com> wrote:
> 
> On Wed, 2025-09-10 at 13:04 +0200, Håkon Bugge wrote:
>> We need to increment i_fastreg_wrs before we bail out from
>> rds_ib_post_reg_frmr().
>> 
>> We have a fixed budget of how many FRWR operations that can be
>> outstanding using the dedicated QP used for memory registrations and
>> de-registrations. This budget is enforced by the atomic_t
>> i_fastreg_wrs. If we bail out early in rds_ib_post_reg_frmr(), we will
>> "leak" the possibility of posting an FRWR operation, and if that
>> accumulates, no FRWR operation can be carried out.
> Hi Håkon,
> 
> This sounds much clearer, thank you!
> 
>> 
>> 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")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
>> 
>> ---
>> 
>> v2 -> v3:
>>   * Amended commit message
>>   * Removed indentation of this section
>>   * Fixing error path from ib_post_send()
>> 
>> v1 -> v2: Added Cc: stable@vger.kernel.org
>> ---
>> net/rds/ib_frmr.c | 20 ++++++++++++--------
>> 1 file changed, 12 insertions(+), 8 deletions(-)
>> 
>> diff --git a/net/rds/ib_frmr.c b/net/rds/ib_frmr.c
>> index 28c1b00221780..395a99b5a65ca 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);
>> 
>> @@ -166,11 +169,10 @@ static int rds_ib_post_reg_frmr(struct rds_ib_mr *ibmr)
>> /* Failure here can be because of -ENOMEM as well */
>> rds_transition_frwr_state(ibmr, FRMR_IS_INUSE, FRMR_IS_STALE);
>> 
>> - atomic_inc(&ibmr->ic->i_fastreg_wrs);
>> if (printk_ratelimit())
>> pr_warn("RDS/IB: %s returned error(%d)\n",
>> __func__, ret);
>> - goto out;
> Just one nit:  This was the only place the out label was used isnt it?  If so, lets go ahead and clear it out.  

That is indeed true!

> 
>> + goto out_inc;
>> }
>> 
>> /* Wait for the registration to complete in order to prevent an invalid
>> @@ -178,9 +180,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:
> With that fixed, you can go ahead and add my rvb:
> Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

Will do, thanks for the r-b!


Håkon

> 
> Thank you!
> 
>> + return ret;
>> 
>> +out_inc:
>> + atomic_inc(&ibmr->ic->i_fastreg_wrs);
>> return ret;
>> }

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.