linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next 00/14] rxrpc: Fixes & miscellany
@ 2016-09-17 23:17 David Howells
  2016-09-17 23:17 ` [PATCH net-next 01/14] rxrpc: Remove some whitespace David Howells
                   ` (15 more replies)
  0 siblings, 16 replies; 17+ messages in thread
From: David Howells @ 2016-09-17 23:17 UTC (permalink / raw)
  To: netdev; +Cc: dhowells, linux-afs, linux-kernel


Here are some more AF_RXRPC fix patches with a couple of miscellaneous
changes also.  Fixes include:

 (1) Make RxRPC IPv6 support conditional on IPv6 being available.

 (2) Move the condition check in rxrpc_locate_data() into the caller and
     check the error return.

 (3) Fix the detection of the last received packet in recvmsg.

 (4) Account calls that need acceptance and clean up any unaccepted ones if
     the socket gets closed.

 (5) Fix the cleanup of client connections.

 (6) Fix the soft-ACK parsing and the retransmission of packets based on
     those ACKs.

 (7) Suppress transmission of an ACK when there's no pending ACK to
     transmit because another thread stole it.

And some miscellany:

 (8) Whitespace removal.

 (9) Switch-value consistency in rxrpc_send_call_packet().

(10) Fix the basic transmission packet size to allow for spur-of-the-moment
     jumbo DATA packet production.


The patches can be found here also (non-terminally on the branch):

	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=rxrpc-rewrite

Tagged thusly:

	git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git
	rxrpc-rewrite-20160917-1

David
---
David Howells (14):
      rxrpc: Remove some whitespace.
      rxrpc: Move the check of rx_pkt_offset from rxrpc_locate_data() to caller
      rxrpc: Check the return value of rxrpc_locate_data()
      rxrpc: Fix handling of the last packet in rxrpc_recvmsg_data()
      rxrpc: Record calls that need to be accepted
      rxrpc: Purge the to_be_accepted queue on socket release
      rxrpc: Fix the putting of client connections
      rxrpc: Call rxrpc_release_call() on error in rxrpc_new_client_call()
      rxrpc: Fix unexposed client conn release
      rxrpc: Fix the parsing of soft-ACKs
      rxrpc: Fix retransmission algorithm
      rxrpc: Don't transmit an ACK if there's no reason set
      rxrpc: Be consistent about switch value in rxrpc_send_call_packet()
      rxrpc: Fix the basic transmit DATA packet content size at 1412 bytes


 net/rxrpc/call_accept.c |    2 ++
 net/rxrpc/call_event.c  |   14 ++++--------
 net/rxrpc/call_object.c |   46 ++++++++++++++++++++---------------------
 net/rxrpc/conn_client.c |   29 ++++++++++++--------------
 net/rxrpc/input.c       |    6 ++++-
 net/rxrpc/output.c      |    7 +++++-
 net/rxrpc/recvmsg.c     |   53 ++++++++++++++++++++++++++++++++---------------
 net/rxrpc/sendmsg.c     |    2 +-
 8 files changed, 89 insertions(+), 70 deletions(-)

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH net-next 01/14] rxrpc: Remove some whitespace.
  2016-09-17 23:17 [PATCH net-next 00/14] rxrpc: Fixes & miscellany David Howells
@ 2016-09-17 23:17 ` David Howells
  2016-09-17 23:17 ` [PATCH net-next 02/14] rxrpc: Move the check of rx_pkt_offset from rxrpc_locate_data() to caller David Howells
                   ` (14 subsequent siblings)
  15 siblings, 0 replies; 17+ messages in thread
From: David Howells @ 2016-09-17 23:17 UTC (permalink / raw)
  To: netdev; +Cc: dhowells, linux-afs, linux-kernel

Remove a tab that's on a line that should otherwise be blank.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 net/rxrpc/call_event.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/rxrpc/call_event.c b/net/rxrpc/call_event.c
index 61432049869b..9367c3be31eb 100644
--- a/net/rxrpc/call_event.c
+++ b/net/rxrpc/call_event.c
@@ -31,7 +31,7 @@ static void rxrpc_set_timer(struct rxrpc_call *call)
 	_enter("{%ld,%ld,%ld:%ld}",
 	       call->ack_at - now, call->resend_at - now, call->expire_at - now,
 	       call->timer.expires - now);
-	
+
 	read_lock_bh(&call->state_lock);
 
 	if (call->state < RXRPC_CALL_COMPLETE) {

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH net-next 02/14] rxrpc: Move the check of rx_pkt_offset from rxrpc_locate_data() to caller
  2016-09-17 23:17 [PATCH net-next 00/14] rxrpc: Fixes & miscellany David Howells
  2016-09-17 23:17 ` [PATCH net-next 01/14] rxrpc: Remove some whitespace David Howells
