[PATCH RFC 1/2] can: Limit default size of CAN_RAW socket send queue

[PATCH RFC 1/2] can: Limit 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 system.

Packets for sending can be queued at three places: socket, queueing
discipline and device driver. Only the socket queue is able to block
the sender; other queues are non-blocking. Since the size of the qdisc
queue was set by default to 10 packets, this queue was full much
earlier than the socket queue and this resulted in ENOBUFS error.

This patch limits the default size of the socket send queue to
approximately 3 CAN frames and increases the size of the qdisc queue
to 100 frames. This setting allows for at least 33 CAN_RAW sockets to
be used simultaneously in the system without getting ENOBUFS errors.

Note that the size of the socket queue is only approximate, because it
is counted in bytes and the realy allocated memory (skb->truesize) can
be bigger than what is reported by SKB_TRUESIZE(). For example, on my
32 bit PowerPC system, SKB_TRUESIZE() = 408, but skb->truesize = 448.

Open issues:
- Will this work with CANFD?
- What about other AF_CAN protocols?

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 RFC 2/2] net: Make minimum SO_SNDBUF size dependent on the protocol family

[Michal Sojka]

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

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

diff --git a/include/net/sock.h b/include/net/sock.h
index 808cbc2..54d26e6 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;
@@ -2072,14 +2073,20 @@ static inline void sk_wake_async(struct sock *sk, int how, int band)
  */
 #define TCP_SKB_MIN_TRUESIZE	(2048 + SKB_DATA_ALIGN(sizeof(struct sk_buff)))
 
-#define SOCK_MIN_SNDBUF		(TCP_SKB_MIN_TRUESIZE * 2)
+#define SOCK_MIN_SNDBUF(sk)	((sk)->sk_prot->min_sndbuf ? \
+				 (sk)->sk_prot->min_sndbuf : \
+				 (TCP_SKB_MIN_TRUESIZE * 2))
 #define SOCK_MIN_RCVBUF		 TCP_SKB_MIN_TRUESIZE
 
 static inline void sk_stream_moderate_sndbuf(struct sock *sk)
 {
 	if (!(sk->sk_userlocks & SOCK_SNDBUF_LOCK)) {
 		sk->sk_sndbuf = min(sk->sk_sndbuf, sk->sk_wmem_queued >> 1);
-		sk->sk_sndbuf = max_t(u32, sk->sk_sndbuf, SOCK_MIN_SNDBUF);
+		/* This seems to be called quite often and mainly for
+		 * TCP. Should we stick with the constant instead of
+		 * changing it to double dereference to not hurt
+		 * performance? */
+		sk->sk_sndbuf = max_t(u32, sk->sk_sndbuf, SOCK_MIN_SNDBUF(sk));
 	}
 }
 
