diff options
author | Ben Pfaff <blp@nicira.com> | 2009-07-08 12:23:32 -0700 |
---|---|---|
committer | Ben Pfaff <blp@nicira.com> | 2009-07-08 14:13:15 -0700 |
commit | 72ca14c15430b1e06b06fa2042dab347c1c7d7df (patch) | |
tree | c5a79176da08f9cae4061e1b2e9153e8094d6002 /datapath/dp_dev.h | |
parent | 6d867ef335eab5cee83e353a0c7e8d143a2d3419 (diff) |
datapath: Fix race against workqueue in dp_dev and simplify code.
The dp_dev_destroy() function failed to cancel the xmit_queue work, which
allowed it to run after the device had been destroyed, accessing freed
memory. However, simply canceling the work with cancel_work_sync() would
be insufficient, since other packets could get queued while the work
function was running. Stopping the queue with netif_tx_disable() doesn't
help, because the final action in dp_dev_do_xmit() is to re-enable the TX
queue.
This issue led me to re-examine why the dp_dev needs to use a work_struct
at all. This was implemented in commit 71f13ed0b "Send of0 packets from
workqueue, to avoid recursive locking of ofN device" due to a complaint
from lockdep about recursive locking.
However, there's no actual reason that we need any locking around
dp_dev_xmit(). Until now, it has accepted the standard locking provided
by the network stack. But looking at the other software devices (veth,
loopback), those use NETIF_F_LLTX, which disables this locking, and
presumably do so for this very reason. In fact, the lwn article at
http://lwn.net/Articles/121566/ hints that NETIF_F_LLTX, which is otherwise
discouraged in the kernel, is acceptable for "certain types of software
device."
So this commit switches to using NETIF_F_LLTX for dp_dev and gets rid
of the work_struct.
In the process, I noticed that veth and loopback also take advantage of
a network device destruction "hook" using the net_device "destructor"
member. Using this we can automatically get called on network device
destruction at the point where rtnl_unlock() is called. This allows us
to stop stringing the dp_devs that are being destroyed onto a list so
that we can free them, and thus simplifies the code along all the paths
that call dp_dev_destroy().
This commit gets rid of a call to synchronize_rcu() (disguised as a call
to synchronize_net(), which is a macro that expands to synchronize_rcu()),
so it probably speeds up deleting ports, too.
Diffstat (limited to 'datapath/dp_dev.h')
-rw-r--r-- | datapath/dp_dev.h | 6 |
1 files changed, 2 insertions, 4 deletions
diff --git a/datapath/dp_dev.h b/datapath/dp_dev.h index 1b17d8f9..015c71ce 100644 --- a/datapath/dp_dev.h +++ b/datapath/dp_dev.h @@ -9,16 +9,14 @@ #ifndef DP_DEV_H #define DP_DEV_H 1 +#include <linux/percpu.h> + struct dp_dev { struct datapath *dp; int port_no; struct net_device *dev; struct net_device_stats stats; - struct sk_buff_head xmit_queue; - struct work_struct xmit_work; - - struct list_head list; }; static inline struct dp_dev *dp_dev_priv(struct net_device *netdev) |