[BUG] SRP daemon and SM migration

[BUG] SRP daemon and SM migration

[Nicolas Morey-Chaisemartin]

Hi

A bug was reported to SUSE concerning the srp_daemon.
When it's running and the matser SM changes host (host shutdown, or new higher priority SM started), srp_daemon outputs these errors at every scan:
srp_daemon[25394]: No response to inform info registration
srp_daemon[25394]: Fail to register to traps, maybe there is no opensm running on fabric or IB port is down

It seems this was introduced by this commit:
commit 4952e5f7df0c93d6f3972975106c5e06623a301d
Author: Roi Dayan <roid-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Date:   Thu Mar 21 17:38:11 2013 +0200

    Fix a memory leak
   
    Avoid leaking one IB AH per rescan. Only allocate a new AH if the
    port LID changed or after a LID has been assigned by the SM.
   
    Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
    Signed-off-by: Roi Dayan <roid-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>


One of the side effect of the leak fix is that create_ah is only called when the local port lid changes.
And register_to_traps uses the sm_id from ud_res which is filled by create_ah.

Thus if the SM lid changes but not the local LID, it keeps trying to contact the previous LID.

I tried fixing it by getting get_port_lid to also return the SM lid and calling create_ah on local lid OR SM lid changes.
It seems to be working at first (at least the call is always done to the right lid).
But after a while (doing ping pong between 2 SM by changing the priority) I still end up getting the error above.
Even through the LID is right this time.

It may not be the same bug though. Is there some calls to do to unregister from the previous SM before registering to the new one ?
Any idea on what could cause this ? I don't seem to get any more infos in all the logs I've checked...

Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BUG] SRP daemon and SM migration

[Hal Rosenstock]

Hi Nicolas,

On 12/4/2017 6:08 AM, Nicolas Morey-Chaisemartin wrote:
> Hi
> 
> A bug was reported to SUSE concerning the srp_daemon.
> When it's running and the matser SM changes host (host shutdown, or new higher priority SM started), srp_daemon outputs these errors at every scan:
> srp_daemon[25394]: No response to inform info registration
> srp_daemon[25394]: Fail to register to traps, maybe there is no opensm running on fabric or IB port is down
> 
> It seems this was introduced by this commit:
> commit 4952e5f7df0c93d6f3972975106c5e06623a301d
> Author: Roi Dayan <roid-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Date:   Thu Mar 21 17:38:11 2013 +0200
> 
>     Fix a memory leak
>    
>     Avoid leaking one IB AH per rescan. Only allocate a new AH if the
>     port LID changed or after a LID has been assigned by the SM.
>    
>     Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
>     Signed-off-by: Roi Dayan <roid-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> 
> One of the side effect of the leak fix is that create_ah is only called when the local port lid changes.
> And register_to_traps uses the sm_id from ud_res which is filled by create_ah.
> 
> Thus if the SM lid changes but not the local LID, it keeps trying to contact the previous LID.
> 
> I tried fixing it by getting get_port_lid to also return the SM lid and calling create_ah on local lid OR SM lid changes.

Yes, client should reregister for traps with SA on local LID or SM LID
change. Note that SMSL could change as well.

> It seems to be working at first (at least the call is always done to the right lid).
> But after a while (doing ping pong between 2 SM by changing the priority) I still end up getting the error above.
> Even through the LID is right this time.

> It may not be the same bug though. 

In srp_handle_traps.c:register_to_trap, try increasing counter to
something more than 3:

        } while (rc == 0 && ++counter < 3);

        if (counter==3) {

It may be that SM/SA handover/failover takes longer than 3 seconds in
some cases.

> Is there some calls to do to unregister from the previous SM before registering to the new one ?

This is complex subject. Although deregistration would eliminate stale
registrations (events/traps, multicast, services) in SA which
potentially can cause timed out event notifications (SA Reports), once
SA is no longer master, it will not respond to SA client requests and
client has no way to control when SM/SA master transition occurs.

-- Hal

> Any idea on what could cause this ? I don't seem to get any more infos in all the logs I've checked...
> 
> Nicolas
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC rdma-core] srp_daemon: handle SM lid change

[Nicolas Morey-Chaisemartin]

When srp_daemon was running and the master SM host changes,
 srp_daemon output these errors at every scan:
srp_daemon[25394]: No response to inform info registration
srp_daemon[25394]: Fail to register to traps, maybe there is no opensm
 running on fabric or IB port is down

This was introduced by commit 4952e5f Fix a memory leak.
A side effect of this patch was that create_ah was only called when the
 port lid changes. Which meant register_to_traps used an older, obsolete,
 version of sm_lid and failed to connect to it.

This patch fixes this behaviour by checking for both local lid changes and
 SM lid changes, and calling create_ah on any of these events.

Signed-off-by: Nicolas Morey-Chaisemartin <NMoreyChaisemartin-IBi9RG/b67k@public.gmane.org>
---

This fixes the bug for me but I'm waiting for feedback from the customer to make sure it fixes the issue for him too.
And not just part of it.
I'll repost a proper patch (non RFC) once he confirms it.

 srp_daemon/srp_daemon.c       | 10 ++++++----
 srp_daemon/srp_daemon.h       |  2 +-
 srp_daemon/srp_handle_traps.c | 16 ++++++++++++----
 3 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/srp_daemon/srp_daemon.c b/srp_daemon/srp_daemon.c
index 2465ccd9..36df5c3b 100644
--- a/srp_daemon/srp_daemon.c
+++ b/srp_daemon/srp_daemon.c
@@ -1103,7 +1103,7 @@ static int get_shared_pkeys(struct resources *res,
 	int i, num_pkeys = 0;
 	uint16_t pkey;
 	uint16_t local_port_lid = get_port_lid(res->ud_res->ib_ctx,
-					       config->port_num);
+					       config->port_num, NULL);
 
 	in_mad_buf = malloc(sizeof(struct ib_user_mad) +
 			    node_table_response_size);
@@ -2092,7 +2092,7 @@ int main(int argc, char *argv[])
 {
 	int			ret;
 	struct resources       *res;
-	uint16_t 		lid;
+	uint16_t 		lid, sm_lid;
 	uint16_t 		pkey;
 	union umad_gid 		gid;
 	struct target_details  *target;
@@ -2196,8 +2196,10 @@ catas_start:
 
 			pr_debug("Starting a recalculation\n");
 			port_lid = get_port_lid(res->ud_res->ib_ctx,
-					   config->port_num);
-			if (port_lid != res->ud_res->port_attr.lid) {
+						config->port_num, &sm_lid);
+			if (port_lid != res->ud_res->port_attr.lid ||
+				sm_lid != res->ud_res->port_attr.sm_lid) {
+
 				if (res->ud_res->ah) {
 					ibv_destroy_ah(res->ud_res->ah);
 					res->ud_res->ah = NULL;
diff --git a/srp_daemon/srp_daemon.h b/srp_daemon/srp_daemon.h
index 5d268ed3..864b3d42 100644
--- a/srp_daemon/srp_daemon.h
+++ b/srp_daemon/srp_daemon.h
@@ -299,7 +299,7 @@ void *run_thread_listen_to_events(void *res_in);
 int get_node(struct umad_resources *umad_res, uint16_t dlid, uint64_t *guid);
 int create_trap_resources(struct ud_resources *ud_res);
 int register_to_traps(struct resources *res, int subscribe);
-uint16_t get_port_lid(struct ibv_context *ib_ctx, int port_num);
+uint16_t get_port_lid(struct ibv_context *ib_ctx, int port_num, uint16_t *sm_lid);
 int create_ah(struct ud_resources *ud_res);
 void push_gid_to_list(struct sync_resources *res, union umad_gid *gid,
 		      uint16_t pkey);
diff --git a/srp_daemon/srp_handle_traps.c b/srp_daemon/srp_handle_traps.c
index 6d94634e..e546d491 100644
--- a/srp_daemon/srp_handle_traps.c
+++ b/srp_daemon/srp_handle_traps.c
@@ -340,12 +340,20 @@ int ud_resources_create(struct ud_resources *res)
 	return 0;
 }
 
-uint16_t get_port_lid(struct ibv_context *ib_ctx, int port_num)
+uint16_t get_port_lid(struct ibv_context *ib_ctx, int port_num, uint16_t *sm_lid)
 {
 	struct ibv_port_attr port_attr;
+	int ret;
+
+	ret = ibv_query_port(ib_ctx, port_num, &port_attr);
 
-	return ibv_query_port(ib_ctx, port_num, &port_attr) == 0 ?
-		port_attr.lid : 0;
+	if (!ret) {
+		if (sm_lid)
+			*sm_lid = port_attr.sm_lid;
+		return port_attr.lid;
+	}
+
+	return 0;
 }
 
 int create_ah(struct ud_resources *ud_res)
@@ -641,7 +649,7 @@ static int register_to_trap(struct sync_resources *sync_res,
 			pthread_mutex_unlock(res->mad_buffer_mutex);
 		} while (rc == 2); // while old response.
 
-	} while (rc == 0 && ++counter < 3);
+	} while (rc == 0 && ++counter < 5);
 
 	if (counter==3) {
 		pr_err("No response to inform info registration\n");
-- 
2.15.1.272.g8e603414b

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC rdma-core] srp_daemon: handle SM lid change

[Hal Rosenstock]

On 12/8/2017 4:36 AM, Nicolas Morey-Chaisemartin wrote:
> When srp_daemon was running and the master SM host changes,
>  srp_daemon output these errors at every scan:
> srp_daemon[25394]: No response to inform info registration
> srp_daemon[25394]: Fail to register to traps, maybe there is no opensm
>  running on fabric or IB port is down
> 
> This was introduced by commit 4952e5f Fix a memory leak.
> A side effect of this patch was that create_ah was only called when the
>  port lid changes. Which meant register_to_traps used an older, obsolete,
>  version of sm_lid and failed to connect to it.
> 
> This patch fixes this behaviour by checking for both local lid changes and
>  SM lid changes, and calling create_ah on any of these events.
> 
> Signed-off-by: Nicolas Morey-Chaisemartin <NMoreyChaisemartin-IBi9RG/b67k@public.gmane.org>
> ---
> 
> This fixes the bug for me but I'm waiting for feedback from the customer to make sure it fixes the issue for him too.
> And not just part of it.
> I'll repost a proper patch (non RFC) once he confirms it.
> 
>  srp_daemon/srp_daemon.c       | 10 ++++++----
>  srp_daemon/srp_daemon.h       |  2 +-
>  srp_daemon/srp_handle_traps.c | 16 ++++++++++++----
>  3 files changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/srp_daemon/srp_daemon.c b/srp_daemon/srp_daemon.c
> index 2465ccd9..36df5c3b 100644
> --- a/srp_daemon/srp_daemon.c
> +++ b/srp_daemon/srp_daemon.c
> @@ -1103,7 +1103,7 @@ static int get_shared_pkeys(struct resources *res,
>  	int i, num_pkeys = 0;
>  	uint16_t pkey;
>  	uint16_t local_port_lid = get_port_lid(res->ud_res->ib_ctx,
> -					       config->port_num);
> +					       config->port_num, NULL);
>  
>  	in_mad_buf = malloc(sizeof(struct ib_user_mad) +
>  			    node_table_response_size);
> @@ -2092,7 +2092,7 @@ int main(int argc, char *argv[])
>  {
>  	int			ret;
>  	struct resources       *res;
> -	uint16_t 		lid;
> +	uint16_t 		lid, sm_lid;
>  	uint16_t 		pkey;
>  	union umad_gid 		gid;
>  	struct target_details  *target;
> @@ -2196,8 +2196,10 @@ catas_start:
>  
>  			pr_debug("Starting a recalculation\n");
>  			port_lid = get_port_lid(res->ud_res->ib_ctx,
> -					   config->port_num);
> -			if (port_lid != res->ud_res->port_attr.lid) {
> +						config->port_num, &sm_lid);
> +			if (port_lid != res->ud_res->port_attr.lid ||
> +				sm_lid != res->ud_res->port_attr.sm_lid) {
> +
>  				if (res->ud_res->ah) {
>  					ibv_destroy_ah(res->ud_res->ah);
>  					res->ud_res->ah = NULL;
> diff --git a/srp_daemon/srp_daemon.h b/srp_daemon/srp_daemon.h
> index 5d268ed3..864b3d42 100644
> --- a/srp_daemon/srp_daemon.h
> +++ b/srp_daemon/srp_daemon.h
> @@ -299,7 +299,7 @@ void *run_thread_listen_to_events(void *res_in);
>  int get_node(struct umad_resources *umad_res, uint16_t dlid, uint64_t *guid);
>  int create_trap_resources(struct ud_resources *ud_res);
>  int register_to_traps(struct resources *res, int subscribe);
> -uint16_t get_port_lid(struct ibv_context *ib_ctx, int port_num);
> +uint16_t get_port_lid(struct ibv_context *ib_ctx, int port_num, uint16_t *sm_lid);
>  int create_ah(struct ud_resources *ud_res);
>  void push_gid_to_list(struct sync_resources *res, union umad_gid *gid,
>  		      uint16_t pkey);
> diff --git a/srp_daemon/srp_handle_traps.c b/srp_daemon/srp_handle_traps.c
> index 6d94634e..e546d491 100644
> --- a/srp_daemon/srp_handle_traps.c
> +++ b/srp_daemon/srp_handle_traps.c
> @@ -340,12 +340,20 @@ int ud_resources_create(struct ud_resources *res)
>  	return 0;
>  }
>  
> -uint16_t get_port_lid(struct ibv_context *ib_ctx, int port_num)
> +uint16_t get_port_lid(struct ibv_context *ib_ctx, int port_num, uint16_t *sm_lid)
>  {
>  	struct ibv_port_attr port_attr;
> +	int ret;
> +
> +	ret = ibv_query_port(ib_ctx, port_num, &port_attr);
>  
> -	return ibv_query_port(ib_ctx, port_num, &port_attr) == 0 ?
> -		port_attr.lid : 0;
> +	if (!ret) {
> +		if (sm_lid)
> +			*sm_lid = port_attr.sm_lid;
> +		return port_attr.lid;
> +	}
> +
> +	return 0;
>  }
>  
>  int create_ah(struct ud_resources *ud_res)
> @@ -641,7 +649,7 @@ static int register_to_trap(struct sync_resources *sync_res,
>  			pthread_mutex_unlock(res->mad_buffer_mutex);
>  		} while (rc == 2); // while old response.
>  
> -	} while (rc == 0 && ++counter < 3);
> +	} while (rc == 0 && ++counter < 5);
>  
>  	if (counter==3) {

This should be changed from 3 to 5 also. Change both to some define ?

>  		pr_err("No response to inform info registration\n");
> 

Other than issue above, looks fine to me.

Reviewed-by: Hal Rosenstock <hal-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC rdma-core] srp_daemon: handle SM lid change

