Re: [PATCH] net: can: Increase tx queue length
On 3/9/19 3:40 PM, Appana Durga Kedareswara Rao wrote: > Hi Andre, > > >> >> On 3/9/19 3:07 PM, Appana Durga Kedareswara rao wrote: >>> While stress testing the CAN interface on xilinx axi can in loopback >>> mode getting message "write: no buffer space available" >>> Increasing device tx queue length resolved the above mentioned issue. >> >> No need to patch the kernel: >> >> $ ip link set txqueuelen 500 >> >> does the same thing. > > Thanks for the review... > Agree but it is not an out of box solution right?? > Do you have any idea for socket can devices why the tx queue length is 10 > whereas > for other network devices (ex: ethernet) it is 1000 ?? I think the 1000 queue length is the system default and CAN just overwrites that (a vcan does curiously does not). There probably is a reason for the short queue for CAN. But I don't know why it was set so low. My guess is as good as yours. I ran into your problem multiple times, too, when replaying recorded CAN traces. We worked around it, by adding the txqueuelen to the setup parameters for the CAN devices. If you use ifupdown (we use a different solution), then you could probably put it in there somewhere. I'd be reluctant to change that default without knowing why it was set in the first place. Maybe Oliver can help here? Regards Andre > > Regards, > Kedar. >> >>> >>> Signed-off-by: Appana Durga Kedareswara rao >>> >>> --- >>> --> Network devices default tx_queue_len is 1000 but for socket >>> can device it is 10 any reason for it?? >>> >>> drivers/net/can/dev.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c index >>> c05e4d5..32bd5be 100644 >>> --- a/drivers/net/can/dev.c >>> +++ b/drivers/net/can/dev.c >>> @@ -642,7 +642,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 = 500; >>> >>> /* New-style flags. */ >>> dev->flags = IFF_NOARP; >>> >
Re: [PATCH] net: can: Increase tx queue length
On 3/9/19 3:07 PM, Appana Durga Kedareswara rao wrote: > While stress testing the CAN interface on xilinx axi can > in loopback mode getting message "write: no buffer space available" > Increasing device tx queue length resolved the above mentioned issue. No need to patch the kernel: $ ip link set txqueuelen 500 does the same thing. > > Signed-off-by: Appana Durga Kedareswara rao > --- > --> Network devices default tx_queue_len is 1000 but for socket > can device it is 10 any reason for it?? > > drivers/net/can/dev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c > index c05e4d5..32bd5be 100644 > --- a/drivers/net/can/dev.c > +++ b/drivers/net/can/dev.c > @@ -642,7 +642,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 = 500; > > /* New-style flags. */ > dev->flags = IFF_NOARP; >
[PATCH 0/1] add IPV6_MULTICAST_ALL sockopt
The patch applies to the current net-next tree. I tried to keep the impact of this to a minimum and to replicate the behaviour of IP_MULTICAST_ALL. Andre Naujoks (1): ipv6: Add sockopt IPV6_MULTICAST_ALL analogue to IP_MULTICAST_ALL include/linux/ipv6.h | 3 ++- include/uapi/linux/in6.h | 1 + net/ipv6/af_inet6.c | 1 + net/ipv6/ipv6_sockglue.c | 11 +++ net/ipv6/mcast.c | 2 +- 5 files changed, 16 insertions(+), 2 deletions(-) -- 2.19.0.rc2
[RFC/PATCH] Add a socketoption IPV6_MULTICAST_ALL analogue to the IPV4 version
Hi.
I was running into a problem, when trying to join multiple multicast groups
on a single socket and thus binding to the any-address on said socket. I
received traffic from multicast groups, I did not join on that socket and
was at first surprised by that. After reading some old e-mails/threads,
which came to the conclusion "It is, as it is."
(e.g https://marc.info/?l=linux-kernel&m=115815686626791&w=2), I discovered
the IPv4 socketoption IP_MULTICAST_ALL, which, when disabled, does exactly
what I would expect from a socket by default.
I propose a socket option for IPv6, which does the same and has the same
default as the IPv4 version. My first thought was, to just apply
IP_MULTICAST_ALL to a ipv6 socket, but that would change the behavior of
current applications and would probably be a big no-no.
Regards
Andre
>From 473653086c05a3de839c3504885053f6254c7bc5 Mon Sep 17 00:00:00 2001
From: Andre Naujoks
Date: Wed, 11 Apr 2018 12:38:28 +0200
Subject: [PATCH] Add a socketoption IPV6_MULTICAST_ALL analogue to the IPV4
version
The socket option will be enabled by default to ensure current behaviour
is not changed. This is the same for the IPv4 version.
A socket bound to in6addr_any and a specific port will receive all traffic
on that port. Analogue to IP_MULTICAST_ALL, disable this behaviour, if
one or more multicast groups were joined (using said socket) and only
pass on multicast traffic from groups, which were explicitly joined via
this socket.
Without this option disabled a socket (system even) joined to multiple
multicast groups is very hard to get right. Filtering by destination
address has to take place in user space to avoid receiving multicast
traffic from other multicast groups, which might have traffic on the same
port.
The extension of the IP_MULTICAST_ALL socketoption to just apply to ipv6,
too, is not done to avoid changing the behaviour of current applications.
Signed-off-by: Andre Naujoks
---
include/linux/ipv6.h | 3 ++-
include/uapi/linux/in6.h | 1 +
net/ipv6/af_inet6.c | 1 +
net/ipv6/ipv6_sockglue.c | 11 +++
net/ipv6/mcast.c | 2 +-
5 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/include/linux/ipv6.h b/include/linux/ipv6.h
index 8415bf1a9776..495e834c1367 100644
--- a/include/linux/ipv6.h
+++ b/include/linux/ipv6.h
@@ -274,7 +274,8 @@ struct ipv6_pinfo {
*/
dontfrag:1,
autoflowlabel:1,
- autoflowlabel_set:1;
+ autoflowlabel_set:1,
+ mc_all:1;
__u8min_hopcount;
__u8tclass;
__be32 rcv_flowinfo;
diff --git a/include/uapi/linux/in6.h b/include/uapi/linux/in6.h
index ed291e55f024..71d82fe15b03 100644
--- a/include/uapi/linux/in6.h
+++ b/include/uapi/linux/in6.h
@@ -177,6 +177,7 @@ struct in6_flowlabel_req {
#define IPV6_V6ONLY26
#define IPV6_JOIN_ANYCAST 27
#define IPV6_LEAVE_ANYCAST 28
+#define IPV6_MULTICAST_ALL 29
/* IPV6_MTU_DISCOVER values */
#define IPV6_PMTUDISC_DONT 0
diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
index 8da0b513f188..7844cd9d2f10 100644
--- a/net/ipv6/af_inet6.c
+++ b/net/ipv6/af_inet6.c
@@ -209,6 +209,7 @@ static int inet6_create(struct net *net, struct socket
*sock, int protocol,
np->hop_limit = -1;
np->mcast_hops = IPV6_DEFAULT_MCASTHOPS;
np->mc_loop = 1;
+ np->mc_all = 1;
np->pmtudisc= IPV6_PMTUDISC_WANT;
np->repflow = net->ipv6.sysctl.flowlabel_reflect;
sk->sk_ipv6only = net->ipv6.sysctl.bindv6only;
diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
index 4d780c7f0130..b2bc1942a2ee 100644
--- a/net/ipv6/ipv6_sockglue.c
+++ b/net/ipv6/ipv6_sockglue.c
@@ -664,6 +664,13 @@ static int do_ipv6_setsockopt(struct sock *sk, int level,
int optname,
retv = ipv6_sock_ac_drop(sk, mreq.ipv6mr_ifindex,
&mreq.ipv6mr_acaddr);
break;
}
+ case IPV6_MULTICAST_ALL:
+ if (optlen < sizeof(int))
+ goto e_inval;
+ np->mc_all = valbool;
+ retv = 0;
+ break;
+
case MCAST_JOIN_GROUP:
case MCAST_LEAVE_GROUP:
{
@@ -1255,6 +1262,10 @@ static int do_ipv6_getsockopt(struct sock *sk, int
level, int optname,
val = np->mcast_oif;
break;
+ case IPV6_MULTICAST_ALL:
+ val = np->mc_all;
+ break;
+
case IPV6_UNICAST_IF:
val = (__force int)htonl((__u32) np->ucast_oif);
break;
diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
index 793159d77d8a..623ad00eb3c2 100644
--- a/net/ipv6/mcast.c
+++ b/net/i
Re: [PATCH 2/2] slcan: Port write_wakeup deadlock fix from slip
On 16.06.2014 04:23, schrieb Tyler Hall:
> The commit "slip: Fix deadlock in write_wakeup" fixes a deadlock caused
> by a change made in both slcan and slip. This is a direct port of that
> fix.
I don't know if it is still needed, but for the slcan part, you can add my:
Tested-by: Andre Naujoks
Regards
Andre
>
> Signed-off-by: Tyler Hall
> Cc: Oliver Hartkopp
> Cc: Andre Naujoks
> Cc: David S. Miller
> Cc: linux-kernel@vger.kernel.org
> ---
> drivers/net/can/slcan.c | 37 +++--
> 1 file changed, 27 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
> index dcf9196..ea4d4f1 100644
> --- a/drivers/net/can/slcan.c
> +++ b/drivers/net/can/slcan.c
> @@ -52,6 +52,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -85,6 +86,7 @@ struct slcan {
> struct tty_struct *tty; /* ptr to TTY structure */
> struct net_device *dev; /* easy for intr handling*/
> spinlock_t lock;
> + struct work_struct tx_work;/* Flushes transmit buffer */
>
> /* These are pointers to the malloc()ed frame buffers. */
> unsigned char rbuff[SLC_MTU]; /* receiver buffer */
> @@ -309,36 +311,46 @@ static void slc_encaps(struct slcan *sl, struct
> can_frame *cf)
> sl->dev->stats.tx_bytes += cf->can_dlc;
> }
>
> -/*
> - * Called by the driver when there's room for more data. If we have
> - * more packets to send, we send them here.
> - */
> -static void slcan_write_wakeup(struct tty_struct *tty)
> +/* Write out any remaining transmit buffer. Scheduled when tty is writable */
> +static void slcan_transmit(struct work_struct *work)
> {
> + struct slcan *sl = container_of(work, struct slcan, tx_work);
> int actual;
> - struct slcan *sl = (struct slcan *) tty->disc_data;
>
> + spin_lock_bh(&sl->lock);
> /* First make sure we're connected. */
> - if (!sl || sl->magic != SLCAN_MAGIC || !netif_running(sl->dev))
> + if (!sl->tty || sl->magic != SLCAN_MAGIC || !netif_running(sl->dev)) {
> + spin_unlock_bh(&sl->lock);
> return;
> + }
>
> - spin_lock_bh(&sl->lock);
> if (sl->xleft <= 0) {
> /* Now serial buffer is almost free & we can start
>* transmission of another packet */
> sl->dev->stats.tx_packets++;
> - clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
> + clear_bit(TTY_DO_WRITE_WAKEUP, &sl->tty->flags);
> spin_unlock_bh(&sl->lock);
> netif_wake_queue(sl->dev);
> return;
> }
>
> - actual = tty->ops->write(tty, sl->xhead, sl->xleft);
> + actual = sl->tty->ops->write(sl->tty, sl->xhead, sl->xleft);
> sl->xleft -= actual;
> sl->xhead += actual;
> spin_unlock_bh(&sl->lock);
> }
>
> +/*
> + * Called by the driver when there's room for more data.
> + * Schedule the transmit.
> + */
> +static void slcan_write_wakeup(struct tty_struct *tty)
> +{
> + struct slcan *sl = tty->disc_data;
> +
> + schedule_work(&sl->tx_work);
> +}
> +
> /* Send a can_frame to a TTY queue. */
> static netdev_tx_t slc_xmit(struct sk_buff *skb, struct net_device *dev)
> {
> @@ -528,6 +540,7 @@ static struct slcan *slc_alloc(dev_t line)
> sl->magic = SLCAN_MAGIC;
> sl->dev = dev;
> spin_lock_init(&sl->lock);
> + INIT_WORK(&sl->tx_work, slcan_transmit);
> slcan_devs[i] = dev;
>
> return sl;
> @@ -626,8 +639,12 @@ static void slcan_close(struct tty_struct *tty)
> if (!sl || sl->magic != SLCAN_MAGIC || sl->tty != tty)
> return;
>
> + spin_lock_bh(&sl->lock);
> tty->disc_data = NULL;
> sl->tty = NULL;
> + spin_unlock_bh(&sl->lock);
> +
> + flush_work(&sl->tx_work);
>
> /* Flush network side */
> unregister_netdev(sl->dev);
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net 1/3] slip/slcan: added locking in wakeup function
On 19.09.2013 11:36, schrieb Marc Kleine-Budde: > On 09/13/2013 07:37 PM, Andre Naujoks wrote: >> The locking is needed, since the the internal buffer for the CAN >> frames is changed during the wakeup call. This could cause buffer >> inconsistencies under high loads, especially for the outgoing >> short CAN packet skbuffs. >> >> The needed locks led to deadlocks before commit >> "5ede52538ee2b2202d9dff5b06c33bfde421e6e4 tty: Remove extra >> wakeup from pty write() path", which removed the direct callback >> to the wakeup function from the tty layer. > > What does that mean for older kernels? (< > 5ede52538ee2b2202d9dff5b06c33bfde421e6e4) It seems the slcan (and slip) driver is broken for older kernels. See this thread for a discussion about the patch in pty.c. http://marc.info/?l=linux-kernel&m=137269017002789&w=2 The patch from Peter Hurley was actually already in the queue, when I ran into the problem, and is now in kernel 3.12. Without the pty patch and slow CAN traffic, the driver works, because the wakeup is called directly from the pty driver. That is also the reason why there was no locking. It would just deadlock. When the pty driver defers the wakeup, we ran into synchronisation problems (which should be fixed by the locking) and eventually into a kernel panic because of a recursive loop (which should be fixed by the pty.c patch). Maybe it is possible to get both patches back into the stable branches? Regards Andre > >> As slcan.c is based on slip.c the issue in the original code is >> fixed, too. >> >> Signed-off-by: Andre Naujoks > Acked-by: Marc Kleine-Budde > > Marc > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net 0/3] SLCAN/SLIP fixes and performance
On 14.09.2013 12:45, Marc Kleine-Budde wrote: On 09/13/2013 07:37 PM, Andre Naujoks wrote: Hi Dave, these are some loosely related patches, that fix an ancient locking problem in the slip and slcan drivers, add general ASCII-HEX to bin functions for uppercase ASCII, fix the handling of CAN RTR frames in the slcan driver Can you get an Acked-by for the ASCII-HEX functions from the appropriate maintainer? The patch went out to the maintainers I got from the get_maintainer.pl script. Is there anything else I can or should do to get an Ack from them? Regards Andre and increase the performance for the slcan driver. As these patches mainly contain fixes for the slip/slcan drivers that require a tty layer fix included in 3.11, I would suggest to get the patches in via the net tree for the 3.12 cycle. They should apply properly on the latest net and mainline tree. Marc -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH net 3/3] slcan: rewrite of slc_bump and slc_encaps
The old implementation was heavy on str* functions and sprintf calls.
This version is more manual, but faster.
Profiling just the printing of a 3 char CAN-id resulted in 60 instructions
for the manual method and over 2000 for the sprintf method. Bear in
mind the profiling was done against libc and not the kernel sprintf.
Together with this rewrite an issue with sending and receiving of RTR frames
has been fixed by Oliver for the cases that the DLC is not zero.
Signed-off-by: Andre Naujoks
Tested-by: Oliver Hartkopp
---
drivers/net/can/slcan.c | 136 +++-
1 file changed, 87 insertions(+), 49 deletions(-)
diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index d571e2e..25377e5 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -76,6 +76,10 @@ MODULE_PARM_DESC(maxdev, "Maximum number of slcan
interfaces");
/* maximum rx buffer len: extended CAN frame with timestamp */
#define SLC_MTU (sizeof("T81122334455667788EA5F\r")+1)
+#define SLC_CMD_LEN 1
+#define SLC_SFF_ID_LEN 3
+#define SLC_EFF_ID_LEN 8
+
struct slcan {
int magic;
@@ -142,47 +146,63 @@ static void slc_bump(struct slcan *sl)
{
struct sk_buff *skb;
struct can_frame cf;
- int i, dlc_pos, tmp;
- unsigned long ultmp;
- char cmd = sl->rbuff[0];
-
- if ((cmd != 't') && (cmd != 'T') && (cmd != 'r') && (cmd != 'R'))
+ int i, tmp;
+ u32 tmpid;
+ char *cmd = sl->rbuff;
+
+ cf.can_id = 0;
+
+ switch (*cmd) {
+ case 'r':
+ cf.can_id = CAN_RTR_FLAG;
+ /* fallthrough */
+ case 't':
+ /* store dlc ASCII value and terminate SFF CAN ID string */
+ cf.can_dlc = sl->rbuff[SLC_CMD_LEN + SLC_SFF_ID_LEN];
+ sl->rbuff[SLC_CMD_LEN + SLC_SFF_ID_LEN] = 0;
+ /* point to payload data behind the dlc */
+ cmd += SLC_CMD_LEN + SLC_SFF_ID_LEN + 1;
+ break;
+ case 'R':
+ cf.can_id = CAN_RTR_FLAG;
+ /* fallthrough */
+ case 'T':
+ cf.can_id |= CAN_EFF_FLAG;
+ /* store dlc ASCII value and terminate EFF CAN ID string */
+ cf.can_dlc = sl->rbuff[SLC_CMD_LEN + SLC_EFF_ID_LEN];
+ sl->rbuff[SLC_CMD_LEN + SLC_EFF_ID_LEN] = 0;
+ /* point to payload data behind the dlc */
+ cmd += SLC_CMD_LEN + SLC_EFF_ID_LEN + 1;
+ break;
+ default:
return;
+ }
- if (cmd & 0x20) /* tiny chars 'r' 't' => standard frame format */
- dlc_pos = 4; /* dlc position tiiid */
- else
- dlc_pos = 9; /* dlc position Td */
-
- if (!((sl->rbuff[dlc_pos] >= '0') && (sl->rbuff[dlc_pos] < '9')))
+ if (kstrtou32(sl->rbuff + SLC_CMD_LEN, 16, &tmpid))
return;
- cf.can_dlc = sl->rbuff[dlc_pos] - '0'; /* get can_dlc from ASCII val */
+ cf.can_id |= tmpid;
- sl->rbuff[dlc_pos] = 0; /* terminate can_id string */
-
- if (kstrtoul(sl->rbuff+1, 16, &ultmp))
+ /* get can_dlc from sanitized ASCII value */
+ if (cf.can_dlc >= '0' && cf.can_dlc < '9')
+ cf.can_dlc -= '0';
+ else
return;
- cf.can_id = ultmp;
-
- if (!(cmd & 0x20)) /* NO tiny chars => extended frame format */
- cf.can_id |= CAN_EFF_FLAG;
-
- if ((cmd | 0x20) == 'r') /* RTR frame */
- cf.can_id |= CAN_RTR_FLAG;
-
*(u64 *) (&cf.data) = 0; /* clear payload */
- for (i = 0, dlc_pos++; i < cf.can_dlc; i++) {
- tmp = hex_to_bin(sl->rbuff[dlc_pos++]);
- if (tmp < 0)
- return;
- cf.data[i] = (tmp << 4);
- tmp = hex_to_bin(sl->rbuff[dlc_pos++]);
- if (tmp < 0)
- return;
- cf.data[i] |= tmp;
+ /* RTR frames may have a dlc > 0 but they never have any data bytes */
+ if (!(cf.can_id & CAN_RTR_FLAG)) {
+ for (i = 0; i < cf.can_dlc; i++) {
+ tmp = hex_to_bin(*cmd++);
+ if (tmp < 0)
+ return;
+ cf.data[i] = (tmp << 4);
+ tmp = hex_to_bin(*cmd++);
+ if (tmp < 0)
+ return;
+ cf.data[i] |= tmp;
+ }
}
skb = dev_alloc_skb(sizeof(struct can_frame) +
@@ -209,7 +229,6 @@ static void slc_b
[PATCH net 1/3] slip/slcan: added locking in wakeup function
The locking is needed, since the the internal buffer for the CAN frames is
changed during the wakeup call. This could cause buffer inconsistencies
under high loads, especially for the outgoing short CAN packet skbuffs.
The needed locks led to deadlocks before commit
"5ede52538ee2b2202d9dff5b06c33bfde421e6e4 tty: Remove extra wakeup from pty
write() path", which removed the direct callback to the wakeup function from the
tty layer.
As slcan.c is based on slip.c the issue in the original code is fixed, too.
Signed-off-by: Andre Naujoks
---
drivers/net/can/slcan.c | 3 +++
drivers/net/slip/slip.c | 3 +++
2 files changed, 6 insertions(+)
diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index 874188b..d571e2e 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -286,11 +286,13 @@ static void slcan_write_wakeup(struct tty_struct *tty)
if (!sl || sl->magic != SLCAN_MAGIC || !netif_running(sl->dev))
return;
+ spin_lock(&sl->lock);
if (sl->xleft <= 0) {
/* Now serial buffer is almost free & we can start
* transmission of another packet */
sl->dev->stats.tx_packets++;
clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
+ spin_unlock(&sl->lock);
netif_wake_queue(sl->dev);
return;
}
@@ -298,6 +300,7 @@ static void slcan_write_wakeup(struct tty_struct *tty)
actual = tty->ops->write(tty, sl->xhead, sl->xleft);
sl->xleft -= actual;
sl->xhead += actual;
+ spin_unlock(&sl->lock);
}
/* Send a can_frame to a TTY queue. */
diff --git a/drivers/net/slip/slip.c b/drivers/net/slip/slip.c
index a34d6bf..cc70ecf 100644
--- a/drivers/net/slip/slip.c
+++ b/drivers/net/slip/slip.c
@@ -429,11 +429,13 @@ static void slip_write_wakeup(struct tty_struct *tty)
if (!sl || sl->magic != SLIP_MAGIC || !netif_running(sl->dev))
return;
+ spin_lock(&sl->lock);
if (sl->xleft <= 0) {
/* Now serial buffer is almost free & we can start
* transmission of another packet */
sl->dev->stats.tx_packets++;
clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
+ spin_unlock(&sl->lock);
sl_unlock(sl);
return;
}
@@ -441,6 +443,7 @@ static void slip_write_wakeup(struct tty_struct *tty)
actual = tty->ops->write(tty, sl->xhead, sl->xleft);
sl->xleft -= actual;
sl->xhead += actual;
+ spin_unlock(&sl->lock);
}
static void sl_tx_timeout(struct net_device *dev)
--
1.8.4.rc3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH net 2/3] lib: introduce upper case hex ascii helpers
To be able to use the hex ascii functions in case sensitive environments
the array hex_asc_upper[] and the needed functions for hex_byte_pack_upper()
are introduced.
Signed-off-by: Andre Naujoks
---
include/linux/kernel.h | 11 +++
lib/hexdump.c | 2 ++
2 files changed, 13 insertions(+)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 482ad2d..672ddc4 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -439,6 +439,17 @@ static inline char *hex_byte_pack(char *buf, u8 byte)
return buf;
}
+extern const char hex_asc_upper[];
+#define hex_asc_upper_lo(x)hex_asc_upper[((x) & 0x0f)]
+#define hex_asc_upper_hi(x)hex_asc_upper[((x) & 0xf0) >> 4]
+
+static inline char *hex_byte_pack_upper(char *buf, u8 byte)
+{
+ *buf++ = hex_asc_upper_hi(byte);
+ *buf++ = hex_asc_upper_lo(byte);
+ return buf;
+}
+
static inline char * __deprecated pack_hex_byte(char *buf, u8 byte)
{
return hex_byte_pack(buf, byte);
diff --git a/lib/hexdump.c b/lib/hexdump.c
index 3f0494c..8499c81 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -14,6 +14,8 @@
const char hex_asc[] = "0123456789abcdef";
EXPORT_SYMBOL(hex_asc);
+const char hex_asc_upper[] = "0123456789ABCDEF";
+EXPORT_SYMBOL(hex_asc_upper);
/**
* hex_to_bin - convert a hex digit to its real value
--
1.8.4.rc3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH net 0/3] SLCAN/SLIP fixes and performance
Hi Dave, these are some loosely related patches, that fix an ancient locking problem in the slip and slcan drivers, add general ASCII-HEX to bin functions for uppercase ASCII, fix the handling of CAN RTR frames in the slcan driver and increase the performance for the slcan driver. As these patches mainly contain fixes for the slip/slcan drivers that require a tty layer fix included in 3.11, I would suggest to get the patches in via the net tree for the 3.12 cycle. They should apply properly on the latest net and mainline tree. Best regards, Andre Andre Naujoks (3): slip/slcan: added locking in wakeup function lib: introduce upper case hex ascii helpers slcan: rewrite of slc_bump and slc_encaps drivers/net/can/slcan.c | 139 +++- drivers/net/slip/slip.c | 3 ++ include/linux/kernel.h | 11 lib/hexdump.c | 2 + 4 files changed, 106 insertions(+), 49 deletions(-) -- 1.8.4.rc3 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel panic, pty.c: remove direct call to tty_wakup in pty_write
On 02.07.2013 20:59, Peter Hurley wrote: On 07/01/2013 10:49 AM, Andre Naujoks wrote: Hello. This patch removes the direct call to tty_wakeup in pty_write. I have not noticed any drawbacks with this but I am not familiar with the pty driver at all. I think what happens is a recursive loop, write_wakeup->write->write_wakeup ... The documentation for the tty interface forbids this direct call: (from Documentation/serial/tty.txt) write_wakeup() - May be called at any point between open and close. The TTY_DO_WRITE_WAKEUP flag indicates if a call is needed but always races versus calls. Thus the ldisc must be careful about setting order and to handle unexpected calls. Must not sleep. The driver is forbidden from calling this directly from the ->write call from the ldisc as the ldisc is permitted to call the driver write method from this function. In such a situation defer it. The direct call caused a reproducable kernel panic (see bottom of this mail) for me with the following setup: - using can-utils from git://gitorious.org/linux-can/can-utils.git slcan_attach and cangen are used - create a network link between two serial CAN interfaces with: $ socat PTY,link=/tmp/slcan0,raw TCP4-LISTEN:5 & $ socat TCP4:localhost:5 PTY,link=/tmp/slcan1,raw & $ slcan_attach /tmp/slcan0 $ slcan_attach /tmp/slcan1 $ ip link set slcan0 up $ ip link set slcan1 up - produce a kernel panic by overloading the CAN interfaces: $ cangen slcan0 -g0 Please keep me in CC. I am not subscribed to the list. If I can provide any more information, I will be glad to do so. This is the patch. It applies to the current linux master branch: An identical patch is in Greg's queue for linux-next: 'tty: Remove extra wakeup from pty write() path' That patch's commit message details why tty_wakeup() is unnecessary, but does not foresee or document the SLIP ldisc write()/write_wakeup() recursion. Since this fix will now likely go back through stable, the commit message should include a description of the recursion, so that Greg can merge the commit messages. Separately, the stack trace for the WARN and the oops implicates the network stack alone. Maybe there is some other problem? I haven't run into any other problems so far with the patch applied. If there is another problem, I will probably run into it soon, because I will use this facility very extensively in the near future. We'll see how that works out. Regards Andre Naujoks Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel panic, pty.c: remove direct call to tty_wakup in pty_write with better commit message
Thanks for the pointer. Since your patch is from April, does that mean,
that we cannot expect it to hit 3.11?
From 6cbfeb6cb9bbe7455cddbf162a7b158e1debd578 Mon Sep 17 00:00:00 2001
From: Andre Naujoks
Date: Tue, 2 Jul 2013 22:11:33 +0200
Subject: [PATCH] Remove the tty_wakeup call inside pty_write
The call to tty_wakeup can cause a recursive loop and therefore a
kernel oops when pty_write is called again from the ldisc wakeup
function but there is not enough room for all data at once.
Signed-off-by: Andre Naujoks
---
drivers/tty/pty.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index abfd990..cae4e4f 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -125,10 +125,8 @@ static int pty_write(struct tty_struct *tty, const
unsigned char *buf, int c)
/* Stuff the data into the input queue of the other end */
c = tty_insert_flip_string(to->port, buf, c);
/* And shovel */
- if (c) {
+ if (c)
tty_flip_buffer_push(to->port);
- tty_wakeup(tty);
- }
}
return c;
}
--
1.8.3.1
On 02.07.2013 20:59, Peter Hurley wrote:
On 07/01/2013 10:49 AM, Andre Naujoks wrote:
Hello.
This patch removes the direct call to tty_wakeup in pty_write. I have
not noticed any drawbacks with this but I am not familiar with the pty
driver at all. I think what happens is a recursive loop,
write_wakeup->write->write_wakeup ...
The documentation for the tty interface forbids this direct call:
(from Documentation/serial/tty.txt)
write_wakeup() - May be called at any point between open and close.
The TTY_DO_WRITE_WAKEUP flag indicates if a call
is needed but always races versus calls. Thus the
ldisc must be careful about setting order and to
handle unexpected calls. Must not sleep.
The driver is forbidden from calling this directly
from the ->write call from the ldisc as the ldisc
is permitted to call the driver write method from
this function. In such a situation defer it.
The direct call caused a reproducable kernel panic (see bottom of this
mail) for me with the following setup:
- using can-utils from git://gitorious.org/linux-can/can-utils.git
slcan_attach and cangen are used
- create a network link between two serial CAN interfaces with:
$ socat PTY,link=/tmp/slcan0,raw TCP4-LISTEN:5 &
$ socat TCP4:localhost:5 PTY,link=/tmp/slcan1,raw &
$ slcan_attach /tmp/slcan0
$ slcan_attach /tmp/slcan1
$ ip link set slcan0 up
$ ip link set slcan1 up
- produce a kernel panic by overloading the CAN interfaces:
$ cangen slcan0 -g0
Please keep me in CC. I am not subscribed to the list.
If I can provide any more information, I will be glad to do so.
This is the patch. It applies to the current linux master branch:
An identical patch is in Greg's queue for linux-next:
'tty: Remove extra wakeup from pty write() path'
That patch's commit message details why tty_wakeup() is unnecessary,
but does not foresee or document the SLIP ldisc write()/write_wakeup()
recursion.
Since this fix will now likely go back through stable, the commit
message should include a description of the recursion, so that Greg can
merge the commit messages.
Separately, the stack trace for the WARN and the oops implicates
the network stack alone. Maybe there is some other problem?
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel panic, pty.c: remove direct call to tty_wakup in pty_write
On 02.07.2013 11:39, schrieb Dean Jenkins:
> On 01/07/13 15:49, Andre Naujoks wrote:
>> Hello.
>>
>> This patch removes the direct call to tty_wakeup in pty_write. I have
>> not noticed any drawbacks with this but I am not familiar with the pty
>> driver at all. I think what happens is a recursive loop,
>> write_wakeup->write->write_wakeup ...
> Indeed there is a recursive loop that I have witnessed whilst using SLIP
> over USB HID.
>
> In the failure case for SLIP, xleft remains positive and recursion
> causes a stack overflow as xleft is not updated during the recursion:
> sl_encaps()-->pty_write()-->tty_wakeup()-->slip_write_wakeup()-->pty_write()-->tty_wakeup()-->slip_write_wakeup()
> etc.
The funny thing about the SLIP driver is, that I could not reproduce
this particular issue with it. The SLIP driver would just hang after a
second of high load but never crash the kernel for me. It may be,
because I used a network connection with socat, which is fast enough for
the data to go through after a few recursions.
>
> The underlying issue being pty_write() calling tty_wakeup() within the
> initial write thread. Note that the TTY wakeup event uses tty_wakeup()
> as a callback to request more data.
>> The documentation for the tty interface forbids this direct call:
>>
>> (from Documentation/serial/tty.txt)
>> write_wakeup() - May be called at any point between open and close.
>>The TTY_DO_WRITE_WAKEUP flag indicates if a call
>>is needed but always races versus calls. Thus the
>>ldisc must be careful about setting order and to
>>handle unexpected calls. Must not sleep.
>>
>>The driver is forbidden from calling this directly
>>from the ->write call from the ldisc as the ldisc
>>is permitted to call the driver write method from
>>this function. In such a situation defer it.
> I also saw that documentation and the code seems to be breaking that
> description.
>
> It is possible to mitigate against
> pty-write-->tty_wakeup()-->slip_write_wakeup() by clearing and setting
> TTY_DO_WRITE_WAKEUP at the "correct" time in the layers bound to
> PTY/TTY. Indeed, I modified SLIP to do that and sent patches to the
> linux-netdev mailing list although I am not aware of any progress in
> having those patches accepted.
See below, why I don't think, that that is a good idea.
>
> Perhaps this mitigation could be applied to your scenario ?
>
> Eventually, your patch could be used when all protocols have mitigration
> in place.
>
>>
>>
>> The direct call caused a reproducable kernel panic (see bottom of this
>> mail) for me with the following setup:
>>
>> - using can-utils from git://gitorious.org/linux-can/can-utils.git
>>slcan_attach and cangen are used
>>
>> - create a network link between two serial CAN interfaces with:
>>$ socat PTY,link=/tmp/slcan0,raw TCP4-LISTEN:5 &
>>$ socat TCP4:localhost:5 PTY,link=/tmp/slcan1,raw &
>>$ slcan_attach /tmp/slcan0
>>$ slcan_attach /tmp/slcan1
>>$ ip link set slcan0 up
>>$ ip link set slcan1 up
>>
>> - produce a kernel panic by overloading the CAN interfaces:
>>$ cangen slcan0 -g0
>>
>>
>> Please keep me in CC. I am not subscribed to the list.
>> If I can provide any more information, I will be glad to do so.
>>
>> This is the patch. It applies to the current linux master branch:
>>
>>
>> From 9f67139bebb938026406a66c1411e0b50628a238 Mon Sep 17 00:00:00 2001
>> From: Andre Naujoks
>> Date: Mon, 1 Jul 2013 15:45:13 +0200
>> Subject: [PATCH 1/2] remove direct call to tty_wakeup in pty_write.
>>
>> Signed-off-by: Andre Naujoks
>> ---
>> drivers/tty/pty.c | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
>> index abfd990..5dcb782 100644
>> --- a/drivers/tty/pty.c
>> +++ b/drivers/tty/pty.c
>> @@ -127,7 +127,6 @@ static int pty_write(struct tty_struct *tty, const
>> unsigned char *buf, int c)
>> /* And shovel */
>> if (c) {
>> tty_flip_buffer_push(to->port);
>> -tty_wakeup(tty);
>> }
>> }
>> return c;
> I agree that this looks to be a simple remedy to the issue. However,
> this code has existed in this state for many years so there is a risk
> that some applications rely on this "feature" in order to work. For
> example, SLIP uses this "feature" although I am not certain that the
> exis
[PATCH] kernel panic, pty.c: remove direct call to tty_wakup in pty_write
Hello.
This patch removes the direct call to tty_wakeup in pty_write. I have
not noticed any drawbacks with this but I am not familiar with the pty
driver at all. I think what happens is a recursive loop,
write_wakeup->write->write_wakeup ...
The documentation for the tty interface forbids this direct call:
(from Documentation/serial/tty.txt)
write_wakeup() - May be called at any point between open and close.
The TTY_DO_WRITE_WAKEUP flag indicates if a call
is needed but always races versus calls. Thus the
ldisc must be careful about setting order and to
handle unexpected calls. Must not sleep.
The driver is forbidden from calling this directly
from the ->write call from the ldisc as the ldisc
is permitted to call the driver write method from
this function. In such a situation defer it.
The direct call caused a reproducable kernel panic (see bottom of this
mail) for me with the following setup:
- using can-utils from git://gitorious.org/linux-can/can-utils.git
slcan_attach and cangen are used
- create a network link between two serial CAN interfaces with:
$ socat PTY,link=/tmp/slcan0,raw TCP4-LISTEN:5 &
$ socat TCP4:localhost:5 PTY,link=/tmp/slcan1,raw &
$ slcan_attach /tmp/slcan0
$ slcan_attach /tmp/slcan1
$ ip link set slcan0 up
$ ip link set slcan1 up
- produce a kernel panic by overloading the CAN interfaces:
$ cangen slcan0 -g0
Please keep me in CC. I am not subscribed to the list.
If I can provide any more information, I will be glad to do so.
This is the patch. It applies to the current linux master branch:
>From 9f67139bebb938026406a66c1411e0b50628a238 Mon Sep 17 00:00:00 2001
From: Andre Naujoks
Date: Mon, 1 Jul 2013 15:45:13 +0200
Subject: [PATCH 1/2] remove direct call to tty_wakeup in pty_write.
Signed-off-by: Andre Naujoks
---
drivers/tty/pty.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index abfd990..5dcb782 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -127,7 +127,6 @@ static int pty_write(struct tty_struct *tty, const
unsigned char *buf, int c)
/* And shovel */
if (c) {
tty_flip_buffer_push(to->port);
- tty_wakeup(tty);
}
}
return c;
--
1.8.3.1
Regards
Andre Naujoks
Kernel-Panic:
[ 61.764168] [ cut here ]
[ 61.765107] WARNING: at
/build/linux-9VFSO6/linux-3.9.4/kernel/softirq.c:160
_local_bh_enable_ip.isra.16+0x33/0x88()
[ 61.766467] Hardware name: Bochs
[ 61.766900] Modules linked in: can_raw
[ 61.768420] [ cut here ]
[ 61.771474] kernel BUG at
/build/linux-9VFSO6/linux-3.9.4/kernel/sched/core.c:524!
[ 61.772378] invalid opcode: [#1] SMP
[ 61.772378] Modules linked in: can_raw can slcan vcan nfsv4 nfsd
auth_rpcgss nfs_acl nfs lockd dns_resolver fscache sunrpc loop snd_pcm
snd_page_alloc kvm_amd snd_timer kvm snd ttm soundcore drm_kms_helper
parport_pc parport drm i2c_piix4 psmouse i2c_core processor pcspkr
serio_raw thermal_sys evdev button ext4 crc16 jbd2 mbcache sg sr_mod
sd_mod crc_t10dif cdrom ata_generic virtio_net floppy ata_piix
virtio_pci virtio_ring virtio libata scsi_mod
[ 61.772378] CPU 0
[ 61.772378] Pid: 2547, comm: socat Not tainted 3.9-1-amd64 #1 Debian
3.9.4-1 Bochs Bochs
[ 61.772378] RIP: 0010:[] []
resched_task+0x26/0x5d
[ 61.772378] RSP: 0018:88007fc03e38 EFLAGS: 00010046
[ 61.772378] RAX: RBX: 88003739f7f0 RCX:
00416036
[ 61.772378] RDX: RSI: 0c00 RDI:
88007a9ba000
[ 61.772378] RBP: 88007fc13f30 R08: 0004 R09:
0001
[ 61.772378] R10: 16af R11: b768 R12:
001e8480
[ 61.772378] R13: 88007fc13ec0 R14: R15:
88007fc03f50
[ 61.772378] FS: 7f2c71d11700() GS:88007fc0()
knlGS:
[ 61.772378] CS: 0010 DS: ES: CR0: 8005003b
[ 61.772378] CR2: 01882808 CR3: 370fe000 CR4:
06f0
[ 61.772378] DR0: DR1: DR2:
[ 61.772378] DR3: DR6: 0ff0 DR7:
0400
[ 61.772378] Process socat (pid: 2547, threadinfo 88007a9ba000,
task 88003739f7f0)
[ 61.772378] Stack:
[ 61.772378] 88003739f838 810685f8 88007fc13ec0
[ 61.772378] 88003739f7f0 88007fc0e2b0 8107b14d
81063a65
[ 61.772378] 88003739f7f0
8104a51d
[ 61.772378] Call Trace:
[ 61.772378]
[ 61.772378] [] ? task_tick_fair+0x91/0xf5
[ 61.772378] [] ? tick_sched_do_timer+0x25/0x25
[ 61.772378] [] ? scheduler_tick+0xb5/0xdd
[ 61.772378] [] ? update_process_times+0x50/0x5c
[ 61.772378] [] ? tick_sched_handle+
