From e9985d6aa45f1081089dedc5aed829b79e149277 Mon Sep 17 00:00:00 2001 From: Daniele Di Proietto Date: Mon, 4 Apr 2016 11:15:12 -0700 Subject: [PATCH] dpif-netdev: Use hmap for ports. 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 performance impact, as readers in the fast path use a thread local cache. Signed-off-by: Daniele Di Proietto Tested-by: Ilya Maximets Acked-by: Ilya Maximets --- lib/dpif-netdev.c | 111 ++++++++++++++++++++++++++++------------------ 1 file changed, 67 insertions(+), 44 deletions(-) diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index a768117ce..0fbb4038e 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, @@ -504,14 +508,17 @@ static void dp_netdev_configure_pmd(struct dp_netdev_pmd_thread *pmd, struct dp_netdev *dp, unsigned core_id, int numa_id); static void dp_netdev_destroy_pmd(struct dp_netdev_pmd_thread *pmd); -static void dp_netdev_set_nonpmd(struct dp_netdev *dp); +static void dp_netdev_set_nonpmd(struct dp_netdev *dp) + OVS_REQUIRES(dp->port_mutex); + static struct dp_netdev_pmd_thread *dp_netdev_get_pmd(struct dp_netdev *dp, unsigned core_id); static struct dp_netdev_pmd_thread * dp_netdev_pmd_get_next(struct dp_netdev *dp, struct cmap_position *pos); static void dp_netdev_destroy_all_pmds(struct dp_netdev *dp); static void dp_netdev_del_pmds_on_numa(struct dp_netdev *dp, int numa_id); -static void dp_netdev_set_pmds_on_numa(struct dp_netdev *dp, int numa_id); +static void dp_netdev_set_pmds_on_numa(struct dp_netdev *dp, int numa_id) + OVS_REQUIRES(dp->port_mutex); static void dp_netdev_pmd_clear_ports(struct dp_netdev_pmd_thread *pmd); static void dp_netdev_del_port_from_all_pmds(struct dp_netdev *dp, struct dp_netdev_port *port); @@ -524,7 +531,8 @@ static void dp_netdev_add_rxq_to_pmd(struct dp_netdev_pmd_thread *pmd, 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); @@ -915,7 +923,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); @@ -928,9 +936,9 @@ create_dp_netdev(const char *name, const struct dpif_class *class, ovs_mutex_init_recursive(&dp->non_pmd_mutex); ovsthread_key_create(&dp->per_pmd_key, NULL); + ovs_mutex_lock(&dp->port_mutex); dp_netdev_set_nonpmd(dp); - ovs_mutex_lock(&dp->port_mutex); error = do_add_port(dp, name, "internal", ODPP_LOCAL); ovs_mutex_unlock(&dp->port_mutex); if (error) { @@ -986,7 +994,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); @@ -995,15 +1003,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 */ @@ -1233,7 +1240,7 @@ do_add_port(struct dp_netdev *dp, const char *devname, const char *type, dp_netdev_add_port_to_pmds(dp, port); - cmap_insert(&dp->ports, &port->node, hash_port_no(port_no)); + hmap_insert(&dp->ports, &port->node, hash_port_no(port_no)); seq_change(dp->port_seq); return 0; @@ -1297,10 +1304,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; } @@ -1311,6 +1319,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; @@ -1347,7 +1356,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; @@ -1382,10 +1391,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; @@ -1400,7 +1410,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); @@ -1437,10 +1447,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; } @@ -1523,7 +1535,7 @@ dpif_netdev_flow_flush(struct dpif *dpif) } struct dp_netdev_port_state { - struct cmap_position position; + struct hmap_position position; char *name; }; @@ -1540,10 +1552,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; @@ -1559,6 +1572,7 @@ dpif_netdev_port_dump_next(const struct dpif *dpif, void *state_, } else { retval = EOF; } + ovs_mutex_unlock(&dp->port_mutex); return retval; } @@ -2425,8 +2439,8 @@ dpif_netdev_execute(struct dpif *dpif, struct dpif_execute *execute) 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; @@ -2467,14 +2481,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); @@ -2494,7 +2511,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) @@ -2516,6 +2534,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; @@ -2536,6 +2555,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; @@ -2646,8 +2666,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; @@ -2657,6 +2678,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(); @@ -2677,7 +2699,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; @@ -2686,6 +2709,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); } @@ -2869,6 +2893,7 @@ dp_netdev_get_pmd(struct dp_netdev *dp, unsigned core_id) /* Sets the 'struct dp_netdev_pmd_thread' for non-pmd threads. */ static void dp_netdev_set_nonpmd(struct dp_netdev *dp) + OVS_REQUIRES(dp->port_mutex) { struct dp_netdev_pmd_thread *non_pmd; struct dp_netdev_port *port; @@ -2876,7 +2901,7 @@ dp_netdev_set_nonpmd(struct dp_netdev *dp) non_pmd = xzalloc(sizeof *non_pmd); dp_netdev_configure_pmd(non_pmd, dp, NON_PMD_CORE_ID, OVS_NUMA_UNSPEC); - CMAP_FOR_EACH (port, node, &dp->ports) { + HMAP_FOR_EACH (port, node, &dp->ports) { dp_netdev_add_port_tx_to_pmd(non_pmd, port); } @@ -3299,6 +3324,7 @@ dp_netdev_add_port_to_pmds(struct dp_netdev *dp, struct dp_netdev_port *port) * The function takes care of filling the threads tx port cache. */ static void dp_netdev_set_pmds_on_numa(struct dp_netdev *dp, int numa_id) + OVS_REQUIRES(dp->port_mutex) { int n_pmds; @@ -3333,7 +3359,7 @@ dp_netdev_set_pmds_on_numa(struct dp_netdev *dp, int numa_id) dp_netdev_configure_pmd(pmd, dp, core_id, numa_id); - CMAP_FOR_EACH (port, node, &dp->ports) { + HMAP_FOR_EACH (port, node, &dp->ports) { dp_netdev_add_port_tx_to_pmd(pmd, port); } @@ -3348,13 +3374,14 @@ 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 hmapx to_reload = HMAPX_INITIALIZER(&to_reload); struct dp_netdev_pmd_thread *pmd; struct dp_netdev_port *port; struct hmapx_node *node; - CMAP_FOR_EACH (port, node, &dp->ports) { + HMAP_FOR_EACH (port, node, &dp->ports) { if (netdev_is_pmd(port->netdev)) { int numa_id = netdev_get_numa_id(port->netdev); @@ -3939,7 +3966,6 @@ dp_execute_userspace_action(struct dp_netdev_pmd_thread *pmd, static void dp_execute_cb(void *aux_, struct dp_packet_batch *packets_, 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(); @@ -4146,8 +4172,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; @@ -4162,7 +4187,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; } @@ -4177,16 +4202,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.20.1