From patchwork Sat Dec 3 02:14:10 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniele Di Proietto X-Patchwork-Id: 702219 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3tVvpN2CJ4z9t1d for ; Sat, 3 Dec 2016 13:18:52 +1100 (AEDT) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id DF77ABC7; Sat, 3 Dec 2016 02:14:53 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id 70EA9A48 for ; Sat, 3 Dec 2016 02:14:47 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from EX13-EDG-OU-002.vmware.com (ex13-edg-ou-002.vmware.com [208.91.0.190]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 3DA55171 for ; Sat, 3 Dec 2016 02:14:46 +0000 (UTC) Received: from sc9-mailhost3.vmware.com (10.113.161.73) by EX13-EDG-OU-002.vmware.com (10.113.208.156) with Microsoft SMTP Server id 15.0.1156.6; Fri, 2 Dec 2016 18:14:03 -0800 Received: from sc9-mailhost1.vmware.com (htb-1n-eng-dhcp161.eng.vmware.com [10.33.74.161]) by sc9-mailhost3.vmware.com (Postfix) with ESMTP id BAA8E40647; Fri, 2 Dec 2016 18:14:42 -0800 (PST) From: Daniele Di Proietto To: Date: Fri, 2 Dec 2016 18:14:10 -0800 Message-ID: <20161203021418.103114-12-diproiettod@vmware.com> X-Mailer: git-send-email 2.10.2 In-Reply-To: <20161203021418.103114-1-diproiettod@vmware.com> References: <20161203021418.103114-1-diproiettod@vmware.com> MIME-Version: 1.0 Received-SPF: None (EX13-EDG-OU-002.vmware.com: diproiettod@vmware.com does not designate permitted sender hosts) X-Spam-Status: No, score=-4.8 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Cc: Ilya Maximets , Daniele Di Proietto Subject: [ovs-dev] [PATCH v2 11/19] dpctl: Avoid making assumptions on pmd threads. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org Currently dpctl depends on ovs-numa module to delete and create flows on different pmd threads for pmd devices. The next commits will move away the pmd threads state from ovs-numa to dpif-netdev, so the ovs-numa interface will not be supported. Also, the assignment between ports and thread is an implementation detail of dpif-netdev, dpctl shouldn't know anything about it. This commit changes the dpif_flow_put() and dpif_flow_del() calls to iterate over all the pmd threads, if pmd_id is PMD_ID_NULL. A simple test is added. Signed-off-by: Daniele Di Proietto --- lib/dpctl.c | 107 ++++---------------------------- lib/dpif-netdev.c | 180 +++++++++++++++++++++++++++++++++++++----------------- lib/dpif.c | 6 +- lib/dpif.h | 12 +++- tests/pmd.at | 44 +++++++++++++ 5 files changed, 194 insertions(+), 155 deletions(-) diff --git a/lib/dpctl.c b/lib/dpctl.c index edccb7f..ae789ea 100644 --- a/lib/dpctl.c +++ b/lib/dpctl.c @@ -40,7 +40,6 @@ #include "netlink.h" #include "odp-util.h" #include "openvswitch/ofpbuf.h" -#include "ovs-numa.h" #include "packets.h" #include "openvswitch/shash.h" #include "simap.h" @@ -876,43 +875,12 @@ out_freefilter: return error; } -/* Extracts the in_port from the parsed keys, and returns the reference - * to the 'struct netdev *' of the dpif port. On error, returns NULL. - * Users must call 'netdev_close()' after finish using the returned - * reference. */ -static struct netdev * -get_in_port_netdev_from_key(struct dpif *dpif, const struct ofpbuf *key) -{ - const struct nlattr *in_port_nla; - struct netdev *dev = NULL; - - in_port_nla = nl_attr_find(key, 0, OVS_KEY_ATTR_IN_PORT); - if (in_port_nla) { - struct dpif_port dpif_port; - odp_port_t port_no; - int error; - - port_no = ODP_PORT_C(nl_attr_get_u32(in_port_nla)); - error = dpif_port_query_by_number(dpif, port_no, &dpif_port); - if (error) { - goto out; - } - - netdev_open(dpif_port.name, dpif_port.type, &dev); - dpif_port_destroy(&dpif_port); - } - -out: - return dev; -} - static int dpctl_put_flow(int argc, const char *argv[], enum dpif_flow_put_flags flags, struct dpctl_params *dpctl_p) { const char *key_s = argv[argc - 2]; const char *actions_s = argv[argc - 1]; - struct netdev *in_port_netdev = NULL; struct dpif_flow_stats stats; struct dpif_port dpif_port; struct dpif_port_dump port_dump; @@ -968,39 +936,15 @@ dpctl_put_flow(int argc, const char *argv[], enum dpif_flow_put_flags flags, goto out_freeactions; } - /* For DPDK interface, applies the operation to all pmd threads - * on the same numa node. */ - in_port_netdev = get_in_port_netdev_from_key(dpif, &key); - if (in_port_netdev && netdev_is_pmd(in_port_netdev)) { - int numa_id; - - numa_id = netdev_get_numa_id(in_port_netdev); - if (ovs_numa_numa_id_is_valid(numa_id)) { - struct ovs_numa_dump *dump = ovs_numa_dump_cores_on_numa(numa_id); - struct ovs_numa_info *iter; - - FOR_EACH_CORE_ON_NUMA (iter, dump) { - if (ovs_numa_core_is_pinned(iter->core_id)) { - error = dpif_flow_put(dpif, flags, - key.data, key.size, - mask.size == 0 ? NULL : mask.data, - mask.size, actions.data, - actions.size, ufid_present ? &ufid : NULL, - iter->core_id, dpctl_p->print_statistics ? &stats : NULL); - } - } - ovs_numa_dump_destroy(dump); - } else { - error = EINVAL; - } - } else { - error = dpif_flow_put(dpif, flags, - key.data, key.size, - mask.size == 0 ? NULL : mask.data, - mask.size, actions.data, - actions.size, ufid_present ? &ufid : NULL, - PMD_ID_NULL, dpctl_p->print_statistics ? &stats : NULL); - } + /* The flow will be added on all pmds currently in the datapath. */ + error = dpif_flow_put(dpif, flags, + key.data, key.size, + mask.size == 0 ? NULL : mask.data, + mask.size, actions.data, + actions.size, ufid_present ? &ufid : NULL, + PMD_ID_NULL, + dpctl_p->print_statistics ? &stats : NULL); + if (error) { dpctl_error(dpctl_p, error, "updating flow table"); goto out_freeactions; @@ -1021,7 +965,6 @@ out_freekeymask: ofpbuf_uninit(&mask); ofpbuf_uninit(&key); dpif_close(dpif); - netdev_close(in_port_netdev); return error; } @@ -1110,7 +1053,6 @@ static int dpctl_del_flow(int argc, const char *argv[], struct dpctl_params *dpctl_p) { const char *key_s = argv[argc - 1]; - struct netdev *in_port_netdev = NULL; struct dpif_flow_stats stats; struct dpif_port dpif_port; struct dpif_port_dump port_dump; @@ -1158,33 +1100,11 @@ dpctl_del_flow(int argc, const char *argv[], struct dpctl_params *dpctl_p) goto out; } - /* For DPDK interface, applies the operation to all pmd threads - * on the same numa node. */ - in_port_netdev = get_in_port_netdev_from_key(dpif, &key); - if (in_port_netdev && netdev_is_pmd(in_port_netdev)) { - int numa_id; - - numa_id = netdev_get_numa_id(in_port_netdev); - if (ovs_numa_numa_id_is_valid(numa_id)) { - struct ovs_numa_dump *dump = ovs_numa_dump_cores_on_numa(numa_id); - struct ovs_numa_info *iter; + /* The flow will be deleted from all pmds currently in the datapath. */ + error = dpif_flow_del(dpif, key.data, key.size, + ufid_present ? &ufid : NULL, PMD_ID_NULL, + dpctl_p->print_statistics ? &stats : NULL); - FOR_EACH_CORE_ON_NUMA (iter, dump) { - if (ovs_numa_core_is_pinned(iter->core_id)) { - error = dpif_flow_del(dpif, key.data, - key.size, ufid_present ? &ufid : NULL, - iter->core_id, dpctl_p->print_statistics ? &stats : NULL); - } - } - ovs_numa_dump_destroy(dump); - } else { - error = EINVAL; - } - } else { - error = dpif_flow_del(dpif, key.data, key.size, - ufid_present ? &ufid : NULL, PMD_ID_NULL, - dpctl_p->print_statistics ? &stats : NULL); - } if (error) { dpctl_error(dpctl_p, error, "deleting flow"); if (error == ENOENT && !ufid_present) { @@ -1212,7 +1132,6 @@ out: ofpbuf_uninit(&key); simap_destroy(&port_names); dpif_close(dpif); - netdev_close(in_port_netdev); return error; } diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index e1366a6..626d921 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -2289,54 +2289,26 @@ dp_netdev_flow_add(struct dp_netdev_pmd_thread *pmd, } static int -dpif_netdev_flow_put(struct dpif *dpif, const struct dpif_flow_put *put) +flow_put_on_pmd(struct dp_netdev_pmd_thread *pmd, + struct netdev_flow_key *key, + struct match *match, + ovs_u128 *ufid, + const struct dpif_flow_put *put, + struct dpif_flow_stats *stats) { - struct dp_netdev *dp = get_dp_netdev(dpif); struct dp_netdev_flow *netdev_flow; - struct netdev_flow_key key; - struct dp_netdev_pmd_thread *pmd; - struct match match; - ovs_u128 ufid; - unsigned pmd_id = put->pmd_id == PMD_ID_NULL - ? NON_PMD_CORE_ID : put->pmd_id; - int error; - - error = dpif_netdev_flow_from_nlattrs(put->key, put->key_len, &match.flow); - if (error) { - return error; - } - error = dpif_netdev_mask_from_nlattrs(put->key, put->key_len, - put->mask, put->mask_len, - &match.flow, &match.wc); - if (error) { - return error; - } - - pmd = dp_netdev_get_pmd(dp, pmd_id); - if (!pmd) { - return EINVAL; - } - - /* Must produce a netdev_flow_key for lookup. - * This interface is no longer performance critical, since it is not used - * for upcall processing any more. */ - netdev_flow_key_from_flow(&key, &match.flow); + int error = 0; - if (put->ufid) { - ufid = *put->ufid; - } else { - dpif_flow_hash(dpif, &match.flow, sizeof match.flow, &ufid); + if (stats) { + memset(stats, 0, sizeof *stats); } ovs_mutex_lock(&pmd->flow_mutex); - netdev_flow = dp_netdev_pmd_lookup_flow(pmd, &key, NULL); + netdev_flow = dp_netdev_pmd_lookup_flow(pmd, key, NULL); if (!netdev_flow) { if (put->flags & DPIF_FP_CREATE) { if (cmap_count(&pmd->flow_table) < MAX_FLOWS) { - if (put->stats) { - memset(put->stats, 0, sizeof *put->stats); - } - dp_netdev_flow_add(pmd, &match, &ufid, put->actions, + dp_netdev_flow_add(pmd, match, ufid, put->actions, put->actions_len); error = 0; } else { @@ -2347,7 +2319,7 @@ dpif_netdev_flow_put(struct dpif *dpif, const struct dpif_flow_put *put) } } else { if (put->flags & DPIF_FP_MODIFY - && flow_equal(&match.flow, &netdev_flow->flow)) { + && flow_equal(&match->flow, &netdev_flow->flow)) { struct dp_netdev_actions *new_actions; struct dp_netdev_actions *old_actions; @@ -2357,8 +2329,8 @@ dpif_netdev_flow_put(struct dpif *dpif, const struct dpif_flow_put *put) old_actions = dp_netdev_flow_get_actions(netdev_flow); ovsrcu_set(&netdev_flow->actions, new_actions); - if (put->stats) { - get_dpif_flow_stats(netdev_flow, put->stats); + if (stats) { + get_dpif_flow_stats(netdev_flow, stats); } if (put->flags & DPIF_FP_ZERO_STATS) { /* XXX: The userspace datapath uses thread local statistics @@ -2382,39 +2354,137 @@ dpif_netdev_flow_put(struct dpif *dpif, const struct dpif_flow_put *put) } } ovs_mutex_unlock(&pmd->flow_mutex); - dp_netdev_pmd_unref(pmd); - return error; } static int -dpif_netdev_flow_del(struct dpif *dpif, const struct dpif_flow_del *del) +dpif_netdev_flow_put(struct dpif *dpif, const struct dpif_flow_put *put) { struct dp_netdev *dp = get_dp_netdev(dpif); - struct dp_netdev_flow *netdev_flow; + struct netdev_flow_key key; struct dp_netdev_pmd_thread *pmd; - unsigned pmd_id = del->pmd_id == PMD_ID_NULL - ? NON_PMD_CORE_ID : del->pmd_id; - int error = 0; + struct match match; + ovs_u128 ufid; + int error; - pmd = dp_netdev_get_pmd(dp, pmd_id); - if (!pmd) { - return EINVAL; + if (put->stats) { + memset(put->stats, 0, sizeof *put->stats); + } + error = dpif_netdev_flow_from_nlattrs(put->key, put->key_len, &match.flow); + if (error) { + return error; + } + error = dpif_netdev_mask_from_nlattrs(put->key, put->key_len, + put->mask, put->mask_len, + &match.flow, &match.wc); + if (error) { + return error; + } + + if (put->ufid) { + ufid = *put->ufid; + } else { + dpif_flow_hash(dpif, &match.flow, sizeof match.flow, &ufid); } + /* Must produce a netdev_flow_key for lookup. + * This interface is no longer performance critical, since it is not used + * for upcall processing any more. */ + netdev_flow_key_from_flow(&key, &match.flow); + + if (put->pmd_id == PMD_ID_NULL) { + if (cmap_count(&dp->poll_threads) == 0) { + return EINVAL; + } + CMAP_FOR_EACH (pmd, node, &dp->poll_threads) { + struct dpif_flow_stats pmd_stats; + int pmd_error; + + pmd_error = flow_put_on_pmd(pmd, &key, &match, &ufid, put, + &pmd_stats); + if (pmd_error) { + error = pmd_error; + } else if (put->stats) { + put->stats->n_packets += pmd_stats.n_packets; + put->stats->n_bytes += pmd_stats.n_bytes; + put->stats->used = MAX(put->stats->used, pmd_stats.used); + put->stats->tcp_flags |= pmd_stats.tcp_flags; + } + } + } else { + pmd = dp_netdev_get_pmd(dp, put->pmd_id); + if (!pmd) { + return EINVAL; + } + error = flow_put_on_pmd(pmd, &key, &match, &ufid, put, put->stats); + dp_netdev_pmd_unref(pmd); + } + + return error; +} + +static int +flow_del_on_pmd(struct dp_netdev_pmd_thread *pmd, + struct dpif_flow_stats *stats, + const struct dpif_flow_del *del) +{ + struct dp_netdev_flow *netdev_flow; + int error = 0; + ovs_mutex_lock(&pmd->flow_mutex); netdev_flow = dp_netdev_pmd_find_flow(pmd, del->ufid, del->key, del->key_len); if (netdev_flow) { - if (del->stats) { - get_dpif_flow_stats(netdev_flow, del->stats); + if (stats) { + get_dpif_flow_stats(netdev_flow, stats); } dp_netdev_pmd_remove_flow(pmd, netdev_flow); } else { error = ENOENT; } ovs_mutex_unlock(&pmd->flow_mutex); - dp_netdev_pmd_unref(pmd); + + return error; +} + +static int +dpif_netdev_flow_del(struct dpif *dpif, const struct dpif_flow_del *del) +{ + struct dp_netdev *dp = get_dp_netdev(dpif); + struct dp_netdev_pmd_thread *pmd; + int error = 0; + + if (del->stats) { + memset(del->stats, 0, sizeof *del->stats); + } + + if (del->pmd_id == PMD_ID_NULL) { + if (cmap_count(&dp->poll_threads) == 0) { + return EINVAL; + } + CMAP_FOR_EACH (pmd, node, &dp->poll_threads) { + struct dpif_flow_stats pmd_stats; + int pmd_error; + + pmd_error = flow_del_on_pmd(pmd, &pmd_stats, del); + if (pmd_error) { + error = pmd_error; + } else if (del->stats) { + del->stats->n_packets += pmd_stats.n_packets; + del->stats->n_bytes += pmd_stats.n_bytes; + del->stats->used = MAX(del->stats->used, pmd_stats.used); + del->stats->tcp_flags |= pmd_stats.tcp_flags; + } + } + } else { + pmd = dp_netdev_get_pmd(dp, del->pmd_id); + if (!pmd) { + return EINVAL; + } + error = flow_del_on_pmd(pmd, del->stats, del); + dp_netdev_pmd_unref(pmd); + } + return error; } diff --git a/lib/dpif.c b/lib/dpif.c index 53958c5..4d8505c 100644 --- a/lib/dpif.c +++ b/lib/dpif.c @@ -904,7 +904,7 @@ dpif_probe_feature(struct dpif *dpif, const char *name, * previous run are still present in the datapath. */ error = dpif_flow_put(dpif, DPIF_FP_CREATE | DPIF_FP_MODIFY | DPIF_FP_PROBE, key->data, key->size, NULL, 0, NULL, 0, - ufid, PMD_ID_NULL, NULL); + ufid, NON_PMD_CORE_ID, NULL); if (error) { if (error != EINVAL) { VLOG_WARN("%s: %s flow probe failed (%s)", @@ -915,7 +915,7 @@ dpif_probe_feature(struct dpif *dpif, const char *name, ofpbuf_use_stack(&reply, &stub, sizeof stub); error = dpif_flow_get(dpif, key->data, key->size, ufid, - PMD_ID_NULL, &reply, &flow); + NON_PMD_CORE_ID, &reply, &flow); if (!error && (!ufid || (flow.ufid_present && ovs_u128_equals(*ufid, flow.ufid)))) { @@ -923,7 +923,7 @@ dpif_probe_feature(struct dpif *dpif, const char *name, } error = dpif_flow_del(dpif, key->data, key->size, ufid, - PMD_ID_NULL, NULL); + NON_PMD_CORE_ID, NULL); if (error) { VLOG_WARN("%s: failed to delete %s feature probe flow", dpif_name(dpif), name); diff --git a/lib/dpif.h b/lib/dpif.h index e69087d..7b28c74 100644 --- a/lib/dpif.h +++ b/lib/dpif.h @@ -630,7 +630,8 @@ enum dpif_op_type { * * - If the datapath implements multiple pmd thread with its own flow * table, 'pmd_id' should be used to specify the particular polling - * thread for the operation. + * thread for the operation. PMD_ID_NULL means that the flow should + * be put on all the polling threads. */ struct dpif_flow_put { /* Input. */ @@ -662,7 +663,8 @@ struct dpif_flow_put { * * If the datapath implements multiple polling thread with its own flow table, * 'pmd_id' should be used to specify the particular polling thread for the - * operation. + * operation. PMD_ID_NULL means that the flow should be deleted from all the + * polling threads. * * If the operation succeeds, then 'stats', if nonnull, will be set to the * flow's statistics before its deletion. */ @@ -727,7 +729,8 @@ struct dpif_execute { * * If the datapath implements multiple polling thread with its own flow table, * 'pmd_id' should be used to specify the particular polling thread for the - * operation. + * operation. PMD_ID_NULL means that the datapath will return the first + * matching flow from any poll thread. * * Succeeds with status 0 if the flow is fetched, or fails with ENOENT if no * such flow exists. Other failures are indicated with a positive errno value. @@ -862,6 +865,9 @@ void dpif_get_netflow_ids(const struct dpif *, int dpif_queue_to_priority(const struct dpif *, uint32_t queue_id, uint32_t *priority); +int dpif_get_pmds_for_port(const struct dpif * dpif, odp_port_t port_no, + unsigned int **pmds, size_t *n); + char *dpif_get_dp_version(const struct dpif *); bool dpif_supports_tnl_push_pop(const struct dpif *); #ifdef __cplusplus diff --git a/tests/pmd.at b/tests/pmd.at index 369120f..147dfda 100644 --- a/tests/pmd.at +++ b/tests/pmd.at @@ -610,3 +610,47 @@ p2 1 0 0 dnl During reconfiguration some packets will be dropped. This is expected OVS_VSWITCHD_STOP(["/dpif(monitor[[0-9]]\+)|WARN|dummy@ovs-dummy: execute [[0-9]]\+ failed/d"]) AT_CLEANUP + +AT_SETUP([PMD - dpctl]) +OVS_VSWITCHD_START( + [del-br br0], [], [], [--dummy-numa 0,0]) +AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg]) + +AT_CHECK([ovs-appctl dpctl/add-dp dummy@dp0]) +AT_CHECK([ovs-appctl dpctl/add-if dummy@dp0 p1,type=dummy-pmd]) +AT_CHECK([ovs-appctl dpctl/add-if dummy@dp0 p2,type=dummy]) + +AT_CHECK([ovs-appctl dpif-netdev/pmd-rxq-show dp0 | parse_pmd_rxq_show], [0], [dnl +p1 0 0 0 +]) + +AT_CHECK([ovs-appctl dpctl/show dummy@dp0], [0], [dnl +dummy@dp0: + lookups: hit:0 missed:0 lost:0 + flows: 0 + port 0: dp0 (dummy-internal) + port 1: p1 (dummy-pmd: configured_rx_queues=1, configured_tx_queues=1, requested_rx_queues=1, requested_tx_queues=1) + port 2: p2 (dummy) +]) + +AT_CHECK([ovs-appctl dpctl/add-flow dummy@dp0 'in_port(1),eth(src=00:00:00:00:00:01,dst=00:00:00:00:00:02),eth_type(0x1234)' 2], [0], [dnl +]) + +AT_CHECK([ovs-appctl dpctl/dump-flows dummy@dp0 | sort], [0], [dnl +flow-dump from non-dpdk interfaces: +flow-dump from pmd on cpu core: 0 +recirc_id(0),in_port(1),eth(src=00:00:00:00:00:01,dst=00:00:00:00:00:02),eth_type(0x1234), packets:0, bytes:0, used:never, actions:2 +recirc_id(0),in_port(1),eth(src=00:00:00:00:00:01,dst=00:00:00:00:00:02),eth_type(0x1234), packets:0, bytes:0, used:never, actions:2 +]) + +AT_CHECK([ovs-appctl dpctl/del-flow dummy@dp0 'in_port(1),eth(src=00:00:00:00:00:01,dst=00:00:00:00:00:02),eth_type(0x1234)'], [0], [dnl +]) + +AT_CHECK([ovs-appctl dpctl/dump-flows dummy@dp0], [0], [dnl +]) + +AT_CHECK([ovs-appctl dpctl/del-dp dummy@dp0], [0], [dnl +]) + +OVS_VSWITCHD_STOP +AT_CLEANUP