* [PATCH RFC 1/2] can: Limit default size of CAN_RAW socket send queue @ 2014-01-16 19:58 Michal Sojka 2014-01-16 19:58 ` [PATCH RFC 2/2] net: Make minimum SO_SNDBUF size dependent on the protocol family Michal Sojka 2014-01-17 9:13 ` [PATCH RFC 1/2] can: Limit default size of CAN_RAW socket send queue Marc Kleine-Budde 0 siblings, 2 replies; 6+ messages in thread From: Michal Sojka @ 2014-01-16 19:58 UTC (permalink / raw) To: linux-can; +Cc: yegorslists, 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH RFC 2/2] net: Make minimum SO_SNDBUF size dependent on the protocol family 2014-01-16 19:58 [PATCH RFC 1/2] can: Limit default size of CAN_RAW socket send queue Michal Sojka @ 2014-01-16 19:58 ` Michal Sojka 2014-01-17 9:08 ` Marc Kleine-Budde 2014-01-17 9:13 ` [PATCH RFC 1/2] can: Limit default size of CAN_RAW socket send queue Marc Kleine-Budde 1 sibling, 1 reply; 6+ messages in thread From: Michal Sojka @ 2014-01-16 19:58 UTC (permalink / raw) To: linux-can; +Cc: yegorslists, 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH RFC 2/2] net: Make minimum SO_SNDBUF size dependent on the protocol family 2014-01-16 19:58 ` [PATCH RFC 2/2] net: Make minimum SO_SNDBUF size dependent on the protocol family Michal Sojka @ 2014-01-17 9:08 ` Marc Kleine-Budde 0 siblings, 0 replies; 6+ messages in thread From: Marc Kleine-Budde @ 2014-01-17 9:08 UTC (permalink / raw) To: Michal Sojka, linux-can; +Cc: yegorslists [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RFC 1/2] can: Limit default size of CAN_RAW socket send queue 2014-01-16 19:58 [PATCH RFC 1/2] can: Limit default size of CAN_RAW socket send queue Michal Sojka 2014-01-16 19:58 ` [PATCH RFC 2/2] net: Make minimum SO_SNDBUF size dependent on the protocol family Michal Sojka @ 2014-01-17 9:13 ` Marc Kleine-Budde 2014-01-17 12:11 ` Michal Sojka 2014-01-17 15:31 ` Michal Sojka 1 sibling, 2 replies; 6+ messages in thread From: Marc Kleine-Budde @ 2014-01-17 9:13 UTC (permalink / raw) To: Michal Sojka, linux-can; +Cc: yegorslists [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RFC 1/2] can: Limit default size of CAN_RAW socket send queue 2014-01-17 9:13 ` [PATCH RFC 1/2] can: Limit default size of CAN_RAW socket send queue Marc Kleine-Budde @ 2014-01-17 12:11 ` Michal Sojka 2014-01-17 15:31 ` Michal Sojka 1 sibling, 0 replies; 6+ messages in thread From: Michal Sojka @ 2014-01-17 12:11 UTC (permalink / raw) To: Marc Kleine-Budde, linux-can; +Cc: yegorslists 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RFC 1/2] can: Limit default size of CAN_RAW socket send queue 2014-01-17 9:13 ` [PATCH RFC 1/2] can: Limit default size of CAN_RAW socket send queue Marc Kleine-Budde 2014-01-17 12:11 ` Michal Sojka @ 2014-01-17 15:31 ` Michal Sojka 1 sibling, 0 replies; 6+ messages in thread From: Michal Sojka @ 2014-01-17 15:31 UTC (permalink / raw) To: Marc Kleine-Budde, linux-can; +Cc: yegorslists 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-01-17 15:31 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-01-16 19:58 [PATCH RFC 1/2] can: Limit default size of CAN_RAW socket send queue Michal Sojka 2014-01-16 19:58 ` [PATCH RFC 2/2] net: Make minimum SO_SNDBUF size dependent on the protocol family Michal Sojka 2014-01-17 9:08 ` Marc Kleine-Budde 2014-01-17 9:13 ` [PATCH RFC 1/2] can: Limit default size of CAN_RAW socket send queue Marc Kleine-Budde 2014-01-17 12:11 ` Michal Sojka 2014-01-17 15:31 ` Michal Sojka
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox