[PATCH net-next 3/7] net/rds: Wait for the FRMR_IS_FREE (or FRMR_IS_STALE) transition after posting IB_WR_LOCAL_INV

[PATCH net-next 3/7] net/rds: Wait for the FRMR_IS_FREE (or FRMR_IS_STALE) transition after posting IB_WR_LOCAL_INV

[Gerd Rausch]

In order to:
1) avoid a silly bouncing between "clean_list" and "drop_list"
   triggered by function "rds_ib_reg_frmr" as it is releases frmr
   regions whose state is not "FRMR_IS_FREE" right away.

2) prevent an invalid access error in a race from a pending
   "IB_WR_LOCAL_INV" operation with a teardown ("dma_unmap_sg", "put_page")
   and de-registration ("ib_dereg_mr") of the corresponding
   memory region.

Signed-off-by: Gerd Rausch <gerd.rausch@oracle.com>
---
 net/rds/ib_frmr.c | 89 ++++++++++++++++++++++++++++++-----------------
 net/rds/ib_mr.h   |  2 ++
 2 files changed, 59 insertions(+), 32 deletions(-)

diff --git a/net/rds/ib_frmr.c b/net/rds/ib_frmr.c
index 9f8aa310c27a..3c953034dca3 100644
--- a/net/rds/ib_frmr.c
+++ b/net/rds/ib_frmr.c
@@ -76,6 +76,7 @@ static struct rds_ib_mr *rds_ib_alloc_frmr(struct rds_ib_device *rds_ibdev,
 
 	frmr->fr_state = FRMR_IS_FREE;
 	init_waitqueue_head(&frmr->fr_inv_done);
+	init_waitqueue_head(&frmr->fr_reg_done);
 	return ibmr;
 
 out_no_cigar:
@@ -124,6 +125,7 @@ static int rds_ib_post_reg_frmr(struct rds_ib_mr *ibmr)
 	 */
 	ib_update_fast_reg_key(frmr->mr, ibmr->remap_count++);
 	frmr->fr_state = FRMR_IS_INUSE;
+	frmr->fr_reg = true;
 
 	memset(&reg_wr, 0, sizeof(reg_wr));
 	reg_wr.wr.wr_id = (unsigned long)(void *)ibmr;
@@ -144,7 +146,29 @@ static int rds_ib_post_reg_frmr(struct rds_ib_mr *ibmr)
 		if (printk_ratelimit())
 			pr_warn("RDS/IB: %s returned error(%d)\n",
 				__func__, ret);
+		goto out;
+	}
+
+	if (!frmr->fr_reg)
+		goto out;
+
+	/* Wait for the registration to complete in order to prevent an invalid
+	 * access error resulting from a race between the memory region already
+	 * being accessed while registration is still pending.
+	 */
+	wait_event_timeout(frmr->fr_reg_done, !frmr->fr_reg,
+			   msecs_to_jiffies(100));
+
+	/* Registration did not complete within one second, something's wrong */
+	if (frmr->fr_reg) {
+		pr_warn("RDS/IB: %s registration still incomplete after 100msec\n",
+			__func__);
+		frmr->fr_state = FRMR_IS_STALE;
+		ret = -EBUSY;
 	}
+
+out:
+
 	return ret;
 }
 
@@ -262,6 +286,26 @@ static int rds_ib_post_inv(struct rds_ib_mr *ibmr)
 		pr_err("RDS/IB: %s returned error(%d)\n", __func__, ret);
 		goto out;
 	}
+
+	if (frmr->fr_state != FRMR_IS_INUSE)
+		goto out;
+
+	/* Wait for the FRMR_IS_FREE (or FRMR_IS_STALE) transition in order to
+	 * 1) avoid a silly bouncing between "clean_list" and "drop_list"
+	 *    triggered by function "rds_ib_reg_frmr" as it is releases frmr
+	 *    regions whose state is not "FRMR_IS_FREE" right away.
+	 * 2) prevents an invalid access error in a race
+	 *    from a pending "IB_WR_LOCAL_INV" operation
+	 *    with a teardown ("dma_unmap_sg", "put_page")
+	 *    and de-registration ("ib_dereg_mr") of the corresponding
+	 *    memory region.
+	 */
+	wait_event_timeout(frmr->fr_inv_done, frmr->fr_state != FRMR_IS_INUSE,
+			   msecs_to_jiffies(50));
+
+	if (frmr->fr_state == FRMR_IS_INUSE)
+		ret = -EBUSY;
+
 out:
 	return ret;
 }
@@ -289,6 +333,11 @@ void rds_ib_mr_cqe_handler(struct rds_ib_connection *ic, struct ib_wc *wc)
 		wake_up(&frmr->fr_inv_done);
 	}
 
+	if (frmr->fr_reg) {
+		frmr->fr_reg = false;
+		wake_up(&frmr->fr_reg_done);
+	}
+
 	atomic_inc(&ic->i_fastreg_wrs);
 }
 