@ 2016-09-17 23:17 ` David Howells
  2016-09-17 23:18 ` [PATCH net-next 03/14] rxrpc: Check the return value of rxrpc_locate_data() David Howells
                   ` (13 subsequent siblings)
  15 siblings, 0 replies; 17+ messages in thread
From: David Howells @ 2016-09-17 23:17 UTC (permalink / raw)
  To: netdev; +Cc: dhowells, linux-afs, linux-kernel

Move the check of rx_pkt_offset from rxrpc_locate_data() to the caller,
rxrpc_recvmsg_data(), so that it's more clear what's going on there.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 net/rxrpc/recvmsg.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/net/rxrpc/recvmsg.c b/net/rxrpc/recvmsg.c
index a284205b8ecf..0d085f5cf1bf 100644
--- a/net/rxrpc/recvmsg.c
+++ b/net/rxrpc/recvmsg.c
@@ -240,9 +240,6 @@ static int rxrpc_locate_data(struct rxrpc_call *call, struct sk_buff *skb,
 	int ret;
 	u8 annotation = *_annotation;
 
-	if (offset > 0)
-		return 0;
-
 	/* Locate the subpacket */
 	offset = sp->offset;
 	len = skb->len - sp->offset;
@@ -303,8 +300,10 @@ static int rxrpc_recvmsg_data(struct socket *sock, struct rxrpc_call *call,
 		if (msg)
 			sock_recv_timestamp(msg, sock->sk, skb);
 
-		ret = rxrpc_locate_data(call, skb, &call->rxtx_annotations[ix],
-					&rx_pkt_offset, &rx_pkt_len);
+		if (rx_pkt_offset == 0)
+			ret = rxrpc_locate_data(call, skb,
+						&call->rxtx_annotations[ix],
+						&rx_pkt_offset, &rx_pkt_len);
 		_debug("recvmsg %x DATA #%u { %d, %d }",
 		       sp->hdr.callNumber, seq, rx_pkt_offset, rx_pkt_len);
 

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH net-next 03/14] rxrpc: Check the return value of rxrpc_locate_data()
  2016-09-17 23:17 [PATCH net-next 00/14] rxrpc: Fixes & miscellany David Howells
  2016-09-17 23:17 ` [PATCH net-next 01/14] rxrpc: Remove some whitespace David Howells
  2016-09-17 23:17 ` [PATCH net-next 02/14] rxrpc: Move the check of rx_pkt_offset from rxrpc_locate_data() to caller David Howells
@ 2016-09-17 23:18 ` David Howells
  2016-09-17 23:18 ` [PATCH net-next 04/14] rxrpc: Fix handling of the last packet in rxrpc_recvmsg_data() David Howells
                   ` (12 subsequent siblings)
  15 siblings, 0 replies; 17+ messages in thread
From: David Howells @ 2016-09-17 23:18 UTC (permalink / raw)
  To: netdev; +Cc: dhowells, linux-afs, linux-kernel

Check the return value of rxrpc_locate_data() in rxrpc_recvmsg_data().

Signed-off-by: David Howells <dhowells@redhat.com>
---

 net/rxrpc/recvmsg.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/rxrpc/recvmsg.c b/net/rxrpc/recvmsg.c
index 0d085f5cf1bf..1edf2cf62cc5 100644
--- a/net/rxrpc/recvmsg.c
+++ b/net/rxrpc/recvmsg.c
@@ -300,10 +300,13 @@ static int rxrpc_recvmsg_data(struct socket *sock, struct rxrpc_call *call,
 		if (msg)
 			sock_recv_timestamp(msg, sock->sk, skb);
 
-		if (rx_pkt_offset == 0)
+		if (rx_pkt_offset == 0) {
 			ret = rxrpc_locate_data(call, skb,
 						&call->rxtx_annotations[ix],
 						&rx_pkt_offset, &rx_pkt_len);
+			if (ret < 0)
+				goto out;
+		}
 		_debug("recvmsg %x DATA #%u { %d, %d }",
 		       sp->hdr.callNumber, seq, rx_pkt_offset, rx_pkt_len);
 

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH net-next 04/14] rxrpc: Fix handling of the last packet in rxrpc_recvmsg_data()
  2016-09-17 23:17 [PATCH net-next 00/14] rxrpc: Fixes & miscellany David Howells
                   ` (2 preceding siblings ...)
  2016-09-17 23:18 ` [PATCH net-next 03/14] rxrpc: Check the return value of rxrpc_locate_data() David Howells
