[PATCH v2 1/2] can: Decrease default size of CAN_RAW socket send queue

[PATCH v2 1/2] can: Decrease default size of CAN_RAW socket send queue

[Michal Sojka]

This fixes the infamous ENOBUFS problem, which appears when an
application sends CAN frames faster than they leave the interface.

Packets for sending can be queued at queueing discipline. Qdisc queue
has two limits: maximum length and per-socket byte limit (SO_SNDBUF).
Only the later limit can cause the sender to block. If maximum queue
length limit is reached before the per-socket limit, the application
receives ENOBUFS and there is no way how it can wait for the queue to
become free again. Since the length of the qdisc queue was set by
default to 10 packets, this is exactly what was happening.

This patch decreases the default per-socket limit to approximately 3
CAN frames and increases the length of the qdisc queue to 100 frames.
This setting allows for at least 33 CAN_RAW sockets to send
simultaneously to the same CAN interface without getting ENOBUFS
errors.

The exact maximum number of CAN frames that fit into the per-socket
limit is: 1+floor(sk_sndbuf/skb->truesize)

The calculation of the default sk_sndbuf value in the patch is only
approximate, because skb->truesize can be slightly greater than
SKB_TRUESIZE(). For example, for CAN frames on my 32 bit PowerPC
system, SKB_TRUESIZE() = 408, but skb->truesize = 448. Therefore, on
my system the per-socket limit allows 1+floor(3*408/448) =
1+floor(2.73) = 3 CAN frames to be queued.

Without this patch, the default per-socket limit is
/proc/sys/net/core/wmem_default, which is 163840 on my system. This
limit allows for queuing of 1+163840/448 = 366 CAN frames, which is
clearly more than the queue length (10 frames).

Since the per-socket limit is expressed in bytes, the number of queued
CANFD frames, which are bigger than CAN frames, may be lower. This is
not a big problem, because at least one frame could be always queued.

Changes since v1:
- Improved the commit message, added some number from my system.

Signed-off-by: Michal Sojka <sojkam1@fel.cvut.cz>
---
 drivers/net/can/dev.c | 2 +-
 net/can/raw.c         | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 1870c47..a0bce83 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -492,7 +492,7 @@ static void can_setup(struct net_device *dev)
 	dev->mtu = CAN_MTU;
 	dev->hard_header_len = 0;
 	dev->addr_len = 0;
-	dev->tx_queue_len = 10;
+	dev->tx_queue_len = 100;
 
 	/* New-style flags. */
 	dev->flags = IFF_NOARP;
