[ovs-dev] [PATCH ovs v2] dpctl-netdev: Add the option "pmd" for dump-flows
Gaëtan Rivet
grive at u256.net
Wed Oct 14 14:27:35 UTC 2020
Hello,
On 24/07/20 15:33 +0800, xiangxia.m.yue at gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue at gmail.com>
>
> "ovs-appctl dpctl/dump-flows" added the option
> "pmd" which allow user to dump pmd specified.
>
> That option is useful to dump rules of pmd
> when we have a large number of rules in dp.
>
Thanks for the patch! I think it can be useful.
A few comments below.
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue at gmail.com>
> ---
> v2:
> * rebase the codes
> * add usage
> ---
> NEWS | 2 ++
> lib/dpctl.c | 28 +++++++++++++++++++++++-----
> lib/dpctl.man | 6 +++++-
> lib/dpif-netdev.c | 25 +++++++++++++++++++++++++
> lib/dpif-provider.h | 2 ++
> 5 files changed, 57 insertions(+), 6 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index dceda95a381a..04a05914c72e 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -38,6 +38,8 @@ v2.14.0 - xx xxx xxxx
> * Add runtime CPU ISA detection to allow optimized ISA functions
> * Add support for dynamically changing DPCLS subtable lookup functions
> * Add ISA optimized DPCLS lookup function using AVX512
> + * Command "ovs-appctl dpctl/dump-flows" added option "pmd" which allow
> + user to dump pmd specified.
It would be better to follow the same format as other items in the list.
Single quotes should be used instead of double, past-tense should be
avoided. I can propose this instead:
* Add the 'pmd' option to "ovs-appctl dpctl/dump-flows", which
restricts a flow dump to a single PMD threads if set.
> - New configuration knob 'other_config:bond-primary' for AB bonds
> that specifies interface will be the preferred port if it is active.
> - Tunnels: TC Flower offload
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index 09ae97f25cf3..a76e3e520804 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -979,6 +979,7 @@ dpctl_dump_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p)
> struct dpif_flow_dump_thread *flow_dump_thread;
> struct dpif_flow_dump *flow_dump;
> struct dpif_flow f;
> + int pmd_id_filter = PMD_ID_NULL;
> int pmd_id = PMD_ID_NULL;
> int lastargc = 0;
> int error;
> @@ -996,6 +997,12 @@ dpctl_dump_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p)
> goto out_free;
> }
> types_list = xstrdup(argv[--argc] + 5);
> + } else if (pmd_id_filter == PMD_ID_NULL &&
> + !strncmp(argv[argc - 1], "pmd=", 4)) {
> + if (!ovs_scan(argv[--argc], "pmd=%"SCNu32, &pmd_id_filter)) {
The SCNu32 format is to scan a uint32_t value.
To stay consistent with 'pmd_id_filter' type as it follows PMD_ID_NULL definition,
the scan format should be '%d' instead.
It could also be useful for the user to be able to dump flows from the
main thread, but we cannot expect them to use NON_PMD_CORE_ID. Parsing
could recognize 'main' as well maybe? Then the filter would be set to
NON_PMD_CORE_ID. I'm not sure it would then work with
dpif_netdev_flow_dump_next(), have you tested it?
> + error = EINVAL;
> + goto out_free;
> + }
> }
> }
>
> @@ -1044,7 +1051,13 @@ dpctl_dump_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p)
> ds_init(&ds);
> memset(&f, 0, sizeof f);
> flow_dump = dpif_flow_dump_create(dpif, false, &dpif_dump_types);
> + flow_dump->pmd_id = pmd_id_filter;
> flow_dump_thread = dpif_flow_dump_thread_create(flow_dump);
> + if (!flow_dump_thread) {
> + error = ENOENT;
> + goto out_dump_destroy;
> + }
> +
> while (dpif_flow_dump_next(flow_dump_thread, &f, 1)) {
> if (filter) {
> struct flow flow;
> @@ -1085,11 +1098,16 @@ dpctl_dump_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p)
> }
> }
> dpif_flow_dump_thread_destroy(flow_dump_thread);
> - error = dpif_flow_dump_destroy(flow_dump);
>
> - if (error) {
> - dpctl_error(dpctl_p, error, "Failed to dump flows from datapath");
> +out_dump_destroy:
> + {
> + int ret = dpif_flow_dump_destroy(flow_dump);
I'm not convinced this is a good idea or necessary to open an anonymous scope
just to avoid declaring ret at the function level. Please move ret
definition at the top instead.
> + if (ret) {
> + dpctl_error(dpctl_p, ret, "Failed to dump flows from datapath");
> + error = ret;
> + }
> }
> +
> ds_destroy(&ds);
>
> out_dpifclose:
> @@ -2503,8 +2521,8 @@ static const struct dpctl_command all_commands[] = {
> { "set-if", "dp iface...", 2, INT_MAX, dpctl_set_if, DP_RW },
> { "dump-dps", "", 0, 0, dpctl_dump_dps, DP_RO },
> { "show", "[dp...]", 0, INT_MAX, dpctl_show, DP_RO },
> - { "dump-flows", "[dp] [filter=..] [type=..]",
> - 0, 3, dpctl_dump_flows, DP_RO },
> + { "dump-flows", "[dp] [filter=..] [type=..] [pmd=..]",
> + 0, 4, dpctl_dump_flows, DP_RO },
> { "add-flow", "[dp] flow actions", 2, 3, dpctl_add_flow, DP_RW },
> { "mod-flow", "[dp] flow actions", 2, 3, dpctl_mod_flow, DP_RW },
> { "get-flow", "[dp] ufid", 1, 2, dpctl_get_flow, DP_RO },
> diff --git a/lib/dpctl.man b/lib/dpctl.man
> index 727d1f7be8d4..192bee489de7 100644
> --- a/lib/dpctl.man
> +++ b/lib/dpctl.man
> @@ -104,7 +104,7 @@ default. When multiple datapaths exist, then a datapath name is
> required.
> .
> .TP
> -.DO "[\fB\-m \fR| \fB\-\-more\fR] [\fB\-\-names \fR| \fB\-\-no\-names\fR]" \*(DX\fBdump\-flows\fR "[\fIdp\fR] [\fBfilter=\fIfilter\fR] [\fBtype=\fItype\fR]"
> +.DO "[\fB\-m \fR| \fB\-\-more\fR] [\fB\-\-names \fR| \fB\-\-no\-names\fR]" \*(DX\fBdump\-flows\fR "[\fIdp\fR] [\fBfilter=\fIfilter\fR] [\fBtype=\fItype\fR] [\fBpmd=\fIpmd\fR]"
I saw the zero-day bot warning. As long as the manpage output is
properly limited in width I think you can ignore it.
> Prints to the console all flow entries in datapath \fIdp\fR's flow
> table. Without \fB\-m\fR or \fB\-\-more\fR, output omits match fields
> that a flow wildcards entirely; with \fB\-m\fR or \fB\-\-more\fR,
> @@ -118,6 +118,10 @@ The \fIfilter\fR is also useful to match wildcarded fields in the datapath
> flow. As an example, \fBfilter='tcp,tp_src=100'\fR will match the
> datapath flow containing '\fBtcp(src=80/0xff00,dst=8080/0xff)\fR'.
> .IP
> +If \fBpmd=\fIpmd\fR is specified, only displays flows of the specified pmd.
> +The \fBpmd=\fI-1\fR means that dump only the flows on the main pthread.
> +This option supported only for \fBuserspace datapath\fR.
> +.IP
> If \fBtype=\fItype\fR is specified, only displays flows of the specified types.
> This option supported only for \fBovs\-appctl dpctl/dump\-flows\fR.
> \fItype\fR is a comma separated list, which can contain any of the following:
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 2aad24511c1f..5f144564d3d6 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -4007,6 +4007,21 @@ dpif_netdev_flow_dump_thread_create(struct dpif_flow_dump *dump_)
> struct dpif_netdev_flow_dump *dump = dpif_netdev_flow_dump_cast(dump_);
> struct dpif_netdev_flow_dump_thread *thread;
>
> + dump->cur_pmd = NULL;
> + if (dump->up.pmd_id != PMD_ID_NULL) {
> + struct dp_netdev *dp = get_dp_netdev(dump->up.dpif);
> + struct dp_netdev_pmd_thread *pmd;
> +
> + pmd = dp_netdev_get_pmd(dp, dump->up.pmd_id);
> + if (!pmd || !dp_netdev_pmd_try_ref(pmd)) {
> + VLOG_DBG("The PMD ID (%u) not found or ref.\n",
> + dump->up.pmd_id);
> + return NULL;
> + }
> +
> + dump->cur_pmd = pmd;
> + }
> +
> thread = xmalloc(sizeof *thread);
> dpif_flow_dump_thread_init(&thread->up, &dump->up);
> thread->dump = dump;
> @@ -4018,6 +4033,11 @@ dpif_netdev_flow_dump_thread_destroy(struct dpif_flow_dump_thread *thread_)
> {
> struct dpif_netdev_flow_dump_thread *thread
> = dpif_netdev_flow_dump_thread_cast(thread_);
> + struct dpif_netdev_flow_dump *dump = thread->dump;
> +
> + if (dump->up.pmd_id != PMD_ID_NULL && dump->cur_pmd) {
> + dp_netdev_pmd_unref(dump->cur_pmd);
> + }
>
> free(thread);
> }
> @@ -4063,6 +4083,11 @@ dpif_netdev_flow_dump_next(struct dpif_flow_dump_thread *thread_,
> struct dp_netdev_flow,
> node);
> }
> +
You need to comment here why the loop must break.
Actually, reading the code I think 'up.pmd_id' does not seem explicit
enough. Within the dpif_flow_dump you could call it 'pmd_id_filter' instead?
But even with this change, a comment would be welcome here.
> + if (dump->up.pmd_id != PMD_ID_NULL) {
> + break;
> + }
> +
> /* When finishing dumping the current pmd thread, moves to
> * the next. */
> if (n_flows < flow_limit) {
> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> index 0e024c1c9851..aef96277d4d7 100644
> --- a/lib/dpif-provider.h
> +++ b/lib/dpif-provider.h
> @@ -57,6 +57,7 @@ static inline void dpif_assert_class(const struct dpif *dpif,
>
> struct dpif_flow_dump {
> struct dpif *dpif;
> + unsigned pmd_id; /* As a filter for PMD flows. */
It seems logical to want an 'unsigned int' for the PMD id, but given all
future comparison and parsing is done on integers, instead it should be
an 'int' to stay consistent.
Reading other definitions it seems that 'unsigned int' should be used
instead if it was to stay an unsigned integer.
> bool terse; /* If true, key/mask/actions may be omitted. */
> };
>
> @@ -64,6 +65,7 @@ static inline void
> dpif_flow_dump_init(struct dpif_flow_dump *dump, const struct dpif *dpif)
> {
> dump->dpif = CONST_CAST(struct dpif *, dpif);
> + dump->pmd_id = PMD_ID_NULL;
> }
>
> struct dpif_flow_dump_thread {
> --
> 2.26.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
--
Gaëtan
More information about the dev
mailing list