@ 2016-09-17 23:18 ` David Howells
  2016-09-17 23:18 ` [PATCH net-next 05/14] rxrpc: Record calls that need to be accepted David Howells
                   ` (11 subsequent siblings)
  15 siblings, 0 replies; 17+ messages in thread
From: David Howells @ 2016-09-17 23:18 UTC (permalink / raw)
  To: netdev; +Cc: dhowells, linux-afs, linux-kernel

The code for determining the last packet in rxrpc_recvmsg_data() has been
using the RXRPC_CALL_RX_LAST flag to determine if the rx_top pointer points
to the last packet or not.  This isn't a good idea, however, as the input
code may be running simultaneously on another CPU and that sets the flag
*before* updating the top pointer.

Fix this by the following means:

 (1) Restrict the use of RXRPC_CALL_RX_LAST to the input routines only.
     There's otherwise a synchronisation problem between detecting the flag
     and checking tx_top.  This could probably be dealt with by appropriate
     application of memory barriers, but there's a simpler way.

 (2) Set RXRPC_CALL_RX_LAST after setting rx_top.

 (3) Make rxrpc_rotate_rx_window() consult the flags header field of the
     DATA packet it's about to discard to see if that was the last packet.
     Use this as the basis for ending the Rx phase.  This shouldn't be a
     problem because the recvmsg side of things is guaranteed to see the
     packets in order.

 (4) Make rxrpc_recvmsg_data() return 1 to indicate the end of the data if:

     (a) the packet it has just processed is marked as RXRPC_LAST_PACKET

     (b) the call's Rx phase has been ended.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 net/rxrpc/input.c   |    4 +++-
 net/rxrpc/recvmsg.c |   49 +++++++++++++++++++++++++++++++++----------------
 2 files changed, 36 insertions(+), 17 deletions(-)

diff --git a/net/rxrpc/input.c b/net/rxrpc/input.c
index 75af0bd316c7..f0d9115b9b7e 100644
--- a/net/rxrpc/input.c
+++ b/net/rxrpc/input.c
@@ -238,7 +238,7 @@ next_subpacket:
 		len = RXRPC_JUMBO_DATALEN;
 
 	if (flags & RXRPC_LAST_PACKET) {
-		if (test_and_set_bit(RXRPC_CALL_RX_LAST, &call->flags) &&
+		if (test_bit(RXRPC_CALL_RX_LAST, &call->flags) &&
 		    seq != call->rx_top)
 			return rxrpc_proto_abort("LSN", call, seq);
 	} else {
@@ -282,6 +282,8 @@ next_subpacket:
 	call->rxtx_buffer[ix] = skb;
 	if (after(seq, call->rx_top))
 		smp_store_release(&call->rx_top, seq);
+	if (flags & RXRPC_LAST_PACKET)
+		set_bit(RXRPC_CALL_RX_LAST, &call->flags);
 	queued = true;
 
 	if (after_eq(seq, call->rx_expect_next)) {
diff --git a/net/rxrpc/recvmsg.c b/net/rxrpc/recvmsg.c
index 1edf2cf62cc5..8b8d7e14f800 100644
--- a/net/rxrpc/recvmsg.c
+++ b/net/rxrpc/recvmsg.c
@@ -134,6 +134,8 @@ static void rxrpc_end_rx_phase(struct rxrpc_call *call)
 {
 	_enter("%d,%s", call->debug_id, rxrpc_call_states[call->state]);
 
+	ASSERTCMP(call->rx_hard_ack, ==, call->rx_top);
+
 	if (call->state == RXRPC_CALL_CLIENT_RECV_REPLY) {
 		rxrpc_propose_ACK(call, RXRPC_ACK_IDLE, 0, 0, true, false);
 		rxrpc_send_call_packet(call, RXRPC_PACKET_TYPE_ACK);
@@ -163,8 +165,10 @@ static void rxrpc_end_rx_phase(struct rxrpc_call *call)
  */
 static void rxrpc_rotate_rx_window(struct rxrpc_call *call)
 {
+	struct rxrpc_skb_priv *sp;
 	struct sk_buff *skb;
 	rxrpc_seq_t hard_ack, top;
+	u8 flags;
 	int ix;
 
 	_enter("%d", call->debug_id);
@@ -177,6 +181,8 @@ static void rxrpc_rotate_rx_window(struct rxrpc_call *call)
 	ix = hard_ack & RXRPC_RXTX_BUFF_MASK;
 	skb = call->rxtx_buffer[ix];
 	rxrpc_see_skb(skb);
+	sp = rxrpc_skb(skb);
+	flags = sp->hdr.flags;
 	call->rxtx_buffer[ix] = NULL;
 	call->rxtx_annotations[ix] = 0;
 	/* Barrier against rxrpc_input_data(). */
@@ -184,8 +190,8 @@ static void rxrpc_rotate_rx_window(struct rxrpc_call *call)
 
 	rxrpc_free_skb(skb);
 
-	_debug("%u,%u,%lx", hard_ack, top, call->flags);
-	if (hard_ack == top && test_bit(RXRPC_CALL_RX_LAST, &call->flags))
+	_debug("%u,%u,%02x", hard_ack, top, flags);
+	if (flags & RXRPC_LAST_PACKET)
 		rxrpc_end_rx_phase(call);
 }
 
@@ -278,13 +284,19 @@ static int rxrpc_recvmsg_data(struct socket *sock, struct rxrpc_call *call,
 	size_t remain;
 	bool last;
 	unsigned int rx_pkt_offset, rx_pkt_len;
-	int ix, copy, ret = 0;
+	int ix, copy, ret = -EAGAIN, ret2;
 
 	_enter("");
 
 	rx_pkt_offset = call->rx_pkt_offset;
 	rx_pkt_len = call->rx_pkt_len;
 
+	if (call->state >= RXRPC_CALL_SERVER_ACK_REQUEST) {
+		seq = call->rx_hard_ack;
+		ret = 1;
+		goto done;
+	}
+
 	/* Barriers against rxrpc_input_data(). */
 	hard_ack = call->rx_hard_ack;
 	top = smp_load_acquire(&call->rx_top);
@@ -301,11 +313,13 @@ static int rxrpc_recvmsg_data(struct socket *sock, struct rxrpc_call *call,
 			sock_recv_timestamp(msg, sock->sk, skb);
 
 		if (rx_pkt_offset == 0) {
-			ret = rxrpc_locate_data(call, skb,
-						&call->rxtx_annotations[ix],
-						&rx_pkt_offset, &rx_pkt_len);
-			if (ret < 0)
+			ret2 = rxrpc_locate_data(call, skb,
+						 &call->rxtx_annotations[ix],
+						 &rx_pkt_offset, &rx_pkt_len);
+			if (ret2 < 0) {
+				ret = ret2;
 				goto out;
+			}
 		}
 		_debug("recvmsg %x DATA #%u { %d, %d }",
 		       sp->hdr.callNumber, seq, rx_pkt_offset, rx_pkt_len);
@@ -316,10 +330,12 @@ static int rxrpc_recvmsg_data(struct socket *sock, struct rxrpc_call *call,
 		if (copy > remain)
 			copy = remain;
 		if (copy > 0) {
-			ret = skb_copy_datagram_iter(skb, rx_pkt_offset, iter,
-						     copy);
-			if (ret < 0)
+			ret2 = skb_copy_datagram_iter(skb, rx_pkt_offset, iter,
+						      copy);
+			if (ret2 < 0) {
+				ret = ret2;
 				goto out;
+			}
 
 			/* handle piecemeal consumption of data packets */
 			_debug("copied %d @%zu", copy, *_offset);
@@ -332,6 +348,7 @@ static int rxrpc_recvmsg_data(struct socket *sock, struct rxrpc_call *call,
 		if (rx_pkt_len > 0) {
 			_debug("buffer full");
 			ASSERTCMP(*_offset, ==, len);
+			ret = 0;
 			break;
 		}
 
@@ -342,19 +359,19 @@ static int rxrpc_recvmsg_data(struct socket *sock, struct rxrpc_call *call,
 		rx_pkt_offset = 0;
 		rx_pkt_len = 0;
 
-		ASSERTIFCMP(last, seq, ==, top);
-	}
-
-	if (after(seq, top)) {
-		ret = -EAGAIN;
-		if (test_bit(RXRPC_CALL_RX_LAST, &call->flags))
+		if (last) {
+			ASSERTCMP(seq, ==, READ_ONCE(call->rx_top));
 			ret = 1;
+			goto out;
+		}
 	}
+
 out:
 	if (!(flags & MSG_PEEK)) {
 		call->rx_pkt_offset = rx_pkt_offset;
 		call->rx_pkt_len = rx_pkt_len;
 	}
+done:
 	_leave(" = %d [%u/%u]", ret, seq, top);
 	return ret;
 }

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH net-next 05/14] rxrpc: Record calls that need to be accepted
  2016-09-17 23:17 [PATCH net-next 00/14] rxrpc: Fixes & miscellany David Howells
                   ` (3 preceding siblings ...)
  2016-09-17 23:18 ` [PATCH net-next 04/14] rxrpc: Fix handling of the last packet in rxrpc_recvmsg_data() David Howells
@ 2016-09-17 23:18 ` David Howells
  2016-09-17 23:18 ` [PATCH net-next 06/14] rxrpc: Purge the to_be_accepted queue on socket release David Howells
                   ` (10 subsequent siblings)
  15 siblings, 0 replies; 17+ messages in thread