[Dennis Dalessandro]

On 12/8/2017 8:33 AM, Hal Rosenstock wrote:
>>   int create_ah(struct ud_resources *ud_res)
>> @@ -641,7 +649,7 @@ static int register_to_trap(struct sync_resources *sync_res,
>>   			pthread_mutex_unlock(res->mad_buffer_mutex);
>>   		} while (rc == 2); // while old response.
>>   
>> -	} while (rc == 0 && ++counter < 3);
>> +	} while (rc == 0 && ++counter < 5);
>>   
>>   	if (counter==3) {
> 
> This should be changed from 3 to 5 also. Change both to some define ?

Yes please change these to a #define with a some what descriptive name. 
That would make the code much better to read.

-Denny

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC rdma-core] srp_daemon: handle SM lid change

[Bart Van Assche]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1099 bytes --]

On Fri, 2017-12-08 at 08:56 -0500, Dennis Dalessandro wrote:
> On 12/8/2017 8:33 AM, Hal Rosenstock wrote:
> > >   int create_ah(struct ud_resources *ud_res)
> > > @@ -641,7 +649,7 @@ static int register_to_trap(struct sync_resources *sync_res,
> > >   			pthread_mutex_unlock(res->mad_buffer_mutex);
> > >   		} while (rc == 2); // while old response.
> > >   
> > > -	} while (rc == 0 && ++counter < 3);
> > > +	} while (rc == 0 && ++counter < 5);
> > >   
> > >   	if (counter==3) {
> > 
> > This should be changed from 3 to 5 also. Change both to some define ?
> 
> Yes please change these to a #define with a some what descriptive name. 
> That would make the code much better to read.

It would be even better to change the loop such that it counts down instead
of up. Then the if-statement after the loop can test whether the counter has
reached zero and the constant that represents the number of iterations will
only occur once.

Bart.N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±­ÙšŠ{ayº\x1dʇڙë,j\a­¢f£¢·hš‹»öì\x17/oSc¾™Ú³9˜uÀ¦æå‰È&jw¨®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿïêäz¹Þ–Šàþf£¢·hšˆ§~ˆmš

Re: [RFC rdma-core] srp_daemon: handle SM lid change

[Nicolas Morey-Chaisemartin]

Le 08/12/2017 à 14:33, Hal Rosenstock a écrit :
> On 12/8/2017 4:36 AM, Nicolas Morey-Chaisemartin wrote:
>>  
>>  int create_ah(struct ud_resources *ud_res)
>> @@ -641,7 +649,7 @@ static int register_to_trap(struct sync_resources *sync_res,
>>  			pthread_mutex_unlock(res->mad_buffer_mutex);
>>  		} while (rc == 2); // while old response.
>>  
>> -	} while (rc == 0 && ++counter < 3);
>> +	} while (rc == 0 && ++counter < 5);
>>  
>>  	if (counter==3) {
> This should be changed from 3 to 5 also. Change both to some define ?
I thought I changed that back to 3 during my tests.
No I have a big doubt that the issue is fully resolved, or just hidden because on timeout, the pr_err is never reached.
I'll retest that in Monday.

Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC rdma-core 2/2] srp_daemon: fix CQ handling