@@ -297,14 +346,18 @@ void rds_ib_unreg_frmr(struct list_head *list, unsigned int *nfreed,
 {
 	struct rds_ib_mr *ibmr, *next;
 	struct rds_ib_frmr *frmr;
-	int ret = 0;
+	int ret = 0, ret2;
 	unsigned int freed = *nfreed;
 
 	/* String all ib_mr's onto one list and hand them to ib_unmap_fmr */
 	list_for_each_entry(ibmr, list, unmap_list) {
-		if (ibmr->sg_dma_len)
-			ret |= rds_ib_post_inv(ibmr);
+		if (ibmr->sg_dma_len) {
+			ret2 = rds_ib_post_inv(ibmr);
+			if (ret2 && !ret)
+				ret = ret2;
+		}
 	}
+
 	if (ret)
 		pr_warn("RDS/IB: %s failed (err=%d)\n", __func__, ret);
 
@@ -347,36 +400,8 @@ struct rds_ib_mr *rds_ib_reg_frmr(struct rds_ib_device *rds_ibdev,
 	}
 
 	do {
-		if (ibmr) {
-			/* Memory regions make it onto the "clean_list" via
-			 * "rds_ib_flush_mr_pool", after the memory region has
-			 * been posted for invalidation via "rds_ib_post_inv".
-			 *
-			 * At that point in time, "fr_state" may still be
-			 * in state "FRMR_IS_INUSE", since the only place where
-			 * "fr_state" transitions to "FRMR_IS_FREE" is in
-			 * is in "rds_ib_mr_cqe_handler", which is
-			 * triggered by a tasklet.
-			 *
-			 * So in case we notice that
-			 * "fr_state != FRMR_IS_FREE" (see below), * we wait for
-			 * "fr_inv_done" to trigger with a maximum of 10msec.
-			 * Then we check again, and only put the memory region
-			 * onto the drop_list (via "rds_ib_free_frmr")
-			 * in case the situation remains unchanged.
-			 *
-			 * This avoids the problem of memory-regions bouncing
-			 * between "clean_list" and "drop_list" before they
-			 * even have a chance to be properly invalidated.
-			 */
-			frmr = &ibmr->u.frmr;
-			wait_event_timeout(frmr->fr_inv_done,
-					   frmr->fr_state == FRMR_IS_FREE,
-					   msecs_to_jiffies(10));
-			if (frmr->fr_state == FRMR_IS_FREE)
-				break;
+		if (ibmr)
 			rds_ib_free_frmr(ibmr, true);
-		}
 		ibmr = rds_ib_alloc_frmr(rds_ibdev, nents);
 		if (IS_ERR(ibmr))
 			return ibmr;
diff --git a/net/rds/ib_mr.h b/net/rds/ib_mr.h
index ab26c20ed66f..9045a8c0edff 100644
--- a/net/rds/ib_mr.h
+++ b/net/rds/ib_mr.h
@@ -58,6 +58,8 @@ struct rds_ib_frmr {
 	enum rds_ib_fr_state	fr_state;
 	bool			fr_inv;
 	wait_queue_head_t	fr_inv_done;
+	bool			fr_reg;
+	wait_queue_head_t	fr_reg_done;
 	struct ib_send_wr	fr_wr;
 	unsigned int		dma_npages;
 	unsigned int		sg_byte_len;
-- 
2.18.0

Re: [PATCH net-next 3/7] net/rds: Wait for the FRMR_IS_FREE (or FRMR_IS_STALE) transition after posting IB_WR_LOCAL_INV

[santosh.shilimkar]

On 7/1/19 9:39 AM, Gerd Rausch wrote:
> In order to:
> 1) avoid a silly bouncing between "clean_list" and "drop_list"
>     triggered by function "rds_ib_reg_frmr" as it is releases frmr
>     regions whose state is not "FRMR_IS_FREE" right away.
> 
> 2) prevent an invalid access error in a race from a pending
>     "IB_WR_LOCAL_INV" operation with a teardown ("dma_unmap_sg", "put_page")
>     and de-registration ("ib_dereg_mr") of the corresponding
>     memory region.
> 
> Signed-off-by: Gerd Rausch <gerd.rausch@oracle.com>
> ---
>   net/rds/ib_frmr.c | 89 ++++++++++++++++++++++++++++++-----------------
>   net/rds/ib_mr.h   |  2 ++
>   2 files changed, 59 insertions(+), 32 deletions(-)
> 
> diff --git a/net/rds/ib_frmr.c b/net/rds/ib_frmr.c
> index 9f8aa310c27a..3c953034dca3 100644
> --- a/net/rds/ib_frmr.c
> +++ b/net/rds/ib_frmr.c
> @@ -76,6 +76,7 @@ static struct rds_ib_mr *rds_ib_alloc_frmr(struct rds_ib_device *rds_ibdev,
>   
>   	frmr->fr_state = FRMR_IS_FREE;
>   	init_waitqueue_head(&frmr->fr_inv_done);
> +	init_waitqueue_head(&frmr->fr_reg_done);
>   	return ibmr;
>   
>   out_no_cigar:
> @@ -124,6 +125,7 @@ static int rds_ib_post_reg_frmr(struct rds_ib_mr *ibmr)
>   	 */
>   	ib_update_fast_reg_key(frmr->mr, ibmr->remap_count++);
>   	frmr->fr_state = FRMR_IS_INUSE;
> +	frmr->fr_reg = true;
>   
>   	memset(&reg_wr, 0, sizeof(reg_wr));
>   	reg_wr.wr.wr_id = (unsigned long)(void *)ibmr;
> @@ -144,7 +146,29 @@ static int rds_ib_post_reg_frmr(struct rds_ib_mr *ibmr)
>   		if (printk_ratelimit())
>   			pr_warn("RDS/IB: %s returned error(%d)\n",
>   				__func__, ret);
> +		goto out;
> +	}
> +
> +	if (!frmr->fr_reg)
> +		goto out;
> +
> +	/* Wait for the registration to complete in order to prevent an invalid
> +	 * access error resulting from a race between the memory region already
> +	 * being accessed while registration is still pending.
> +	 */
> +	wait_event_timeout(frmr->fr_reg_done, !frmr->fr_reg,
> +			   msecs_to_jiffies(100));
> +
This arbitrary timeout in this patch as well as pacth 1/7 which
Dave pointed out has any logic ?

MR registration command issued to hardware can at times take as
much as command timeout(e.g 60 seconds in CX3) and upto that its still
legitimate operation and not necessary failure. We shouldn't add
arbitrary time outs in ULPs.

Regards,
Santosh

Re: [PATCH net-next 3/7] net/rds: Wait for the FRMR_IS_FREE (or FRMR_IS_STALE) transition after posting IB_WR_LOCAL_INV

[Gerd Rausch]

Hi Santosh,

On 01/07/2019 13.41, santosh.shilimkar@oracle.com wrote:
>> @@ -144,7 +146,29 @@ static int rds_ib_post_reg_frmr(struct rds_ib_mr *ibmr)
>>           if (printk_ratelimit())
>>               pr_warn("RDS/IB: %s returned error(%d)\n",
>>                   __func__, ret);
>> +        goto out;
>> +    }
>> +
>> +    if (!frmr->fr_reg)
>> +        goto out;
>> +
>> +    /* Wait for the registration to complete in order to prevent an invalid
>> +     * access error resulting from a race between the memory region already
>> +     * being accessed while registration is still pending.
>> +     */
>> +    wait_event_timeout(frmr->fr_reg_done, !frmr->fr_reg,
>> +               msecs_to_jiffies(100));
>> +
> This arbitrary timeout in this patch as well as pacth 1/7 which
> Dave pointed out has any logic ?
> 

It's empirical (see my response to David's question):
Memory registrations took longer than invalidations, hence 100msec instead of 10msec.