From: David Howells @ 2016-09-17 23:18 UTC (permalink / raw)
  To: netdev; +Cc: dhowells, linux-afs, linux-kernel

Record calls that need to be accepted using sk_acceptq_added() otherwise
the backlog counter goes negative because sk_acceptq_removed() is called.
This causes the preallocator to malfunction.

Calls that are preaccepted by AFS within the kernel aren't affected by
this.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 net/rxrpc/call_accept.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/rxrpc/call_accept.c b/net/rxrpc/call_accept.c
index 26c293ef98eb..323b8da50163 100644
--- a/net/rxrpc/call_accept.c
+++ b/net/rxrpc/call_accept.c
@@ -369,6 +369,8 @@ found_service:
 
 	if (rx->notify_new_call)
 		rx->notify_new_call(&rx->sk, call, call->user_call_ID);
+	else
+		sk_acceptq_added(&rx->sk);
 
 	spin_lock(&conn->state_lock);
 	switch (conn->state) {

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH net-next 06/14] rxrpc: Purge the to_be_accepted queue on socket release
  2016-09-17 23:17 [PATCH net-next 00/14] rxrpc: Fixes & miscellany David Howells
                   ` (4 preceding siblings ...)
  2016-09-17 23:18 ` [PATCH net-next 05/14] rxrpc: Record calls that need to be accepted David Howells
@ 2016-09-17 23:18 ` David Howells
  2016-09-17 23:18 ` [PATCH net-next 07/14] rxrpc: Fix the putting of client connections David Howells
                   ` (9 subsequent siblings)
  15 siblings, 0 replies; 17+ messages in thread
From: David Howells @ 2016-09-17 23:18 UTC (permalink / raw)
  To: netdev; +Cc: dhowells, linux-afs, linux-kernel

Purge the queue of to_be_accepted calls on socket release.  Note that
purging sock_calls doesn't release the ref owned by to_be_accepted.

Probably the sock_calls list is redundant given a purges of the recvmsg_q,
the to_be_accepted queue and the calls tree.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 net/rxrpc/call_object.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/net/rxrpc/call_object.c b/net/rxrpc/call_object.c
index 22f9b0d1a138..b0ffbd9664e6 100644
--- a/net/rxrpc/call_object.c
+++ b/net/rxrpc/call_object.c
@@ -476,6 +476,16 @@ void rxrpc_release_calls_on_socket(struct rxrpc_sock *rx)
 
 	_enter("%p", rx);
 
+	while (!list_empty(&rx->to_be_accepted)) {
+		call = list_entry(rx->to_be_accepted.next,
+				  struct rxrpc_call, accept_link);
+		list_del(&call->accept_link);
+		rxrpc_abort_call("SKR", call, 0, RX_CALL_DEAD, ECONNRESET);
+		rxrpc_send_call_packet(call, RXRPC_PACKET_TYPE_ABORT);
+		rxrpc_release_call(rx, call);
+		rxrpc_put_call(call, rxrpc_call_put);
+	}
+
 	while (!list_empty(&rx->sock_calls)) {
 		call = list_entry(rx->sock_calls.next,
 				  struct rxrpc_call, sock_link);

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH net-next 07/14] rxrpc: Fix the putting of client connections
  2016-09-17 23:17 [PATCH net-next 00/14] rxrpc: Fixes & miscellany David Howells
                   ` (5 preceding siblings ...)
  2016-09-17 23:18 ` [PATCH net-next 06/14] rxrpc: Purge the to_be_accepted queue on socket release David Howells
@ 2016-09-17 23:18 ` David Howells
  2016-09-17 23:18 ` [PATCH net-next 08/14] rxrpc: Call rxrpc_release_call() on error in rxrpc_new_client_call() David Howells
                   ` (8 subsequent siblings)
  15 siblings, 0 replies; 17+ messages in thread
From: David Howells @ 2016-09-17 23:18 UTC (permalink / raw)
  To: netdev; +Cc: dhowells, linux-afs, linux-kernel

In rxrpc_put_one_client_conn(), if a connection has RXRPC_CONN_COUNTED set
on it, then it's accounted for in rxrpc_nr_client_conns and may be on
various lists - and this is cleaned up correctly.

However, if the connection doesn't have RXRPC_CONN_COUNTED set on it, then
the put routine returns rather than just skipping the extra bit of cleanup.

Fix this by making the extra bit of clean up conditional instead and always
killing off the connection.

