[PATCH] tipc: flow control should not account for sk_rcvbuf

[PATCH] tipc: flow control should not account for sk_rcvbuf

[erik.hugne]

From: Erik Hugne <erik.hugne@ericsson.com>

The TIPC flow control is design around message count, and it should not
account for the sk_rcvbuf when enqueueing messages to the socket
receive queue.

This fixes a problem when the sk_add_backlog fails due to this check
and TIPC_ERR_OVERLOAD is reported back to the sender.
The sender would then drop it's side of the connection only, leaving
a stale connection on the other end.

Signed-off-by: Erik Hugne <erik.hugne@ericsson.com>
---
 net/tipc/socket.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 09dc5b9..02fed90 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -1269,10 +1269,8 @@ static u32 dispatch(struct tipc_port *tport, struct sk_buff *buf)
 	if (!sock_owned_by_user(sk)) {
 		res = filter_rcv(sk, buf);
 	} else {
-		if (sk_add_backlog(sk, buf, sk->sk_rcvbuf))
-			res = TIPC_ERR_OVERLOAD;
-		else
-			res = TIPC_OK;
+		__sk_add_backlog(sk, buf);
+		res = TIPC_OK;
 	}
 	bh_unlock_sock(sk);
 
-- 
1.7.5.4

Re: [PATCH] tipc: flow control should not account for sk_rcvbuf

[Eric Dumazet]

On Thu, 2012-10-04 at 11:14 +0200, erik.hugne@ericsson.com wrote:
> From: Erik Hugne <erik.hugne@ericsson.com>
> 
> The TIPC flow control is design around message count, and it should not
> account for the sk_rcvbuf when enqueueing messages to the socket
> receive queue.
> 
> This fixes a problem when the sk_add_backlog fails due to this check
> and TIPC_ERR_OVERLOAD is reported back to the sender.
> The sender would then drop it's side of the connection only, leaving
> a stale connection on the other end.
> 
> Signed-off-by: Erik Hugne <erik.hugne@ericsson.com>
> ---
>  net/tipc/socket.c |    6 ++----
>  1 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> index 09dc5b9..02fed90 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -1269,10 +1269,8 @@ static u32 dispatch(struct tipc_port *tport, struct sk_buff *buf)
>  	if (!sock_owned_by_user(sk)) {
>  		res = filter_rcv(sk, buf);
>  	} else {
> -		if (sk_add_backlog(sk, buf, sk->sk_rcvbuf))
> -			res = TIPC_ERR_OVERLOAD;
> -		else
> -			res = TIPC_OK;
> +		__sk_add_backlog(sk, buf);
> +		res = TIPC_OK;
>  	}
>  	bh_unlock_sock(sk);
>  


What guarantee do we have this cannot use all kernel memory ?

If sk->sk_rcvbuf is not an acceptable limit here, you must use a
different limit, but not infinity.

Re: [PATCH] tipc: flow control should not account for sk_rcvbuf

[Erik Hugne]

> What guarantee do we have this cannot use all kernel memory ?
>
> If sk->sk_rcvbuf is not an acceptable limit here, you must use a
> different limit, but not infinity.
>
There is an implicit limit on how much data that can be buffered on each 
socket, controlled by TIPC_FLOW_CONTROL_WIN.

This limit is:
TIPC_FLOW_CONTROL_WIN * 2 * TIPC_MAX_USER_MSG_SIZE

//E

Re: [PATCH] tipc: flow control should not account for sk_rcvbuf

[Eric Dumazet]

On Thu, 2012-10-04 at 11:59 +0200, Erik Hugne wrote:
> > What guarantee do we have this cannot use all kernel memory ?
> >
> > If sk->sk_rcvbuf is not an acceptable limit here, you must use a
> > different limit, but not infinity.
> >
> There is an implicit limit on how much data that can be buffered on each 
> socket, controlled by TIPC_FLOW_CONTROL_WIN.
> 
> This limit is:
> TIPC_FLOW_CONTROL_WIN * 2 * TIPC_MAX_USER_MSG_SIZE

And this limit is tested _before_ queueing to backlog if socket is owned
by the user ?

You'll have to demonstrate this in the changelog.

Again, I dont think this patch is safe, we need an explicit limit.

