From: Ben Pfaff Date: Thu, 8 Jan 2009 20:25:18 +0000 (-0800) Subject: datapath: Fix deadlock in switch port removal. X-Git-Url: https://pintos-os.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=a9744caf6347f3af54362d39465011b9bf17a61d;p=openvswitch datapath: Fix deadlock in switch port removal. dp_del_switch_port() cancels the port_task and waits for it to finish, but the port_task requires the RTNL lock to complete, which dp_del_switch_port() holds, thus a deadlock. This commit fixes the problem, by deleting the port_task entirely and moving its work into secchan. --- diff --git a/datapath/datapath.c b/datapath/datapath.c index efa84f9f..4b5103e3 100644 --- a/datapath/datapath.c +++ b/datapath/datapath.c @@ -387,7 +387,6 @@ static struct net_bridge_port *new_nbp(struct datapath *dp, p->dev = dev; p->port_no = port_no; spin_lock_init(&p->lock); - INIT_WORK(&p->port_task, NULL); if (port_no != OFPP_LOCAL) rcu_assign_pointer(dev->br_port, p); if (port_no < DP_MAX_PORTS) @@ -432,7 +431,6 @@ int dp_del_switch_port(struct net_bridge_port *p) #endif /* First drop references to device. */ - cancel_work_sync(&p->port_task); dev_set_promiscuity(p->dev, -1); list_del_rcu(&p->node); if (p->port_no != OFPP_LOCAL) @@ -949,36 +947,6 @@ dp_send_hello(struct datapath *dp, const struct sender *sender, } } -/* Callback function for a workqueue to disable an interface */ -static void -down_port_cb(struct work_struct *work) -{ - struct net_bridge_port *p = container_of(work, struct net_bridge_port, - port_task); - - rtnl_lock(); - if (dev_change_flags(p->dev, p->dev->flags & ~IFF_UP) < 0) - if (net_ratelimit()) - printk("problem bringing up port %s\n", p->dev->name); - rtnl_unlock(); - p->config |= OFPPC_PORT_DOWN; -} - -/* Callback function for a workqueue to enable an interface */ -static void -up_port_cb(struct work_struct *work) -{ - struct net_bridge_port *p = container_of(work, struct net_bridge_port, - port_task); - - rtnl_lock(); - if (dev_change_flags(p->dev, p->dev->flags | IFF_UP) < 0) - if (net_ratelimit()) - printk("problem bringing down port %s\n", p->dev->name); - rtnl_unlock(); - p->config &= ~OFPPC_PORT_DOWN; -} - int dp_update_port_flags(struct datapath *dp, const struct ofp_port_mod *opm) { @@ -999,22 +967,6 @@ dp_update_port_flags(struct datapath *dp, const struct ofp_port_mod *opm) p->config &= ~config_mask; p->config |= ntohl(opm->config) & config_mask; } - - /* Modifying the status of an interface requires taking a lock - * that cannot be done from here. For this reason, we use a shared - * workqueue, which will cause it to be executed from a safer - * context. */ - if (opm->mask & htonl(OFPPC_PORT_DOWN)) { - if ((opm->config & htonl(OFPPC_PORT_DOWN)) - && (p->config & OFPPC_PORT_DOWN) == 0) { - PREPARE_WORK(&p->port_task, down_port_cb); - schedule_work(&p->port_task); - } else if ((opm->config & htonl(OFPPC_PORT_DOWN)) == 0 - && (p->config & OFPPC_PORT_DOWN)) { - PREPARE_WORK(&p->port_task, up_port_cb); - schedule_work(&p->port_task); - } - } spin_unlock_irqrestore(&p->lock, flags); return 0; diff --git a/datapath/datapath.h b/datapath/datapath.h index 697a4c48..4f71e371 100644 --- a/datapath/datapath.h +++ b/datapath/datapath.h @@ -79,7 +79,6 @@ struct net_bridge_port { u32 config; /* Some subset of OFPPC_* flags. */ u32 state; /* Some subset of OFPPS_* flags. */ spinlock_t lock; - struct work_struct port_task; struct datapath *dp; struct net_device *dev; struct snat_conf *snat; /* Only set if SNAT is configured for this port. */ diff --git a/secchan/port-watcher.c b/secchan/port-watcher.c index 7daa370e..1f6a53d8 100644 --- a/secchan/port-watcher.c +++ b/secchan/port-watcher.c @@ -1,4 +1,4 @@ -/* Copyright (c) 2008 The Board of Trustees of The Leland Stanford +/* Copyright (c) 2008, 2009 The Board of Trustees of The Leland Stanford * Junior University * * We are making the OpenFlow specification and associated documentation @@ -38,6 +38,7 @@ #include #include #include "dynamic-string.h" +#include "netdev.h" #include "ofpbuf.h" #include "openflow/openflow.h" #include "poll-loop.h" @@ -254,6 +255,31 @@ port_watcher_local_packet_cb(struct relay *r, void *pw_) return false; } +static void +bring_netdev_up_or_down(const char *name, bool down) +{ + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); + struct netdev *netdev; + int retval; + + retval = netdev_open(name, NETDEV_ETH_TYPE_NONE, &netdev); + if (!retval) { + if (down) { + retval = netdev_turn_flags_on(netdev, NETDEV_UP, true); + } else { + retval = netdev_turn_flags_off(netdev, NETDEV_UP, true); + } + if (retval) { + VLOG_WARN_RL(&rl, "failed to bring network device %s %s: %s", + name, down ? "down" : "up", strerror(retval)); + } + netdev_close(netdev); + } else { + VLOG_WARN_RL(&rl, "failed to open network device %s: %s", + name, strerror(retval)); + } +} + static bool port_watcher_remote_packet_cb(struct relay *r, void *pw_) { @@ -274,6 +300,11 @@ port_watcher_remote_packet_cb(struct relay *r, void *pw_) if (pw_opp->port_no == htons(OFPP_LOCAL)) { call_local_port_changed_callbacks(pw); } + + if (opm->mask & htonl(OFPPC_PORT_DOWN)) { + bring_netdev_up_or_down((const char *) pw_opp->name, + opm->config & htonl(OFPPC_PORT_DOWN)); + } } } return false; diff --git a/udatapath/datapath.c b/udatapath/datapath.c index 5c3f1374..aee71ee5 100644 --- a/udatapath/datapath.c +++ b/udatapath/datapath.c @@ -1,4 +1,4 @@ -/* Copyright (c) 2008 The Board of Trustees of The Leland Stanford +/* Copyright (c) 2008, 2009 The Board of Trustees of The Leland Stanford * Junior University * * We are making the OpenFlow specification and associated documentation @@ -745,18 +745,6 @@ update_port_flags(struct datapath *dp, const struct ofp_port_mod *opm) p->config &= ~config_mask; p->config |= ntohl(opm->config) & config_mask; } - - if (opm->mask & htonl(OFPPC_PORT_DOWN)) { - if ((opm->config & htonl(OFPPC_PORT_DOWN)) - && (p->config & OFPPC_PORT_DOWN) == 0) { - p->config |= OFPPC_PORT_DOWN; - netdev_turn_flags_off(p->netdev, NETDEV_UP, true); - } else if ((opm->config & htonl(OFPPC_PORT_DOWN)) == 0 - && (p->config & OFPPC_PORT_DOWN)) { - p->config &= ~OFPPC_PORT_DOWN; - netdev_turn_flags_on(p->netdev, NETDEV_UP, true); - } - } } /* Update the port status field of the bridge port. A non-zero return