netpoll: recursion fixes, queueing, and cleanups
Subsystems (3)
cover netpoll: recursion fixes, queueing, and cleanups
This patch series against 2.6.11 fixes up some recursion deadlocks in netpoll and adds support for fallback to queueing. Various cleanups along the way.
Holds up under load testing via ipt_LOG on a dual Opteron with tg3. Please apply.
PATCH 1/7 netpoll: shorten carrier detect timeout
Shorten carrier detect timeout to 4 seconds.
net/core/netpoll.c +2 -2
@@ -593,7 +593,7 @@ int netpoll_setup(struct netpoll *np)rtnl_shunlock (); atleast = jiffies +HZ /10 ; - atmost = jiffies +10 *HZ ; + atmost = jiffies +4 *HZ ;while (!netif_carrier_ok (ndev)) {if (time_after (jiffies, atmost)) { printk(KERN_NOTICE
@@ -606,7 +606,7 @@ int netpoll_setup(struct netpoll *np)if (time_before (jiffies, atleast)) {printk (KERN_NOTICE "%s: carrier detect appears flaky," -" waiting 10 seconds\n" , +" waiting 4 seconds\n" , np->name);while (time_before (jiffies, atmost))cond_resched ();
PATCH 2/7 netpoll: filter inlines
Add netpoll rx helpers
Move skb_free for rx into __netpoll_rx
net/core/netpoll.c +8 -2
@@ -368,7 +368,7 @@netpoll_send_skb (np, send_skb); } -int netpoll_rx (struct sk_buff *skb) +int __netpoll_rx (struct sk_buff *skb) {int proto, len, ulen;struct iphdr *iph;
@@ -440,12 +440,18 @@ (char *)(uh+1 ), ulen - sizeof(struct udphdr)); +kfree_skb (skb);return 1 ; }spin_unlock_irqrestore (&rx_list_lock, flags); out: -return atomic_read (&trapped); +if (atomic_read (&trapped)) { +kfree_skb (skb); +return 1 ; + } + +return 0 ; }int netpoll_parse_options (struct netpoll *np,char *opt)
include/linux/netpoll.h +9 -1
@@ -30,7 +30,15 @@int netpoll_trap (void );void netpoll_set_trap (int trap);void netpoll_cleanup (struct netpoll *np); -int netpoll_rx (struct sk_buff *skb); +int __netpoll_rx (struct sk_buff *skb); +#ifdef CONFIG_NETPOLL +static inline int netpoll_rx (struct sk_buff *skb) +{ +return skb->dev->netpoll_rx &&__netpoll_rx (skb); +} +#else +#define netpoll_rx (a) 0 +#endif #endif
net/core/dev.c +5 -11
@@ -1427,13 +1427,10 @@struct softnet_data *queue;unsigned long flags; -#ifdef CONFIG_NETPOLL -if (skb->dev->netpoll_rx &&netpoll_rx (skb)) { -kfree_skb (skb); +/* if netpoll wants it, pretend we never saw it */ +if (netpoll_rx (skb))return NET_RX_DROP ; - } -#endif - +if (!skb->stamp.tv_sec)net_timestamp (&skb->stamp);
@@ -1629,12 +1626,9 @@int ret =NET_RX_DROP ;unsigned short type; -#ifdef CONFIG_NETPOLL -if (skb->dev->netpoll_rx && skb->dev->poll &&netpoll_rx (skb)) { -kfree_skb (skb); +/* if we've gotten here through NAPI, check netpoll */ +if (skb->dev->poll &&netpoll_rx (skb))return NET_RX_DROP ; - } -#endif if (!skb->stamp.tv_sec)net_timestamp (&skb->stamp);
PATCH 3/7 netpoll: add netpoll point to net_device
Add struct netpoll pointer to struct netdevice Move netpoll rx flags to netpoll struct
Stop traversing rx_list and get np pointer from skb->dev->np Remove now unneeded rx_list
include/linux/netdevice.h +2 -2
@@ -41,7 +41,7 @@struct divert_blk ;struct vlan_group ;struct ethtool_ops ; - +struct netpoll ;/* source back-compat hooks */ #define SET_ETHTOOL_OPS (netdev,ops) \ ( (netdev)->ethtool_ops = (ops) )
@@ -471,7 +471,7 @@int (*neigh_setup)(struct net_device *dev,struct neigh_parms *);int (*accept_fastpath)(struct net_device *,struct dst_entry *);#ifdef CONFIG_NETPOLL -int netpoll_rx; +struct netpoll *np;#endif #ifdef CONFIG_NET_POLL_CONTROLLER void (*poll_controller)(struct net_device *dev);
net/core/netpoll.c +27 -62
149 lines not on this page · raw message
include/linux/netpoll.h +2 -2
@@ -16,11 +16,11 @@struct netpoll {struct net_device *dev;char dev_name[16 ], *name; +int rx_flags;void (*rx_hook)(struct netpoll *,int ,char *,int );u32 local_ip, remote_ip;u16 local_port, remote_port;unsigned char local_mac[6 ], remote_mac[6 ]; -struct list_head rx_list; };void netpoll_poll (struct netpoll *np);
@@ -35,7 +35,7 @@#ifdef CONFIG_NETPOLL static inline int netpoll_rx (struct sk_buff *skb) { -return skb->dev->netpoll_rx &&__netpoll_rx (skb); +return skb->dev->np && skb->dev->np->rx_flags &&__netpoll_rx (skb); }#else #define netpoll_rx (a) 0
PATCH 4/7 netpoll: fix ->poll() locking
Introduce a per-client poll lock and flag. The lock assures we never have more than one caller in dev->poll(). The flag provides recursion avoidance on UP where the lock disappears.
net/core/netpoll.c +21 -9
@@ -36,7 +36,6 @@static struct sk_buff *skbs;static atomic_t trapped; -static DEFINE_SPINLOCK (netpoll_poll_lock);#define NETPOLL_RX_ENABLED 1#define NETPOLL_RX_DROP 2
@@ -63,8 +62,15 @@ }/* -* Check whether delayed processing was scheduled for our current CPU, -* and then manually invoke NAPI polling to pump data off the card. +* Check whether delayed processing was scheduled for our NIC. If so, +* we attempt to grab the poll lock and use ->poll() to pump the card. +* If this fails, either we've recursed in ->poll() or it's already +* running on another CPU. +* +* Note: we don't mask interrupts with this lock because we're using +* trylock here and interrupts are already disabled in the softirq +* case. Further, we test the poll_owner to avoid recursion on UP +* systems where the lock doesn't exist. * * In cases where there is bi-directional communications, reading only * one message at a time can lead to packets being dropped by the
@@ -74,13 +80,10 @@static void poll_napi (struct netpoll *np) {int budget =16 ; -unsigned long flags; -struct softnet_data *queue; -spin_lock_irqsave (&netpoll_poll_lock, flags); - queue = &__get_cpu_var (softnet_data);if (test_bit (__LINK_STATE_RX_SCHED, &np->dev->state) && - !list_empty (&queue->poll_list)) { + np->poll_owner !=__smp_processor_id () && +spin_trylock (&np->poll_lock)) { np->rx_flags |=NETPOLL_RX_DROP ;atomic_inc (&trapped);
@@ -88,8 +91,8 @@atomic_dec (&trapped); np->rx_flags &= ~NETPOLL_RX_DROP ; +spin_unlock (&np->poll_lock); } -spin_unlock_irqrestore (&netpoll_poll_lock, flags); }void netpoll_poll (struct netpoll *np)
@@ -194,6 +197,12 @@return ; } +/* avoid ->poll recursion */ +if (np->poll_owner ==__smp_processor_id ()) { +__kfree_skb (skb); +return ; + } +spin_lock (&np->dev->xmit_lock); np->dev->xmit_lock_owner =smp_processor_id ();
@@ -542,6 +551,9 @@struct net_device *ndev =NULL ;struct in_device *in_dev; + np->poll_lock =SPIN_LOCK_UNLOCKED ; + np->poll_owner =-1 ; +if (np->dev_name) ndev =dev_get_by_name (np->dev_name);if (!ndev) {
include/linux/netpoll.h +21 -0
@@ -21,6 +21,8 @@u32 local_ip, remote_ip;u16 local_port, remote_port;unsigned char local_mac[6 ], remote_mac[6 ]; +spinlock_t poll_lock; +int poll_owner; };void netpoll_poll (struct netpoll *np);
@@ -37,8 +39,27 @@ {return skb->dev->np && skb->dev->np->rx_flags &&__netpoll_rx (skb); } + +static inline void netpoll_poll_lock (struct net_device *dev) +{ +if (dev->np) { +spin_lock (&dev->np->poll_lock); + dev->np->poll_owner =__smp_processor_id (); + } +} + +static inline void netpoll_poll_unlock (struct net_device *dev) +{ +if (dev->np) { +spin_unlock (&dev->np->poll_lock); + dev->np->poll_owner =-1 ; + } +} +#else #define netpoll_rx (a) 0 +#define netpoll_poll_lock (a) +#define netpoll_poll_unlock (a)#endif #endif
net/core/dev.c +3 -0
@@ -1775,8 +1775,10 @@ dev =list_entry (queue->poll_list.next, struct net_device, poll_list); +netpoll_poll_lock (dev);if (dev->quota <=0 || dev->poll (dev, &budget)) { +netpoll_poll_unlock (dev);local_irq_disable ();list_del (&dev->poll_list);list_add_tail (&dev->poll_list, &queue->poll_list);
@@ -1785,6 +1787,7 @@else dev->quota = dev->weight; }else { +netpoll_poll_unlock (dev);dev_put (dev);local_irq_disable (); }
PATCH 5/7 netpoll: add optional dropping and queueing support
This adds a callback for packets we can't deliver immediately and a helper function for clients to queue such packets to the device post-interrupt.
Netconsole is modified to use the queueing function for best-effort delivery.
drivers/net/netconsole.c +1 -0
@@ -60,6 +60,7 @@ .local_port = 6665, .remote_port = 6666, .remote_mac = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff}, + .drop = netpoll_queue, };static int configured =0 ;
net/core/netpoll.c +57 -2
95 lines not on this page · raw message
include/linux/netpoll.h +2 -0
@@ -18,6 +18,7 @@char dev_name[16 ], *name;int rx_flags;void (*rx_hook)(struct netpoll *,int ,char *,int ); +void (*drop)(struct sk_buff *skb);u32 local_ip, remote_ip;u16 local_port, remote_port;unsigned char local_mac[6 ], remote_mac[6 ];
@@ -33,6 +34,7 @@void netpoll_set_trap (int trap);void netpoll_cleanup (struct netpoll *np);int __netpoll_rx (struct sk_buff *skb); +void netpoll_queue (struct sk_buff *skb);#ifdef CONFIG_NETPOLL static inline int netpoll_rx (struct sk_buff *skb)
PATCH 6/7 netpoll: handle xmit_lock recursion similarly
Handle possible recursion on xmit_lock while we're at it.
net/core/netpoll.c +3 -2
@@ -247,8 +247,9 @@return ; } -/* avoid ->poll recursion */ -if (np->poll_owner ==__smp_processor_id ()) { +/* avoid recursion */ +if (np->poll_owner ==__smp_processor_id () || + np->dev->xmit_lock_owner ==__smp_processor_id ()) {if (np->drop) np->drop (skb);else
PATCH 7/7 netpoll: avoid kfree_skb on packets with destructo
Packets that have destructors should not be zapped here as that might produce additional printk warnings via netconsole.
net/core/netpoll.c +2 -1
@@ -192,7 +192,8 @@ static void zap_completion_queue(void)while (clist !=NULL ) {struct sk_buff *skb = clist; clist = clist->next; -__kfree_skb (skb); +if (!skb->destructor) +__kfree_skb (skb); } }
David S. Miller
maintainerRe: [PATCH 7/7] netpoll: avoid kfree_skb on packets with destructo
Quoted 4 lines · On Thu, 03 Mar 2005 14:46:32 -0600 Matt Mackall <[email protected]> wrote:
Packets that have destructors should not be zapped here as that might produce additional printk warnings via netconsole.
Signed-off-by Matt Mackall <[email protected]>
Then where will they be freed, eh? :-)
This patch adds an SKB leak. Since you've NULL'd out the list, any SKB skipped will never be freed up at all.
Re: [PATCH 7/7] netpoll: avoid kfree_skb on packets with destructo
Quoted 7 lines · David S. Miller
Quoted 3 lines · On Thu, 03 Mar 2005 14:46:32 -0600 Matt Mackall <[email protected]> wrote:
Packets that have destructors should not be zapped here as that might produce additional printk warnings via netconsole.
Quoted 7 lines
Quoted 1 line
Signed-off-by Matt Mackall <[email protected]>Then where will they be freed, eh? :-)
This patch adds an SKB leak. Since you've NULL'd out the list, any SKB skipped will never be freed up at all.
Heh, I was just writing this same message.
On a related note... David, I would prefer if you merged up the netpoll stuff, since it touches mainly net/*
Is that cool w/ you?
Jeff
David S. Miller
maintainerRe: [PATCH 7/7] netpoll: avoid kfree_skb on packets with destructo
Quoted 6 lines · On Thu, 03 Mar 2005 16:17:10 -0500 Jeff Garzik <[email protected]> wrote:
Heh, I was just writing this same message.
On a related note... David, I would prefer if you merged up the netpoll stuff, since it touches mainly
net/*Is that cool w/ you?
No problem. I still don't like this code in that it adds a locking penalty to everyone just by virtue of enabling netpoll. We've worked so hard with things like NETIF_F_LLTX to eliminate locking, so this would be a huge step backwards.
Re: [PATCH 7/7] netpoll: avoid kfree_skb on packets with destructo
Quoted 8 lines · David S. Miller
Quoted 5 lines · On Thu, 03 Mar 2005 16:17:10 -0500 Jeff Garzik <[email protected]> wrote:
Heh, I was just writing this same message.
On a related note... David, I would prefer if you merged up the netpoll stuff, since it touches mainly
net/*
Quoted 6 lines
Quoted 1 line
Is that cool w/ you?
No problem. I still don't like this code in that it adds a locking penalty to everyone just by virtue of enabling netpoll. We've worked so hard with things like
NETIF_F_LLTXto eliminate locking, so this would be a huge step backwards.
Agreed.
Jeff
Re: [PATCH 7/7] netpoll: avoid kfree_skb on packets with destructo
In reply to David S. MillerQuoted 8 lines · David S. Miller
Quoted 5 lines · On Thu, 03 Mar 2005 16:17:10 -0500 Jeff Garzik <[email protected]> wrote:
Heh, I was just writing this same message.
On a related note... David, I would prefer if you merged up the netpoll stuff, since it touches mainly
net/*
Quoted 6 lines
Quoted 1 line
Is that cool w/ you?
No problem. I still don't like this code in that it adds a locking penalty to everyone just by virtue of enabling netpoll. We've worked so hard with things like
NETIF_F_LLTXto eliminate locking, so this would be a huge step backwards.
The lock only happens if CONFIG_NETPOLL=y _and_ a netpoll client (eg netconsole) is registered on the device in question.
I'm certainly open to ideas that improve upon that, but everything I've come up with is equivalent in cost to a lock.
Signature · 1 line
Mathematics is the supreme nostalgia of our time.
David S. Miller
maintainerRe: [PATCH 7/7] netpoll: avoid kfree_skb on packets with destructo
Quoted 2 lines · On Thu, 3 Mar 2005 13:39:11 -0800 Matt Mackall <[email protected]> wrote:
The lock only happens if
CONFIG_NETPOLL=y_and_ a netpoll client (eg netconsole) is registered on the device in question.
I actually missed that condition. This is less intrusive then. I'm actually ok with these changes therefore.
I'll merge these patches in (with the SKB leak fix of course) and will push upstream once we work out how 2.6.x development is going to process :-)
Re: [PATCH 7/7] netpoll: avoid kfree_skb on packets with destructo
In reply to David S. MillerQuoted 6 lines · David S. Miller
Quoted 3 lines · On Thu, 03 Mar 2005 14:46:32 -0600 Matt Mackall <[email protected]> wrote:
Packets that have destructors should not be zapped here as that might produce additional printk warnings via netconsole.
Quoted 6 lines
Quoted 1 line
Signed-off-by Matt Mackall <[email protected]>Then where will they be freed, eh? :-)
This patch adds an SKB leak. Since you've NULL'd out the list, any SKB skipped will never be freed up at all.
Doh. Plain as day. How's this look?
Packets that have destructors should not be zapped here as that might produce additional printk warnings via netconsole.
net/core/netpoll.c +4 -1
@@ -192,7 +192,10 @@ static void zap_completion_queue(void)while (clist !=NULL ) {struct sk_buff *skb = clist; clist = clist->next; -__kfree_skb (skb); +if (skb->destructor) +dev_kfree_skb_any (skb);/* put this one back */ +else +__kfree_skb (skb); } }
Signature · 1 line
Mathematics is the supreme nostalgia of our time.
David S. Miller
maintainerRe: [PATCH 7/7] netpoll: avoid kfree_skb on packets with destructo
Quoted 6 lines · On Thu, 3 Mar 2005 13:32:28 -0800 Matt Mackall <[email protected]> wrote:
Doh. Plain as day. How's this look?
Packets that have destructors should not be zapped here as that might produce additional printk warnings via netconsole.
Signed-off-by Matt Mackall <[email protected]>
Looks great Matt, applied.
Thanks.
Re: [PATCH 4/7] netpoll: fix ->poll() locking
In reply to Matt Mackall==> Regarding [PATCH 4/7] netpoll: fix ->poll() locking; Matt Mackall <[email protected]> adds:
mpm> Introduce a per-client poll lock and flag. The lock assures we never mpm> have more than one caller in dev->poll(). The flag provides recursion mpm> avoidance on UP where the lock disappears.
I don't think it makes sense to have the poll lock associated with a struct netpoll. We want to guard simultaneous access to the device's poll routine. With this implementation, if we have multiple netpoll clients running at the same time, we can have multiple callers of dev->poll at the same time. In other words, there is not a 1:1 relationship between struct netpoll's and struct net_device's.
Please consider making this a per device lock.
-Jeff
@@ -74,13 +80,10 @@
static void poll_napi(struct netpoll *np)
{
int budget = 16;
- unsigned long flags;
- struct softnet_data *queue;
- spin_lock_irqsave(&netpoll_poll_lock, flags);
- queue = &__get_cpu_var(softnet_data);
if (test_bit(__LINK_STATE_RX_SCHED, &np->dev->state) &&
- !list_empty(&queue->poll_list)) {
+ np->poll_owner != __smp_processor_id() &&
+ spin_trylock(&np->poll_lock)) { np->rx_flags |= NETPOLL_RX_DROP;
atomic_inc(&trapped);
David S. Miller
maintainerRe: [PATCH 4/7] netpoll: fix ->poll() locking
Quoted 8 lines · On Fri, 22 Apr 2005 18:24:46 -0400 Jeff Moyer <[email protected]> wrote:
==> Regarding [PATCH 4/7] netpoll: fix ->poll() locking; Matt Mackall <[email protected]> adds:
mpm> Introduce a per-client poll lock and flag. The lock assures we never mpm> have more than one caller in
dev->poll(). The flag provides recursion mpm> avoidance on UP where the lock disappears.I don't think it makes sense to have the poll lock associated with a struct netpoll.
There should be a 1 to 1 relationship from netdev to netpoll, but I see no problems with a many to 1 relationship from netdev to netpoll, that is perfectly legal. It would give more stringent locking on dev->poll() invocations, not forget to lock when necessary.
The only thing which is wrong is that netpoll_setup() should verify that netdev->np is NULL, and if it is not it should return an error.
Re: [PATCH 4/7] netpoll: fix ->poll() locking
==> Regarding Re: [PATCH 4/7] netpoll: fix ->poll() locking; "David S. Miller" <[email protected]> adds:
davem> On Fri, 22 Apr 2005 18:24:46 -0400 Jeff Moyer <[email protected]>
Quoted 3 lines · davem> wrote:
Quoted 3 lines
==> Regarding [PATCH 4/7] netpoll: fix ->poll() locking; Matt Mackall <[email protected]> adds:
mpm> Introduce a per-client poll lock and flag. The lock assures we never mpm> have more than one caller in dev->poll(). The flag provides recursion mpm> avoidance on UP where the lock disappears.
Quoted 2 lines
Quoted 2 lines
I don't think it makes sense to have the poll lock associated with a struct netpoll.
davem> There should be a 1 to 1 relationship from netdev to netpoll, but I davem> see no problems with a many to 1 relationship from netdev to davem> netpoll, that is perfectly legal. It would give more stringent davem> locking on dev->poll() invocations, not forget to lock when davem> necessary.
davem> The only thing which is wrong is that netpoll_setup() should verify davem> that netdev-> np is NULL, and if it is not it should return an error.
Oh yes, of course. Somehow I managed to forget that we now squirrel away the struct netpoll pointer in the net_device. Previously you could register multiple netpoll clients to one device, and this was useful for say doing netconsole and netdump over the same interface. If we've removed this ability, this is a bad thing. Oh dear...
-Jeff
David S. Miller
maintainerRe: [PATCH 4/7] netpoll: fix ->poll() locking
Quoted 5 lines · On Fri, 22 Apr 2005 19:02:43 -0400 Jeff Moyer <[email protected]> wrote:
Oh yes, of course. Somehow I managed to forget that we now squirrel away the struct netpoll pointer in the
net_device. Previously you could register multiple netpoll clients to one device, and this was useful for say doing netconsole and netdump over the same interface. If we've removed this ability, this is a bad thing. Oh dear...
In such a configuration, how did the netpoll code "tell" who the receive packets were for or did it send them to all registered netpoll clients for that device?
Just curious.
Re: [PATCH 4/7] netpoll: fix ->poll() locking
Quoted 8 lines · David S. Miller
Quoted 5 lines · On Fri, 22 Apr 2005 19:02:43 -0400 Jeff Moyer <[email protected]> wrote:
Oh yes, of course. Somehow I managed to forget that we now squirrel away the struct netpoll pointer in the
net_device. Previously you could register multiple netpoll clients to one device, and this was useful for say doing netconsole and netdump over the same interface. If we've removed this ability, this is a bad thing. Oh dear...
Hrm, it appears I forgot about that when I did the recent rework.
Quoted 3 lines
In such a configuration, how did the netpoll code "tell" who the receive packets were for or did it send them to all registered netpoll clients for that device?
It walked a global list of clients.
Signature · 1 line
Mathematics is the supreme nostalgia of our time.
David S. Miller
maintainerRe: [PATCH 4/7] netpoll: fix ->poll() locking
Quoted 5 lines · On Fri, 22 Apr 2005 19:14:40 -0700 Matt Mackall <[email protected]> wrote:
Quoted 3 lines
In such a configuration, how did the netpoll code "tell" who the receive packets were for or did it send them to all registered netpoll clients for that device?
It walked a global list of clients.
Ok, we definitely need to fix this.
Re: [PATCH 1/7] netpoll: shorten carrier detect timeout
In reply to Matt MackallQuoted 1 line · Matt Mackall
Shorten carrier detect timeout to 4 seconds.
The carrier detection looks partially broken to me. The current logic detects an instantly available carrier as flaky because netif_carrier_ok() takes less than 1/10s. This patch does what I assume is intended, make sure the carrier is stable for 1/10s.
===== net/core/netpoll.c 1.27 vs edited =====
net/core/netpoll.c +8 -3
@@ -592,8 +592,7 @@ }rtnl_shunlock (); - atleast = jiffies +HZ /10 ; - atmost = jiffies +10 *HZ ; + atmost = jiffies +4 *HZ ;while (!netif_carrier_ok (ndev)) {if (time_after (jiffies, atmost)) { printk(KERN_NOTICE
@@ -604,9 +603,15 @@cond_resched (); } + atleast = jiffies +HZ /10 ; +while (netif_carrier_ok (ndev)) { +if (time_after (jiffies, atleast)) +break ; +cond_resched (); + }if (time_before (jiffies, atleast)) {printk (KERN_NOTICE "%s: carrier detect appears flaky," -" waiting 10 seconds\n" , +" waiting 4 seconds\n" , np->name);while (time_before (jiffies, atmost))cond_resched ();
Re: [PATCH 1/7] netpoll: shorten carrier detect timeout
Quoted 7 lines · Patrick McHardy
Quoted 1 line · Matt Mackall
Shorten carrier detect timeout to 4 seconds.
The carrier detection looks partially broken to me. The current logic detects an instantly available carrier as flaky because
netif_carrier_ok()takes less than 1/10s. This patch does what I assume is intended, make sure the carrier is stable for 1/10s.
Looks ok, but I've been meaning to change the second loop to something like msleep().
Did you try this with a card that otherwise goes into the wait?
Signature · 1 line
Mathematics is the supreme nostalgia of our time.
Re: [PATCH 1/7] netpoll: shorten carrier detect timeout
Quoted 10 lines · Matt Mackall
Quoted 5 lines · Patrick McHardy
The carrier detection looks partially broken to me. The current logic detects an instantly available carrier as flaky because
netif_carrier_ok()takes less than 1/10s. This patch does what I assume is intended, make sure the carrier is stable for 1/10s.Looks ok, but I've been meaning to change the second loop to something like
msleep().
How about this one ? I also removed oflags, reading it outside of the locked section was racy.
Quoted 1 line
Did you try this with a card that otherwise goes into the wait?
Yes, I tried with sk98lin. I don't have a card here that actually supports netpoll.
===== net/core/netpoll.c 1.27 vs edited =====
net/core/netpoll.c +11 -9
@@ -18,6 +18,7 @@#include <linux/interrupt.h> #include <linux/netpoll.h> #include <linux/sched.h> +#include <linux/delay.h> #include <linux/rcupdate.h> #include <net/tcp.h> #include <net/udp.h>
@@ -575,16 +576,14 @@ }if (!netif_running (ndev)) { -unsigned short oflags;unsigned long atmost, atleast; +long left;printk (KERN_INFO "%s: device %s not up yet, forcing it\n" , np->name, np->dev_name); - oflags = ndev->flags; -rtnl_shlock (); -if (dev_change_flags (ndev, oflags |IFF_UP ) <0 ) { +if (dev_change_flags (ndev, ndev->flags |IFF_UP ) <0 ) {printk (KERN_ERR "%s: failed to open %s\n" , np->name, np->dev_name);rtnl_shunlock ();
@@ -592,8 +591,7 @@ }rtnl_shunlock (); - atleast = jiffies +HZ /10 ; - atmost = jiffies +10 *HZ ; + atmost = jiffies +4 *HZ ;while (!netif_carrier_ok (ndev)) {if (time_after (jiffies, atmost)) { printk(KERN_NOTICE
@@ -604,12 +602,16 @@cond_resched (); } + atleast = jiffies +HZ /10 ; +while (netif_carrier_ok (ndev) &&time_before (jiffies, atleast)) +cond_resched ();if (time_before (jiffies, atleast)) {printk (KERN_NOTICE "%s: carrier detect appears flaky," -" waiting 10 seconds\n" , +" waiting 4 seconds\n" , np->name); -while (time_before (jiffies, atmost)) -cond_resched (); + left = atmost - jiffies; +if (left >0 ) +msleep (jiffies_to_msecs (left)); } }
Re: [PATCH 1/7] netpoll: shorten carrier detect timeout
Quoted 7 lines · Patrick McHardy
Quoted 6 lines · Matt Mackall
Quoted 5 lines · Patrick McHardy
The carrier detection looks partially broken to me. The current logic detects an instantly available carrier as flaky because
netif_carrier_ok()takes less than 1/10s. This patch does what I assume is intended, make sure the carrier is stable for 1/10s.
Ok, on closer inspection, the current logic is: the NIC reports carrier detect nearly instaneously and thus its carrier detect reporting is considered unreliable. Rather than immediately sending packets, we wait for some interval for it to really be up so that the backlog of console messages doesn't get pumped into the bit bucket.
So I'm going to change it from "flaky" to "untrustworthy" and add a comment.
Quoted 2 lines
How about this one ? I also removed oflags, reading it outside of the locked section was racy.
I'll do this as a separate patch.
Quoted 4 lines
Quoted 1 line
Did you try this with a card that otherwise goes into the wait?
Yes, I tried with sk98lin. I don't have a card here that actually supports netpoll.
Huh? sk98lin appears to support it?
Signature · 1 line
Mathematics is the supreme nostalgia of our time.
Re: [PATCH 1/7] netpoll: shorten carrier detect timeout
Quoted 8 lines · Matt Mackall
Ok, on closer inspection, the current logic is: the NIC reports carrier detect nearly instaneously and thus its carrier detect reporting is considered unreliable. Rather than immediately sending packets, we wait for some interval for it to really be up so that the backlog of console messages doesn't get pumped into the bit bucket.
So I'm going to change it from "flaky" to "untrustworthy" and add a comment.
Why don't you trust an instaneously available carrier? Any reason to assume there will be false positives?
Regards
Patrick
Re: [PATCH 1/7] netpoll: shorten carrier detect timeout
Quoted 7 lines · Patrick McHardy
Quoted 6 lines · Matt Mackall
Ok, on closer inspection, the current logic is: the NIC reports carrier detect nearly instaneously and thus its carrier detect reporting is considered unreliable. Rather than immediately sending packets, we wait for some interval for it to really be up so that the backlog of console messages doesn't get pumped into the bit bucket.
Quoted 5 lines
Quoted 2 lines
So I'm going to change it from "flaky" to "untrustworthy" and add a comment.
Why don't you trust an instaneously available carrier? Any reason to assume there will be false positives?
Because I had reports of people losing all their boot messages until this logic was added (about a year ago now?). I don't remember which NICs were implicated, but some apparently report carrier is always available.
Signature · 1 line
Mathematics is the supreme nostalgia of our time.
Re: [PATCH 1/7] netpoll: shorten carrier detect timeout
Quoted 5 lines · Matt Mackall
Quoted 3 lines · Patrick McHardy
Quoted 2 lines
So I'm going to change it from "flaky" to "untrustworthy" and add a comment.
Quoted 8 lines
Quoted 2 lines
Why don't you trust an instaneously available carrier? Any reason to assume there will be false positives?
Because I had reports of people losing all their boot messages until this logic was added (about a year ago now?). I don't remember which NICs were implicated, but some apparently report carrier is always available.
If this problem is not common, I think it would be better to make this behaviour dependant on a boot parameter instead of forcing everyone to wait for 4s. Additionally you could have a blacklist of flaky NICs.
Regards
Patrick