This manifests itself as connections with a zero usage count hanging around
in /proc/net/rxrpc_conns because the connection allocated, but discarded,
due to a race with another process that set up a parallel connection, which
was then shared instead.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 net/rxrpc/conn_client.c |   28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/net/rxrpc/conn_client.c b/net/rxrpc/conn_client.c
index 9344a8416ceb..5a675c43cace 100644
--- a/net/rxrpc/conn_client.c
+++ b/net/rxrpc/conn_client.c
@@ -818,7 +818,7 @@ idle_connection:
 static struct rxrpc_connection *
 rxrpc_put_one_client_conn(struct rxrpc_connection *conn)
 {
-	struct rxrpc_connection *next;
+	struct rxrpc_connection *next = NULL;
 	struct rxrpc_local *local = conn->params.local;
 	unsigned int nr_conns;
 
@@ -834,24 +834,22 @@ rxrpc_put_one_client_conn(struct rxrpc_connection *conn)
 
 	ASSERTCMP(conn->cache_state, ==, RXRPC_CONN_CLIENT_INACTIVE);
 
-	if (!test_bit(RXRPC_CONN_COUNTED, &conn->flags))
-		return NULL;
-
-	spin_lock(&rxrpc_client_conn_cache_lock);
-	nr_conns = --rxrpc_nr_client_conns;
+	if (test_bit(RXRPC_CONN_COUNTED, &conn->flags)) {
+		spin_lock(&rxrpc_client_conn_cache_lock);
+		nr_conns = --rxrpc_nr_client_conns;
+
+		if (nr_conns < rxrpc_max_client_connections &&
+		    !list_empty(&rxrpc_waiting_client_conns)) {
+			next = list_entry(rxrpc_waiting_client_conns.next,
+					  struct rxrpc_connection, cache_link);
+			rxrpc_get_connection(next);
+			rxrpc_activate_conn(next);
+		}
 
-	next = NULL;
-	if (nr_conns < rxrpc_max_client_connections &&
-	    !list_empty(&rxrpc_waiting_client_conns)) {
-		next = list_entry(rxrpc_waiting_client_conns.next,
-				  struct rxrpc_connection, cache_link);
-		rxrpc_get_connection(next);
-		rxrpc_activate_conn(next);
+		spin_unlock(&rxrpc_client_conn_cache_lock);
 	}
 
-	spin_unlock(&rxrpc_client_conn_cache_lock);
 	rxrpc_kill_connection(conn);
-
 	if (next)
 		rxrpc_activate_channels(next);
 

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH net-next 08/14] rxrpc: Call rxrpc_release_call() on error in rxrpc_new_client_call()
  2016-09-17 23:17 [PATCH net-next 00/14] rxrpc: Fixes & miscellany David Howells
                   ` (6 preceding siblings ...)
  2016-09-17 23:18 ` [PATCH net-next 07/14] rxrpc: Fix the putting of client connections David Howells
@ 2016-09-17 23:18 ` David Howells
  2016-09-17 23:18 ` [PATCH net-next 09/14] rxrpc: Fix unexposed client conn release David Howells
                   ` (7 subsequent siblings)
  15 siblings, 0 replies; 17+ messages in thread
From: David Howells @ 2016-09-17 23:18 UTC (permalink / raw)
  To: netdev; +Cc: dhowells, linux-afs, linux-kernel

Call rxrpc_release_call() on getting an error in rxrpc_new_client_call()
rather than trying to do the cleanup ourselves.  This isn't a problem,
provided we set RXRPC_CALL_HAS_USERID only if we actually add the call to
the calls tree as cleanup code fragments that would otherwise cause
problems are conditional.

Without this, we miss some of the cleanup.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 net/rxrpc/call_object.c |   36 ++++++++++++------------------------
 1 file changed, 12 insertions(+), 24 deletions(-)

diff --git a/net/rxrpc/call_object.c b/net/rxrpc/call_object.c
index b0ffbd9664e6..23f5a5f58282 100644
--- a/net/rxrpc/call_object.c
+++ b/net/rxrpc/call_object.c
@@ -226,9 +226,6 @@ struct rxrpc_call *rxrpc_new_client_call(struct rxrpc_sock *rx,
 			 (const void *)user_call_ID);
 
 	/* Publish the call, even though it is incompletely set up as yet */
-	call->user_call_ID = user_call_ID;
-	__set_bit(RXRPC_CALL_HAS_USERID, &call->flags);
-
 	write_lock(&rx->call_lock);
 
 	pp = &rx->calls.rb_node;
@@ -242,10 +239,12 @@ struct rxrpc_call *rxrpc_new_client_call(struct rxrpc_sock *rx,
 		else if (user_call_ID > xcall->user_call_ID)
 			pp = &(*pp)->rb_right;
 		else
-			goto found_user_ID_now_present;
+			goto error_dup_user_ID;
 	}
 
 	rcu_assign_pointer(call->socket, rx);
+	call->user_call_ID = user_call_ID;
+	__set_bit(RXRPC_CALL_HAS_USERID, &call->flags);
 	rxrpc_get_call(call, rxrpc_call_got_userid);
 	rb_link_node(&call->sock_node, parent, pp);
 	rb_insert_color(&call->sock_node, &rx->calls);
@@ -276,33 +275,22 @@ struct rxrpc_call *rxrpc_new_client_call(struct rxrpc_sock *rx,
 	_leave(" = %p [new]", call);
 	return call;
 
-error:
-	write_lock(&rx->call_lock);
-	rb_erase(&call->sock_node, &rx->calls);
-	write_unlock(&rx->call_lock);
-	rxrpc_put_call(call, rxrpc_call_put_userid);
-
-	write_lock(&rxrpc_call_lock);
-	list_del_init(&call->link);
-	write_unlock(&rxrpc_call_lock);
-
-error_out:
-	__rxrpc_set_call_completion(call, RXRPC_CALL_LOCAL_ERROR,
-				    RX_CALL_DEAD, ret);
-	set_bit(RXRPC_CALL_RELEASED, &call->flags);
-	rxrpc_put_call(call, rxrpc_call_put);
-	_leave(" = %d", ret);
-	return ERR_PTR(ret);
-
 	/* We unexpectedly found the user ID in the list after taking
 	 * the call_lock.  This shouldn't happen unless the user races
 	 * with itself and tries to add the same user ID twice at the
 	 * same time in different threads.
 	 */
-found_user_ID_now_present:
+error_dup_user_ID:
 	write_unlock(&rx->call_lock);
 	ret = -EEXIST;
-	goto error_out;
+
+error:
+	__rxrpc_set_call_completion(call, RXRPC_CALL_LOCAL_ERROR,
+				    RX_CALL_DEAD, ret);
+	rxrpc_release_call(rx, call);
+	rxrpc_put_call(call, rxrpc_call_put);
+	_leave(" = %d", ret);
+	return ERR_PTR(ret);
 }
 
 /*

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH net-next 09/14] rxrpc: Fix unexposed client conn release
  2016-09-17 23:17 [PATCH net-next 00/14] rxrpc: Fixes & miscellany David Howells
                   ` (7 preceding siblings ...)
  2016-09-17 23:18 ` [PATCH net-next 08/14] rxrpc: Call rxrpc_release_call() on error in rxrpc_new_client_call() David Howells
@ 2016-09-17 23:18 ` David Howells
  2016-09-17 23:18 ` [PATCH net-next 10/14] rxrpc: Fix the parsing of soft-ACKs David Howells
                   ` (6 subsequent siblings)
  15 siblings, 0 replies; 17+ messages in thread
From: David Howells @ 2016-09-17 23:18 UTC (permalink / raw)
  To: netdev; +Cc: dhowells, linux-afs, linux-kernel

If the last call on a client connection is release after the connection has
had a bunch of calls allocated but before any DATA packets are sent (so
that it's not yet marked RXRPC_CONN_EXPOSED), an assertion will happen in
rxrpc_disconnect_client_call().

	af_rxrpc: Assertion failed - 1(0x1) >= 2(0x2) is false
	------------[ cut here ]------------
	kernel BUG at ../net/rxrpc/conn_client.c:753!

This is because it's expecting the conn to have been exposed and to have 2
or more refs - but this isn't necessarily the case.

Simply remove the assertion.  This allows the conn to be moved into the
inactive state and deleted if it isn't resurrected before the final put is
called.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 net/rxrpc/conn_client.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/net/rxrpc/conn_client.c b/net/rxrpc/conn_client.c
index 5a675c43cace..226bc910e556 100644
--- a/net/rxrpc/conn_client.c
+++ b/net/rxrpc/conn_client.c
@@ -721,7 +721,6 @@ void rxrpc_disconnect_client_call(struct rxrpc_call *call)
 	}
 
 	ASSERTCMP(rcu_access_pointer(chan->call), ==, call);
-	ASSERTCMP(atomic_read(&conn->usage), >=, 2);
 
 	/* If a client call was exposed to the world, we save the result for
 	 * retransmission.

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH net-next 10/14] rxrpc: Fix the parsing of soft-ACKs
  2016-09-17 23:17 [PATCH net-next 00/14] rxrpc: Fixes & miscellany David Howells
                   ` (8 preceding siblings ...)
  2016-09-17 23:18 ` [PATCH net-next 09/14] rxrpc: Fix unexposed client conn release David Howells
@ 2016-09-17 23:18 ` David Howells
  2016-09-17 23:19 ` [PATCH net-next 11/14] rxrpc: Fix retransmission algorithm David Howells
                   ` (5 subsequent siblings)
  15 siblings, 0 replies; 17+ messages in thread
From: David Howells @ 2016-09-17 23:18 UTC (permalink / raw)
  To: netdev; +Cc: dhowells, linux-afs, linux-kernel

The soft-ACK parser doesn't increment the pointer into the soft-ACK list,
resulting in the first ACK/NACK value being applied to all the relevant
packets in the Tx queue.  This has the potential to miss retransmissions
and cause excessive retransmissions.

Fix this by incrementing the pointer.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 net/rxrpc/input.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/rxrpc/input.c b/net/rxrpc/input.c
index f0d9115b9b7e..c1f83d22f9b7 100644
--- a/net/rxrpc/input.c
+++ b/net/rxrpc/input.c
@@ -384,7 +384,7 @@ static void rxrpc_input_soft_acks(struct rxrpc_call *call, u8 *acks,
 
 	for (; nr_acks > 0; nr_acks--, seq++) {
 		ix = seq & RXRPC_RXTX_BUFF_MASK;
-		switch (*acks) {
+		switch (*acks++) {
 		case RXRPC_ACK_TYPE_ACK:
 			call->rxtx_annotations[ix] = RXRPC_TX_ANNO_ACK;
 			break;

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH net-next 11/14] rxrpc: Fix retransmission algorithm
  2016-09-17 23:17 [PATCH net-next 00/14] rxrpc: Fixes & miscellany David Howells
                   ` (9 preceding siblings ...)
  2016-09-17 23:18 ` [PATCH net-next 10/14] rxrpc: Fix the parsing of soft-ACKs David Howells
@ 2016-09-17 23:19 ` David Howells
  2016-09-17 23:19 ` [PATCH net-next 12/14] rxrpc: Don't transmit an ACK if there's no reason set David Howells
                   ` (4 subsequent siblings)
  15 siblings, 0 replies; 17+ messages in thread
From: David Howells @ 2016-09-17 23:19 UTC (permalink / raw)
  To: netdev; +Cc: dhowells, linux-afs, linux-kernel

Make the retransmission algorithm use for-loops instead of do-loops and
move the counter increments into the for-statement increment slots.

Though the do-loops are slighly more efficient since there will be at least
one pass through the each loop, the counter increments are harder to get
right as the continue-statements skip them.

Without this, if there are any positive acks within the loop, the do-loop
will cycle forever because the counter increment is never done.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 net/rxrpc/call_event.c |   12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/net/rxrpc/call_event.c b/net/rxrpc/call_event.c
index 9367c3be31eb..f0cabc48a1b7 100644
--- a/net/rxrpc/call_event.c
+++ b/net/rxrpc/call_event.c
@@ -163,8 +163,7 @@ static void rxrpc_resend(struct rxrpc_call *call)
 	 */
 	now = jiffies;
 	resend_at = now + rxrpc_resend_timeout;
-	seq = cursor + 1;
-	do {
+	for (seq = cursor + 1; before_eq(seq, top); seq++) {
 		ix = seq & RXRPC_RXTX_BUFF_MASK;
 		annotation = call->rxtx_annotations[ix];
 		if (annotation == RXRPC_TX_ANNO_ACK)
@@ -184,8 +183,7 @@ static void rxrpc_resend(struct rxrpc_call *call)
 
 		/* Okay, we need to retransmit a packet. */
 		call->rxtx_annotations[ix] = RXRPC_TX_ANNO_RETRANS;
-		seq++;
-	} while (before_eq(seq, top));
+	}
 
 	call->resend_at = resend_at;
 
@@ -194,8 +192,7 @@ static void rxrpc_resend(struct rxrpc_call *call)
 	 * lock is dropped, it may clear some of the retransmission markers for
 	 * packets that it soft-ACKs.
 	 */
-	seq = cursor + 1;
-	do {
+	for (seq = cursor + 1; before_eq(seq, top); seq++) {
 		ix = seq & RXRPC_RXTX_BUFF_MASK;
 		annotation = call->rxtx_annotations[ix];
 		if (annotation != RXRPC_TX_ANNO_RETRANS)
@@ -237,8 +234,7 @@ static void rxrpc_resend(struct rxrpc_call *call)
 
 		if (after(call->tx_hard_ack, seq))
 			seq = call->tx_hard_ack;
-		seq++;
-	} while (before_eq(seq, top));
+	}
 
 out_unlock:
 	spin_unlock_bh(&call->lock);

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH net-next 12/14] rxrpc: Don't transmit an ACK if there's no reason set
  2016-09-17 23:17 [PATCH net-next 00/14] rxrpc: Fixes & miscellany David Howells
                   ` (10 preceding siblings ...)
  2016-09-17 23:19 ` [PATCH net-next 11/14] rxrpc: Fix retransmission algorithm David Howells