diff --git a/include/net/tcp.h b/include/net/tcp.h
index b1aa324..814cef5 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -306,7 +306,7 @@ static inline bool between(__u32 seq1, __u32 seq2, __u32 seq3)
 
 static inline bool tcp_out_of_memory(struct sock *sk)
 {
-	if (sk->sk_wmem_queued > SOCK_MIN_SNDBUF &&
+	if (sk->sk_wmem_queued > SOCK_MIN_SNDBUF(sk) &&
 	    sk_memory_allocated(sk) > sk_prot_mem_limits(sk, 2))
 		return true;
 	return false;
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..3f0d8a0 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -681,7 +681,7 @@ 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);
+		sk->sk_sndbuf = max_t(u32, val * 2, SOCK_MIN_SNDBUF(sk));
 		/* Wake up sending tasks if we upped the value. */
 		sk->sk_write_space(sk);
 		break;
-- 
1.8.5.2

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

[Marc Kleine-Budde]

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

On 01/16/2014 08:58 PM, Michal Sojka wrote:
> For CAN bus it is desired to have the size of the socket send queue
> much smaller than for Ethernet-based protocols. This patch makes the
> limit for setsockopt(SO_SNDBUF) values smaller for PF_CAN sockets.

Why don't you introduce another define, that's only used in
sock_setsockopt, so you don't have to change any tcp hot path related
functions?

Marc

> Signed-off-by: Michal Sojka <sojkam1@fel.cvut.cz>
> ---
>  include/net/sock.h | 11 +++++++++--
>  include/net/tcp.h  |  2 +-
>  net/can/raw.c      |  1 +
>  net/core/sock.c    |  2 +-
>  4 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 808cbc2..54d26e6 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;
> @@ -2072,14 +2073,20 @@ static inline void sk_wake_async(struct sock *sk, int how, int band)
>   */
>  #define TCP_SKB_MIN_TRUESIZE	(2048 + SKB_DATA_ALIGN(sizeof(struct sk_buff)))
>  
> -#define SOCK_MIN_SNDBUF		(TCP_SKB_MIN_TRUESIZE * 2)
> +#define SOCK_MIN_SNDBUF(sk)	((sk)->sk_prot->min_sndbuf ? \
> +				 (sk)->sk_prot->min_sndbuf : \
> +				 (TCP_SKB_MIN_TRUESIZE * 2))
>  #define SOCK_MIN_RCVBUF		 TCP_SKB_MIN_TRUESIZE
>  
>  static inline void sk_stream_moderate_sndbuf(struct sock *sk)
>  {
>  	if (!(sk->sk_userlocks & SOCK_SNDBUF_LOCK)) {
>  		sk->sk_sndbuf = min(sk->sk_sndbuf, sk->sk_wmem_queued >> 1);
> -		sk->sk_sndbuf = max_t(u32, sk->sk_sndbuf, SOCK_MIN_SNDBUF);
> +		/* This seems to be called quite often and mainly for
> +		 * TCP. Should we stick with the constant instead of
> +		 * changing it to double dereference to not hurt
> +		 * performance? */
> +		sk->sk_sndbuf = max_t(u32, sk->sk_sndbuf, SOCK_MIN_SNDBUF(sk));
>  	}
>  }
>  
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index b1aa324..814cef5 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -306,7 +306,7 @@ static inline bool between(__u32 seq1, __u32 seq2, __u32 seq3)
>  
>  static inline bool tcp_out_of_memory(struct sock *sk)
>  {
> -	if (sk->sk_wmem_queued > SOCK_MIN_SNDBUF &&
> +	if (sk->sk_wmem_queued > SOCK_MIN_SNDBUF(sk) &&
>  	    sk_memory_allocated(sk) > sk_prot_mem_limits(sk, 2))
>  		return true;
>  	return false;
> 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..3f0d8a0 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -681,7 +681,7 @@ 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);
> +		sk->sk_sndbuf = max_t(u32, val * 2, SOCK_MIN_SNDBUF(sk));
>  		/* Wake up sending tasks if we upped the value. */
>  		sk->sk_write_space(sk);
>  		break;
> 


-- 
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 RFC 1/2] can: Limit default size of CAN_RAW socket send queue

[Marc Kleine-Budde]

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

On 01/16/2014 08:58 PM, Michal Sojka wrote:
> This fixes the infamous ENOBUFS problem, which appears when an
> application sends CAN frames faster than they leave the system.
> 
> Packets for sending can be queued at three places: socket, queueing
> discipline and device driver. Only the socket queue is able to block
> the sender; other queues are non-blocking. Since the size of the qdisc
> queue was set by default to 10 packets, this queue was full much
> earlier than the socket queue and this resulted in ENOBUFS error.
> 
> This patch limits the default size of the socket send queue to
> approximately 3 CAN frames and increases the size of the qdisc queue
> to 100 frames. This setting allows for at least 33 CAN_RAW sockets to
> be used simultaneously in the system without getting ENOBUFS errors.

Do you mean in the system, per CAN interface or per socket? How many CAN
frames does a socket take before it blocks (with the default values), in
case the CAN interface doesn't send any messages?

> Note that the size of the socket queue is only approximate, because it
> is counted in bytes and the realy allocated memory (skb->truesize) can
> be bigger than what is reported by SKB_TRUESIZE(). For example, on my
> 32 bit PowerPC system, SKB_TRUESIZE() = 408, but skb->truesize = 448.

