On 20.04.2016 01:28, diproiettod at vmware.com (Daniele Di Proietto) wrote:
> 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 <diproiettod at 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);
OVS_REQUIRES(dp->port_mutex);
here and 2 times more below.
>
> 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)
here
> {
> 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)
and here.
> {
> 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);
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