@ 2016-09-17 23:19 ` David Howells
  2016-09-17 23:19 ` [PATCH net-next 13/14] rxrpc: Be consistent about switch value in rxrpc_send_call_packet() David Howells
                   ` (3 subsequent siblings)
  15 siblings, 0 replies; 17+ messages in thread
From: David Howells @ 2016-09-17 23:19 UTC (permalink / raw)
  To: netdev; +Cc: dhowells, linux-afs, linux-kernel

Don't transmit an ACK if call->ackr_reason in unset.  There's the
possibility of a race between recvmsg() sending an ACK and the background
processing thread trying to send the same one.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 net/rxrpc/output.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/rxrpc/output.c b/net/rxrpc/output.c
index 06a9aca739d1..aa0507214b31 100644
--- a/net/rxrpc/output.c
+++ b/net/rxrpc/output.c
@@ -137,6 +137,11 @@ int rxrpc_send_call_packet(struct rxrpc_call *call, u8 type)
 	switch (type) {
 	case RXRPC_PACKET_TYPE_ACK:
 		spin_lock_bh(&call->lock);
+		if (!call->ackr_reason) {
+			spin_unlock_bh(&call->lock);
+			ret = 0;
+			goto out;
+		}
 		n = rxrpc_fill_out_ack(call, pkt);
 		call->ackr_reason = 0;
 

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH net-next 13/14] rxrpc: Be consistent about switch value in rxrpc_send_call_packet()
  2016-09-17 23:17 [PATCH net-next 00/14] rxrpc: Fixes & miscellany David Howells
                   ` (11 preceding siblings ...)
  2016-09-17 23:19 ` [PATCH net-next 12/14] rxrpc: Don't transmit an ACK if there's no reason set David Howells
@ 2016-09-17 23:19 ` David Howells
  2016-09-17 23:19 ` [PATCH net-next 14/14] rxrpc: Fix the basic transmit DATA packet content size at 1412 bytes David Howells
                   ` (2 subsequent siblings)
  15 siblings, 0 replies; 17+ messages in thread
