[ovs-dev] [cmap 2/2] dpif-netdev: Use cmap for ports.
Jarno Rajahalme
jrajahalme at nicira.com
Fri May 2 18:42:46 UTC 2014
On May 1, 2014, at 5:14 PM, Ben Pfaff <blp at nicira.com> wrote:
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
> lib/dpif-netdev.c | 153 +++++++++++++++++++++++++----------------------------
> 1 file changed, 73 insertions(+), 80 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 55712dd..2dddad5 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -32,6 +32,7 @@
> #include <unistd.h>
>
> #include "classifier.h"
> +#include "cmap.h"
> #include "csum.h"
> #include "dpif.h"
> #include "dpif-provider.h"
> @@ -120,7 +121,7 @@ struct dp_netdev_queue {
> * Acquisition order is, from outermost to innermost:
> *
> * dp_netdev_mutex (global)
> - * port_rwlock
> + * port_mutex
> * flow_mutex
> * cls.rwlock
> * queue_rwlock
> @@ -159,10 +160,9 @@ struct dp_netdev {
>
> /* Ports.
> *
> - * Any lookup into 'ports' or any access to the dp_netdev_ports found
> - * through 'ports' requires taking 'port_rwlock'. */
> - struct ovs_rwlock port_rwlock;
> - struct hmap ports OVS_GUARDED;
> + * Protected by RCU. Take the mutex to add or remove ports. */
> + struct ovs_mutex port_mutex;
> + struct cmap ports;
> struct seq *port_seq; /* Incremented whenever a port changes. */
>
> /* Forwarding threads. */
> @@ -173,8 +173,7 @@ struct dp_netdev {
> };
>
> static struct dp_netdev_port *dp_netdev_lookup_port(const struct dp_netdev *dp,
> - odp_port_t)
> - OVS_REQ_RDLOCK(dp->port_rwlock);
> + odp_port_t);
>
> enum dp_stat_type {
> DP_STAT_HIT, /* Packets that matched in the flow table. */
> @@ -194,7 +193,7 @@ struct dp_netdev_stats {
>
> /* A port in a netdev-based datapath. */
> struct dp_netdev_port {
> - struct hmap_node node; /* Node in dp_netdev's 'ports'. */
> + struct cmap_node node; /* Node in dp_netdev's 'ports'. */
> odp_port_t port_no;
> struct netdev *netdev;
> struct netdev_saved_flags *sf;
> @@ -333,19 +332,17 @@ struct dpif_netdev {
> };
>
> static int get_port_by_number(struct dp_netdev *dp, odp_port_t port_no,
> - struct dp_netdev_port **portp)
> - OVS_REQ_RDLOCK(dp->port_rwlock);
> + struct dp_netdev_port **portp);
> static int get_port_by_name(struct dp_netdev *dp, const char *devname,
> - struct dp_netdev_port **portp)
> - OVS_REQ_RDLOCK(dp->port_rwlock);
> + struct dp_netdev_port **portp);
> static void dp_netdev_free(struct dp_netdev *)
> OVS_REQUIRES(dp_netdev_mutex);
> static void dp_netdev_flow_flush(struct dp_netdev *);
> static int do_add_port(struct dp_netdev *dp, const char *devname,
> const char *type, odp_port_t port_no)
> - OVS_REQ_WRLOCK(dp->port_rwlock);
> + OVS_REQ_WRLOCK(dp->port_mutex);
OVS_REQUIRES
> static int do_del_port(struct dp_netdev *dp, odp_port_t port_no)
> - OVS_REQ_WRLOCK(dp->port_rwlock);
> + OVS_REQ_WRLOCK(dp->port_mutex);
OVS_REQUIRES
> static void dp_netdev_destroy_all_queues(struct dp_netdev *dp)
> OVS_REQ_WRLOCK(dp->queue_rwlock);
> static int dpif_netdev_open(const struct dpif_class *, const char *name,
> @@ -426,7 +423,7 @@ create_dpif_netdev(struct dp_netdev *dp)
> * Return ODPP_NONE on failure. */
> static odp_port_t
> choose_port(struct dp_netdev *dp, const char *name)
> - OVS_REQ_RDLOCK(dp->port_rwlock)
> + OVS_REQ_RDLOCK(dp->port_mutex)
OVS_REQUIRES
> {
> uint32_t port_no;
>
> @@ -488,14 +485,14 @@ create_dp_netdev(const char *name, const struct dpif_class *class,
>
> ovsthread_stats_init(&dp->stats);
>
> - ovs_rwlock_init(&dp->port_rwlock);
> - hmap_init(&dp->ports);
> + ovs_mutex_init(&dp->port_mutex);
> + cmap_init(&dp->ports);
> dp->port_seq = seq_create();
> latch_init(&dp->exit_latch);
>
> - ovs_rwlock_wrlock(&dp->port_rwlock);
> + ovs_mutex_lock(&dp->port_mutex);
> error = do_add_port(dp, name, "internal", ODPP_LOCAL);
> - ovs_rwlock_unlock(&dp->port_rwlock);
> + ovs_mutex_unlock(&dp->port_mutex);
> if (error) {
> dp_netdev_free(dp);
> return error;
> @@ -554,8 +551,9 @@ static void
> dp_netdev_free(struct dp_netdev *dp)
> OVS_REQUIRES(dp_netdev_mutex)
> {
> - struct dp_netdev_port *port, *next;
> + struct dp_netdev_port *port;
> struct dp_netdev_stats *bucket;
> + struct cmap_cursor cursor;
> int i;
>
> shash_find_and_delete(&dp_netdevs, dp->name);
> @@ -564,11 +562,11 @@ dp_netdev_free(struct dp_netdev *dp)
> free(dp->pmd_threads);
>
> dp_netdev_flow_flush(dp);
> - ovs_rwlock_wrlock(&dp->port_rwlock);
> - HMAP_FOR_EACH_SAFE (port, next, node, &dp->ports) {
> + ovs_mutex_lock(&dp->port_mutex);
> + CMAP_FOR_EACH (port, node, &cursor, &dp->ports) {
> do_del_port(dp, port->port_no);
> }
> - ovs_rwlock_unlock(&dp->port_rwlock);
> + ovs_mutex_unlock(&dp->port_mutex);
>
> OVSTHREAD_STATS_FOR_EACH_BUCKET (bucket, i, &dp->stats) {
> ovs_mutex_destroy(&bucket->mutex);
> @@ -586,7 +584,7 @@ dp_netdev_free(struct dp_netdev *dp)
> hmap_destroy(&dp->flow_table);
> ovs_mutex_destroy(&dp->flow_mutex);
> seq_destroy(dp->port_seq);
> - hmap_destroy(&dp->ports);
> + cmap_destroy(&dp->ports);
> latch_destroy(&dp->exit_latch);
> free(CONST_CAST(char *, dp->name));
> free(dp);
> @@ -671,7 +669,7 @@ dp_netdev_reload_pmd_threads(struct dp_netdev *dp)
> static int
> do_add_port(struct dp_netdev *dp, const char *devname, const char *type,
> odp_port_t port_no)
> - OVS_REQ_WRLOCK(dp->port_rwlock)
> + OVS_REQ_WRLOCK(dp->port_mutex)
OVS_REQUIRES
> {
> struct netdev_saved_flags *sf;
> struct dp_netdev_port *port;
> @@ -733,7 +731,7 @@ do_add_port(struct dp_netdev *dp, const char *devname, const char *type,
> }
> ovs_refcount_init(&port->ref_cnt);
>
> - hmap_insert(&dp->ports, &port->node, hash_int(odp_to_u32(port_no), 0));
> + cmap_insert(&dp->ports, &port->node, hash_int(odp_to_u32(port_no), 0));
> seq_change(dp->port_seq);
>
> return 0;
> @@ -749,7 +747,7 @@ dpif_netdev_port_add(struct dpif *dpif, struct netdev *netdev,
> odp_port_t port_no;
> int error;
>
> - ovs_rwlock_wrlock(&dp->port_rwlock);
> + ovs_mutex_lock(&dp->port_mutex);
> dpif_port = netdev_vport_get_dpif_port(netdev, namebuf, sizeof namebuf);
> if (*port_nop != ODPP_NONE) {
> port_no = *port_nop;
> @@ -762,7 +760,7 @@ dpif_netdev_port_add(struct dpif *dpif, struct netdev *netdev,
> *port_nop = port_no;
> error = do_add_port(dp, dpif_port, netdev_get_type(netdev), port_no);
> }
> - ovs_rwlock_unlock(&dp->port_rwlock);
> + ovs_mutex_unlock(&dp->port_mutex);
>
> return error;
> }
> @@ -773,9 +771,9 @@ dpif_netdev_port_del(struct dpif *dpif, odp_port_t port_no)
> struct dp_netdev *dp = get_dp_netdev(dpif);
> int error;
>
> - ovs_rwlock_wrlock(&dp->port_rwlock);
> + ovs_mutex_lock(&dp->port_mutex);
> error = port_no == ODPP_LOCAL ? EINVAL : do_del_port(dp, port_no);
> - ovs_rwlock_unlock(&dp->port_rwlock);
> + ovs_mutex_unlock(&dp->port_mutex);
>
> return error;
> }
> @@ -788,11 +786,10 @@ 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_REQ_RDLOCK(dp->port_rwlock)
> {
> struct dp_netdev_port *port;
>
> - HMAP_FOR_EACH_IN_BUCKET (port, node, hash_int(odp_to_u32(port_no), 0),
> + CMAP_FOR_EACH_WITH_HASH (port, node, hash_int(odp_to_u32(port_no), 0),
> &dp->ports) {
> if (port->port_no == port_no) {
> return port;
> @@ -804,7 +801,6 @@ 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_REQ_RDLOCK(dp->port_rwlock)
> {
> if (!is_valid_port_number(port_no)) {
> *portp = NULL;
> @@ -824,30 +820,36 @@ port_ref(struct dp_netdev_port *port)
> }
>
> static void
> -port_unref(struct dp_netdev_port *port)
> +port_destroy__(struct dp_netdev_port *port)
> {
> - if (port && ovs_refcount_unref(&port->ref_cnt) == 1) {
> - int i;
> + int i;
>
> - netdev_close(port->netdev);
> - netdev_restore_flags(port->sf);
> + netdev_close(port->netdev);
> + netdev_restore_flags(port->sf);
>
> - for (i = 0; i < netdev_n_rxq(port->netdev); i++) {
> - netdev_rxq_close(port->rxq[i]);
> - }
> - free(port->type);
> - free(port);
> + for (i = 0; i < netdev_n_rxq(port->netdev); i++) {
> + netdev_rxq_close(port->rxq[i]);
> + }
> + free(port->type);
> + free(port);
> +}
> +
> +static void
> +port_unref(struct dp_netdev_port *port)
> +{
> + if (port && ovs_refcount_unref(&port->ref_cnt) == 1) {
> + ovsrcu_postpone(port_destroy__, port);
> }
> }
>
> static int
> get_port_by_name(struct dp_netdev *dp,
> const char *devname, struct dp_netdev_port **portp)
> - OVS_REQ_RDLOCK(dp->port_rwlock)
> {
> struct dp_netdev_port *port;
> + struct cmap_cursor cursor;
>
> - HMAP_FOR_EACH (port, node, &dp->ports) {
> + CMAP_FOR_EACH (port, node, &cursor, &dp->ports) {
Would it be possible for the iteration to miss a port if the camp is being modified at the same time with iteration? E.g., a node is moved from a “later” bucket to an “earlier” one, so that the node is not seen by the iteration at all?
(It is also possible that all modifications to ports map come from the same (main) thread)
> if (!strcmp(netdev_get_name(port->netdev), devname)) {
> *portp = port;
> return 0;
> @@ -858,7 +860,7 @@ get_port_by_name(struct dp_netdev *dp,
>
> static int
> do_del_port(struct dp_netdev *dp, odp_port_t port_no)
> - OVS_REQ_WRLOCK(dp->port_rwlock)
> + OVS_REQUIRES(dp->port_mutex)
> {
> struct dp_netdev_port *port;
> int error;
> @@ -868,7 +870,7 @@ do_del_port(struct dp_netdev *dp, odp_port_t port_no)
> return error;
> }
>
> - hmap_remove(&dp->ports, &port->node);
> + cmap_remove(&dp->ports, &port->node);
> seq_change(dp->port_seq);
> if (netdev_is_pmd(port->netdev)) {
> dp_netdev_reload_pmd_threads(dp);
> @@ -895,12 +897,10 @@ dpif_netdev_port_query_by_number(const struct dpif *dpif, odp_port_t port_no,
> struct dp_netdev_port *port;
> int error;
>
> - ovs_rwlock_rdlock(&dp->port_rwlock);
> error = get_port_by_number(dp, port_no, &port);
> if (!error && dpif_port) {
> answer_port_query(port, dpif_port);
> }
> - ovs_rwlock_unlock(&dp->port_rwlock);
>
> return error;
> }
> @@ -913,12 +913,10 @@ dpif_netdev_port_query_by_name(const struct dpif *dpif, const char *devname,
> struct dp_netdev_port *port;
> int error;
>
> - ovs_rwlock_rdlock(&dp->port_rwlock);
> error = get_port_by_name(dp, devname, &port);
> if (!error && dpif_port) {
> answer_port_query(port, dpif_port);
> }
> - ovs_rwlock_unlock(&dp->port_rwlock);
>
> return error;
> }
> @@ -978,8 +976,7 @@ dpif_netdev_flow_flush(struct dpif *dpif)
> }
>
> struct dp_netdev_port_state {
> - uint32_t bucket;
> - uint32_t offset;
> + struct cmap_position position;
> char *name;
> };
>
> @@ -996,11 +993,10 @@ 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 hmap_node *node;
> + struct cmap_node *node;
> int retval;
>
> - ovs_rwlock_rdlock(&dp->port_rwlock);
> - node = hmap_at_position(&dp->ports, &state->bucket, &state->offset);
> + node = cmap_next_position(&dp->ports, &state->position);
> if (node) {
> struct dp_netdev_port *port;
>
> @@ -1016,7 +1012,6 @@ dpif_netdev_port_dump_next(const struct dpif *dpif, void *state_,
> } else {
> retval = EOF;
> }
> - ovs_rwlock_unlock(&dp->port_rwlock);
>
> return retval;
> }
> @@ -1541,10 +1536,8 @@ dpif_netdev_execute(struct dpif *dpif, struct dpif_execute *execute)
> miniflow_initialize(&key.flow, key.buf);
> miniflow_extract(execute->packet, md, &key.flow);
>
> - ovs_rwlock_rdlock(&dp->port_rwlock);
What was the lock protecting in this case?
> dp_netdev_execute_actions(dp, &key.flow, execute->packet, false, md,
> execute->actions, execute->actions_len);
> - ovs_rwlock_unlock(&dp->port_rwlock);
>
> return 0;
> }
> @@ -1783,10 +1776,9 @@ dpif_netdev_run(struct dpif *dpif)
> {
> struct dp_netdev_port *port;
> struct dp_netdev *dp = get_dp_netdev(dpif);
> + struct cmap_cursor cursor;
>
> - ovs_rwlock_rdlock(&dp->port_rwlock);
> -
> - HMAP_FOR_EACH (port, node, &dp->ports) {
> + CMAP_FOR_EACH (port, node, &cursor, &dp->ports) {
> if (!netdev_is_pmd(port->netdev)) {
> int i;
>
> @@ -1795,8 +1787,6 @@ dpif_netdev_run(struct dpif *dpif)
> }
> }
> }
> -
> - ovs_rwlock_unlock(&dp->port_rwlock);
> }
>
> static void
> @@ -1804,10 +1794,9 @@ dpif_netdev_wait(struct dpif *dpif)
> {
> struct dp_netdev_port *port;
> struct dp_netdev *dp = get_dp_netdev(dpif);
> + struct cmap_cursor cursor;
>
> - ovs_rwlock_rdlock(&dp->port_rwlock);
> -
> - HMAP_FOR_EACH (port, node, &dp->ports) {
> + CMAP_FOR_EACH (port, node, &cursor, &dp->ports) {
> if (!netdev_is_pmd(port->netdev)) {
> int i;
>
> @@ -1816,7 +1805,6 @@ dpif_netdev_wait(struct dpif *dpif)
> }
> }
> }
> - ovs_rwlock_unlock(&dp->port_rwlock);
> }
>
> struct rxq_poll {
> @@ -1831,12 +1819,12 @@ pmd_load_queues(struct pmd_thread *f,
> struct dp_netdev *dp = f->dp;
> struct rxq_poll *poll_list = *ppoll_list;
> struct dp_netdev_port *port;
> + struct cmap_cursor cursor;
> int id = f->id;
> int index;
> int i;
>
> /* Simple scheduler for netdev rx polling. */
> - ovs_rwlock_rdlock(&dp->port_rwlock);
> for (i = 0; i < poll_cnt; i++) {
> port_unref(poll_list[i].port);
> }
> @@ -1844,7 +1832,7 @@ pmd_load_queues(struct pmd_thread *f,
> poll_cnt = 0;
> index = 0;
>
> - HMAP_FOR_EACH (port, node, &f->dp->ports) {
> + CMAP_FOR_EACH (port, node, &cursor, &f->dp->ports) {
> if (netdev_is_pmd(port->netdev)) {
> int i;
>
> @@ -1862,7 +1850,6 @@ pmd_load_queues(struct pmd_thread *f,
> }
> }
>
> - ovs_rwlock_unlock(&dp->port_rwlock);
> *ppoll_list = poll_list;
> return poll_cnt;
> }
> @@ -2008,7 +1995,6 @@ dp_netdev_count_packet(struct dp_netdev *dp, enum dp_stat_type type)
> static void
> dp_netdev_input(struct dp_netdev *dp, struct ofpbuf *packet,
> struct pkt_metadata *md)
> - OVS_REQ_RDLOCK(dp->port_rwlock)
> {
> struct dp_netdev_flow *netdev_flow;
> struct {
> @@ -2046,7 +2032,6 @@ dp_netdev_input(struct dp_netdev *dp, struct ofpbuf *packet,
> static void
> dp_netdev_port_input(struct dp_netdev *dp, struct ofpbuf *packet,
> struct pkt_metadata *md)
> - OVS_REQ_RDLOCK(dp->port_rwlock)
> {
> uint32_t *recirc_depth = recirc_depth_get();
>
> @@ -2260,7 +2245,8 @@ 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 *port;
> + struct dp_netdev_port *old_port;
> + struct dp_netdev_port *new_port;
> struct dp_netdev *dp;
> odp_port_t port_no;
>
> @@ -2274,8 +2260,8 @@ dpif_dummy_change_port_number(struct unixctl_conn *conn, int argc OVS_UNUSED,
> ovs_refcount_ref(&dp->ref_cnt);
> ovs_mutex_unlock(&dp_netdev_mutex);
>
> - ovs_rwlock_wrlock(&dp->port_rwlock);
> - if (get_port_by_name(dp, argv[2], &port)) {
> + ovs_mutex_lock(&dp->port_mutex);
> + if (get_port_by_name(dp, argv[2], &old_port)) {
> unixctl_command_reply_error(conn, "unknown port");
> goto exit;
> }
> @@ -2289,14 +2275,21 @@ dpif_dummy_change_port_number(struct unixctl_conn *conn, int argc OVS_UNUSED,
> unixctl_command_reply_error(conn, "port number already in use");
> goto exit;
> }
> - hmap_remove(&dp->ports, &port->node);
> - port->port_no = port_no;
> - hmap_insert(&dp->ports, &port->node, hash_int(odp_to_u32(port_no), 0));
> +
> + /* Remove old port. */
> + cmap_remove(&dp->ports, &old_port->node);
> + ovsrcu_postpone(free, old_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_int(odp_to_u32(port_no), 0));
> +
> seq_change(dp->port_seq);
> unixctl_command_reply(conn, NULL);
>
> exit:
> - ovs_rwlock_unlock(&dp->port_rwlock);
> + ovs_mutex_unlock(&dp->port_mutex);
> dp_netdev_unref(dp);
> }
>
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
More information about the dev
mailing list