> MR registration command issued to hardware can at times take as
> much as command timeout(e.g 60 seconds in CX3) and upto that its still
> legitimate operation and not necessary failure. We shouldn't add
> arbitrary time outs in ULPs.

Where did you find the 60 seconds for CX3 you are referring to?
Is there a "generic" upper-bound that is not tied to a specific vendor / HCA?
Can you provide a pointer?

Thanks,

  Gerd

Re: [PATCH net-next 3/7] net/rds: Wait for the FRMR_IS_FREE (or FRMR_IS_STALE) transition after posting IB_WR_LOCAL_INV

[santosh.shilimkar]

On 7/1/19 1:55 PM, Gerd Rausch wrote:
> Hi Santosh,
> 
> On 01/07/2019 13.41, santosh.shilimkar@oracle.com wrote:
>>> @@ -144,7 +146,29 @@ static int rds_ib_post_reg_frmr(struct rds_ib_mr *ibmr)
>>>            if (printk_ratelimit())
>>>                pr_warn("RDS/IB: %s returned error(%d)\n",
>>>                    __func__, ret);
>>> +        goto out;
>>> +    }
>>> +
>>> +    if (!frmr->fr_reg)
>>> +        goto out;
>>> +
>>> +    /* Wait for the registration to complete in order to prevent an invalid
>>> +     * access error resulting from a race between the memory region already
>>> +     * being accessed while registration is still pending.
>>> +     */
>>> +    wait_event_timeout(frmr->fr_reg_done, !frmr->fr_reg,
>>> +               msecs_to_jiffies(100));
>>> +
>> This arbitrary timeout in this patch as well as pacth 1/7 which
>> Dave pointed out has any logic ?
>>
> 
> It's empirical (see my response to David's question):
> Memory registrations took longer than invalidations, hence 100msec instead of 10msec.
> 
>> MR registration command issued to hardware can at times take as
>> much as command timeout(e.g 60 seconds in CX3) and upto that its still
>> legitimate operation and not necessary failure. We shouldn't add
>> arbitrary time outs in ULPs.
> 
> Where did you find the 60 seconds for CX3 you are referring to?
> Is there a "generic" upper-bound that is not tied to a specific vendor / HCA?
> Can you provide a pointer?
> 
Look for command timeout in CX3 sources. 60 second is upper bound in
CX3. Its not standard in specs(at least not that I know) though
and may vary from vendor to vendor.