From: David Howells @ 2016-09-17 23:19 UTC (permalink / raw)
  To: netdev; +Cc: dhowells, linux-afs, linux-kernel

rxrpc_send_call_packet() should use type in both its switch-statements
rather than using pkt->whdr.type.  This might give the compiler an easier
job of uninitialised variable checking.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 net/rxrpc/output.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/rxrpc/output.c b/net/rxrpc/output.c
index aa0507214b31..0b21ed859de7 100644
--- a/net/rxrpc/output.c
+++ b/net/rxrpc/output.c
@@ -182,7 +182,7 @@ int rxrpc_send_call_packet(struct rxrpc_call *call, u8 type)
 			     &msg, iov, ioc, len);
 
 	if (ret < 0 && call->state < RXRPC_CALL_COMPLETE) {
-		switch (pkt->whdr.type) {
+		switch (type) {
 		case RXRPC_PACKET_TYPE_ACK:
 			rxrpc_propose_ACK(call, pkt->ack.reason,
 					  ntohs(pkt->ack.maxSkew),

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH net-next 14/14] rxrpc: Fix the basic transmit DATA packet content size at 1412 bytes
  2016-09-17 23:17 [PATCH net-next 00/14] rxrpc: Fixes & miscellany David Howells
                   ` (12 preceding siblings ...)
  2016-09-17 23:19 ` [PATCH net-next 13/14] rxrpc: Be consistent about switch value in rxrpc_send_call_packet() David Howells
@ 2016-09-17 23:19 ` David Howells
  2016-09-18 11:22 ` [PATCH net-next 00/14] rxrpc: Fixes & miscellany David Miller
  2016-09-19  5:53 ` David Miller
  15 siblings, 0 replies; 17+ messages in thread
From: David Howells @ 2016-09-17 23:19 UTC (permalink / raw)
  To: netdev; +Cc: dhowells, linux-afs, linux-kernel

Fix the basic transmit DATA packet content size at 1412 bytes so that they
can be arbitrarily assembled into jumbo packets.

In the future, I'm thinking of moving to keeping a jumbo packet header at
the beginning of each packet in the Tx queue and creating the packet header
on the spot when kernel_sendmsg() is invoked.  That way, jumbo packets can
be assembled on the spur of the moment for (re-)transmission.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 net/rxrpc/sendmsg.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/rxrpc/sendmsg.c b/net/rxrpc/sendmsg.c
index cba236575073..8bfddf4e338c 100644
--- a/net/rxrpc/sendmsg.c
+++ b/net/rxrpc/sendmsg.c
@@ -214,7 +214,7 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
 					goto maybe_error;
 			}
 
-			max = call->conn->params.peer->maxdata;
+			max = RXRPC_JUMBO_DATALEN;
 			max -= call->conn->security_size;
 			max &= ~(call->conn->size_align - 1UL);
 

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next 00/14] rxrpc: Fixes & miscellany
  2016-09-17 23:17 [PATCH net-next 00/14] rxrpc: Fixes & miscellany David Howells
                   ` (13 preceding siblings ...)
  2016-09-17 23:19 ` [PATCH net-next 14/14] rxrpc: Fix the basic transmit DATA packet content size at 1412 bytes David Howells
@ 2016-09-18 11:22 ` David Miller
  2016-09-19  5:53 ` David Miller
  15 siblings, 0 replies; 17+ messages in thread
From: David Miller @ 2016-09-18 11:22 UTC (permalink / raw)
  To: dhowells; +Cc: netdev, linux-afs, linux-kernel


David, could you please stop submitting two sets of series at
the same time?

I want people to have a single, reasonably sized, patch series
in flight at one time for a given subsystem/driver/whatever.

But if you send both an 14 and an 11 patch series at the same
time, that defeats this entirely.

Thank you.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next 00/14] rxrpc: Fixes & miscellany
  2016-09-17 23:17 [PATCH net-next 00/14] rxrpc: Fixes & miscellany David Howells
                   ` (14 preceding siblings ...)
  2016-09-18 11:22 ` [PATCH net-next 00/14] rxrpc: Fixes & miscellany David Miller
@ 2016-09-19  5:53 ` David Miller
  15 siblings, 0 replies; 17+ messages in thread
From: David Miller @ 2016-09-19  5:53 UTC (permalink / raw)
  To: dhowells; +Cc: netdev, linux-afs, linux-kernel

From: David Howells <dhowells@redhat.com>
Date: Sun, 18 Sep 2016 00:17:44 +0100

> Tagged thusly:
> 
> 	git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git
> 	rxrpc-rewrite-20160917-1

Pulled.

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2016-09-19  5:53 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-17 23:17 [PATCH net-next 00/14] rxrpc: Fixes & miscellany David Howells
2016-09-17 23:17 ` [PATCH net-next 01/14] rxrpc: Remove some whitespace David Howells
2016-09-17 23:17 ` [PATCH net-next 02/14] rxrpc: Move the check of rx_pkt_offset from rxrpc_locate_data() to caller David Howells
2016-09-17 23:18 ` [PATCH net-next 03/14] rxrpc: Check the return value of rxrpc_locate_data() David Howells
2016-09-17 23:18 ` [PATCH net-next 04/14] rxrpc: Fix handling of the last packet in rxrpc_recvmsg_data() David Howells
2016-09-17 23:18 ` [PATCH net-next 05/14] rxrpc: Record calls that need to be accepted David Howells
2016-09-17 23:18 ` [PATCH net-next 06/14] rxrpc: Purge the to_be_accepted queue on socket release David Howells
2016-09-17 23:18 ` [PATCH net-next 07/14] rxrpc: Fix the putting of client connections David Howells
2016-09-17 23:18 ` [PATCH net-next 08/14] rxrpc: Call rxrpc_release_call() on error in rxrpc_new_client_call() David Howells
2016-09-17 23:18 ` [PATCH net-next 09/14] rxrpc: Fix unexposed client conn release David Howells
2016-09-17 23:18 ` [PATCH net-next 10/14] rxrpc: Fix the parsing of soft-ACKs David Howells
2016-09-17 23:19 ` [PATCH net-next 11/14] rxrpc: Fix retransmission algorithm David Howells
2016-09-17 23:19 ` [PATCH net-next 12/14] rxrpc: Don't transmit an ACK if there's no reason set David Howells
2016-09-17 23:19 ` [PATCH net-next 13/14] rxrpc: Be consistent about switch value in rxrpc_send_call_packet() David Howells
2016-09-17 23:19 ` [PATCH net-next 14/14] rxrpc: Fix the basic transmit DATA packet content size at 1412 bytes David Howells
2016-09-18 11:22 ` [PATCH net-next 00/14] rxrpc: Fixes & miscellany David Miller
2016-09-19  5:53 ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).