[PATCH] rds: Fix inaccurate accounting of unsignaled wrs

[PATCH] rds: Fix inaccurate accounting of unsignaled wrs

[Håkon Bugge]

The number of unsignaled work-requests posted to the IB send queue is
tracked by a counter in the rds_ib_connection struct. When it reaches
zero, or the caller explicitly asks for it, the send-signaled bit is
set in send_flags and the counter is reset. This is performed by the
rds_ib_set_wr_signal_state() function.

However, this function is not always used which yields inaccurate
accounting. This commit fixes this, re-factors a code bloat related to
the matter, and makes the actual parameter type to the function
consistent.

Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
---
 net/rds/ib_send.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/net/rds/ib_send.c b/net/rds/ib_send.c
index 6ab39db..d07ecb0 100644
--- a/net/rds/ib_send.c
+++ b/net/rds/ib_send.c
@@ -661,13 +661,15 @@ int rds_ib_xmit(struct rds_connection *conn, struct rds_message *rm,
 			}
 		}
 
-		rds_ib_set_wr_signal_state(ic, send, 0);
+		rds_ib_set_wr_signal_state(ic, send, false);
 
 		/*
 		 * Always signal the last one if we're stopping due to flow control.
 		 */
-		if (ic->i_flowctl && flow_controlled && i == (work_alloc-1))
-			send->s_wr.send_flags |= IB_SEND_SIGNALED | IB_SEND_SOLICITED;
+		if (ic->i_flowctl && flow_controlled && i == (work_alloc - 1)) {
+			rds_ib_set_wr_signal_state(ic, send, true);
+			send->s_wr.send_flags |= IB_SEND_SOLICITED;
+		}
 
 		if (send->s_wr.send_flags & IB_SEND_SIGNALED)
 			nr_sig++;
@@ -705,11 +707,8 @@ int rds_ib_xmit(struct rds_connection *conn, struct rds_message *rm,
 	if (scat == &rm->data.op_sg[rm->data.op_count]) {
 		prev->s_op = ic->i_data_op;
 		prev->s_wr.send_flags |= IB_SEND_SOLICITED;
-		if (!(prev->s_wr.send_flags & IB_SEND_SIGNALED)) {
-			ic->i_unsignaled_wrs = rds_ib_sysctl_max_unsig_wrs;
-			prev->s_wr.send_flags |= IB_SEND_SIGNALED;
-			nr_sig++;
-		}
+		if (!(prev->s_wr.send_flags & IB_SEND_SIGNALED))
+			nr_sig += rds_ib_set_wr_signal_state(ic, prev, true);
 		ic->i_data_op = NULL;
 	}
 
-- 
1.8.3.1

Re: [PATCH] rds: Fix inaccurate accounting of unsignaled wrs

[Santosh Shilimkar]

On 10/24/2017 7:16 AM, Håkon Bugge wrote:
> The number of unsignaled work-requests posted to the IB send queue is
> tracked by a counter in the rds_ib_connection struct. When it reaches
> zero, or the caller explicitly asks for it, the send-signaled bit is
> set in send_flags and the counter is reset. This is performed by the
> rds_ib_set_wr_signal_state() function.
> 
> However, this function is not always used which yields inaccurate
> accounting. This commit fixes this, re-factors a code bloat related to
> the matter, and makes the actual parameter type to the function
> consistent.
> 
> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
> ---
Instead of partially doing changes inside/outside helper,
can also add inline helper for solicited state like
rds_ib_set_wr_solicited_state() and use that along
with this change.

Regards,
Santosh

Re: [PATCH] rds: Fix inaccurate accounting of unsignaled wrs

[Santosh Shilimkar]

On 10/24/2017 9:15 AM, Håkon Bugge wrote:
> 
>> On 24 Oct 2017, at 18:05, Santosh Shilimkar 
>> <santosh.shilimkar-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org <mailto:santosh.shilimkar-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>> 
>> wrote:
>>

[...]
>> Instead of partially doing changes inside/outside helper,
>> can also add inline helper for solicited state like
>> rds_ib_set_wr_solicited_state() and use that along
>> with this change.
> 
> Why? There is no book-keeping associated with setting send-solicited. 
> Its set on the last fragment of a message and on the last fragment sent 
> before throttling due to flow-control.
>
> Creating a function to perform:
> 
>          FOO |= BAR;
> 
> seems like an overkill to me.
>
Its just inline helper and keep code consistent for flag
manipulation. Compiler output will be like "FOO =| BAR;" :-)

> That being said, in my opinion the fragments of a (large) send should be 
> scattered with send-solicited. But that is another commit. But with such 
> a commit, I agree with you, a helper function is required.
> 
We already talked about it so lets leave it there.

Regards,
Santosh
--
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: [PATCH] rds: Fix inaccurate accounting of unsignaled wrs

[David Miller]

From: Håkon Bugge <Haakon.Bugge@oracle.com>
Date: Tue, 24 Oct 2017 16:16:28 +0200

> The number of unsignaled work-requests posted to the IB send queue is
> tracked by a counter in the rds_ib_connection struct. When it reaches
> zero, or the caller explicitly asks for it, the send-signaled bit is
> set in send_flags and the counter is reset. This is performed by the
> rds_ib_set_wr_signal_state() function.
> 
> However, this function is not always used which yields inaccurate
> accounting. This commit fixes this, re-factors a code bloat related to
> the matter, and makes the actual parameter type to the function
> consistent.
> 
> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>

Applied.