[Nicolas Morey-Chaisemartin]

SM traps are polled through poll_cq which waited for a CQ event
before polling the CQ itself.
However it may happens that multiple completions are attached
to a single event. As stated by the ibv_get_cq_event, it is required
to poll the the CQ to get those event after the call to
ibv_req_notify_cq.

As completion need to be handled one by one in an outer function,
start by polling the CQ and return the event (if any) before waiting
for the next completion event.

The buggy use case seem to appear when the master SM is switched multiple
times between two nodes. As the number of ping-pong between the SM increases,
the number of traps sent to notify that the SM just became master increases
too. This causes burst of completions linked to a single event.
Note that the race condition is also possible in other scenario.

Signed-off-by: Nicolas Morey-Chaisemartin <NMoreyChaisemartin-IBi9RG/b67k@public.gmane.org>
---
 srp_daemon/srp_handle_traps.c | 40 ++++++++++++++++++++++++++++------------

This patch goes on top of the one proposed in the thread.

 1 file changed, 28 insertions(+), 12 deletions(-)

diff --git a/srp_daemon/srp_handle_traps.c b/srp_daemon/srp_handle_traps.c
index b1d051c6..eaa57979 100644
--- a/srp_daemon/srp_handle_traps.c
+++ b/srp_daemon/srp_handle_traps.c
@@ -496,6 +496,27 @@ static int stop_threads(struct sync_resources *sync_res)
 	return result;
 }
 