Re: [PATCH] tipc: flow control should not account for sk_rcvbuf

[Erik Hugne]

> And this limit is tested _before_ queueing to backlog if socket is owned
> by the user ?
>
> You'll have to demonstrate this in the changelog.
>
> Again, I dont think this patch is safe, we need an explicit limit.
You're right Eric..

Another way of solving it is to increase the default sk_rcvbuf size to
(TIPC_FLOW_CONTROL_WIN * 2 * TIPC_MAX_USER_MSG_SIZE)
at socket creation.

Do you think that would be acceptable?

Re: [PATCH] tipc: flow control should not account for sk_rcvbuf

[Eric Dumazet]

On Thu, 2012-10-04 at 14:12 +0200, Erik Hugne wrote:
> > And this limit is tested _before_ queueing to backlog if socket is owned
> > by the user ?
> >
> > You'll have to demonstrate this in the changelog.
> >
> > Again, I dont think this patch is safe, we need an explicit limit.
> You're right Eric..
> 
> Another way of solving it is to increase the default sk_rcvbuf size to
> (TIPC_FLOW_CONTROL_WIN * 2 * TIPC_MAX_USER_MSG_SIZE)
> at socket creation.
> 
> Do you think that would be acceptable?
> 

If its a tipc constant, you also could use

if (sk_add_backlog(sk, buf,
                   TIPC_FLOW_CONTROL_WIN * 2 * TIPC_MAX_USER_MSG_SIZE))

no ?

But yes, a protocol is allowed to change sk_rcvbuf value (its done for
TCP for example, with a limit to tcp_rmem[2] (between 4 and 6 Mbytes)

[PATCH v2] tipc: prevent dropped connections due to rcvbuf overflow

[erik.hugne]

From: Erik Hugne <erik.hugne@ericsson.com>

When large buffers are sent over connected TIPC sockets, it
is likely that the sk_backlog will be filled up on the
receiver side, but the TIPC flow control mechanism is happily
unaware of this since that is based on message count.

The sender will receive a TIPC_ERR_OVERLOAD message when this occurs
and drop it's side of the connection, leaving it stale on
the receiver end.

By increasing the sk_rcvbuf to a 'worst case' value, we avoid the
overload caused by a full backlog queue and the flow control
will work properly.

This worst case value is the max TIPC message size times
the flow control window, multiplied by two because a sender
will transmit up to double the window size before a port is marked
congested.
We multiply this by 2 to account for the sk_buff and other overheads.

Signed-off-by: Erik Hugne <erik.hugne@ericsson.com>
---
 net/tipc/socket.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 09dc5b9..fd5f042 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -220,6 +220,7 @@ static int tipc_create(struct net *net, struct socket *sock, int protocol,
 
 	sock_init_data(sock, sk);
 	sk->sk_backlog_rcv = backlog_rcv;
+	sk->sk_rcvbuf = TIPC_FLOW_CONTROL_WIN * 2 * TIPC_MAX_USER_MSG_SIZE * 2;
 	tipc_sk(sk)->p = tp_ptr;
 	tipc_sk(sk)->conn_timeout = CONN_TIMEOUT_DEFAULT;
 
-- 
1.7.5.4

Re: [PATCH v2] tipc: prevent dropped connections due to rcvbuf overflow

[David Miller]

From: <erik.hugne@ericsson.com>
Date: Thu, 4 Oct 2012 17:00:43 +0200

> From: Erik Hugne <erik.hugne@ericsson.com>
> 
> When large buffers are sent over connected TIPC sockets, it
> is likely that the sk_backlog will be filled up on the
> receiver side, but the TIPC flow control mechanism is happily
> unaware of this since that is based on message count.
> 
> The sender will receive a TIPC_ERR_OVERLOAD message when this occurs
> and drop it's side of the connection, leaving it stale on
> the receiver end.
> 
> By increasing the sk_rcvbuf to a 'worst case' value, we avoid the
> overload caused by a full backlog queue and the flow control
> will work properly.
> 
> This worst case value is the max TIPC message size times
> the flow control window, multiplied by two because a sender
> will transmit up to double the window size before a port is marked
> congested.
> We multiply this by 2 to account for the sk_buff and other overheads.
> 
> Signed-off-by: Erik Hugne <erik.hugne@ericsson.com>

Applied.