* Re: [rfc | patch 0/6] netpoll: add support for the bonding driver
2005-07-01 23:05 [rfc | patch 0/6] netpoll: add support for the bonding driver Jeff Moyer
@ 2005-07-01 23:06 ` Jeff Moyer
2005-07-01 23:06 ` Jeff Moyer
` (5 subsequent siblings)
6 siblings, 0 replies; 9+ messages in thread
From: Jeff Moyer @ 2005-07-01 23:06 UTC (permalink / raw)
To: mpm, netdev, linux-kernel
Initialize npinfo->rx_flags. The way it stands now, this will have random
garbage, and so will incur a locking penalty even when an rx_hook isn't
registered and we are not active in the netpoll polling code.
Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
---
--- linux-2.6.12/net/core/netpoll.c.orig 2005-07-01 14:02:56.039174635 -0400
+++ linux-2.6.12/net/core/netpoll.c 2005-07-01 14:03:16.688739508 -0400
@@ -639,6 +639,7 @@ int netpoll_setup(struct netpoll *np)
if (!npinfo)
goto release;
+ npinfo->rx_flags = 0;
npinfo->rx_np = NULL;
npinfo->poll_lock = SPIN_LOCK_UNLOCKED;
npinfo->poll_owner = -1;
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [rfc | patch 0/6] netpoll: add support for the bonding driver
2005-07-01 23:05 [rfc | patch 0/6] netpoll: add support for the bonding driver Jeff Moyer
2005-07-01 23:06 ` Jeff Moyer
@ 2005-07-01 23:06 ` Jeff Moyer
2005-07-01 23:08 ` [rfc | patch 3/6] netpoll: change poll_napi to take a net_device Jeff Moyer
` (4 subsequent siblings)
6 siblings, 0 replies; 9+ messages in thread
From: Jeff Moyer @ 2005-07-01 23:06 UTC (permalink / raw)
To: mpm, netdev, linux-kernel
Move the poll_lock and poll_owner to the struct net_device. This change is
important for the bonding driver support, as multiple real devices will
point at the same netpoll_info. In such cases, we would be artificially
limiting ourselves to calling the polling routine of only one member of a
bond at a time.
To understand how this benefits the bonding driver, let's consider a
netpoll client that sends packets in response to receiving packets. Let's
assume a packet comes in on eth0, and the response is to be delivered over
eth1. If the poll_lock lived in the npinfo (and hence, there was only one
per bond), then we would have to queue the packet. With this patch in
place, we can send the outbound packet immediately.
Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
---
--- linux-2.6.12/net/core/netpoll.c.orig 2005-07-01 14:52:11.932101811 -0400
+++ linux-2.6.12/net/core/netpoll.c 2005-07-01 14:53:59.322183590 -0400
@@ -131,19 +131,20 @@ static int checksum_udp(struct sk_buff *
static void poll_napi(struct netpoll *np)
{
struct netpoll_info *npinfo = np->dev->npinfo;
+ struct net_device *dev = np->dev;
int budget = 16;
- if (test_bit(__LINK_STATE_RX_SCHED, &np->dev->state) &&
- npinfo->poll_owner != smp_processor_id() &&
- spin_trylock(&npinfo->poll_lock)) {
+ if (test_bit(__LINK_STATE_RX_SCHED, &dev->state) &&
+ dev->poll_owner != smp_processor_id() &&
+ spin_trylock(&dev->poll_lock)) {
npinfo->rx_flags |= NETPOLL_RX_DROP;
atomic_inc(&trapped);
- np->dev->poll(np->dev, &budget);
+ dev->poll(dev, &budget);
atomic_dec(&trapped);
npinfo->rx_flags &= ~NETPOLL_RX_DROP;
- spin_unlock(&npinfo->poll_lock);
+ spin_unlock(&dev->poll_lock);
}
}
@@ -246,7 +247,6 @@ repeat:
static void netpoll_send_skb(struct netpoll *np, struct sk_buff *skb)
{
int status;
- struct netpoll_info *npinfo;
repeat:
if(!np || !np->dev || !netif_running(np->dev)) {
@@ -255,8 +255,7 @@ repeat:
}
/* avoid recursion */
- npinfo = np->dev->npinfo;
- if (npinfo->poll_owner == smp_processor_id() ||
+ if (dev->poll_owner == smp_processor_id() ||
np->dev->xmit_lock_owner == smp_processor_id()) {
if (np->drop)
np->drop(skb);
@@ -641,8 +640,6 @@ int netpoll_setup(struct netpoll *np)
npinfo->rx_flags = 0;
npinfo->rx_np = NULL;
- npinfo->poll_lock = SPIN_LOCK_UNLOCKED;
- npinfo->poll_owner = -1;
npinfo->rx_lock = SPIN_LOCK_UNLOCKED;
} else
npinfo = ndev->npinfo;
--- linux-2.6.12/net/core/dev.c.orig 2005-07-01 14:52:08.613655521 -0400
+++ linux-2.6.12/net/core/dev.c 2005-07-01 14:52:38.495669512 -0400
@@ -3069,6 +3069,10 @@ struct net_device *alloc_netdev(int size
setup(dev);
strcpy(dev->name, name);
+#ifdef CONFIG_NETPOLL
+ dev->poll_owner = -1;
+ dev->poll_lock = SPIN_LOCK_UNLOCKED;
+#endif
return dev;
}
EXPORT_SYMBOL(alloc_netdev);
--- linux-2.6.12/include/linux/netpoll.h.orig 2005-07-01 14:52:16.016420312 -0400
+++ linux-2.6.12/include/linux/netpoll.h 2005-07-01 14:52:38.495669512 -0400
@@ -24,8 +24,6 @@ struct netpoll {
};
struct netpoll_info {
- spinlock_t poll_lock;
- int poll_owner;
int rx_flags;
spinlock_t rx_lock;
struct netpoll *rx_np; /* netpoll that registered an rx_hook */
@@ -63,16 +61,16 @@ static inline int netpoll_rx(struct sk_b
static inline void netpoll_poll_lock(struct net_device *dev)
{
if (dev->npinfo) {
- spin_lock(&dev->npinfo->poll_lock);
- dev->npinfo->poll_owner = smp_processor_id();
+ spin_lock(&dev->poll_lock);
+ dev->poll_owner = smp_processor_id();
}
}
static inline void netpoll_poll_unlock(struct net_device *dev)
{
if (dev->npinfo) {
- dev->npinfo->poll_owner = -1;
- spin_unlock(&dev->npinfo->poll_lock);
+ dev->poll_owner = -1;
+ spin_unlock(&dev->poll_lock);
}
}
--- linux-2.6.12/include/linux/netdevice.h.orig 2005-07-01 14:52:22.666310731 -0400
+++ linux-2.6.12/include/linux/netdevice.h 2005-07-01 14:52:38.496669345 -0400
@@ -469,6 +469,8 @@ struct net_device
int (*neigh_setup)(struct net_device *dev, struct neigh_parms *);
#ifdef CONFIG_NETPOLL
struct netpoll_info *npinfo;
+ spinlock_t poll_lock;
+ int poll_owner;
#endif
#ifdef CONFIG_NET_POLL_CONTROLLER
void (*poll_controller)(struct net_device *dev);
^ permalink raw reply [flat|nested] 9+ messages in thread* [rfc | patch 3/6] netpoll: change poll_napi to take a net_device
2005-07-01 23:05 [rfc | patch 0/6] netpoll: add support for the bonding driver Jeff Moyer
2005-07-01 23:06 ` Jeff Moyer
2005-07-01 23:06 ` Jeff Moyer
@ 2005-07-01 23:08 ` Jeff Moyer
2005-07-01 23:08 ` [rfc | patch 4/6] netpoll: add netpoll hooks to support the bonding driver Jeff Moyer
` (3 subsequent siblings)
6 siblings, 0 replies; 9+ messages in thread
From: Jeff Moyer @ 2005-07-01 23:08 UTC (permalink / raw)
To: mpm, netdev, linux-kernel
(sorry for munging the subject of the prior two patches)
The poll_napi routine is not specific to a netpoll. It really should be
able to be called with a struct net_device. Changing the routine to take a
struct net_device makes it easier to support the bonding driver, since we
will be implementing a netpoll_poll_dev routine, that will not have a
struct netpoll associated with it.
Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
---
--- linux-2.6.12/net/core/netpoll.c.orig 2005-07-01 14:12:33.986337259 -0400
+++ linux-2.6.12/net/core/netpoll.c 2005-07-01 14:20:45.848186887 -0400
@@ -128,10 +128,9 @@ static int checksum_udp(struct sk_buff *
* network adapter, forcing superfluous retries and possibly timeouts.
* Thus, we set our budget to greater than 1.
*/
-static void poll_napi(struct netpoll *np)
+static void poll_napi(struct net_device *dev)
{
- struct netpoll_info *npinfo = np->dev->npinfo;
- struct net_device *dev = np->dev;
+ struct netpoll_info *npinfo = dev->npinfo;
int budget = 16;
if (test_bit(__LINK_STATE_RX_SCHED, &dev->state) &&
@@ -156,7 +155,7 @@ void netpoll_poll(struct netpoll *np)
/* Process pending work on NIC */
np->dev->poll_controller(np->dev);
if (np->dev->poll)
- poll_napi(np);
+ poll_napi(np->dev);
zap_completion_queue();
}
^ permalink raw reply [flat|nested] 9+ messages in thread* [rfc | patch 4/6] netpoll: add netpoll hooks to support the bonding driver
2005-07-01 23:05 [rfc | patch 0/6] netpoll: add support for the bonding driver Jeff Moyer
` (2 preceding siblings ...)
2005-07-01 23:08 ` [rfc | patch 3/6] netpoll: change poll_napi to take a net_device Jeff Moyer
@ 2005-07-01 23:08 ` Jeff Moyer
2005-07-01 23:09 ` [rfc | patch 5/6] netpoll: modify bonding driver to support netpoll Jeff Moyer
` (2 subsequent siblings)
6 siblings, 0 replies; 9+ messages in thread
From: Jeff Moyer @ 2005-07-01 23:08 UTC (permalink / raw)
To: mpm, netdev, linux-kernel
Implement hooks in the netpoll code to support the bonding driver (and
hopefully other virtual device drivers, as well). The key additions here
are netpoll_poll_dev(new) and netpoll_send_skb(newly exported).
netpoll_send_skb
This is exported so that the bonding driver can queue a packet to be sent
via the real ethernet device it has chosen.
netpoll_poll_dev
This is a new routine that was created and exported so that the
poll_controller implementation in the bonding driver could poll each of
the underlying real devices without duplicating all of the logic that
exists internally to netpoll already.
Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
---
--- linux-2.6.12/net/core/netpoll.c.orig 2005-07-01 15:02:05.503951082 -0400
+++ linux-2.6.12/net/core/netpoll.c 2005-07-01 15:02:36.718705979 -0400
@@ -147,19 +147,27 @@ static void poll_napi(struct net_device
}
}
-void netpoll_poll(struct netpoll *np)
+void netpoll_poll_dev(struct net_device *dev)
{
- if(!np->dev || !netif_running(np->dev) || !np->dev->poll_controller)
+ if(!netif_running(dev) || !dev->poll_controller)
return;
/* Process pending work on NIC */
- np->dev->poll_controller(np->dev);
- if (np->dev->poll)
- poll_napi(np->dev);
+ dev->poll_controller(dev);
+ if (dev->poll)
+ poll_napi(dev);
zap_completion_queue();
}
+void netpoll_poll(struct netpoll *np)
+{
+ if(!np->dev)
+ return;
+
+ netpoll_poll_dev(np->dev);
+}
+
static void refill_skbs(void)
{
struct sk_buff *skb;
@@ -243,48 +251,59 @@ repeat:
return skb;
}
-static void netpoll_send_skb(struct netpoll *np, struct sk_buff *skb)
+/*
+ * This function can be called from virtual device drivers, such as the
+ * bonding driver. In this case, skb->dev is filled in with the real
+ * device over which the packet needs to be sent. For this reason, we use
+ * the skb->dev instead of np->dev. Essentially, np->dev can be, for
+ * example, bond0, while we actually need to send the packet out over eth0.
+ */
+void netpoll_send_skb(struct netpoll *np, struct sk_buff *skb)
{
int status;
+ struct net_device *dev = skb->dev;
repeat:
- if(!np || !np->dev || !netif_running(np->dev)) {
+ if(!np || !dev || !netif_running(dev)) {
__kfree_skb(skb);
return;
}
/* avoid recursion */
if (dev->poll_owner == smp_processor_id() ||
- np->dev->xmit_lock_owner == smp_processor_id()) {
- if (np->drop)
+ dev->xmit_lock_owner == smp_processor_id()) {
+ if (np && np->drop)
np->drop(skb);
else
__kfree_skb(skb);
return;
}
- spin_lock(&np->dev->xmit_lock);
- np->dev->xmit_lock_owner = smp_processor_id();
+ spin_lock(&dev->xmit_lock);
+ dev->xmit_lock_owner = smp_processor_id();
/*
* network drivers do not expect to be called if the queue is
* stopped.
*/
- if (netif_queue_stopped(np->dev)) {
- np->dev->xmit_lock_owner = -1;
- spin_unlock(&np->dev->xmit_lock);
+ if (netif_queue_stopped(dev)) {
+ dev->xmit_lock_owner = -1;
+ spin_unlock(&dev->xmit_lock);
- netpoll_poll(np);
+ netpoll_poll_dev(dev);
goto repeat;
}
- status = np->dev->hard_start_xmit(skb, np->dev);
- np->dev->xmit_lock_owner = -1;
- spin_unlock(&np->dev->xmit_lock);
+ if (dev->netpoll_start_xmit)
+ status = dev->netpoll_start_xmit(np, skb, dev);
+ else
+ status = dev->hard_start_xmit(skb, dev);
+ dev->xmit_lock_owner = -1;
+ spin_unlock(&dev->xmit_lock);
/* transmit busy */
if(status) {
- netpoll_poll(np);
+ netpoll_poll_dev(dev);
goto repeat;
}
}
@@ -715,6 +734,11 @@ int netpoll_setup(struct netpoll *np)
npinfo->rx_np = np;
spin_unlock_irqrestore(&npinfo->rx_lock, flags);
}
+
+ /* Call the device specific netpoll initialization routine. */
+ if (ndev->netpoll_setup)
+ ndev->netpoll_setup(ndev, npinfo);
+
/* last thing to do is link it to the net device structure */
ndev->npinfo = npinfo;
@@ -766,5 +790,7 @@ EXPORT_SYMBOL(netpoll_parse_options);
EXPORT_SYMBOL(netpoll_setup);
EXPORT_SYMBOL(netpoll_cleanup);
EXPORT_SYMBOL(netpoll_send_udp);
+EXPORT_SYMBOL(netpoll_send_skb);
EXPORT_SYMBOL(netpoll_poll);
+EXPORT_SYMBOL(netpoll_poll_dev);
EXPORT_SYMBOL(netpoll_queue);
--- linux-2.6.12/net/core/dev.c.orig 2005-07-01 14:59:50.161654219 -0400
+++ linux-2.6.12/net/core/dev.c 2005-07-01 15:00:23.437103656 -0400
@@ -1655,15 +1655,15 @@ int netif_receive_skb(struct sk_buff *sk
int ret = NET_RX_DROP;
unsigned short type;
- /* if we've gotten here through NAPI, check netpoll */
- if (skb->dev->poll && netpoll_rx(skb))
+ skb_bond(skb);
+
+ /* if there is a netpoll client registered, check netpoll */
+ if (skb->dev->npinfo && netpoll_rx(skb))
return NET_RX_DROP;
if (!skb->stamp.tv_sec)
net_timestamp(&skb->stamp);
- skb_bond(skb);
-
__get_cpu_var(netdev_rx_stat).total++;
skb->h.raw = skb->nh.raw = skb->data;
--- linux-2.6.12/include/linux/netpoll.h.orig 2005-07-01 14:59:56.135657708 -0400
+++ linux-2.6.12/include/linux/netpoll.h 2005-07-01 15:00:23.437103656 -0400
@@ -30,7 +30,9 @@ struct netpoll_info {
};
void netpoll_poll(struct netpoll *np);
+void netpoll_poll_dev(struct net_device *dev);
void netpoll_send_udp(struct netpoll *np, const char *msg, int len);
+void netpoll_send_skb(struct netpoll *np, struct sk_buff *skb);
int netpoll_parse_options(struct netpoll *np, char *opt);
int netpoll_setup(struct netpoll *np);
int netpoll_trap(void);
--- linux-2.6.12/include/linux/netdevice.h.orig 2005-07-01 14:59:59.835040624 -0400
+++ linux-2.6.12/include/linux/netdevice.h 2005-07-01 15:00:23.438103489 -0400
@@ -41,6 +41,7 @@
struct divert_blk;
struct vlan_group;
struct ethtool_ops;
+struct netpoll;
struct netpoll_info;
/* source back-compat hooks */
#define SET_ETHTOOL_OPS(netdev,ops) \
@@ -473,7 +474,12 @@ struct net_device
int poll_owner;
#endif
#ifdef CONFIG_NET_POLL_CONTROLLER
+ void (*netpoll_setup)(struct net_device *dev,
+ struct netpoll_info *npinfo);
void (*poll_controller)(struct net_device *dev);
+ int (*netpoll_start_xmit)(struct netpoll *np,
+ struct sk_buff *skb,
+ struct net_device *dev);
#endif
/* bridge stuff */
^ permalink raw reply [flat|nested] 9+ messages in thread* [rfc | patch 5/6] netpoll: modify bonding driver to support netpoll
2005-07-01 23:05 [rfc | patch 0/6] netpoll: add support for the bonding driver Jeff Moyer
` (3 preceding siblings ...)
2005-07-01 23:08 ` [rfc | patch 4/6] netpoll: add netpoll hooks to support the bonding driver Jeff Moyer
@ 2005-07-01 23:09 ` Jeff Moyer
2005-07-01 23:10 ` [rfc | patch 6/6] netpoll: fix deadlock in arp_reply Jeff Moyer
2005-07-01 23:38 ` [rfc | patch 0/6] netpoll: add support for the bonding driver Matt Mackall
6 siblings, 0 replies; 9+ messages in thread
From: Jeff Moyer @ 2005-07-01 23:09 UTC (permalink / raw)
To: mpm, netdev, linux-kernel
Implement netpoll hooks in the bonding driver. We register the following
netpoll specific routines:
bond_netpoll_setup
This routine associates the struct netpoll_info of the master device with
each of the slave devices.
bond_xmit_netpoll
This routine sets bonding->netpoll to the struct netpoll provided by
netpoll_send_skb. This pointer is then passed along to netpoll_send_skb
in bond_dev_queue_xmit, when we are ready to send the skb out over the
real network device.
bond_poll_controller
This routine calls netpoll_poll_dev for each of the slaves in the bond.
When a new slave is added to the bond, if it does not support netpoll, then
netpoll is disabled for the bond. When a slave is released, that slave's
npinfo pointer is cleared.
I have tested this code extensively, and it works well in my environment.
Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
---
--- linux-2.6.12/drivers/net/bonding/bond_main.c.orig 2005-06-29 14:28:08.000000000 -0400
+++ linux-2.6.12/drivers/net/bonding/bond_main.c 2005-06-30 19:33:50.641009622 -0400
@@ -821,8 +821,12 @@ int bond_dev_queue_xmit(struct bonding *
}
skb->priority = 1;
- dev_queue_xmit(skb);
-
+#ifdef CONFIG_NET_POLL_CONTROLLER
+ if (bond->netpoll)
+ netpoll_send_skb(bond->netpoll, skb);
+ else
+#endif
+ dev_queue_xmit(skb);
return 0;
}
@@ -1567,6 +1571,45 @@ static void bond_detach_slave(struct bon
bond->slave_cnt--;
}
+#ifdef CONFIG_NET_POLL_CONTROLLER
+static int slaves_support_netpoll(struct net_device *bond_dev)
+{
+ struct bonding *bond = bond_dev->priv;
+ struct slave *slave;
+ int i;
+
+ bond_for_each_slave(bond, slave, i) {
+ if (!slave->dev->poll_controller)
+ return 0;
+ }
+
+ return 1;
+}
+
+static void bond_poll_controller(struct net_device *bond_dev)
+{
+ struct bonding *bond = bond_dev->priv;
+ struct slave *slave;
+ int i;
+
+ bond_for_each_slave(bond, slave, i) {
+ if (slave->dev->poll_controller)
+ netpoll_poll_dev(slave->dev);
+ }
+}
+
+static void bond_netpoll_setup(struct net_device *bond_dev,
+ struct netpoll_info *npinfo)
+{
+ struct bonding *bond = bond_dev->priv;
+ struct slave *slave;
+ int i;
+
+ bond_for_each_slave(bond, slave, i)
+ slave->dev->npinfo = npinfo;
+}
+#endif
+
/*---------------------------------- IOCTL ----------------------------------*/
static int bond_sethwaddr(struct net_device *bond_dev, struct net_device *slave_dev)
@@ -1969,6 +2012,17 @@ static int bond_enslave(struct net_devic
new_slave->state == BOND_STATE_ACTIVE ? "n active" : " backup",
new_slave->link != BOND_LINK_DOWN ? "n up" : " down");
+#ifdef CONFIG_NET_POLL_CONTROLLER
+ if (slaves_support_netpoll(bond_dev)) {
+ bond_dev->poll_controller = bond_poll_controller;
+ slave_dev->npinfo = bond_dev->npinfo;
+ } else if (bond_dev->poll_controller) {
+ bond_dev->poll_controller = NULL;
+ printk("New slave device %s does not support netpoll.\n",
+ slave_dev->name);
+ printk("netpoll disabled for %s.\n", bond_dev->name);
+ }
+#endif
/* enslave is successful */
return 0;
@@ -2173,6 +2227,9 @@ static int bond_release(struct net_devic
slave_dev->flags &= ~IFF_NOARP;
}
+#ifdef CONFIG_NET_POLL_CONTROLLER
+ slave_dev->npinfo = NULL;
+#endif
kfree(slave);
return 0; /* deletion OK */
@@ -4203,6 +4260,21 @@ out:
return 0;
}
+#ifdef CONFIG_NET_POLL_CONTROLLER
+int bond_xmit_netpoll(struct netpoll *np, struct sk_buff *skb,
+ struct net_device *bond_dev)
+{
+ struct bonding *bond = bond_dev->priv;
+ int ret;
+
+ bond->netpoll = np;
+ ret = bond_dev->hard_start_xmit(skb, bond_dev);
+ bond->netpoll = NULL;
+
+ return ret;
+}
+#endif
+
/*------------------------- Device initialization ---------------------------*/
/*
@@ -4277,6 +4349,10 @@ static int __init bond_init(struct net_d
bond_dev->destructor = free_netdev;
+#ifdef CONFIG_NET_POLL_CONTROLLER
+ bond_dev->netpoll_setup = bond_netpoll_setup;
+ bond_dev->netpoll_start_xmit = bond_xmit_netpoll;
+#endif
/* Initialize the device options */
bond_dev->tx_queue_len = 0;
bond_dev->flags |= IFF_MASTER|IFF_MULTICAST;
--- linux-2.6.12/drivers/net/bonding/bonding.h.orig 2005-06-29 14:29:40.000000000 -0400
+++ linux-2.6.12/drivers/net/bonding/bonding.h 2005-06-30 19:02:54.000000000 -0400
@@ -33,6 +33,7 @@
#include <linux/timer.h>
#include <linux/proc_fs.h>
#include <linux/if_bonding.h>
+#include <linux/netpoll.h>
#include "bond_3ad.h"
#include "bond_alb.h"
@@ -203,6 +204,9 @@ struct bonding {
struct bond_params params;
struct list_head vlan_list;
struct vlan_group *vlgrp;
+#ifdef CONFIG_NET_POLL_CONTROLLER
+ struct netpoll *netpoll;
+#endif
};
/**
^ permalink raw reply [flat|nested] 9+ messages in thread* [rfc | patch 6/6] netpoll: fix deadlock in arp_reply
2005-07-01 23:05 [rfc | patch 0/6] netpoll: add support for the bonding driver Jeff Moyer
` (4 preceding siblings ...)
2005-07-01 23:09 ` [rfc | patch 5/6] netpoll: modify bonding driver to support netpoll Jeff Moyer
@ 2005-07-01 23:10 ` Jeff Moyer
2005-07-01 23:38 ` [rfc | patch 0/6] netpoll: add support for the bonding driver Matt Mackall
6 siblings, 0 replies; 9+ messages in thread
From: Jeff Moyer @ 2005-07-01 23:10 UTC (permalink / raw)
To: mpm, netdev, linux-kernel
This fixes an obvious deadlock in the netpoll code. netpoll_rx takes the
npinfo->rx_lock. netpoll_rx is also the only caller of arp_reply (through
__netpoll_rx). As such, it is not necessary to take this lock.
Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
--- linux-2.6.12/net/core/netpoll.c.orig 2005-07-01 17:55:24.313339068 -0400
+++ linux-2.6.12/net/core/netpoll.c 2005-07-01 17:55:57.354863472 -0400
@@ -370,11 +370,8 @@ static void arp_reply(struct sk_buff *sk
struct sk_buff *send_skb;
struct netpoll *np = NULL;
- spin_lock_irqsave(&npinfo->rx_lock, flags);
if (npinfo->rx_np && npinfo->rx_np->dev == skb->dev)
np = npinfo->rx_np;
- spin_unlock_irqrestore(&npinfo->rx_lock, flags);
-
if (!np)
return;
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [rfc | patch 0/6] netpoll: add support for the bonding driver
2005-07-01 23:05 [rfc | patch 0/6] netpoll: add support for the bonding driver Jeff Moyer
` (5 preceding siblings ...)
2005-07-01 23:10 ` [rfc | patch 6/6] netpoll: fix deadlock in arp_reply Jeff Moyer
@ 2005-07-01 23:38 ` Matt Mackall
2005-07-01 23:43 ` Jeff Moyer
6 siblings, 1 reply; 9+ messages in thread
From: Matt Mackall @ 2005-07-01 23:38 UTC (permalink / raw)
To: Jeff Moyer; +Cc: netdev, linux-kernel
On Fri, Jul 01, 2005 at 07:05:54PM -0400, Jeff Moyer wrote:
> New netpoll function implemented by the network drivers:
>
> net_device->netpoll_setup
> This is required, since the bonding device has to walk through each slave
> and point its slave_dev->npinfo at the npinfo for the master device. The
> reason for this is so that when we're doing the napi polling, we can set
> the rx_flags appropriately.
>
> net_device->netpoll_start_xmit
> This routine is required since, otherwise, there is no way to intercept
> packets bound for interfaces that are not ready for them. Of course, it
> requires further logic in the bonding driver to then call into the
> netpoll_send_skb routine (which is a new export).
>
> Note that neither of these pointers has to be filled in by the driver.
> These functions should only be implemented where needed, and to date, that
> is only in the bonding driver.
>
> Newly exported are:
>
> netpoll_send_skb
> This is exported so that the bonding driver can queue a packet to be sent
> via the real ethernet device it has chosen.
>
> netpoll_poll_dev
> This is a new routine that was created and exported so that the
> poll_controller implementation in the bonding driver could poll each of
> the underlying real devices without duplicating all of the logic that
> exists internally to netpoll already.
>
>
> To test this, as I mentioned above, I wrote a simple module which, upon
> receipt of any packet, sends out a packet with the message "PONG". I fired
> up netcat to send test packets, and receive the responses. I also loaded
> the netconsole module for the very same interface, bond0, and issue a
> series of sysrq-X's, both via sysrq-trigger and via the keyboard. I did
> this while simultaneously testing the PING server on an SMP machine. As
> things stand, it is very stable in my environment.
>
> And so, the patch set follows. Any and all comments are appreciated.
Patches 1, 3, and 6 are unrelated bugfixes and should just go in.
I don't like that we rely on queueing to process round trips for PONG.
Is this really unavoidable?
And I think the most controversial thing here is moving locks from npinfo
into the device.
Not really happy about how incestuous this makes the bonding driver
with netpoll. I'll try to think more about it over the weekend.
--
Mathematics is the supreme nostalgia of our time.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [rfc | patch 0/6] netpoll: add support for the bonding driver
2005-07-01 23:38 ` [rfc | patch 0/6] netpoll: add support for the bonding driver Matt Mackall
@ 2005-07-01 23:43 ` Jeff Moyer
0 siblings, 0 replies; 9+ messages in thread
From: Jeff Moyer @ 2005-07-01 23:43 UTC (permalink / raw)
To: Matt Mackall; +Cc: netdev, linux-kernel
==> Regarding Re: [rfc | patch 0/6] netpoll: add support for the bonding driver; Matt Mackall <mpm@selenic.com> adds:
mpm> On Fri, Jul 01, 2005 at 07:05:54PM -0400, Jeff Moyer wrote:
>> New netpoll function implemented by the network drivers:
>>
net_device-> netpoll_setup
>> This is required, since the bonding device has to walk through each
>> slave and point its slave_dev->npinfo at the npinfo for the master
>> device. The reason for this is so that when we're doing the napi
>> polling, we can set the rx_flags appropriately.
>>
net_device-> netpoll_start_xmit
>> This routine is required since, otherwise, there is no way to intercept
>> packets bound for interfaces that are not ready for them. Of course, it
>> requires further logic in the bonding driver to then call into the
>> netpoll_send_skb routine (which is a new export).
>>
>> Note that neither of these pointers has to be filled in by the driver.
>> These functions should only be implemented where needed, and to date,
>> that is only in the bonding driver.
>>
>> Newly exported are:
>>
>> netpoll_send_skb This is exported so that the bonding driver can queue a
>> packet to be sent via the real ethernet device it has chosen.
>>
>> netpoll_poll_dev This is a new routine that was created and exported so
>> that the poll_controller implementation in the bonding driver could poll
>> each of the underlying real devices without duplicating all of the logic
>> that exists internally to netpoll already.
>>
>>
>> To test this, as I mentioned above, I wrote a simple module which, upon
>> receipt of any packet, sends out a packet with the message "PONG". I
>> fired up netcat to send test packets, and receive the responses. I also
>> loaded the netconsole module for the very same interface, bond0, and
>> issue a series of sysrq-X's, both via sysrq-trigger and via the
>> keyboard. I did this while simultaneously testing the PING server on an
>> SMP machine. As things stand, it is very stable in my environment.
>>
>> And so, the patch set follows. Any and all comments are appreciated.
mpm> Patches 1, 3, and 6 are unrelated bugfixes and should just go in.
Right. Sorry I lumped them together.
mpm> I don't like that we rely on queueing to process round trips for PONG.
mpm> Is this really unavoidable?
That's how it works without these patches, too.
mpm> And I think the most controversial thing here is moving locks from
mpm> npinfo into the device.
Well, as I mentioned, that's a bit of an optimization, if you will.
mpm> Not really happy about how incestuous this makes the bonding driver
mpm> with netpoll. I'll try to think more about it over the weekend.
I'd be delighted if you came up with a better way to do things. However, I
think you'll find that this is about as clean as it gets.
-Jeff
^ permalink raw reply [flat|nested] 9+ messages in thread