diff --git a/net/can/raw.c b/net/can/raw.c
index fdda5f6..4ad0bb2 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -291,6 +291,10 @@ static int raw_init(struct sock *sk)
 {
 	struct raw_sock *ro = raw_sk(sk);
 
+	/* allow at most 3 frames to wait for transmission in socket queue */
+	sk->sk_sndbuf = 3 * SKB_TRUESIZE(sizeof(struct can_frame) +
+					 sizeof(struct can_skb_priv));
+
 	ro->bound            = 0;
 	ro->ifindex          = 0;
 
-- 
1.8.5.2

[PATCH v2 2/2] net: Make minimum SO_SNDBUF size dependent on the protocol family

[Michal Sojka]

For CAN bus it is desired to have the per-socket send queue limit much
smaller than for Ethernet-based protocols (see the previous patch).
This patch makes the lower limit of accepted setsockopt(SO_SNDBUF)
values smaller for PF_CAN sockets.

The value of SOCK_MIN_SNDBUF is kept unchanged, because it is only
used in two functions (sk_stream_moderate_sndbuf, tcp_out_of_memory)
that seem to be called for TCP/IP related protocols only and the
change from a constant to sk->sk_prot->min_sndbuf could have negative
performance impact.

Changes since v1:
- SOCK_MIN_SNDBUF changed back to a constant.

Signed-off-by: Michal Sojka <sojkam1@fel.cvut.cz>
---
 include/net/sock.h | 1 +
 net/can/raw.c      | 1 +
 net/core/sock.c    | 6 ++++--
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/net/sock.h b/include/net/sock.h
index 808cbc2..9ca3c0c 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -969,6 +969,7 @@ struct proto {
 	int			*sysctl_rmem;
 	int			max_header;
 	bool			no_autobind;
+	int			min_sndbuf;
 
 	struct kmem_cache	*slab;
 	unsigned int		obj_size;
diff --git a/net/can/raw.c b/net/can/raw.c
index 4ad0bb2..b58f53f 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -818,6 +818,7 @@ static struct proto raw_proto __read_mostly = {
 	.owner      = THIS_MODULE,
 	.obj_size   = sizeof(struct raw_sock),
 	.init       = raw_init,
+	.min_sndbuf = SKB_TRUESIZE(sizeof(struct can_frame) + sizeof(struct can_skb_priv)),
 };
 
 static const struct can_proto raw_can_proto = {
diff --git a/net/core/sock.c b/net/core/sock.c
index 0b39e7a..fddb6be 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -625,7 +625,7 @@ int sock_setsockopt(struct socket *sock, int level, int optname,
 		    char __user *optval, unsigned int optlen)
 {
 	struct sock *sk = sock->sk;
-	int val;
+	int val, min;
 	int valbool;
 	struct linger ling;
 	int ret = 0;
@@ -681,7 +681,9 @@ int sock_setsockopt(struct socket *sock, int level, int optname,
 		val = min_t(u32, val, sysctl_wmem_max);
 set_sndbuf:
 		sk->sk_userlocks |= SOCK_SNDBUF_LOCK;
-		sk->sk_sndbuf = max_t(u32, val * 2, SOCK_MIN_SNDBUF);
+		min = sk->sk_prot->min_sndbuf ?
+			sk->sk_prot->min_sndbuf : SOCK_MIN_SNDBUF;
+		sk->sk_sndbuf = max_t(u32, val * 2, min);
 		/* Wake up sending tasks if we upped the value. */
 		sk->sk_write_space(sk);
 		break;
-- 
1.8.5.2

Re: [PATCH v2 1/2] can: Decrease default size of CAN_RAW socket send queue

[David Miller]

From: Michal Sojka <sojkam1@fel.cvut.cz>
Date: Wed, 22 Jan 2014 09:27:36 +0100

> Since the length of the qdisc queue was set by default to 10
> packets, this is exactly what was happening.

This is your bug, set the qdisc limit to something more reasonable.

Something large enough to absorb the traffic wrt. the speed at which
the CAN device can sink the data.

These two patches are something I am not willing to apply to my
tree, this is not how you solve this problem, sorry.

Re: [PATCH v2 1/2] can: Decrease default size of CAN_RAW socket send queue

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1263 bytes --]

On 01/23/2014 09:41 PM, David Miller wrote:
> From: Michal Sojka <sojkam1@fel.cvut.cz>
> Date: Wed, 22 Jan 2014 09:27:36 +0100
> 
>> Since the length of the qdisc queue was set by default to 10
>> packets, this is exactly what was happening.
> 
> This is your bug, set the qdisc limit to something more reasonable.
> 
> Something large enough to absorb the traffic wrt. the speed at which
> the CAN device can sink the data.

Hmmm, the problem is on i686 I have to increase the txqueuelen to 366
before the socket works as expected (writes are blocking). This means if
there are two processes one sending a stream of bulk data and the other
one occasionally more important data there will be a lot of frames in
front of the important ones.

With Michal's patch we can limit the number of frames in flight to a
reasonably low amount.

> These two patches are something I am not willing to apply to my
> tree, this is not how you solve this problem, sorry.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

Re: [PATCH v2 1/2] can: Decrease default size of CAN_RAW socket send queue

[David Miller]

From: Marc Kleine-Budde <mkl@pengutronix.de>
Date: Thu, 23 Jan 2014 22:01:37 +0100

> On 01/23/2014 09:41 PM, David Miller wrote:
>> From: Michal Sojka <sojkam1@fel.cvut.cz>
>> Date: Wed, 22 Jan 2014 09:27:36 +0100
>> 
>>> Since the length of the qdisc queue was set by default to 10
>>> packets, this is exactly what was happening.
>> 
>> This is your bug, set the qdisc limit to something more reasonable.
>> 
>> Something large enough to absorb the traffic wrt. the speed at which
>> the CAN device can sink the data.
> 
> Hmmm, the problem is on i686 I have to increase the txqueuelen to 366
> before the socket works as expected (writes are blocking). This means if
> there are two processes one sending a stream of bulk data and the other
> one occasionally more important data there will be a lot of frames in
> front of the important ones.
> 
> With Michal's patch we can limit the number of frames in flight to a
> reasonably low amount.

What do you think UDP applications have to do on slow interfaces?
If they care about overrunning the device queue and getting packet
drops, they themselves set a smaller socket send buffer size.

CAN is not special in this regard, please stop treating it as such.

Re: [PATCH v2 1/2] can: Decrease default size of CAN_RAW socket send queue

[Michal Sojka]

On Thu, Jan 23 2014, David Miller wrote:
> From: Michal Sojka <sojkam1@fel.cvut.cz>
> Date: Wed, 22 Jan 2014 09:27:36 +0100
>
>> Since the length of the qdisc queue was set by default to 10
>> packets, this is exactly what was happening.
>
> This is your bug, set the qdisc limit to something more reasonable.

This is what that patch does. It increases qdisc limit from 10 to 100.

> What do you think UDP applications have to do on slow interfaces?
> If they care about overrunning the device queue and getting packet
> drops, they themselves set a smaller socket send buffer size.
>
> CAN is not special in this regard, please stop treating it as such.

CAN is different from UDP in that it cannot send big datagrams. So it
makes sense to change the default sk_sndbuf value. Otherwise, you're
right. I've checked that skb->truesize is the same for small UDP
datagrams and CAN frames.

What about the following?

----8<--------8<-----
Subject: [PATCH v3] can: Decrease default size of CAN_RAW socket send queue

This fixes the infamous ENOBUFS problem, which appears when an
application sends CAN frames faster than they leave the interface.

Packets for sending can be queued at queueing discipline. Qdisc queue
has two limits: maximum length and per-socket byte limit (SO_SNDBUF).
Only the later limit can cause the sender to block. If maximum queue
length limit is reached before the per-socket limit, the application
receives ENOBUFS and there is no way how it can wait for the queue to
become free again. Since the length of the qdisc queue was set by
default to 10 packets, this is exactly what was happening.

This patch decreases the default per-socket limit to the minimum value
(which corresponds to approximately 11 CAN frames) and increases the
length of the qdisc queue to 100 frames. This setting allows for at
least 9 CAN_RAW sockets to send simultaneously to the same CAN
interface without getting ENOBUFS errors.

The exact maximum number of CAN frames that fit into the per-socket
limit is: 1+floor(sk_sndbuf/skb->truesize). On my 32 bit PowerPC
system, skb->truesize = 448 and SOCK_MIN_SNDBUF = 4480. This gives the
limit of 1+floor(4480/448) = 11 CAN frames.

Changes since v2:
- Used SOCK_MIN_SNDBUF instead of a custom lower value.
- Dropped second patch that set SOCK_MIN_SNDBUF differently for
  AF_CAN.

Changes since v1:
- Improved the commit message, added some number from my system.

Signed-off-by: Michal Sojka <sojkam1@fel.cvut.cz>
---
 drivers/net/can/dev.c | 2 +-
 net/can/raw.c         | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 1870c47..a0bce83 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -492,7 +492,7 @@ static void can_setup(struct net_device *dev)
 	dev->mtu = CAN_MTU;
 	dev->hard_header_len = 0;
 	dev->addr_len = 0;
-	dev->tx_queue_len = 10;
+	dev->tx_queue_len = 100;
 
 	/* New-style flags. */
 	dev->flags = IFF_NOARP;
diff --git a/net/can/raw.c b/net/can/raw.c
index fdda5f6..4293197 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -291,6 +291,9 @@ static int raw_init(struct sock *sk)
 {
 	struct raw_sock *ro = raw_sk(sk);
 
+	/* This limits the number of queued CAN frames to approximately 11 */
+	sk->sk_sndbuf = SOCK_MIN_SNDBUF;
+
 	ro->bound            = 0;
 	ro->ifindex          = 0;
 
-- 
1.8.5.2

Re: [PATCH v2 1/2] can: Decrease default size of CAN_RAW socket send queue

[Michal Sojka]

On Thu, Jan 23 2014, Marc Kleine-Budde wrote:
> On 01/23/2014 09:41 PM, David Miller wrote:
>> From: Michal Sojka <sojkam1@fel.cvut.cz>
>> Date: Wed, 22 Jan 2014 09:27:36 +0100
>> 
>>> Since the length of the qdisc queue was set by default to 10
>>> packets, this is exactly what was happening.
>> 
>> This is your bug, set the qdisc limit to something more reasonable.
>> 
>> Something large enough to absorb the traffic wrt. the speed at which
>> the CAN device can sink the data.
>
> Hmmm, the problem is on i686 I have to increase the txqueuelen to 366
> before the socket works as expected (writes are blocking). This means if
> there are two processes one sending a stream of bulk data and the other
> one occasionally more important data there will be a lot of frames in
> front of the important ones.

This problem can be solved with queuing disciplines. See
https://rtime.felk.cvut.cz/can/socketcan-qdisc-final.pdf.

This reminds me that I have another patch that makes qdisc work better
with CAN raw sockets. I'll send the patch to linux-can.

-Michal