+tatic int poll_cq_once(struct sync_resources *sync_res, struct ibv_cq *cq,
+           struct ibv_wc *wc)
+{
+	int ret;
+	ret = ibv_poll_cq(cq, 1, wc);
+	if (ret < 0) {
+		pr_err("poll CQ failed\n");
+		return ret;
+	}
+
+	if (ret > 0 && wc->status != IBV_WC_SUCCESS) {
+		if (!stop_threads(sync_res))
+			pr_err("got bad completion with status: 0x%x\n",
+			       wc->status);
+		return -ret;
+	}
+
+	return ret;
+}
+
+
 static int poll_cq(struct sync_resources *sync_res, struct ibv_cq *cq,
 		   struct ibv_wc *wc, struct ibv_comp_channel *channel)
 {
@@ -504,6 +525,12 @@ static int poll_cq(struct sync_resources *sync_res, struct ibv_cq *cq,
 	void          *ev_ctx;
 
 	if (channel) {
+		/* Poll CQ once. There may be extra completion that
+		 * were associated to the previous event */
+		ret = poll_cq_once(sync_res, cq, wc);
+		if (ret)
+			return ret;
+
 		if (ibv_get_cq_event(channel, &ev_cq, &ev_ctx)) {
 			pr_err("Failed to get cq_event\n");
 			return -1;
@@ -524,18 +551,7 @@ static int poll_cq(struct sync_resources *sync_res, struct ibv_cq *cq,
 	}
 
 	do {
-		ret = ibv_poll_cq(cq, 1, wc);
-		if (ret < 0) {
-			pr_err("poll CQ failed\n");
-			return ret;
-		}
-
-		if (ret > 0 && wc->status != IBV_WC_SUCCESS) {
-			if (!stop_threads(sync_res))
-				pr_err("got bad completion with status: 0x%x\n",
-				       wc->status);
-			return -ret;
-		}
+		ret = poll_cq_once(sync_res, cq, wc);
 
 		if (ret == 0 && channel) {
 			pr_err("Weird poll returned no cqe after CQ event\n");
-- 
2.15.1.272.g8e603414b

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC rdma-core 2/2] srp_daemon: fix CQ handling

[Bart Van Assche]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 763 bytes --]

On Mon, 2017-12-11 at 15:46 +0100, Nicolas Morey-Chaisemartin wrote:
> +tatic int poll_cq_once(struct sync_resources *sync_res, struct ibv_cq *cq,
> +           struct ibv_wc *wc)

Has this patch been tested? I don't think that "tatic" is a valid keyword?

> +		/* Poll CQ once. There may be extra completion that
> +		 * were associated to the previous event */
> +		ret = poll_cq_once(sync_res, cq, wc);
> +		if (ret)
> +			return ret;

How many extra completions can be associated with an event? Is polling once
sufficient? If so, please update the comment ("an" is missing / "were" ->
"was").

Thanks,

Bart.N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±­ÙšŠ{ayº\x1dʇڙë,j\a­¢f£¢·hš‹»öì\x17/oSc¾™Ú³9˜uÀ¦æå‰È&jw¨®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿïêäz¹Þ–Šàþf£¢·hšˆ§~ˆmš

Re: [RFC rdma-core 2/2] srp_daemon: fix CQ handling

[Nicolas Morey-Chaisemartin]

Le 11/12/2017 à 17:30, Bart Van Assche a écrit :
> On Mon, 2017-12-11 at 15:46 +0100, Nicolas Morey-Chaisemartin wrote:
>> +tatic int poll_cq_once(struct sync_resources *sync_res, struct ibv_cq *cq,
>> +           struct ibv_wc *wc)
> Has this patch been tested? I don't think that "tatic" is a valid keyword?

Typo during the rebase. But the overall fix was tested yes. I'm looking for insight to make sure my understanding of the issue (and the way to fix it) is valid.
>
>> +		/* Poll CQ once. There may be extra completion that
>> +		 * were associated to the previous event */
>> +		ret = poll_cq_once(sync_res, cq, wc);
>> +		if (ret)
>> +			return ret;
> How many extra completions can be associated with an event? Is polling once
> sufficient? If so, please update the comment ("an" is missing / "were" ->
> "was").
There can be more than one.
This should be enough becausethey would all be consumed through the call to poll_cq.
As long as there as completion in the CQ, they will be returned directly without waiting for an event.
Once there are no more, the code will wait for the next event again.

I'll fix the typo and the log.

Thanks

Nicolas

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html