What does that mean for number of CAN frames in the above question?

> Open issues:
> - Will this work with CANFD?

Can you calculate the size in bytes depending if we have a CAN or a
CANFD network card at this socket?

> - What about other AF_CAN protocols?
> 
> 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;
>  
> 

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 RFC 1/2] can: Limit default size of CAN_RAW socket send queue

[Michal Sojka]

On Fri, Jan 17 2014, Marc Kleine-Budde wrote:
> On 01/16/2014 08:58 PM, Michal Sojka wrote:
>> This fixes the infamous ENOBUFS problem, which appears when an
>> application sends CAN frames faster than they leave the system.
>> 
>> Packets for sending can be queued at three places: socket, queueing
>> discipline and device driver. Only the socket queue is able to block
>> the sender; other queues are non-blocking. Since the size of the qdisc
>> queue was set by default to 10 packets, this queue was full much
>> earlier than the socket queue and this resulted in ENOBUFS error.
>> 
>> This patch limits the default size of the socket send queue to
>> approximately 3 CAN frames and increases the size of the qdisc queue
>> to 100 frames. This setting allows for at least 33 CAN_RAW sockets to
>> be used simultaneously in the system without getting ENOBUFS errors.
>
> Do you mean in the system, per CAN interface or per socket? 

Per CAN interface is correct. I'll send an updated patch later.

> How many CAN frames does a socket take before it blocks (with the
> default values), in case the CAN interface doesn't send any messages?

Without this patch, the answer depends on the value of
/proc/sys/net/core/wmem_default and the size of sk_buff which in turn
depends on the kernel configuration.

wmem_default is 163840 on my PowerPC and 212992 on my laptop. This gives
163840/448 = 366 frames. Then, 10 more frames can sit in qdisc and a few
more in the driver/hardware.

With this patch, 366 changes to 3.

>> Note that the size of the socket queue is only approximate, because it
>> is counted in bytes and the realy allocated memory (skb->truesize) can
>> be bigger than what is reported by SKB_TRUESIZE(). For example, on my
>> 32 bit PowerPC system, SKB_TRUESIZE() = 408, but skb->truesize = 448.
>
> What does that mean for number of CAN frames in the above question?

The exact maximal number of CAN frames sitting in the send socket queue
is 1+floor(sk_sndbuf/skb->truesize). For the numbers above this gives:

  1+floor(3*408/448) = 1+floor(2.73) = 3

Of course, this only holds if skb->truesize is the same for all CAN
sk_buffs, which I expect that be true. The truesize depends on how much
memory is really allocated by a memory allocator, which may be more than
what you ask [1]. That's why SKB_TRUESIZE()=408, but skb->truesize=448
on my system.

[1]:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/net/core/skbuff.c?id=v3.12#n255

>
>> Open issues:
>> - Will this work with CANFD?
>
> Can you calculate the size in bytes depending if we have a CAN or a
> CANFD network card at this socket?

I'm not sure whether this is possible at socket creation time. You could
do it at bind time, but first, nobody would expect that sk_sndbuf gets
changed by calling bind and second, you can have a socket not bound to
any interface and use dest_addr parameter of sendto() to specify the
interface.

Thanks,
-Michal

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

[Michal Sojka]

On Fri, Jan 17 2014, Marc Kleine-Budde wrote:
> On 01/16/2014 08:58 PM, Michal Sojka wrote:

[...]

>> Open issues:
>> - Will this work with CANFD?
>
> Can you calculate the size in bytes depending if we have a CAN or a
> CANFD network card at this socket?

One option would be to change the sk_sndbuf in raw_setsockopt when
optname == CAN_RAW_FD_FRAMES and sk_sndbuf is at the default value.

The other option would be ignore CANFD completely, which would mean that
the default socket queue would allow only 1 or 2 CANFD frames before
blocking (I don't have exact numbers at hand).

What do you think?

-Michal