netdev objects are hard to use with RCU, because it's not possible to
split removal and reclamation. Postponing the removal means that the
port is not removed and cannot be readded immediately. Waiting for
reclamation means introducing a quiescent state, and that may introduce
subtle bugs, due to the RCU model we use in userspace.
This commit changes the port container from cmap to hmap. 'port_mutex'
must be held by readers and writers. This shouldn't have performace
impact, as readers in the fast path use a thread local cache.
Signed-off-by: Daniele Di Proietto <diproiet...@vmware.com>
---
lib/dpif-netdev.c | 96 +++++++++++++++++++++++++++++++++----------------------
1 file changed, 57 insertions(+), 39 deletions(-)
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index bd2249e..8cc37e2 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -195,9 +195,10 @@ struct dp_netdev {
/* Ports.
*
- * Protected by RCU. Take the mutex to add or remove ports. */
+ * Any lookup into 'ports' or any access to the dp_netdev_ports found
+ * through 'ports' requires taking 'port_mutex'. */
struct ovs_mutex port_mutex;
- struct cmap ports;
+ struct hmap ports;
struct seq *port_seq; /* Incremented whenever a port changes. */
/* Protects access to ofproto-dpif-upcall interface during revalidator
@@ -228,7 +229,8 @@ struct dp_netdev {
};
static struct dp_netdev_port *dp_netdev_lookup_port(const struct dp_netdev *dp,
- odp_port_t);
+ odp_port_t)
+ OVS_REQUIRES(&dp->port_mutex);
enum dp_stat_type {
DP_STAT_EXACT_HIT, /* Packets that had an exact match (emc). */
@@ -248,7 +250,7 @@ enum pmd_cycles_counter_type {
struct dp_netdev_port {
odp_port_t port_no;
struct netdev *netdev;
- struct cmap_node node; /* Node in dp_netdev's 'ports'. */
+ struct hmap_node node; /* Node in dp_netdev's 'ports'. */
struct netdev_saved_flags *sf;
unsigned n_rxq; /* Number of elements in 'rxq' */
struct netdev_rxq **rxq;
@@ -476,9 +478,11 @@ struct dpif_netdev {
};
static int get_port_by_number(struct dp_netdev *dp, odp_port_t port_no,
- struct dp_netdev_port **portp);
+ struct dp_netdev_port **portp)
+ OVS_REQUIRES(dp->port_mutex);
static int get_port_by_name(struct dp_netdev *dp, const char *devname,
- struct dp_netdev_port **portp);
+ struct dp_netdev_port **portp)
+ OVS_REQUIRES(dp->port_mutex);
static void dp_netdev_free(struct dp_netdev *)
OVS_REQUIRES(dp_netdev_mutex);
static int do_add_port(struct dp_netdev *dp, const char *devname,
@@ -522,7 +526,8 @@ dp_netdev_add_rxq_to_pmd(struct dp_netdev_pmd_thread *pmd,
struct dp_netdev_port *port, struct netdev_rxq *rx);
static struct dp_netdev_pmd_thread *
dp_netdev_less_loaded_pmd_on_numa(struct dp_netdev *dp, int numa_id);
-static void dp_netdev_reset_pmd_threads(struct dp_netdev *dp);
+static void dp_netdev_reset_pmd_threads(struct dp_netdev *dp)
+ OVS_REQUIRES(dp->port_mutex);
static bool dp_netdev_pmd_try_ref(struct dp_netdev_pmd_thread *pmd);
static void dp_netdev_pmd_unref(struct dp_netdev_pmd_thread *pmd);
static void dp_netdev_pmd_flow_flush(struct dp_netdev_pmd_thread *pmd);
@@ -913,7 +918,7 @@ create_dp_netdev(const char *name, const struct dpif_class
*class,
atomic_flag_clear(&dp->destroyed);
ovs_mutex_init(&dp->port_mutex);
- cmap_init(&dp->ports);
+ hmap_init(&dp->ports);
dp->port_seq = seq_create();
fat_rwlock_init(&dp->upcall_rwlock);
@@ -984,7 +989,7 @@ static void
dp_netdev_free(struct dp_netdev *dp)
OVS_REQUIRES(dp_netdev_mutex)
{
- struct dp_netdev_port *port;
+ struct dp_netdev_port *port, *next;
shash_find_and_delete(&dp_netdevs, dp->name);
@@ -993,15 +998,14 @@ dp_netdev_free(struct dp_netdev *dp)
ovsthread_key_delete(dp->per_pmd_key);
ovs_mutex_lock(&dp->port_mutex);
- CMAP_FOR_EACH (port, node, &dp->ports) {
- /* PMD threads are destroyed here. do_del_port() cannot quiesce */
+ HMAP_FOR_EACH_SAFE (port, next, node, &dp->ports) {
do_del_port(dp, port);
}
ovs_mutex_unlock(&dp->port_mutex);
cmap_destroy(&dp->poll_threads);
seq_destroy(dp->port_seq);
- cmap_destroy(&dp->ports);
+ hmap_destroy(&dp->ports);
ovs_mutex_destroy(&dp->port_mutex);
/* Upcalls must be disabled at this point */
@@ -1222,7 +1226,7 @@ do_add_port(struct dp_netdev *dp, const char *devname,
const char *type,
return error;
}
- cmap_insert(&dp->ports, &port->node, hash_port_no(port_no));
+ hmap_insert(&dp->ports, &port->node, hash_port_no(port_no));
dp_netdev_add_port_to_pmds(dp, port);
seq_change(dp->port_seq);
@@ -1288,10 +1292,11 @@ is_valid_port_number(odp_port_t port_no)
static struct dp_netdev_port *
dp_netdev_lookup_port(const struct dp_netdev *dp, odp_port_t port_no)
+ OVS_REQUIRES(&dp->port_mutex)
{
struct dp_netdev_port *port;
- CMAP_FOR_EACH_WITH_HASH (port, node, hash_port_no(port_no), &dp->ports) {
+ HMAP_FOR_EACH_WITH_HASH (port, node, hash_port_no(port_no), &dp->ports) {
if (port->port_no == port_no) {
return port;
}
@@ -1302,6 +1307,7 @@ dp_netdev_lookup_port(const struct dp_netdev *dp,
odp_port_t port_no)
static int
get_port_by_number(struct dp_netdev *dp,
odp_port_t port_no, struct dp_netdev_port **portp)
+ OVS_REQUIRES(&dp->port_mutex)
{
if (!is_valid_port_number(port_no)) {
*portp = NULL;
@@ -1338,7 +1344,7 @@ get_port_by_name(struct dp_netdev *dp,
{
struct dp_netdev_port *port;
- CMAP_FOR_EACH (port, node, &dp->ports) {
+ HMAP_FOR_EACH (port, node, &dp->ports) {
if (!strcmp(netdev_get_name(port->netdev), devname)) {
*portp = port;
return 0;
@@ -1373,10 +1379,11 @@ get_n_pmd_threads_on_numa(struct dp_netdev *dp, int
numa_id)
* is on numa node 'numa_id'. */
static bool
has_pmd_port_for_numa(struct dp_netdev *dp, int numa_id)
+ OVS_REQUIRES(dp->port_mutex)
{
struct dp_netdev_port *port;
- CMAP_FOR_EACH (port, node, &dp->ports) {
+ HMAP_FOR_EACH (port, node, &dp->ports) {
if (netdev_is_pmd(port->netdev)
&& netdev_get_numa_id(port->netdev) == numa_id) {
return true;
@@ -1391,7 +1398,7 @@ static void
do_del_port(struct dp_netdev *dp, struct dp_netdev_port *port)
OVS_REQUIRES(dp->port_mutex)
{
- cmap_remove(&dp->ports, &port->node, hash_odp_port(port->port_no));
+ hmap_remove(&dp->ports, &port->node);
seq_change(dp->port_seq);
dp_netdev_del_port_from_all_pmds(dp, port);
@@ -1428,10 +1435,12 @@ dpif_netdev_port_query_by_number(const struct dpif
*dpif, odp_port_t port_no,
struct dp_netdev_port *port;
int error;
+ ovs_mutex_lock(&dp->port_mutex);
error = get_port_by_number(dp, port_no, &port);
if (!error && dpif_port) {
answer_port_query(port, dpif_port);
}
+ ovs_mutex_unlock(&dp->port_mutex);
return error;
}
@@ -1516,7 +1525,7 @@ dpif_netdev_flow_flush(struct dpif *dpif)
}
struct dp_netdev_port_state {
- struct cmap_position position;
+ struct hmap_position position;
char *name;
};
@@ -1533,10 +1542,11 @@ dpif_netdev_port_dump_next(const struct dpif *dpif,
void *state_,
{
struct dp_netdev_port_state *state = state_;
struct dp_netdev *dp = get_dp_netdev(dpif);
- struct cmap_node *node;
+ struct hmap_node *node;
int retval;
- node = cmap_next_position(&dp->ports, &state->position);
+ ovs_mutex_lock(&dp->port_mutex);
+ node = hmap_at_position(&dp->ports, &state->position);
if (node) {
struct dp_netdev_port *port;
@@ -1552,6 +1562,7 @@ dpif_netdev_port_dump_next(const struct dpif *dpif, void
*state_,
} else {
retval = EOF;
}
+ ovs_mutex_unlock(&dp->port_mutex);
return retval;
}
@@ -2407,8 +2418,8 @@ dpif_netdev_execute(struct dpif *dpif, struct
dpif_execute *execute)
dp_netdev_execute_actions(pmd, &pp, 1, false, execute->actions,
execute->actions_len);
if (pmd->core_id == NON_PMD_CORE_ID) {
- dp_netdev_pmd_unref(pmd);
ovs_mutex_unlock(&dp->non_pmd_mutex);
+ dp_netdev_pmd_unref(pmd);
}
return 0;
@@ -2449,14 +2460,17 @@ pmd_config_changed(const struct dp_netdev *dp, const
char *cmask)
{
struct dp_netdev_port *port;
- CMAP_FOR_EACH (port, node, &dp->ports) {
+ ovs_mutex_lock(&dp->port_mutex);
+ HMAP_FOR_EACH (port, node, &dp->ports) {
struct netdev *netdev = port->netdev;
int requested_n_rxq = netdev_requested_n_rxq(netdev);
if (netdev_is_pmd(netdev)
&& port->latest_requested_n_rxq != requested_n_rxq) {
+ ovs_mutex_unlock(&dp->port_mutex);
return true;
}
}
+ ovs_mutex_unlock(&dp->port_mutex);
if (dp->pmd_cmask != NULL && cmask != NULL) {
return strcmp(dp->pmd_cmask, cmask);
@@ -2476,7 +2490,8 @@ dpif_netdev_pmd_set(struct dpif *dpif, const char *cmask)
dp_netdev_destroy_all_pmds(dp);
- CMAP_FOR_EACH (port, node, &dp->ports) {
+ ovs_mutex_lock(&dp->port_mutex);
+ HMAP_FOR_EACH (port, node, &dp->ports) {
struct netdev *netdev = port->netdev;
int requested_n_rxq = netdev_requested_n_rxq(netdev);
if (netdev_is_pmd(port->netdev)
@@ -2498,6 +2513,7 @@ dpif_netdev_pmd_set(struct dpif *dpif, const char *cmask)
VLOG_ERR("Failed to set dpdk interface %s rx_queue to:"
" %u", netdev_get_name(port->netdev),
requested_n_rxq);
+ ovs_mutex_unlock(&dp->port_mutex);
return err;
}
port->latest_requested_n_rxq = requested_n_rxq;
@@ -2518,6 +2534,7 @@ dpif_netdev_pmd_set(struct dpif *dpif, const char *cmask)
dp_netdev_set_nonpmd(dp);
/* Restores all pmd threads. */
dp_netdev_reset_pmd_threads(dp);
+ ovs_mutex_unlock(&dp->port_mutex);
}
return 0;
@@ -2627,8 +2644,9 @@ dpif_netdev_run(struct dpif *dpif)
NON_PMD_CORE_ID);
uint64_t new_tnl_seq;
+ ovs_mutex_lock(&dp->port_mutex);
ovs_mutex_lock(&dp->non_pmd_mutex);
- CMAP_FOR_EACH (port, node, &dp->ports) {
+ HMAP_FOR_EACH (port, node, &dp->ports) {
if (!netdev_is_pmd(port->netdev)) {
int i;
@@ -2638,6 +2656,7 @@ dpif_netdev_run(struct dpif *dpif)
}
}
ovs_mutex_unlock(&dp->non_pmd_mutex);
+ ovs_mutex_unlock(&dp->port_mutex);
dp_netdev_pmd_unref(non_pmd);
tnl_neigh_cache_run();
@@ -2658,7 +2677,8 @@ dpif_netdev_wait(struct dpif *dpif)
struct dp_netdev *dp = get_dp_netdev(dpif);
ovs_mutex_lock(&dp_netdev_mutex);
- CMAP_FOR_EACH (port, node, &dp->ports) {
+ ovs_mutex_lock(&dp->port_mutex);
+ HMAP_FOR_EACH (port, node, &dp->ports) {
if (!netdev_is_pmd(port->netdev)) {
int i;
@@ -2667,6 +2687,7 @@ dpif_netdev_wait(struct dpif *dpif)
}
}
}
+ ovs_mutex_unlock(&dp->port_mutex);
ovs_mutex_unlock(&dp_netdev_mutex);
seq_wait(tnl_conf_seq, dp->last_tnl_conf_seq);
}
@@ -3296,12 +3317,13 @@ dp_netdev_set_pmds_on_numa(struct dp_netdev *dp, int
numa_id)
* new configuration. */
static void
dp_netdev_reset_pmd_threads(struct dp_netdev *dp)
+ OVS_REQUIRES(dp->port_mutex)
{
struct dp_netdev_port *port;
struct hmapx to_reload = HMAPX_INITIALIZER(&to_reload);
struct hmapx_node *node;
- CMAP_FOR_EACH (port, node, &dp->ports) {
+ HMAP_FOR_EACH (port, node, &dp->ports) {
if (netdev_is_pmd(port->netdev)) {
dp_netdev_add_port_to_pmds__(dp, port, &to_reload);
}
@@ -3857,7 +3879,6 @@ dp_netdev_clone_pkt_batch(struct dp_packet **dst_pkts,
static void
dp_execute_cb(void *aux_, struct dp_packet **packets, int cnt,
const struct nlattr *a, bool may_steal)
- OVS_NO_THREAD_SAFETY_ANALYSIS
{
struct dp_netdev_execute_aux *aux = aux_;
uint32_t *depth = recirc_depth_get();
@@ -4076,8 +4097,7 @@ static void
dpif_dummy_change_port_number(struct unixctl_conn *conn, int argc OVS_UNUSED,
const char *argv[], void *aux OVS_UNUSED)
{
- struct dp_netdev_port *old_port;
- struct dp_netdev_port *new_port;
+ struct dp_netdev_port *port;
struct dp_netdev *dp;
odp_port_t port_no;
@@ -4092,7 +4112,7 @@ dpif_dummy_change_port_number(struct unixctl_conn *conn,
int argc OVS_UNUSED,
ovs_mutex_unlock(&dp_netdev_mutex);
ovs_mutex_lock(&dp->port_mutex);
- if (get_port_by_name(dp, argv[2], &old_port)) {
+ if (get_port_by_name(dp, argv[2], &port)) {
unixctl_command_reply_error(conn, "unknown port");
goto exit;
}
@@ -4107,16 +4127,14 @@ dpif_dummy_change_port_number(struct unixctl_conn
*conn, int argc OVS_UNUSED,
goto exit;
}
- /* Remove old port. */
- cmap_remove(&dp->ports, &old_port->node, hash_port_no(old_port->port_no));
- dp_netdev_del_port_from_all_pmds(dp, old_port);
- ovsrcu_postpone(free, old_port);
+ /* Remove port. */
+ hmap_remove(&dp->ports, &port->node);
+ dp_netdev_del_port_from_all_pmds(dp, port);
- /* Insert new port (cmap semantics mean we cannot re-insert 'old_port'). */
- new_port = xmemdup(old_port, sizeof *old_port);
- new_port->port_no = port_no;
- cmap_insert(&dp->ports, &new_port->node, hash_port_no(port_no));
- dp_netdev_add_port_to_pmds(dp, new_port);
+ /* Reinsert with new port number. */
+ port->port_no = port_no;
+ hmap_insert(&dp->ports, &port->node, hash_port_no(port_no));
+ dp_netdev_add_port_to_pmds(dp, port);
seq_change(dp->port_seq);
unixctl_command_reply(conn, NULL);
--
2.1.4
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
