On 20/04/2016 07:21, "Ilya Maximets" <[email protected]> wrote:
>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.
I've changed them, thanks. I think the analyzer accepts both (a pointer
or the
object itself), but I prefer the syntax you suggested.
>
>>
>> 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
[email protected]
http://openvswitch.org/mailman/listinfo/dev
