| Message ID | 160155937152.470185.6352533521999573298.stgit@ebuild |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2] dpctl: add add-flows command to dpctl | expand |
Eelco Chaudron <echaudro@redhat.com> writes: > When you would like to add, modify, or delete a lot of flows in the > datapath, for example when you want to measure performance, adding > one flow at the time won't scale. This as it takes a decent amount > of time to set up the datapath connection. > > This new command is in-line with the same command available in > ovs-ofctl which allows the same thing, with the only difference that > we do not verify all lines before we start execution. This allows for > a continuous add/delete stream. For example with a command like this: > > python3 -c 'while True: > for i in range(0, 1000): > print("add in_port(0),eth(),eth_type(0x800),ipv4(src=100.1.{}.{}) 1".format(int(i / 256), i % 256)) > for i in range(0, 1000): > print("delete in_port(0),eth(),eth_type(0x800),ipv4(src=100.1.{}.{})".format(int(i / 256), i % 256))' \ > | sudo utilities/ovs-dpctl add-flows - > > > Signed-off-by: Eelco Chaudron <echaudro@redhat.com> > --- Acked-by: Aaron Conole <aconole@redhat.com>
On 10/1/20 3:37 PM, Eelco Chaudron wrote: > When you would like to add, modify, or delete a lot of flows in the > datapath, for example when you want to measure performance, adding > one flow at the time won't scale. This as it takes a decent amount > of time to set up the datapath connection. > > This new command is in-line with the same command available in > ovs-ofctl which allows the same thing, with the only difference that > we do not verify all lines before we start execution. This allows for > a continuous add/delete stream. For example with a command like this: > > python3 -c 'while True: > for i in range(0, 1000): > print("add in_port(0),eth(),eth_type(0x800),ipv4(src=100.1.{}.{}) 1".format(int(i / 256), i % 256)) > for i in range(0, 1000): > print("delete in_port(0),eth(),eth_type(0x800),ipv4(src=100.1.{}.{})".format(int(i / 256), i % 256))' \ > | sudo utilities/ovs-dpctl add-flows - > > > Signed-off-by: Eelco Chaudron <echaudro@redhat.com> > --- > v2: - Added change to NEWS > - Updated man page to be more clear > > NEWS | 3 + > lib/dpctl.c | 179 ++++++++++++++++++++++++++++++++++++++++++++----- > lib/dpctl.man | 11 +++ > utilities/ovs-dpctl.c | 5 + > 4 files changed, 177 insertions(+), 21 deletions(-) > > diff --git a/NEWS b/NEWS > index 015facff5..93909ab07 100644 > --- a/NEWS > +++ b/NEWS > @@ -35,7 +35,8 @@ Post-v2.13.0 > - Tunnels: TC Flower offload > * Tunnel Local endpoint address masked match are supported. > * Tunnel Romte endpoint address masked match are supported. > - > + - 'ovs-dpctl add-flows' command has been added which allows adding, > + deleting, or modifying flows based on information read from a file. This in a wrong section. Anyway, this looks a bit wierd that in order to delete flows I need to run 'add-flows' with 'delete' argument. IIUC, ability to add/remove/modify flows within single add-flows command of ovs-ofclt was added to utilize OF1.4 --bundle support, i.e. to make such modifications atomically. So, this is additional functionality, not the basic one. This is a bit annoying that I can not delete flows using the same file that I used for addition. Basic commands are missing. Maybe it's better to add 'del-flows' and, probably, 'mod-flows' commands so it will be easier to use? Best regards, Ilya Maximets.
On 8 Oct 2020, at 17:43, Ilya Maximets wrote: > On 10/1/20 3:37 PM, Eelco Chaudron wrote: >> When you would like to add, modify, or delete a lot of flows in the >> datapath, for example when you want to measure performance, adding >> one flow at the time won't scale. This as it takes a decent amount >> of time to set up the datapath connection. >> >> This new command is in-line with the same command available in >> ovs-ofctl which allows the same thing, with the only difference that >> we do not verify all lines before we start execution. This allows for >> a continuous add/delete stream. For example with a command like this: >> >> python3 -c 'while True: >> for i in range(0, 1000): >> print("add in_port(0),eth(),eth_type(0x800),ipv4(src=100.1.{}.{}) >> 1".format(int(i / 256), i % 256)) >> for i in range(0, 1000): >> print("delete >> in_port(0),eth(),eth_type(0x800),ipv4(src=100.1.{}.{})".format(int(i >> / 256), i % 256))' \ >> | sudo utilities/ovs-dpctl add-flows - >> >> >> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> >> --- >> v2: - Added change to NEWS >> - Updated man page to be more clear >> >> NEWS | 3 + >> lib/dpctl.c | 179 >> ++++++++++++++++++++++++++++++++++++++++++++----- >> lib/dpctl.man | 11 +++ >> utilities/ovs-dpctl.c | 5 + >> 4 files changed, 177 insertions(+), 21 deletions(-) >> >> diff --git a/NEWS b/NEWS >> index 015facff5..93909ab07 100644 >> --- a/NEWS >> +++ b/NEWS >> @@ -35,7 +35,8 @@ Post-v2.13.0 >> - Tunnels: TC Flower offload >> * Tunnel Local endpoint address masked match are supported. >> * Tunnel Romte endpoint address masked match are supported. >> - >> + - 'ovs-dpctl add-flows' command has been added which allows >> adding, >> + deleting, or modifying flows based on information read from a >> file. > > This in a wrong section. Guess it should be in “Linux datapath:”? > Anyway, this looks a bit wierd that in order to delete flows I need to > run 'add-flows' with 'delete' argument. > IIUC, ability to add/remove/modify flows within single add-flows > command > of ovs-ofclt was added to utilize OF1.4 --bundle support, i.e. to make > such modifications atomically. So, this is additional functionality, > not > the basic one. This is a bit annoying that I can not delete flows > using > the same file that I used for addition. Basic commands are missing. > Maybe it's better to add 'del-flows' and, probably, 'mod-flows' > commands > so it will be easier to use? I still would like to do add/delete/mod in a single file, as it will help testing. However, I could add ‘del-flows' and, probably, 'mod-flows' which will use their perspective name as the default action?
On 10/8/20 5:50 PM, Eelco Chaudron wrote: > > > On 8 Oct 2020, at 17:43, Ilya Maximets wrote: > >> On 10/1/20 3:37 PM, Eelco Chaudron wrote: >>> When you would like to add, modify, or delete a lot of flows in the >>> datapath, for example when you want to measure performance, adding >>> one flow at the time won't scale. This as it takes a decent amount >>> of time to set up the datapath connection. >>> >>> This new command is in-line with the same command available in >>> ovs-ofctl which allows the same thing, with the only difference that >>> we do not verify all lines before we start execution. This allows for >>> a continuous add/delete stream. For example with a command like this: >>> >>> python3 -c 'while True: >>> for i in range(0, 1000): >>> print("add in_port(0),eth(),eth_type(0x800),ipv4(src=100.1.{}.{}) 1".format(int(i / 256), i % 256)) >>> for i in range(0, 1000): >>> print("delete in_port(0),eth(),eth_type(0x800),ipv4(src=100.1.{}.{})".format(int(i / 256), i % 256))' \ >>> | sudo utilities/ovs-dpctl add-flows - >>> >>> >>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> >>> --- >>> v2: - Added change to NEWS >>> - Updated man page to be more clear >>> >>> NEWS | 3 + >>> lib/dpctl.c | 179 ++++++++++++++++++++++++++++++++++++++++++++----- >>> lib/dpctl.man | 11 +++ >>> utilities/ovs-dpctl.c | 5 + >>> 4 files changed, 177 insertions(+), 21 deletions(-) >>> >>> diff --git a/NEWS b/NEWS >>> index 015facff5..93909ab07 100644 >>> --- a/NEWS >>> +++ b/NEWS >>> @@ -35,7 +35,8 @@ Post-v2.13.0 >>> - Tunnels: TC Flower offload >>> * Tunnel Local endpoint address masked match are supported. >>> * Tunnel Romte endpoint address masked match are supported. >>> - >>> + - 'ovs-dpctl add-flows' command has been added which allows adding, >>> + deleting, or modifying flows based on information read from a file. >> >> This in a wrong section. > > Guess it should be in “Linux datapath:”? It should be in Post-v2.14.0. :) And, please, keep 2 empty lines between releases. For the entry itself, it might look like: - ovs-dpctl and 'ovs-appctl dpctl/': * New command ... > >> Anyway, this looks a bit wierd that in order to delete flows I need to >> run 'add-flows' with 'delete' argument. >> IIUC, ability to add/remove/modify flows within single add-flows command >> of ovs-ofclt was added to utilize OF1.4 --bundle support, i.e. to make >> such modifications atomically. So, this is additional functionality, not >> the basic one. This is a bit annoying that I can not delete flows using >> the same file that I used for addition. Basic commands are missing. >> Maybe it's better to add 'del-flows' and, probably, 'mod-flows' commands >> so it will be easier to use? > > I still would like to do add/delete/mod in a single file, as it will help testing. Sure. > However, I could add ‘del-flows' and, probably, 'mod-flows' which will use their perspective name as the default action? Yes, that would be good. Additionally it might make sense to add some tests to tests/dpctl.at. Just basic things, i.e. add and del some flows with single commands and via flows listed in some file. Best reards, Ilya Maximets.
On 8 Oct 2020, at 18:00, Ilya Maximets wrote: > On 10/8/20 5:50 PM, Eelco Chaudron wrote: >> >> >> On 8 Oct 2020, at 17:43, Ilya Maximets wrote: >> >>> On 10/1/20 3:37 PM, Eelco Chaudron wrote: >>>> When you would like to add, modify, or delete a lot of flows in the >>>> datapath, for example when you want to measure performance, adding >>>> one flow at the time won't scale. This as it takes a decent amount >>>> of time to set up the datapath connection. >>>> >>>> This new command is in-line with the same command available in >>>> ovs-ofctl which allows the same thing, with the only difference >>>> that >>>> we do not verify all lines before we start execution. This allows >>>> for >>>> a continuous add/delete stream. For example with a command like >>>> this: >>>> >>>> python3 -c 'while True: >>>> for i in range(0, 1000): >>>> print("add >>>> in_port(0),eth(),eth_type(0x800),ipv4(src=100.1.{}.{}) >>>> 1".format(int(i / 256), i % 256)) >>>> for i in range(0, 1000): >>>> print("delete >>>> in_port(0),eth(),eth_type(0x800),ipv4(src=100.1.{}.{})".format(int(i >>>> / 256), i % 256))' \ >>>> | sudo utilities/ovs-dpctl add-flows - >>>> >>>> >>>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> >>>> --- >>>> v2: - Added change to NEWS >>>> - Updated man page to be more clear >>>> >>>> NEWS | 3 + >>>> lib/dpctl.c | 179 >>>> ++++++++++++++++++++++++++++++++++++++++++++----- >>>> lib/dpctl.man | 11 +++ >>>> utilities/ovs-dpctl.c | 5 + >>>> 4 files changed, 177 insertions(+), 21 deletions(-) >>>> >>>> diff --git a/NEWS b/NEWS >>>> index 015facff5..93909ab07 100644 >>>> --- a/NEWS >>>> +++ b/NEWS >>>> @@ -35,7 +35,8 @@ Post-v2.13.0 >>>> - Tunnels: TC Flower offload >>>> * Tunnel Local endpoint address masked match are >>>> supported. >>>> * Tunnel Romte endpoint address masked match are >>>> supported. >>>> - >>>> + - 'ovs-dpctl add-flows' command has been added which allows >>>> adding, >>>> + deleting, or modifying flows based on information read >>>> from a file. >>> >>> This in a wrong section. >> >> Guess it should be in “Linux datapath:”? > > It should be in Post-v2.14.0. :) > And, please, keep 2 empty lines between releases. > > For the entry itself, it might look like: > > - ovs-dpctl and 'ovs-appctl dpctl/': > * New command ... Ack, just sent a v3. >> >>> Anyway, this looks a bit wierd that in order to delete flows I need >>> to >>> run 'add-flows' with 'delete' argument. >>> IIUC, ability to add/remove/modify flows within single add-flows >>> command >>> of ovs-ofclt was added to utilize OF1.4 --bundle support, i.e. to >>> make >>> such modifications atomically. So, this is additional >>> functionality, not >>> the basic one. This is a bit annoying that I can not delete flows >>> using >>> the same file that I used for addition. Basic commands are >>> missing. >>> Maybe it's better to add 'del-flows' and, probably, 'mod-flows' >>> commands >>> so it will be easier to use? >> >> I still would like to do add/delete/mod in a single file, as it will >> help testing. > > Sure. > >> However, I could add ‘del-flows' and, probably, 'mod-flows' which >> will use their perspective name as the default action? > > Yes, that would be good. Done in v3, as well as the tests suggested below. > Additionally it might make sense to add some tests to tests/dpctl.at. > Just basic things, i.e. add and del some flows with single commands > and via flows listed in some file.
diff --git a/NEWS b/NEWS index 015facff5..93909ab07 100644 --- a/NEWS +++ b/NEWS @@ -35,7 +35,8 @@ Post-v2.13.0 - Tunnels: TC Flower offload * Tunnel Local endpoint address masked match are supported. * Tunnel Romte endpoint address masked match are supported. - + - 'ovs-dpctl add-flows' command has been added which allows adding, + deleting, or modifying flows based on information read from a file. v2.13.0 - 14 Feb 2020 --------------------- diff --git a/lib/dpctl.c b/lib/dpctl.c index db2b1f896..eba2bdfa2 100644 --- a/lib/dpctl.c +++ b/lib/dpctl.c @@ -52,6 +52,12 @@ #include "openvswitch/ofp-flow.h" #include "openvswitch/ofp-port.h" +enum { + DPCTL_FLOWS_ADD = 0, + DPCTL_FLOWS_DEL, + DPCTL_FLOWS_MOD +}; + typedef int dpctl_command_handler(int argc, const char *argv[], struct dpctl_params *); struct dpctl_command { @@ -1102,28 +1108,22 @@ out_free: } static int -dpctl_put_flow(int argc, const char *argv[], enum dpif_flow_put_flags flags, - struct dpctl_params *dpctl_p) +dpctl_put_flow_dpif(struct dpif *dpif, const char *key_s, + const char *actions_s, + 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 dpif_flow_stats stats; struct dpif_port dpif_port; struct dpif_port_dump port_dump; struct ofpbuf actions; struct ofpbuf key; struct ofpbuf mask; - struct dpif *dpif; ovs_u128 ufid; bool ufid_present; struct simap port_names; int n, error; - error = opt_dpif_open(argc, argv, dpctl_p, 4, &dpif); - if (error) { - return error; - } - ufid_present = false; n = odp_ufid_from_string(key_s, &ufid); if (n < 0) { @@ -1185,6 +1185,24 @@ out_freeactions: out_freekeymask: ofpbuf_uninit(&mask); ofpbuf_uninit(&key); + return error; +} + +static int +dpctl_put_flow(int argc, const char *argv[], enum dpif_flow_put_flags flags, + struct dpctl_params *dpctl_p) +{ + struct dpif *dpif; + int error; + + error = opt_dpif_open(argc, argv, dpctl_p, 4, &dpif); + if (error) { + return error; + } + + error = dpctl_put_flow_dpif(dpif, argv[argc - 2], argv[argc - 1], flags, + dpctl_p); + dpif_close(dpif); return error; } @@ -1258,25 +1276,20 @@ out: } static int -dpctl_del_flow(int argc, const char *argv[], struct dpctl_params *dpctl_p) +dpctl_del_flow_dpif(struct dpif *dpif, const char *key_s, + struct dpctl_params *dpctl_p) { - const char *key_s = argv[argc - 1]; struct dpif_flow_stats stats; struct dpif_port dpif_port; struct dpif_port_dump port_dump; struct ofpbuf key; struct ofpbuf mask; /* To be ignored. */ - struct dpif *dpif; + ovs_u128 ufid; bool ufid_present; struct simap port_names; int n, error; - error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif); - if (error) { - return error; - } - ufid_present = false; n = odp_ufid_from_string(key_s, &ufid); if (n < 0) { @@ -1334,6 +1347,23 @@ out: ofpbuf_uninit(&mask); ofpbuf_uninit(&key); simap_destroy(&port_names); + return error; +} + +static int +dpctl_del_flow(int argc, const char *argv[], struct dpctl_params *dpctl_p) +{ + const char *key_s = argv[argc - 1]; + struct dpif *dpif; + int error; + + error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif); + if (error) { + return error; + } + + error = dpctl_del_flow_dpif(dpif, key_s, dpctl_p); + dpif_close(dpif); return error; } @@ -1356,6 +1386,118 @@ dpctl_del_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p) return error; } +static int +dpctl_parse_add_flows_line(struct ds *s, char **flow, char **action) +{ + int command = DPCTL_FLOWS_ADD; + const char *line = ds_cstr(s); + size_t len; + + /* First figure out the command, or fallback to FLOWS_ADD. */ + line += strspn(line, " \t\r\n"); + len = strcspn(line, ", \t\r\n"); + + if (!strncmp(line, "add", len)) { + command = DPCTL_FLOWS_ADD; + } else if (!strncmp(line, "delete", len)) { + command = DPCTL_FLOWS_DEL; + } else if (!strncmp(line, "modify", len)) { + command = DPCTL_FLOWS_MOD; + } else { + len = 0; + } + line += len; + + /* Isolate flow and action (for add/modify). */ + line += strspn(line, " \t\r\n"); + len = strcspn(line, " \t\r\n"); + + if (len == 0) { + *flow = NULL; + *action = NULL; + return command; + } + + *flow = strndup(line, len); + + line += len; + line += strspn(line, " \t\r\n"); + if (strlen(line)) { + *action = xstrdup(line); + } else { + *action = NULL; + } + + return command; +} + +static int +dpctl_add_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p) +{ + const char *file_name = argv[argc - 1]; + int line_number = 0; + struct dpif *dpif; + struct ds line; + FILE *stream; + int error; + + error = opt_dpif_open(argc, argv, dpctl_p, 4, &dpif); + if (error) { + return error; + } + + stream = !strcmp(file_name, "-") ? stdin : fopen(file_name, "r"); + if (!stream) { + error = errno; + dpctl_error(dpctl_p, error, "Opening file \"%s\" failed", file_name); + goto out_close_dpif; + } + + ds_init(&line); + while (!ds_get_preprocessed_line(&line, stream, &line_number)) { + /* We do not process all the lines first and then execute the actions + * as we would like to take commands as a continuous stream of + * commands from stdin. + */ + char *flow = NULL; + char *action = NULL; + int cmd = dpctl_parse_add_flows_line(&line, &flow, &action); + + if ((!flow && !action) + || ((cmd == DPCTL_FLOWS_ADD || cmd == DPCTL_FLOWS_MOD) && !action) + || (cmd == DPCTL_FLOWS_DEL && action)) { + dpctl_error(dpctl_p, 0, + "Failed parsing line number %u, skipped!", + line_number); + } else { + switch (cmd) { + case DPCTL_FLOWS_ADD: + dpctl_put_flow_dpif(dpif, flow, action, + DPIF_FP_CREATE, dpctl_p); + break; + case DPCTL_FLOWS_MOD: + dpctl_put_flow_dpif(dpif, flow, action, + DPIF_FP_MODIFY, dpctl_p); + break; + case DPCTL_FLOWS_DEL: + dpctl_del_flow_dpif(dpif, flow, dpctl_p); + break; + } + } + + free(flow); + free(action); + } + + ds_destroy(&line); + if (stream != stdin) { + fclose(stream); + } +out_close_dpif: + dpif_close(dpif); + return 0; +} + static int dpctl_help(int argc OVS_UNUSED, const char *argv[] OVS_UNUSED, struct dpctl_params *dpctl_p) @@ -2506,6 +2648,7 @@ static const struct dpctl_command all_commands[] = { { "dump-flows", "[dp] [filter=..] [type=..]", 0, 3, dpctl_dump_flows, DP_RO }, { "add-flow", "[dp] flow actions", 2, 3, dpctl_add_flow, DP_RW }, + { "add-flows", "[dp] file", 1, 2, dpctl_add_flows, 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 }, { "del-flow", "[dp] flow", 1, 2, dpctl_del_flow, DP_RW }, diff --git a/lib/dpctl.man b/lib/dpctl.man index 727d1f7be..2addea521 100644 --- a/lib/dpctl.man +++ b/lib/dpctl.man @@ -194,6 +194,17 @@ ovs-dpctl add-flow myDP \\ . .RE .TP +.DO "\*(DX\fBadd\-flows\fR [\fIdp\fR] \fIfile\fR" +Reads flow entries from \fIfile\fR (or \fBstdin\fR if \fIfile\fR is +\fB\-\fR) and adds each entry to the datapath. +. +Each flow specification (e.g., each line in \fIfile\fR) may start with +\fBadd\fR, \fBmodify\fR, or \fBdelete\fR keyword to specify whether a +flow is to be added, modified, or deleted. A flow specification without +one of these keywords is treated as a flow add. All flow modifications +are executed as individual transactions in the order specified. +. +.TP .DO "[\fB\-s\fR | \fB\-\-statistics\fR]" "\*(DX\fBdel\-flow\fR" "[\fIdp\fR] \fIflow\fR" Deletes the flow from \fIdp\fR's flow table that matches \fIflow\fR. If \fB\-s\fR or \fB\-\-statistics\fR is specified, then diff --git a/utilities/ovs-dpctl.c b/utilities/ovs-dpctl.c index 7d99607f4..3ff3f714c 100644 --- a/utilities/ovs-dpctl.c +++ b/utilities/ovs-dpctl.c @@ -191,6 +191,7 @@ usage(void *userdata OVS_UNUSED) " show DP... show basic info on each DP\n" " dump-flows [DP] display flows in DP\n" " add-flow [DP] FLOW ACTIONS add FLOW with ACTIONS to DP\n" + " add-flows [DP] FILE add flows from FILE\n" " mod-flow [DP] FLOW ACTIONS change FLOW actions to ACTIONS in DP\n" " get-flow [DP] ufid:UFID fetch flow corresponding to UFID\n" " del-flow [DP] FLOW delete FLOW from DP\n" @@ -205,8 +206,8 @@ usage(void *userdata OVS_UNUSED) "Each IFACE on add-dp, add-if, and set-if may be followed by\n" "comma-separated options. See ovs-dpctl(8) for syntax, or the\n" "Interface table in ovs-vswitchd.conf.db(5) for an options list.\n" - "For COMMAND dump-flows, add-flow, mod-flow, del-flow and\n" - "del-flows, DP is optional if there is only one datapath.\n", + "For COMMAND dump-flows, add-flow, add-flows, mod-flow, del-flow\n" + "and del-flows, DP is optional if there is only one datapath.\n", program_name, program_name); vlog_usage(); printf("\nOptions for show and mod-flow:\n"
When you would like to add, modify, or delete a lot of flows in the datapath, for example when you want to measure performance, adding one flow at the time won't scale. This as it takes a decent amount of time to set up the datapath connection. This new command is in-line with the same command available in ovs-ofctl which allows the same thing, with the only difference that we do not verify all lines before we start execution. This allows for a continuous add/delete stream. For example with a command like this: python3 -c 'while True: for i in range(0, 1000): print("add in_port(0),eth(),eth_type(0x800),ipv4(src=100.1.{}.{}) 1".format(int(i / 256), i % 256)) for i in range(0, 1000): print("delete in_port(0),eth(),eth_type(0x800),ipv4(src=100.1.{}.{})".format(int(i / 256), i % 256))' \ | sudo utilities/ovs-dpctl add-flows - Signed-off-by: Eelco Chaudron <echaudro@redhat.com> --- v2: - Added change to NEWS - Updated man page to be more clear NEWS | 3 + lib/dpctl.c | 179 ++++++++++++++++++++++++++++++++++++++++++++----- lib/dpctl.man | 11 +++ utilities/ovs-dpctl.c | 5 + 4 files changed, 177 insertions(+), 21 deletions(-)