| Message ID | 20200824024408.56559-1-xiangxia.m.yue@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,ovs,resend,v2] dpctl-netdev: Add the option "pmd" for dump-flows | expand |
Bleep bloop. Greetings Tonghao Zhang, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Line is 175 characters long (recommended limit is 79) #113 FILE: lib/dpctl.man:107: .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]" Lines checked: 200, Warnings: 1, Errors: 0 Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
Hi, Thanks for the patch. I just did a quick look and found couple things to discuss. On Mon, Aug 24, 2020 at 10:44:08AM +0800, xiangxia.m.yue@gmail.com wrote: > From: Tonghao Zhang <xiangxia.m.yue@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. > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > --- > 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 2f67d504790c..6a7308aaaafd 100644 > --- a/NEWS > +++ b/NEWS > @@ -40,6 +40,8 @@ v2.14.0 - 17 Aug 2020 > * 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. > - 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)) { We can simplify that by not checking pmd_id_filter. In the worse case scenario it will use the last pmd= parameter, right? > + if (!ovs_scan(argv[--argc], "pmd=%"SCNu32, &pmd_id_filter)) { > + 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; > + } This should not be possible, see below. > + > 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); > + 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]" > 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 02df8f11e9ac..678badf02532 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; This is part of a documented API and returning NULL is not possible. See struct dpif_class declaration in lib/dpif-provider.h, which I copy&pasted below the relevant part below: /* Flow dumping interface. * * This is the back-end for the flow dumping interface described in * dpif.h. Please read the comments there first, because this code * closely follows it. * * 'flow_dump_create' and 'flow_dump_thread_create' must always return an * initialized and usable data structure and defer error return until * flow_dump_destroy(). This hasn't been a problem for the dpifs that * exist so far. * * 'flow_dump_create' and 'flow_dump_thread_create' must initialize the * structures that they return with dpif_flow_dump_init() and * dpif_flow_dump_thread_init(), respectively. -- fbl > + } > + > + 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); > } > + > + 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. */ > 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 { > -- > 1.8.3.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 8/24/20 4:44 AM, xiangxia.m.yue@gmail.com wrote: > From: Tonghao Zhang <xiangxia.m.yue@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. Is this a performance concern or just a matter of output readability? In case it's only to reduce the size of a printed data, we could just avoid printing flows from PMDs other than requested the same way as other filters work without flow_dump API modifications. This will be much more simple change. What do you think? Best regards, Ilya Maximets.
On Thu, Sep 3, 2020 at 8:31 PM Flavio Leitner <fbl@sysclose.org> wrote: > > > Hi, > > Thanks for the patch. I just did a quick look and found couple > things to discuss. > > On Mon, Aug 24, 2020 at 10:44:08AM +0800, xiangxia.m.yue@gmail.com wrote: > > From: Tonghao Zhang <xiangxia.m.yue@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. > > > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > --- > > 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 2f67d504790c..6a7308aaaafd 100644 > > --- a/NEWS > > +++ b/NEWS > > @@ -40,6 +40,8 @@ v2.14.0 - 17 Aug 2020 > > * 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. > > - 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)) { > > We can simplify that by not checking pmd_id_filter. In the worse > case scenario it will use the last pmd= parameter, right? Yes, I will remove the check > > > + if (!ovs_scan(argv[--argc], "pmd=%"SCNu32, &pmd_id_filter)) { > > + 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; > > + } > > This should not be possible, see below. > > > > + > > 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); > > + 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]" > > 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 02df8f11e9ac..678badf02532 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; > > This is part of a documented API and returning NULL is not possible. > See struct dpif_class declaration in lib/dpif-provider.h, which I > copy&pasted below the relevant part below: Hi can we change the API? and allow it to return the NULL ? > /* Flow dumping interface. > * > * This is the back-end for the flow dumping interface described in > * dpif.h. Please read the comments there first, because this code > * closely follows it. > * > * 'flow_dump_create' and 'flow_dump_thread_create' must always return an > * initialized and usable data structure and defer error return until > * flow_dump_destroy(). This hasn't been a problem for the dpifs that > * exist so far. > * > * 'flow_dump_create' and 'flow_dump_thread_create' must initialize the > * structures that they return with dpif_flow_dump_init() and > * dpif_flow_dump_thread_init(), respectively. > > -- > fbl > > > + } > > + > > + 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); > > } > > + > > + 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. */ > > 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 { > > -- > > 1.8.3.1 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > -- Best regards, Tonghao
On Thu, Sep 3, 2020 at 8:51 PM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 8/24/20 4:44 AM, xiangxia.m.yue@gmail.com wrote: > > From: Tonghao Zhang <xiangxia.m.yue@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. > > Is this a performance concern or just a matter of output readability? Yes, performance and the matter of output are considered. Think about there is a lots of flows on every pmd which may be offloaded. > In case it's only to reduce the size of a printed data, we could just > avoid printing flows from PMDs other than requested the same way as > other filters work without flow_dump API modifications. This will be > much more simple change. > > What do you think? > > Best regards, Ilya Maximets.
On 9/7/20 7:29 AM, Tonghao Zhang wrote: > On Thu, Sep 3, 2020 at 8:51 PM Ilya Maximets <i.maximets@ovn.org> wrote: >> >> On 8/24/20 4:44 AM, xiangxia.m.yue@gmail.com wrote: >>> From: Tonghao Zhang <xiangxia.m.yue@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. >> >> Is this a performance concern or just a matter of output readability? > Yes, performance and the matter of output are considered. Think about > there is a lots of flows on every > pmd which may be offloaded. I understand that it will be faster to not dump flows that wasn't requested, however, dpctl interface is a debugging interface and should not be used frequently in normal conditions. And if you have so many flows that dumping takes significant amount of time, you might have much more important issues with inability to change them in time by revalidators. So, why the performance of this operation is such a concern for you? >> In case it's only to reduce the size of a printed data, we could just >> avoid printing flows from PMDs other than requested the same way as >> other filters work without flow_dump API modifications. This will be >> much more simple change. >> >> What do you think? >> >> Best regards, Ilya Maximets. > > >
On Mon, Sep 7, 2020 at 9:01 PM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 9/7/20 7:29 AM, Tonghao Zhang wrote: > > On Thu, Sep 3, 2020 at 8:51 PM Ilya Maximets <i.maximets@ovn.org> wrote: > >> > >> On 8/24/20 4:44 AM, xiangxia.m.yue@gmail.com wrote: > >>> From: Tonghao Zhang <xiangxia.m.yue@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. > >> > >> Is this a performance concern or just a matter of output readability? > > Yes, performance and the matter of output are considered. Think about > > there is a lots of flows on every > > pmd which may be offloaded. > > I understand that it will be faster to not dump flows that wasn't requested, > however, dpctl interface is a debugging interface and should not be used > frequently in normal conditions. And if you have so many flows that dumping > takes significant amount of time, you might have much more important issues > with inability to change them in time by revalidators. Yes, you are right. In my branch, I change the logic of revalidator which will not cleanup the flows which installed by dpctl. and disable the upcall(via upcall-limit). In that way, all flows are installed to HW, and packets will be processed in the hardware. > So, why the performance of this operation is such a concern for you? If all flows are installed to hardware and there are multi-pmds(for example 4), I don't want to dump flows from every pmd even though they have same flows. By the way, if we don't use the dpctl to install flow, To support ct flow offload, there are a lots of flows in hardware ? for example there are too many TCP connections. > >> In case it's only to reduce the size of a printed data, we could just > >> avoid printing flows from PMDs other than requested the same way as > >> other filters work without flow_dump API modifications. This will be > >> much more simple change. > >> > >> What do you think? > >> > >> Best regards, Ilya Maximets. > > > > > > >
diff --git a/NEWS b/NEWS index 2f67d504790c..6a7308aaaafd 100644 --- a/NEWS +++ b/NEWS @@ -40,6 +40,8 @@ v2.14.0 - 17 Aug 2020 * 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. - 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)) { + 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); + 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]" 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 02df8f11e9ac..678badf02532 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); } + + 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. */ 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 {