Regards,
Santosh

Re: [PATCH net-next 3/7] net/rds: Wait for the FRMR_IS_FREE (or FRMR_IS_STALE) transition after posting IB_WR_LOCAL_INV

[Gerd Rausch]

Hi Santosh,

On 01/07/2019 14.00, santosh.shilimkar@oracle.com wrote:
>>
> Look for command timeout in CX3 sources. 60 second is upper bound in
> CX3. Its not standard in specs(at least not that I know) though
> and may vary from vendor to vendor.
> 

I am not seeing it. Can you point me to the right place?

% grep -ni timeout drivers/net/ethernet/mellanox/mlx4/*.[ch]
drivers/net/ethernet/mellanox/mlx4/cmd.c:116:	GO_BIT_TIMEOUT_MSECS	= 10000
[...]
drivers/net/ethernet/mellanox/mlx4/mlx4_en.h:101:#define MLX4_EN_WATCHDOG_TIMEOUT	(15 * HZ)
drivers/net/ethernet/mellanox/mlx4/mlx4_en.h:155:#define MLX4_EN_TX_POLL_TIMEOUT	(HZ / 4)
drivers/net/ethernet/mellanox/mlx4/mlx4_en.h:171:#define MLX4_EN_LOOPBACK_TIMEOUT	100
[...]
drivers/net/ethernet/mellanox/mlx4/reset.c:61:#define MLX4_SEM_TIMEOUT_JIFFIES	(10 * HZ)
drivers/net/ethernet/mellanox/mlx4/reset.c:62:#define MLX4_RESET_TIMEOUT_JIFFIES	(2 * HZ)


% grep -i timeout drivers/infiniband/hw/mlx4/*.[ch] 
drivers/infiniband/hw/mlx4/cm.c:42:#define CM_CLEANUP_CACHE_TIMEOUT  (30 * HZ)
[...]
drivers/infiniband/hw/mlx4/mcg.c:46:#define MAD_TIMEOUT_MS	2000
[...]
drivers/infiniband/hw/mlx4/qp.c:4358:		while (wait_for_completion_timeout(&sdrain->done, HZ / 10) <= 0)

Thanks,

  Gerd

Re: [PATCH net-next 3/7] net/rds: Wait for the FRMR_IS_FREE (or FRMR_IS_STALE) transition after posting IB_WR_LOCAL_INV

[santosh.shilimkar]

On 7/1/19 2:06 PM, Gerd Rausch wrote:
> Hi Santosh,
> 
> On 01/07/2019 14.00, santosh.shilimkar@oracle.com wrote:
>>>
>> Look for command timeout in CX3 sources. 60 second is upper bound in
>> CX3. Its not standard in specs(at least not that I know) though
>> and may vary from vendor to vendor.
>>
> 
> I am not seeing it. Can you point me to the right place?
>
Below. All command timeouts are 60 seconds.

enum {
         MLX4_CMD_TIME_CLASS_A   = 60000,
         MLX4_CMD_TIME_CLASS_B   = 60000,
         MLX4_CMD_TIME_CLASS_C   = 60000,
};

But having said that, I re-looked the code you are patching
and thats actually only FRWR code which is purely work-request
based so this command timeout shouldn't matter.

If the work request fails, then it will lead to flush errors and
MRs will be marked as STALE. So this wait may not be necessary

There is a socket call RDS_GET_MR which needs to be synchronous
and that Avinash has actually fixed by making this MR registration
processes synchronous. Inline registration is still kept async.
RDS_GET_MR case is what actually showing the issue you saw
and the fix for that Avinash has it in production kernel.

I believe with that change, registration issue becomes non-issue
already.

And as far as invalidation concerned with proxy qp, it not longer
races with data path qp.

May be you can try those changes if not already to see if it
addresses the couple of cases where you ended up adding
timeouts.

Regards,
Santosh

Re: [PATCH net-next 3/7] net/rds: Wait for the FRMR_IS_FREE (or FRMR_IS_STALE) transition after posting IB_WR_LOCAL_INV

[Gerd Rausch]

Hi Santosh,

On 01/07/2019 19.28, santosh.shilimkar@oracle.com wrote:
>>
> Below. All command timeouts are 60 seconds.
> 
> enum {
>         MLX4_CMD_TIME_CLASS_A   = 60000,
>         MLX4_CMD_TIME_CLASS_B   = 60000,
>         MLX4_CMD_TIME_CLASS_C   = 60000,
> };
> 

Thank you for the pointer.

> But having said that, I re-looked the code you are patching
> and thats actually only FRWR code which is purely work-request
> based so this command timeout shouldn't matter.
> 

Which brings us back full circle to the question of
what the timeout ought to be?

Please keep in mind that prior to this fix,
the RDS code didn't wait at all:

It simply posted those registration (IB_WR_REG_MR)
and invalidation (IB_WR_LOCAL_INV)
work-requests, with no regards to when the firmware
would execute them.

Arguably, waiting any amount time greater than zero
for the operation to complete is better than not waiting at all.

We can change the timeout to a high value, or even make it infinite
by using "wait_event" instead of "wait_event_timeout".

For the registration work-requests there is a benefit to wait a short
amount of time only (the trade-off described in patch #1 of this series).

For de-registration work-requests, it is beneficial to wait
until they are truly done.
But: Function "rds_ib_unreg_frmr" prior and post this change
simply moves on after a failed de-registration attempt,
and releases the pages owned by the memory region.

This patch does _not_ change that behavior.

> If the work request fails, then it will lead to flush errors and
> MRs will be marked as STALE. So this wait may not be necessary
> 

This wait is necessary to avoid the 2 scenarios described
in the commit-log message:

#1) Memory regions bouncing between "drop_list" and "clean_list"
    as items on the "clean_list" aren't really clean until
    their state transitions to "FRMR_IS_FREE".

#2) Prevent an access error as "rds_ib_post_inv" is called
    just prior to de-referencing pages via "__rds_ib_teardown_mr".
    And you certainly don't want those pages populated in the
    HCA's memory-translation-table with full access, while
    the Linux kernel 'thinks' you gave them back already
    and starts re-purposing them.

> RDS_GET_MR case is what actually showing the issue you saw
> and the fix for that Avinash has it in production kernel.

Actually, no:
Socket option RDS_GET_MR wasn't even in the code-path of the
tests I performed:

It were there RDS_CMSG_RDMA_MAP / RDS_CMSG_RDMA_DEST control
messages that ended up calling '__rds_rdma_map".

> 
> I believe with that change, registration issue becomes non-issue
> already.
> 

Please explain how that is related to this fix-suggestion?

I submitted this patch #3 and the others in this series in order
to fix bugs in the RDS that is currently shipped with Linux.

It may very well be the case that there are other changes
that Avinash put into production kernels that would be better
suited to fix this and other problems.

But that should not eliminate the need to fix what is currently broken.

Fixing what's broken does not preclude replacing the fixed code
with newer or better versions of the same.

> And as far as invalidation concerned with proxy qp, it not longer
> races with data path qp.
> 

I don't understand, please elaborate.

> May be you can try those changes if not already to see if it
> addresses the couple of cases where you ended up adding
> timeouts.
> 

I don't understand, please elaborate:
a) Are you saying this issue should not be fixed?
b) Or are you suggesting to replace this fix with a different fix?
   If it's the later, please point out what you have in mind.
c) ???

Thanks,

  Gerd

Re: [PATCH net-next 3/7] net/rds: Wait for the FRMR_IS_FREE (or FRMR_IS_STALE) transition after posting IB_WR_LOCAL_INV

[santosh.shilimkar]

On 7/1/19 10:11 PM, Gerd Rausch wrote:
> Hi Santosh,
> 
> On 01/07/2019 19.28, santosh.shilimkar@oracle.com wrote:
>>>
>> Below. All command timeouts are 60 seconds.
>>
>> enum {
>>          MLX4_CMD_TIME_CLASS_A   = 60000,
>>          MLX4_CMD_TIME_CLASS_B   = 60000,
>>          MLX4_CMD_TIME_CLASS_C   = 60000,
>> };
>>
> 
> Thank you for the pointer.
> 
>> But having said that, I re-looked the code you are patching
>> and thats actually only FRWR code which is purely work-request
>> based so this command timeout shouldn't matter.
>>
> 
> Which brings us back full circle to the question of
> what the timeout ought to be?
> 
> Please keep in mind that prior to this fix,
> the RDS code didn't wait at all:
> 
> It simply posted those registration (IB_WR_REG_MR)
> and invalidation (IB_WR_LOCAL_INV)
> work-requests, with no regards to when the firmware
> would execute them.
> 
> Arguably, waiting any amount time greater than zero
> for the operation to complete is better than not waiting at all.
> 
> We can change the timeout to a high value, or even make it infinite
> by using "wait_event" instead of "wait_event_timeout".
> 
> For the registration work-requests there is a benefit to wait a short
> amount of time only (the trade-off described in patch #1 of this series).
>
Actually we should just switch this code to what Avinash has
finally made in downstream code. That keeps the RDS_GET_MR
semantics and makes sure MR is really valid before handing over
the key to userland. There is no need for any timeout
for registration case.

> For de-registration work-requests, it is beneficial to wait
> until they are truly done.
> But: Function "rds_ib_unreg_frmr" prior and post this change
> simply moves on after a failed de-registration attempt,
> and releases the pages owned by the memory region.
> 
> This patch does _not_ change that behavior.
> 
>> If the work request fails, then it will lead to flush errors and
>> MRs will be marked as STALE. So this wait may not be necessary
>>
> 
> This wait is necessary to avoid the 2 scenarios described
> in the commit-log message:
> 
> #1) Memory regions bouncing between "drop_list" and "clean_list"
>      as items on the "clean_list" aren't really clean until
>      their state transitions to "FRMR_IS_FREE".

> 
> #2) Prevent an access error as "rds_ib_post_inv" is called
>      just prior to de-referencing pages via "__rds_ib_teardown_mr".
>      And you certainly don't want those pages populated in the
>      HCA's memory-translation-table with full access, while
>      the Linux kernel 'thinks' you gave them back already
>      and starts re-purposing them.
> 
>> RDS_GET_MR case is what actually showing the issue you saw
>> and the fix for that Avinash has it in production kernel.
> 
> Actually, no:
> Socket option RDS_GET_MR wasn't even in the code-path of the
> tests I performed:
> 
> It were there RDS_CMSG_RDMA_MAP / RDS_CMSG_RDMA_DEST control
> messages that ended up calling '__rds_rdma_map".
>
What option did you use ? Default option with rds-stress is
RDS_GET_MR and hence the question.

>>
>> I believe with that change, registration issue becomes non-issue
>> already.
>>
> 
> Please explain how that is related to this fix-suggestion?
> 
> I submitted this patch #3 and the others in this series in order
> to fix bugs in the RDS that is currently shipped with Linux.
> 
> It may very well be the case that there are other changes
> that Avinash put into production kernels that would be better
> suited to fix this and other problems.
> 
> But that should not eliminate the need to fix what is currently broken.
> 
> Fixing what's broken does not preclude replacing the fixed code
> with newer or better versions of the same.
> 
>> And as far as invalidation concerned with proxy qp, it not longer
>> races with data path qp.
>>
> 
> I don't understand, please elaborate.
> 
>> May be you can try those changes if not already to see if it
>> addresses the couple of cases where you ended up adding
>> timeouts.
>>
> 
> I don't understand, please elaborate:
> a) Are you saying this issue should not be fixed?
> b) Or are you suggesting to replace this fix with a different fix?
>     If it's the later, please point out what you have in mind.
> c) ???
> 
All am saying is the code got changed for good reason and that changed
code makes some of these race conditions possibly not applicable.
So instead of these timeout fixes, am suggesting to use that
code as fix. At least test it with those changes and see whats
the behavior.

Regards,
Santosh

Re: [PATCH net-next 3/7] net/rds: Wait for the FRMR_IS_FREE (or FRMR_IS_STALE) transition after posting IB_WR_LOCAL_INV

[Gerd Rausch]

On 02/07/2019 09.49, santosh.shilimkar@oracle.com wrote:
> On 7/1/19 10:11 PM, Gerd Rausch wrote:
>> For the registration work-requests there is a benefit to wait a short
>> amount of time only (the trade-off described in patch #1 of this series).
>>
> Actually we should just switch this code to what Avinash has
> finally made in downstream code. That keeps the RDS_GET_MR
> semantics and makes sure MR is really valid before handing over
> the key to userland. There is no need for any timeout
> for registration case.
> 

What do you call "RDS_GET_MR" semantics?

The purpose of waiting for a IB_WR_REG_MR request to complete
(inside rds_ib_post_reg_frmr) is in fact to make sure
the memory region is valid.

Regardless of this being true after a specific time-out,
or an infinite timeout.

For the non-infinite time-out case, there is a check if the request
was handled by the firmware.

And if a time-out occurred and the firmware didn't handle the request,
function "rds_ib_post_reg_frmr" will return -EBUSY.

>> Actually, no:
>> Socket option RDS_GET_MR wasn't even in the code-path of the
>> tests I performed:
>>
>> It were there RDS_CMSG_RDMA_MAP / RDS_CMSG_RDMA_DEST control
>> messages that ended up calling '__rds_rdma_map".
>>
> What option did you use ? Default option with rds-stress is
> RDS_GET_MR and hence the question.
> 

Not true!:

Socket option RDS_GET_MR is only used by "rds-stress"
if it is invoked with "--rdma-use-get-mr <non-zero-value>".

Verify this with the following patch applied to rds-tools-2.0.7-1.18:
--- a/rds-stress.c
+++ b/rds-stress.c
@@ -705,6 +705,7 @@ static uint64_t get_rdma_key(int fd, uint64_t addr, uint32_t size)
        if (opt.rdma_use_once)
                mr_args.flags |= RDS_RDMA_USE_ONCE;
 
+       abort();
        if (setsockopt(fd, sol, RDS_GET_MR, &mr_args, sizeof(mr_args)))
                die_errno("setsockopt(RDS_GET_MR) failed (%u allocated)", mrs_allocated);


And why is socket option RDS_GET_MR a subject of this discussion?

The proposed patch suggests to wait for a firmware response in handling
_all_ cases that end up in "rds_ib_post_reg_frmr", issuing a "IB_WR_REG_MR" request.

It doesn't matter where they came from:
Whether they came from an RDS_GET_MR socket option, or a  RDS_CMSG_RDMA_MAP:
They all go through "__rds_rdma_map" and end up calling "rds_ib_get_mr".

How is socket option RDS_GET_MR special with regards to this proposed fix?

>> I don't understand, please elaborate:
>> a) Are you saying this issue should not be fixed?
>> b) Or are you suggesting to replace this fix with a different fix?
>>     If it's the later, please point out what you have in mind.
>> c) ???
>>
> All am saying is the code got changed for good reason and that changed
> code makes some of these race conditions possibly not applicable.

I don't understand this. Please elaborate.

> So instead of these timeout fixes, am suggesting to use that
> code as fix. At least test it with those changes and see whats
> the behavior.
> 

Are you suggesting to
a) Not fix this bug right now and wait until some later point in time
b) Use a different fix. If you've got a different fix, please share.

And besides these options, is there anything wrong with this fix
(other than the discussion of what the timeout value ought to be,
 which we can address)?

Thanks,

  Gerd

Re: [PATCH net-next 3/7] net/rds: Wait for the FRMR_IS_FREE (or FRMR_IS_STALE) transition after posting IB_WR_LOCAL_INV

[santosh.shilimkar]

On 7/2/19 2:05 PM, Gerd Rausch wrote:
> On 02/07/2019 09.49, santosh.shilimkar@oracle.com wrote:
>> On 7/1/19 10:11 PM, Gerd Rausch wrote:
>>> For the registration work-requests there is a benefit to wait a short
>>> amount of time only (the trade-off described in patch #1 of this series).
>>>
>> Actually we should just switch this code to what Avinash has
>> finally made in downstream code. That keeps the RDS_GET_MR
>> semantics and makes sure MR is really valid before handing over
>> the key to userland. There is no need for any timeout
>> for registration case.
>>
> 
> What do you call "RDS_GET_MR" semantics?
> 
Its a blocking socket call. Meaning after this call return to the
user, the key must be valid. With async registration that can't be
guaranteed.

> The purpose of waiting for a IB_WR_REG_MR request to complete
> (inside rds_ib_post_reg_frmr) is in fact to make sure
> the memory region is valid.
> 
> Regardless of this being true after a specific time-out,
> or an infinite timeout.
> 
> For the non-infinite time-out case, there is a check if the request
> was handled by the firmware.
> 
> And if a time-out occurred and the firmware didn't handle the request,
> function "rds_ib_post_reg_frmr" will return -EBUSY.
> 
>>> Actually, no:
>>> Socket option RDS_GET_MR wasn't even in the code-path of the
>>> tests I performed:
>>>
>>> It were there RDS_CMSG_RDMA_MAP / RDS_CMSG_RDMA_DEST control
>>> messages that ended up calling '__rds_rdma_map".
>>>
>> What option did you use ? Default option with rds-stress is
>> RDS_GET_MR and hence the question.
>>
> 
> Not true!:
Its other way round. Thanks for info so default its using inline
registration instead of explicit call.

> How is socket option RDS_GET_MR special with regards to this proposed fix?
> 
>>> I don't understand, please elaborate:
>>> a) Are you saying this issue should not be fixed?
>>> b) Or are you suggesting to replace this fix with a different fix?
>>>      If it's the later, please point out what you have in mind.
>>> c) ???
>>>
>> All am saying is the code got changed for good reason and that changed
>> code makes some of these race conditions possibly not applicable.
> 
> I don't understand this. Please elaborate.
> 
>> So instead of these timeout fixes, am suggesting to use that
>> code as fix. At least test it with those changes and see whats
>> the behavior.
>>
> 
> Are you suggesting to
> a) Not fix this bug right now and wait until some later point in time
When did I say that ? I said have you explored alternate approach to
fix the issue and if not could you try it out.

> b) Use a different fix. If you've got a different fix, please share.
>
I don't but its a review of the fix and possible alternate needs to
be discussed. It is not like take my fix or provide an alternate fix.

> And besides these options, is there anything wrong with this fix
> (other than the discussion of what the timeout value ought to be,
>   which we can address)?
> 
That timeout is a problem because it doesn't guarantee the failure
of operation since its an asyn operation for registration. Instead
of timing out if you poll the CQ for that operation completion, it
makes it full proof. That is the change Avinash has done iirc and
am requesting to look at that fix.

Other 5 fixes from the series looks fine.

Regards,
Santosh

Regards,
Santosh

Re: [PATCH net-next 3/7] net/rds: Wait for the FRMR_IS_FREE (or FRMR_IS_STALE) transition after posting IB_WR_LOCAL_INV

[Gerd Rausch]

On 02/07/2019 14.18, santosh.shilimkar@oracle.com wrote:
> On 7/2/19 2:05 PM, Gerd Rausch wrote:
>> What do you call "RDS_GET_MR" semantics?
>>
> Its a blocking socket call. Meaning after this call return to the
> user, the key must be valid. With async registration that can't be
> guaranteed.
> 

If the "IB_WR_REG_MR" operation does not complete successfully within 
the given (to-be-discussed?) timeout, "rds_ib_post_reg_frmr" will return
"-EBUSY".

And that should propagate up the entire stack and make its way into
"setsockopt" returning "-1" with "errno == EBUSY".

Do you see a problem with this approach?
Did you observe a situation where this did not work?

Are you saying that no timeout, no matter how large, is large enough?
If that's the case, we can consider turning the "wait_event_timeout"
into a "wait_event".

>> Are you suggesting to
>> a) Not fix this bug right now and wait until some later point in time
> When did I say that ? I said have you explored alternate approach to
> fix the issue and if not could you try it out.
> 

Why explore an alternate approach?
Do you see a problem with the proposed patch (other than the choice of timeout)?

>> b) Use a different fix. If you've got a different fix, please share.
>>
> I don't but its a review of the fix and possible alternate needs to
> be discussed. It is not like take my fix or provide an alternate fix.
> 

As it is, the upstream implementation of RDS does not work.
IMO, it is desirable to make it work.

If there are future and better implementation of existing functionality
that is fine.
That should not preclude us from fixing what is broken as soon as we can.

>> And besides these options, is there anything wrong with this fix
>> (other than the discussion of what the timeout value ought to be,
>>   which we can address)?
>>
> That timeout is a problem because it doesn't guarantee the failure
> of operation since its an asyn operation for registration.

The fact that the work-request is asynchroneous is precisely what
makes it necessary to wait for the completion before moving on.

That is the proposed change of waiting for the completion
(or a time-out to put an upper bound to the wait)
does.

Replace the "wait_event_timeout" mentally with a "wait_event":
In that case, the process will be stuck in the corresponding function
(e.g. "rds_ib_post_reg_frmr") until the completion handler has occurred
and the "wake_up" call was issued.

It is the job of the "rds_ib_mr_cqe_handler" handler to inspect
the status of the work-completion and set the "fr_state" accordingly.

As far as I can tell, that is happening.

The debate on whether to use a "wait_event" or "wait_event_timeout"
is strictly a debate over whether or not there should
be an upper bound for the firmware to respond.

If there is not: It should be "wait_event".

If there is: It should be "wait_event_timeout", and we need to specify
what that upper bound is.

> Instead of timing out if you poll the CQ for that operation completion, it
> makes it full proof. That is the change Avinash has done iirc and
> am requesting to look at that fix.
> 

The "wake_up" call is issued from within the completion handler.

The completion handler "rds_ib_mr_cqe_handler" is called upon
handling the work-completions coming out of "ib_poll_cq".

That is necessary in order to check the status of the completion.

There are many changes that were done in the Oracle internal repository
(including changes that Avinash had done),
that we will go through in order to see what needs to be fixed in the
upstream version of RDS.

But unless you can see something that is wrong with this proposed fix,
I would suggest we don't leave the upstream RDS broken for extended
periods of time, but rather fix it.

> Other 5 fixes from the series looks fine.
> 

Thank you,

  Gerd

Re: [PATCH net-next 3/7] net/rds: Wait for the FRMR_IS_FREE (or FRMR_IS_STALE) transition after posting IB_WR_LOCAL_INV

[santosh.shilimkar]

On 7/2/19 3:12 PM, Gerd Rausch wrote:
> On 02/07/2019 14.18, santosh.shilimkar@oracle.com wrote:
>> On 7/2/19 2:05 PM, Gerd Rausch wrote:
>>> What do you call "RDS_GET_MR" semantics?
>>>
>> Its a blocking socket call. Meaning after this call return to the
>> user, the key must be valid. With async registration that can't be
>> guaranteed.
>>
> 
> If the "IB_WR_REG_MR" operation does not complete successfully within
> the given (to-be-discussed?) timeout, "rds_ib_post_reg_frmr" will return
> "-EBUSY".
> 
> And that should propagate up the entire stack and make its way into
> "setsockopt" returning "-1" with "errno == EBUSY".

This is an easy case and this doesn't need any waiting since call just
came back without posting work request.
> 
> Do you see a problem with this approach?
> Did you observe a situation where this did not work?
>
Calling rds_ib_post_reg_frmr() and looking at return value doesn't
grantee that the work request postred is gping to be successful.

> Are you saying that no timeout, no matter how large, is large enough?
> If that's the case, we can consider turning the "wait_event_timeout"
> into a "wait_event".
>
Yep. Basically till the ceq handler reports successful completion of
reg_mr or inval_mr, mr is not guaranteed to be registered or
invalidated.

>>> Are you suggesting to
>>> a) Not fix this bug right now and wait until some later point in time
>> When did I say that ? I said have you explored alternate approach to
>> fix the issue and if not could you try it out.
>>
> 
> Why explore an alternate approach?
> Do you see a problem with the proposed patch (other than the choice of timeout)?
> 
Yes the timeout based proceeding isn't safe. wait_event without timeout
would make it guaranteed and give sync like behavior. This is the
behavior with FMR reg and inval calls. If you make that as
wait_event then am fine with the change.

Regards,
Santosh