Re: [ovs-dev] [PATCHv7 1/3] Improved Packet Drop Statistics in OVS
Hi Ben/Ilya,
I have addressed the comments in the below patch. Can you tell me if this is
fine?
Regards
Anju
-Original Message-
From: Anju Thomas [mailto:anju.tho...@ericsson.com]
Sent: Tuesday, January 29, 2019 5:21 PM
To: d...@openvswitch.org
Cc: Anju Thomas
Subject: [PATCHv7 1/3] Improved Packet Drop Statistics in OVS
Currently OVS maintains explicit packet drop/error counters only on port
level. Packets that are dropped as part of normal OpenFlow processing are
counted in flow stats of “drop” flows or as table misses in table stats.
These can only be interpreted by controllers that know the semantics of
the configured OpenFlow pipeline. Without that knowledge, it is impossible
for an OVS user to obtain e.g. the total number of packets dropped due to
OpenFlow rules.
Furthermore, there are numerous other reasons for which packets can be
dropped by OVS slow path that are not related to the OpenFlow pipeline.
The generated datapath flow entries include a drop action to avoid further
expensive upcalls to the slow path, but subsequent packets dropped by the
datapath are not accounted anywhere.
Finally, the datapath itself drops packets in certain error situations.
Also, these drops are today not accounted for.
This makes it difficult for OVS users to monitor packet drop in an OVS
instance and to alert a management system in case of a unexpected increase
of such drops. Also OVS trouble-shooters face difficulties in analysing
packet drops.
With this patch we implement following changes to address the issues
mentioned above.
1. Identify and account all the silent packet drop scenarios
2. Display these drops in ovs-appctl coverage/show
A detailed presentation on this was presented at OvS conference 2017 and
link for the corresponding presentation is available at:
https://www.slideshare.net/LF_OpenvSwitch/lfovs17troubleshooting-the-data-plane-in-ovs-82280329
Co-authored-by: Rohith Basavaraja
Co-authored-by: Keshav Gupta
Signed-off-by: Anju Thomas
Signed-off-by: Rohith Basavaraja
Signed-off-by: Keshav Gupta
---
datapath/linux/compat/include/linux/openvswitch.h | 1 +
lib/dpif-netdev.c | 39 ++-
lib/dpif.c| 7 ++
lib/dpif.h| 3 +
lib/netdev-dpdk.c | 4 ++
lib/odp-execute.c | 81 +++
lib/odp-execute.h | 2 +
lib/odp-util.c| 10 ++-
ofproto/ofproto-dpif-ipfix.c | 1 +
ofproto/ofproto-dpif-sflow.c | 1 +
ofproto/ofproto-dpif-xlate.c | 31 +
ofproto/ofproto-dpif-xlate.h | 4 ++
ofproto/ofproto-dpif.c| 8 +++
ofproto/ofproto-dpif.h| 8 ++-
tests/automake.mk | 3 +-
tests/dpif-netdev.at | 10 +++
tests/ofproto-dpif.at | 4 +-
tests/testsuite.at| 1 +
tests/tunnel-push-pop.at | 28 +++-
tests/tunnel.at | 14 +++-
20 files changed, 248 insertions(+), 12 deletions(-)
diff --git a/datapath/linux/compat/include/linux/openvswitch.h
b/datapath/linux/compat/include/linux/openvswitch.h
index 9b087f1..92db378 100644
--- a/datapath/linux/compat/include/linux/openvswitch.h
+++ b/datapath/linux/compat/include/linux/openvswitch.h
@@ -938,6 +938,7 @@ enum ovs_action_attr {
OVS_ACTION_ATTR_POP_NSH, /* No argument. */
OVS_ACTION_ATTR_METER,/* u32 meter number. */
OVS_ACTION_ATTR_CLONE,/* Nested OVS_CLONE_ATTR_*. */
+ OVS_ACTION_ATTR_DROP,
#ifndef __KERNEL__
OVS_ACTION_ATTR_TUNNEL_PUSH, /* struct ovs_action_push_tnl*/
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 0f57e3f..c726463 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -77,6 +77,7 @@
#include "unixctl.h"
#include "util.h"
#include "uuid.h"
+#include "ofproto/ofproto-dpif-xlate.h"
VLOG_DEFINE_THIS_MODULE(dpif_netdev);
@@ -100,6 +101,17 @@ enum { MAX_METERS = 65536 };/* Maximum number of
meters. */
enum { MAX_BANDS = 8 }; /* Maximum number of bands / meter. */
enum { N_METER_LOCKS = 64 };/* Maximum number of meters. */
+COVERAGE_DEFINE(datapath_drop_meter);
+COVERAGE_DEFINE(datapath_drop_upcall_error);
+COVERAGE_DEFINE(datapath_drop_lock_error);
+COVERAGE_DEFINE(datapath_drop_userspace_action_error);
+COVERAGE_DEFINE(datapath_drop_tunnel_push_error);
+COVERAGE_DEFINE(datapath_drop_tunnel_pop_error);
+COVERAGE_DEFINE(datapath_drop_recirc_error);
+COVERAGE_DEFINE(datapath_drop_invalid_port);
+COVERAGE_DEFINE(datapath_drop_invalid_tnl_port);
Re: [ovs-dev] [PATCH v2] ofp-monitor: Added support for OpenFlow 1.4+ Flow Monitor
On Tue, Feb 05, 2019 at 04:26:56PM -0800, Ashish Varma wrote: > In the parse_flow_monitor_request(), usable_protocols is an out parameter. > (and is set in parse_flow_monitor_request() only if a match field is > provided. ) > allowed_versions on the other hand is used only in the ofctl_monitor > function. That is the reason for the check in ofctl_monitor. The usual way we handle this sort of version dependency is a bitmap like 'usable_protocols'. It works well for flows, for example. You can see lots of examples in ovs-ofctl.c if you search for the identifier usable_protocols. Looking closer at this instance, the existing code is buggy. Currently, only OpenFlow 1.0 should be supported, because that's the only protocol where OVS actually supports flow monitoring. parse_flow_monitor_request() should be returning that consistently as the out parameter (as you noted). Or it should be returning 0 if it's impossible to request a flow monitor at all (in the case where OF1.0 can't support whatever field is specified when parse_flow_monitor_request__ parses a field). But it's buggy and nothing ever properly initializes it. That bug is masked by another bug: nothing ever checks it, either! And, finally, there is a third bug: ovs-ofctl calls ofputil_append_flow_monitor_request() and blindly always uses OF1.0, which might not be the protocol actually in use on the vconn. The manpage for the ovs-ofctl monitor command doesn't say that "watch:" is OF1.0 only or that it is an Open vSwitch extension, either, although it should. It would be for the best if we can fix all these bugs before we add support for OF1.4+ flow monitor, and then backport the bug fixes. Does my description of the problems make sense? Can you tackle these problems too? Thanks, Ben. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] ofp-monitor: Added support for OpenFlow 1.4+ Flow Monitor
Thanks for the review! In the parse_flow_monitor_request(), usable_protocols is an out parameter. (and is set in parse_flow_monitor_request() only if a match field is provided. ) allowed_versions on the other hand is used only in the ofctl_monitor function. That is the reason for the check in ofctl_monitor. Would add the examples in tests/ofp-print.at I will add the monitoring support for OpenFlow 1.1, 1.2 and 1.3 as a separate patch. Thanks, Ashish On Mon, Feb 4, 2019 at 3:48 PM Ben Pfaff wrote: > On Thu, Jan 31, 2019 at 03:49:39PM -0800, Ashish Varma wrote: > > OVS supports Nicira version of Flow Monitor feature which allows an > OpenFlow > > controller to keep track of any changes in the flow table. (The changes > can > > done by the controller itself or by any other controller connected to > OVS.) > > This patch adds support for the OpenFlow 1.4+ OFPMP_FLOW_MONITOR > multipart > > message. > > Also added support in "ovs-ofctl monitor" to send OpenFlow 1.4+ messages > to > > OVS. > > Added unit test cases to test the OpenFlow version of Flow Monitor > messages. > > > > Signed-off-by: Ashish Varma > > Thanks for the revision! It will be nice to finally have this feature! > > Since parse_flow_monitor_request() already has a 'usable_protocols' > parameter, I would suggest using that to limit the support for > out_group, rather than putting a special case into ofctl_monitor(). > > I would add an example of the new message to tests/ofp-print.at. > > I think we talked about supporting flow monitors in OF1.1, 1.2, and > 1.3. I seem to recall that you plan to add that afterward, in a > separate patch. Is that correct? I hope that, if so, you can start > work on it soon after this patch is accepted. > > I would add an item to NEWS. > > Thanks, > > Ben. > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 5/6] vswitchd: Allow user to configure controllers as "primary" or "service".
On Tue, Feb 05, 2019 at 12:50:16PM -0800, Justin Pettit wrote: > > > On Oct 29, 2018, at 3:57 PM, Ben Pfaff wrote: > > > > Normally it makes sense for an active connection to be primary and a > > passive connection to be a service connection, but I've run into a corner > > case where it is better for a passive connection to be a primary > > connection. This specific case is for use with OFtest, which expects to be > > a primary controller. However, it also wants to reconnect frequently, > > which is slow for active connections because of the backoff; by > > configuring a passive, primary controller, OFtest can reconnect as > > frequently and as quickly as it wants, making the overall test much faster. > > > > Signed-off-by: Ben Pfaff > > Nice. Have you made the necessary changes for OFtest? I've been holding back further patches until these got in. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 6/6] ofproto: Don't always treat passive controllers as "equal".
On Tue, Feb 05, 2019 at 12:51:12PM -0800, Justin Pettit wrote: > > > On Oct 29, 2018, at 3:57 PM, Ben Pfaff wrote: > > > > If a passive controller chooses to configure itself as a slave controller, > > I don't know a reason why it should be considered "equal" for some > > purposes. > > > > Signed-off-by: Ben Pfaff > > Acked-by: Justin Pettit Thank you for the reviews. I applied them to master. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC PATCH v1 3/3] OVN ACL: Allow a user to input ct.label value for an acl
On Fri, Jan 11, 2019 at 12:16:39AM +, Ankur Sharma wrote: > This patch allows user to associate a value with acl, > which will be assigned to ct.label of the corresponding > connection tracking entry. > > This value can be used to map a ct entry with corresponding > OVN ACL or higher level constructs like security group. > > signed-off-by: Ankur Sharma Thanks for the patch! Please capitalize the "S" in "Signed-off-by". This adds a column in ovn-sb.ovsschema, so it should increment the minor version (the y in x.y.z). The documentation for the new column explains what it does, but it does not explain the purpose. Why would a user set this column? What are its effects? The column is a string, but its value is an integer. Maybe that is because OVSDB integer columns are limited to 64 bits, but this value can be 128 bits. That is a very large space. What is the reason that so much space should be dedicated to this identifier? Even 64 bits is more identifiers than any deployment will ever use, so there must be some other reason. Please do not use // comments. Please document the new option in the ovn-sbctl manpage. Please add a NEWS item for the new feature. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC PATCH v1 2/3] OVN ACL: Allow ct_mark and ct_label values to be set from register as well
On Fri, Jan 11, 2019 at 12:16:38AM +, Ankur Sharma wrote: > OVN allows only an integer (or masked integer) to be assigned to > ct_mark and ct_label. > > This patch, enhances the parser code to allow ct_mark and ct_label > to be assigned from 32 bit registers (MFF_REG0 - MFF_REG15) and 128 > bit registers (MFF_XXREG0 - MFF_XXREG3) respectively. > > signed-off-by: Ankur Sharma Thanks for the patch! Please capitalize "Signed-off-by" <-- just like that. When we add new support for OVN actions, we also add tests to the "ovn -- action parsing" test in ovn.at. Please add a test for a correctly formed action as well as at least one negative form that exercises each of the new error messages. Please also document the new form of the action in ovn-sb.xml. The error messages don't seem all that user friendly. Make sure that they clearly point out the problem. (That's probably easier when you add the tests.) The new forms break the level of abstraction that OVN fields are supposed to provide, that is, please use OVN field names rather than OpenFlow ones. I don't know why only registers are allowed. I don't believe that such a restriction exists at the OpenFlow level. I also don't know why only full, aligned registers are allowed. I don't think that restriction exists at the OpenFlow level either. It looks like the indentation is wrong here in encode_CT_COMMIT(). Thanks, Ben. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC PATCH v1 1/3] OVN ACL: Replace the usage of ct_label with ct_mark
On Fri, Jan 11, 2019 at 12:16:35AM +, Ankur Sharma wrote: > OVN ACL implementation used ct_label to indicate if a previosuly > allowed connection shoudl not be allowed anymore and vice versa. > > However, ct_label is a 128 bit value and we should rather leverage > on ct_mark which is a 32 bit value. > > Using ct_mark for this purpose, allows us to use ct_label for storing > other values like, identifier for corresponidng OVN ACL/Security group etc. > > signed-off-by: Ankur Sharma Thanks for the patch. I think that the idea here is to retain the existing ct_label.blocked for compatibility with older ovn-northd, so that during an upgrade the older logical flows continue to work. That is a good idea. I think that there should be a comment in logical-fields.c explaining why ct_label.blocked is still there. Then, someday in the future, when we think that upgrades from such an old version is no longer important, we will have an idea why it is there and that we can now to remove it. I find myself wondering, though, why we have ct_label.blocked at all. In some other cases where ovn-northd uses a bit specifically, it has a macro for it, e.g. REGBIT_CONNTRACK_COMMIT. Another option would be to have better abstraction, i.e. to name the bit "ct.blocked" instead of ct_mark.blocked or ct_label.blocked. Thanks, Ben. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1] rhel: retain OVS_CTL_OPTS for systemd service files
On 1/31/2019 10:38 AM, Aaron Conole wrote:
Martin Xu writes:
OVS init.d script calls ovs-ctl with $OVS_CTL_OPTS defined in the
config file. This variable is replaced by OPTIONS in systemd service
files. This patch addes $OVS_CTL_OPTS back to be passed along with $OPTIONS
for backward compatibility.
VMware-BZ: #2036847
Signed-off-by: Martin Xu
CC: Aaron Conole
---
I'm not sure why there should be two variables in the sysconfig file for
this. The following would preserve the old and not introduce a new
variable (I think.. it's completely untested). I guess this is because
the debian-distro openvswitch-switch.template file doesn't match the
rhel-distro template file, and we want to make a common set of systemd
scripts? Otherwise I don't see what the purpose is - what is the
migration path that this is addressing?
Aaron,
I owe you a response on this and will get to it but some fires need
putting out at the moment.
Thanks,
- Greg
---
diff --git a/rhel/usr_lib_systemd_system_ovsdb-server.service
b/rhel/usr_lib_systemd_system_ovsdb-server.service
index 09f946bb1..660ec75ef 100644
--- a/rhel/usr_lib_systemd_system_ovsdb-server.service
+++ b/rhel/usr_lib_systemd_system_ovsdb-server.service
@@ -12,6 +12,7 @@ EnvironmentFile=/etc/openvswitch/default.conf
EnvironmentFile=-/etc/sysconfig/openvswitch
ExecStartPre=/usr/bin/chown ${OVS_USER_ID} /var/run/openvswitch
/var/log/openvswitch
ExecStartPre=/bin/sh -c 'rm -f /run/openvswitch/useropts; if [ "$${OVS_USER_ID/:*/}" !=
"root" ]; then /usr/bin/echo "OVSUSER=--ovs-user=${OVS_USER_ID}" >
/run/openvswitch/useropts; fi'
+ExecStartPre=/bin/sh -c 'if [ "${OVS_CTL_OPTS}" != "" -a "${OPTIONS}" == "" ]; then /usr/bin/echo
"OPTIONS=\"${OPTIONS} ${OVS_CTL_OPTS}\"" >> /run/openvswitch/useropts; fi'
EnvironmentFile=-/run/openvswitch/useropts
ExecStart=/usr/share/openvswitch/scripts/ovs-ctl \
--no-ovs-vswitchd --no-monitor --system-id=random \
---
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 6/6] ofproto: Don't always treat passive controllers as "equal".
> On Oct 29, 2018, at 3:57 PM, Ben Pfaff wrote: > > If a passive controller chooses to configure itself as a slave controller, > I don't know a reason why it should be considered "equal" for some > purposes. > > Signed-off-by: Ben Pfaff Acked-by: Justin Pettit --Justin ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 5/6] vswitchd: Allow user to configure controllers as "primary" or "service".
> On Oct 29, 2018, at 3:57 PM, Ben Pfaff wrote: > > Normally it makes sense for an active connection to be primary and a > passive connection to be a service connection, but I've run into a corner > case where it is better for a passive connection to be a primary > connection. This specific case is for use with OFtest, which expects to be > a primary controller. However, it also wants to reconnect frequently, > which is slow for active connections because of the backoff; by > configuring a passive, primary controller, OFtest can reconnect as > frequently and as quickly as it wants, making the overall test much faster. > > Signed-off-by: Ben Pfaff Nice. Have you made the necessary changes for OFtest? Acked-by: Justin Pettit --Justin ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] checkpatch: Normalize exit code for Windows
Alin Gabriel Serdean writes:
> Using python `sys.exit(-1)` on Windows produces mixed results.
> Let's take the following results from different shells:
> CMD
>>python -c "import sys; sys.exit(-1)" & echo %errorlevel%
> 1
> MSYS
> $ python -c "import sys; sys.exit(-1)" && echo $?
> 0
> WSL
> $ python -c "import sys; sys.exit(-1)"; echo $?
> 255
>
> this results in the following tests to fail:
> checkpatch
>
> 10: checkpatch - sign-offs FAILED (checkpatch.at:32)
> 11: checkpatch - parenthesized constructs FAILED (checkpatch.at:32)
> 12: checkpatch - parenthesized constructs - for FAILED (checkpatch.at:32)
> 13: checkpatch - comments FAILED (checkpatch.at:32)
>
> because of:
> ./checkpatch.at:32: exit code was 0, expected 255
>
> This patch introduces a positive constant for the default exit code.
>
> Signed-off-by: Alin Gabriel Serdean
> ---
Acked-by: Aaron Conole
With one little fixup:
1. either we should eliminate the result argument to
ovs_checkpatch_print_result
2. or the test for the result < 0 needs to change to if result:
Otherwise, when an error is introduced, the exit code will be
propagated, but the printout will be confusing:
03:30:50 aconole {(0a8c1ec04...)} ~/git/ovs$ git commit -m "dpdk: This is a
test
Just testing
"
ERROR: C99 style comment
#10 FILE: lib/dpdk.c:17:
// BREAK!!
Lines checked: 15, no obvious problems found
:-)
> utilities/checkpatch.py | 13 +++--
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index 8eab31f60..8b305f2ad 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -24,6 +24,7 @@ import sys
> RETURN_CHECK_INITIAL_STATE = 0
> RETURN_CHECK_STATE_WITH_RETURN = 1
> RETURN_CHECK_AWAITING_BRACE = 2
> +EXIT_CODE = 255
> __errors = 0
> __warnings = 0
> empty_return_check_state = 0
> @@ -837,7 +838,7 @@ def ovs_checkpatch_parse(text, filename, author=None,
> committer=None):
>
> run_file_checks(text)
> if __errors or __warnings:
> -return -1
> +return EXIT_CODE
> return 0
>
>
> @@ -920,7 +921,7 @@ if __name__ == '__main__':
> "quiet"])
> except:
> print("Unknown option encountered. Please rerun with -h for help.")
> -sys.exit(-1)
> +sys.exit(EXIT_CODE)
>
> for o, a in optlist:
> if o in ("-h", "--help"):
> @@ -946,7 +947,7 @@ if __name__ == '__main__':
> quiet = True
> else:
> print("Unknown option '%s'" % o)
> -sys.exit(-1)
> +sys.exit(EXIT_CODE)
>
> if sys.stdout.isatty():
> colors = True
> @@ -974,13 +975,13 @@ Subject: %s
> result = ovs_checkpatch_parse(patch, revision)
> ovs_checkpatch_print_result(result)
> if result:
> -status = -1
> +status = EXIT_CODE
> sys.exit(status)
>
> if not args:
> if sys.stdin.isatty():
> usage()
> -sys.exit(-1)
> +sys.exit(EXIT_CODE)
> result = ovs_checkpatch_parse(sys.stdin.read(), '-')
> ovs_checkpatch_print_result(result)
> sys.exit(result)
> @@ -991,5 +992,5 @@ Subject: %s
> print('== Checking "%s" ==' % filename)
> result = ovs_checkpatch_file(filename)
> if result:
> -status = -1
> +status = EXIT_CODE
> sys.exit(status)
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next V2 1/1] openvswitch: Declare ovs key structures using macros
On Tue, Feb 05, 2019 at 10:22:09AM -0800, Gregory Rose wrote:
>
> On 2/5/2019 4:02 AM, Eli Britstein wrote:
> > On 2/4/2019 10:07 PM, David Miller wrote:
> > > From: Yi-Hung Wei
> > > Date: Mon, 4 Feb 2019 10:47:18 -0800
> > >
> > > > For example, to see how 'struct ovs_key_ipv6' is defined, now we need
> > > > to trace how OVS_KEY_IPV6_FIELDS is defined, and how OVS_KEY_FIELD_ARR
> > > > and OVS_KEY_FIELD defined. I think it makes the header file to be
> > > > more complicated.
> > > I completely agree.
> > >
> > > Unless this is totally unavoidable, I do not want to apply a patch
> > > which makes reading and auditing the networking code more difficult.
> > This technique is discussed for example in
> > https://stackoverflow.com/questions/6635851/real-world-use-of-x-macros,
> > and I found existing examples of using it in the kernel tree:
> >
> > __BPF_FUNC_MAPPER in commit ebb676daa1a34 ("bpf: Print function name in
> > addition to function id")
> >
> > __AAL_STAT_ITEMS and __SONET_ITEMS in commit 607ca46e97a1b ("UAPI:
> > (Scripted) Disintegrate include/linux"), the successor of commit
> > 1da177e4c3f4 ("Linux-2.6.12-rc2"). I didn't dig deeper to the past.
> >
> > I can agree it makes that H file a bit more complicated, but for sure
> > less than ## macros that are widely used.
> >
> > However, I think the alternatives of generating such defines by some
> > scripts, or having the fields in more than one place are even worse, so
> > it is a kind of unavoidable.
>
> Why is using a script to generate defines for the requirements of your
> application or driver so bad? Your patch
> turns openvswitch.h into some hardly readable code - using a script and
> having a machine output the macros
> your application or driver needs seems like a much better alternative to me.
In addition, one of the reasons that developers prefer to avoid
duplication is because of the need to synchronize copies when one of
them changes. This doesn't apply in the same way to these structures,
because they are ABIs that will not change.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC 1/3] dpdk: Unify passing of DPDK EAL arguments.
On Tue, Feb 05, 2019 at 06:43:08PM +0300, Ilya Maximets wrote: > Hi Flavio. > Thanks for taking a look. > > On 05.02.2019 15:38, Flavio Leitner wrote: > > > > Hi Ilya, > > > > Thanks for the patch and I think we knew about that pain when we > > exposed the very first parameter. I still remember Aaron struggling > > to find the best path forward. Time flies and here we are. > > > > The problem is that this change is not friendly from the community > > perspective to its users. That is like an exposed API which we should > > avoid break as much as possible. > > > > For instance, there are users (OpenStack) with support life cycle of > > 5+ years that cannot keep up with this kind of change but they expect > > to be able to update to newer OVS. > > Sure, there are users that wants stable API that will never change. > But this is really impossible in practice. I'm working with OpenStack > too and will have to fixup deployment tools with these changes. BTW, from > the whole OpenStack only few deployment projects (like TripleO) will need > to make changes and these changes are not very big. That's only part of the work. There will be work on QA, documentation and even migration path from one to another. And we can't change the past for existing deployments. > > One idea is to freeze whatever we have now and update the documentation > > to recommend the new way. We give like a couple OVS releases and only > > then ignore (or remove?) those parameters. > > Yes, In cover letter I proposed these patches to be applied one per release. > And current (first) patch does not remove the functionality, only docs. > Users still will be able to use old interface, but will have warnings > in the log. In the next release cycle we'll start ignore the values > while still printing the warnings. This should give enough time for > adaptation. > If you feel that we need more time, we could apply the second patch to 2.14 > (or whatever number will be in 2 releases from now). I don't think we should remove the docs if the parameters are there as a first step. I mean, assume an existing deployment, there is a parameter which might be in use but there is no documentation available. That doesn't sound like a good user experience to me. On another hand, you could introduce the new interface and update the docs to recommend using the new one because the old one will be removed in the future. Warning messages come next, and then finally its removal. > > IMO in the end we are moving the problem from one place to another > > because even with a single string, OVS users will be caught off guard > > when DPDK changes. Yes, less pain to OVS community in the sense that > > we don't have to add/remove/deprecate stuff, but it is still a bad > > user experience regardless, which is not what OVS is known for. > > Unfortunately, DPDK was never user-friendly enough. It tries, but still not. Agreed. > We're keeping few sane defaults like providing default lcore and setting the > socket-limit if needed. And we'll try to do that in the future. The thing > this patch tries to eliminate is the dependency tracking between different > EAL arguments, i.e. duplicating the work with rte_eal_init() and having too > many configuration knobs with similar meanings what does not work at the > same time. > > Right now there are no critical arguments that user must provide. > So, IMHO, having special configs for them is really unnecessary. Do you think the defaults work for the majority of DPDK users? If we expose only the necessary things to consume DPDK it means we have tight control (less combinations to worry about), otherwise if the user can pass anything, a can of worms is opened and we have no control of all things the user can do. fbl ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next V2 1/1] openvswitch: Declare ovs key structures using macros
On 2/5/2019 4:02 AM, Eli Britstein wrote:
On 2/4/2019 10:07 PM, David Miller wrote:
From: Yi-Hung Wei
Date: Mon, 4 Feb 2019 10:47:18 -0800
For example, to see how 'struct ovs_key_ipv6' is defined, now we need
to trace how OVS_KEY_IPV6_FIELDS is defined, and how OVS_KEY_FIELD_ARR
and OVS_KEY_FIELD defined. I think it makes the header file to be
more complicated.
I completely agree.
Unless this is totally unavoidable, I do not want to apply a patch
which makes reading and auditing the networking code more difficult.
This technique is discussed for example in
https://stackoverflow.com/questions/6635851/real-world-use-of-x-macros,
and I found existing examples of using it in the kernel tree:
__BPF_FUNC_MAPPER in commit ebb676daa1a34 ("bpf: Print function name in
addition to function id")
__AAL_STAT_ITEMS and __SONET_ITEMS in commit 607ca46e97a1b ("UAPI:
(Scripted) Disintegrate include/linux"), the successor of commit
1da177e4c3f4 ("Linux-2.6.12-rc2"). I didn't dig deeper to the past.
I can agree it makes that H file a bit more complicated, but for sure
less than ## macros that are widely used.
However, I think the alternatives of generating such defines by some
scripts, or having the fields in more than one place are even worse, so
it is a kind of unavoidable.
Why is using a script to generate defines for the requirements of your
application or driver so bad? Your patch
turns openvswitch.h into some hardly readable code - using a script and
having a machine output the macros
your application or driver needs seems like a much better alternative to me.
- Greg
Please reconsider regarding applying this patch.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] Remove support for OpenFlow 1.6 (draft).
On Mon, Feb 04, 2019 at 05:25:50PM -0800, Justin Pettit wrote: > > > On Jan 17, 2019, at 4:20 PM, Ben Pfaff wrote: > > > > ONF abandoned the OpenFlow specification, so that OpenFlow 1.6 will never > > be completed. It did not contain much in the way of useful features, so > > remove what support Open vSwitch already had. > > > > Signed-off-by: Ben Pfaff > > Acked-by: Justin Pettit Thanks, applied to master. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 4/6] connmgr: Make treatment of active and passive connections more uniform.
On Mon, Feb 04, 2019 at 05:51:11PM -0800, Justin Pettit wrote: > > > On Oct 29, 2018, at 3:57 PM, Ben Pfaff wrote: > > > > Until now, connmgr has handled active and passive OpenFlow connections in > > quite different ways. Any active connection, whether it was currently > > connected or not, was always maintained as an ofconn. Whenever such a > > connection (re)connected, its settings were cleared. On the other hand, > > passive connections had a separate listener which created an ofconn when > > a new connection came in, and these ofconns would be deleted when such a > > connection was closed. This approach is inelegant and has occasionally > > led to bugs when reconnection didn't clear all of the state that it > > should have. > > > > There's another motivation here. Currently, active connections are > > always primary controllers and passive connections are always service > > controllers (as documented in ovs-vswitchd.conf.db(5)). Sometimes it would > > be useful to have passive primary controllers (maybe active service > > controllers too but I haven't personally run into that use case). As is, > > this is difficult to implement because there is so much different code in > > use between active and passive connections. This commit will make it > > easier. > > > > Signed-off-by: Ben Pfaff > > Acked-by: Justin Pettit Thanks, applied to master. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] checkpatch: Normalize exit code for Windows
On Tue, Feb 05, 2019 at 05:05:19PM +0200, Alin Gabriel Serdean wrote: > Using python `sys.exit(-1)` on Windows produces mixed results. > Let's take the following results from different shells: > CMD > >python -c "import sys; sys.exit(-1)" & echo %errorlevel% > 1 > MSYS > $ python -c "import sys; sys.exit(-1)" && echo $? > 0 > WSL > $ python -c "import sys; sys.exit(-1)"; echo $? > 255 > > this results in the following tests to fail: > checkpatch > > 10: checkpatch - sign-offs FAILED (checkpatch.at:32) > 11: checkpatch - parenthesized constructs FAILED (checkpatch.at:32) > 12: checkpatch - parenthesized constructs - for FAILED (checkpatch.at:32) > 13: checkpatch - comments FAILED (checkpatch.at:32) > > because of: > ./checkpatch.at:32: exit code was 0, expected 255 > > This patch introduces a positive constant for the default exit code. > > Signed-off-by: Alin Gabriel Serdean Curious! I did not know about this particular nonportable behavior. Acked-by: Ben Pfaff ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC 1/3] dpdk: Unify passing of DPDK EAL arguments.
Hi Flavio. Thanks for taking a look. On 05.02.2019 15:38, Flavio Leitner wrote: > > Hi Ilya, > > Thanks for the patch and I think we knew about that pain when we > exposed the very first parameter. I still remember Aaron struggling > to find the best path forward. Time flies and here we are. > > The problem is that this change is not friendly from the community > perspective to its users. That is like an exposed API which we should > avoid break as much as possible. > > For instance, there are users (OpenStack) with support life cycle of > 5+ years that cannot keep up with this kind of change but they expect > to be able to update to newer OVS. Sure, there are users that wants stable API that will never change. But this is really impossible in practice. I'm working with OpenStack too and will have to fixup deployment tools with these changes. BTW, from the whole OpenStack only few deployment projects (like TripleO) will need to make changes and these changes are not very big. > > One idea is to freeze whatever we have now and update the documentation > to recommend the new way. We give like a couple OVS releases and only > then ignore (or remove?) those parameters. Yes, In cover letter I proposed these patches to be applied one per release. And current (first) patch does not remove the functionality, only docs. Users still will be able to use old interface, but will have warnings in the log. In the next release cycle we'll start ignore the values while still printing the warnings. This should give enough time for adaptation. If you feel that we need more time, we could apply the second patch to 2.14 (or whatever number will be in 2 releases from now). > > IMO in the end we are moving the problem from one place to another > because even with a single string, OVS users will be caught off guard > when DPDK changes. Yes, less pain to OVS community in the sense that > we don't have to add/remove/deprecate stuff, but it is still a bad > user experience regardless, which is not what OVS is known for. Unfortunately, DPDK was never user-friendly enough. It tries, but still not. We're keeping few sane defaults like providing default lcore and setting the socket-limit if needed. And we'll try to do that in the future. The thing this patch tries to eliminate is the dependency tracking between different EAL arguments, i.e. duplicating the work with rte_eal_init() and having too many configuration knobs with similar meanings what does not work at the same time. Right now there are no critical arguments that user must provide. So, IMHO, having special configs for them is really unnecessary. > > Thanks, > fbl > > > On Fri, Feb 01, 2019 at 04:11:12PM +0300, Ilya Maximets wrote: >> This patch deprecates the usage of current configuration knobs for >> DPDK EAL arguments: >> >> - dpdk-alloc-mem >> - dpdk-socket-mem >> - dpdk-socket-limit >> - dpdk-lcore-mask >> - dpdk-hugepage-dir >> - dpdk-extra >> >> All above configuration options replaced with single 'dpdk-options' >> which has same format as old 'dpdk-extra', i.e. just a string with >> all the DPDK EAL arguments. >> >> All the documentation about old configuration knobs removed. Users >> could still use an old interface, but the deprecation warning will be >> printed. In case 'dpdk-options' provided, values of old options will >> be ignored. New users will start using new 'dpdk-options' as this is >> the only documented way providing EAL arguments. >> >> Rationale: >> >> From release to release DPDK becomes more complex. New EAL arguments >> appears, old arguments becomes deprecated. Some changes their meaning >> and behaviour. It's not easy task to manage all this and current >> code in OVS that tries to manage DPDK options is not flexible/extendable >> enough even to track all the dependencies between options in DPDK 18.11. >> For example, '--socket-mem' changed its meaning, '--legacy-mem' and >> '--socket-limit' appeared. But '--legacy-mem' and '--socket-limit' >> could not be used at the same time and, also, we want to provide >> good default value for '--socket-limit' to keep the consistent >> behaviour of OVS memory usage. And this will be worse in the future. >> >> Another point is that even now we have mutually exclusive database >> configs in OVS and we have to handle them. i.e we're providing >> 'dpdk-alloc-mem' and 'dpdk-socket-mem' at the same time. Additionally, >> user allowed to configure same options in 'dpdk-extra'. So, we have >> similar/equal options in a three places in ovsdb and we need to >> choose which one to use. It's pretty easy for user to get into >> inconsistent database configuration, because users could update >> one field without removing others, and OVS has to resolve all the >> conflicts somehow constructing the EAL args. But we do not know in >> practice, if the resulted arguments are the arguments that user wanted >> to see or just forget to remove the higher priority knob. >> >> Next point is that we
[ovs-dev] [PATCH] checkpatch: Normalize exit code for Windows
Using python `sys.exit(-1)` on Windows produces mixed results.
Let's take the following results from different shells:
CMD
>python -c "import sys; sys.exit(-1)" & echo %errorlevel%
1
MSYS
$ python -c "import sys; sys.exit(-1)" && echo $?
0
WSL
$ python -c "import sys; sys.exit(-1)"; echo $?
255
this results in the following tests to fail:
checkpatch
10: checkpatch - sign-offs FAILED (checkpatch.at:32)
11: checkpatch - parenthesized constructs FAILED (checkpatch.at:32)
12: checkpatch - parenthesized constructs - for FAILED (checkpatch.at:32)
13: checkpatch - comments FAILED (checkpatch.at:32)
because of:
./checkpatch.at:32: exit code was 0, expected 255
This patch introduces a positive constant for the default exit code.
Signed-off-by: Alin Gabriel Serdean
---
utilities/checkpatch.py | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 8eab31f60..8b305f2ad 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -24,6 +24,7 @@ import sys
RETURN_CHECK_INITIAL_STATE = 0
RETURN_CHECK_STATE_WITH_RETURN = 1
RETURN_CHECK_AWAITING_BRACE = 2
+EXIT_CODE = 255
__errors = 0
__warnings = 0
empty_return_check_state = 0
@@ -837,7 +838,7 @@ def ovs_checkpatch_parse(text, filename, author=None,
committer=None):
run_file_checks(text)
if __errors or __warnings:
-return -1
+return EXIT_CODE
return 0
@@ -920,7 +921,7 @@ if __name__ == '__main__':
"quiet"])
except:
print("Unknown option encountered. Please rerun with -h for help.")
-sys.exit(-1)
+sys.exit(EXIT_CODE)
for o, a in optlist:
if o in ("-h", "--help"):
@@ -946,7 +947,7 @@ if __name__ == '__main__':
quiet = True
else:
print("Unknown option '%s'" % o)
-sys.exit(-1)
+sys.exit(EXIT_CODE)
if sys.stdout.isatty():
colors = True
@@ -974,13 +975,13 @@ Subject: %s
result = ovs_checkpatch_parse(patch, revision)
ovs_checkpatch_print_result(result)
if result:
-status = -1
+status = EXIT_CODE
sys.exit(status)
if not args:
if sys.stdin.isatty():
usage()
-sys.exit(-1)
+sys.exit(EXIT_CODE)
result = ovs_checkpatch_parse(sys.stdin.read(), '-')
ovs_checkpatch_print_result(result)
sys.exit(result)
@@ -991,5 +992,5 @@ Subject: %s
print('== Checking "%s" ==' % filename)
result = ovs_checkpatch_file(filename)
if result:
-status = -1
+status = EXIT_CODE
sys.exit(status)
--
2.16.1.windows.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] travis: Speed up linux kernel downloads.
On Tue, Feb 05, 2019 at 10:16:04AM +0300, Ilya Maximets wrote: > CDN links are much faster in average. https://www.kernel.org/ > links shows usually less than 10 MB/s, while https://cdn.kernel.org/ > could give up to 200 MB/s and usually shows speeds much higher than > 10 MB/s. Also, 'xz' archives are 30-50 MB smaller than gzip ones. > It takes a bit more time to unpack them, but it's negligible in Thanks, Ilya! I applied this to master. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [ovs-dev, v2] netdev-dpdk: Memset rte_flow_item on a need basis.
On 05.02.2019 16:09, Asaf Penso wrote:
>
>
> Regards,
> Asaf Penso
>
>> -Original Message-
>> From: Ilya Maximets
>> Sent: Tuesday, February 5, 2019 1:46 PM
>> To: Asaf Penso ; ovs-dev@openvswitch.org
>> Cc: Roni Bar Yanai ; Stokes, Ian
>> ; Finn Christensen
>> Subject: Re: [ovs-dev,v2] netdev-dpdk: Memset rte_flow_item on a need
>> basis.
>>
>> On 04.02.2019 19:14, Asaf Penso wrote:
>>> In netdev_dpdk_add_rte_flow_offload function different rte_flow_item
>>> are created as part of the pattern matching.
>>>
>>> For most of them, there is a check whether the wildcards are not zero.
>>> In case of zero, nothing is being done with the rte_flow_item.
>>>
>>> Befor the wildcard check, and regardless of the result, the
>>> rte_flow_item is being memset.
>>>
>>> The patch moves the memset function only if the condition of the
>>> wildcard is true, thus saving cycles of memset if not needed.
>>>
>>> Signed-off-by: Asaf Penso
>>> ---
>>
>> I thought about this part of code a bit. IMHO, we could create a union from
>> the L4 items and clear them all at once. Like this:
>>
>> uint8_t proto = 0;
>> struct flow_items {
>> struct rte_flow_item_eth eth;
>> struct rte_flow_item_vlan vlan;
>> struct rte_flow_item_ipv4 ipv4;
>> union {
>> struct rte_flow_item_tcp tcp;
>> struct rte_flow_item_udp udp;
>> struct rte_flow_item_sctp sctp;
>> struct rte_flow_item_icmp icmp;
>> };
>> } spec, mask;
>> uint8_t *ipv4_next_proto_mask = _proto_id;
>>
>> memset(, 0, sizeof spec);
>> memset(, 0, sizeof mask);
>>
>>
>> Ethernet is a common case, userspace datapath always has exact match on
>> vlan,
>> IPv4 is also the common case, I think. So current code in most cases we'll
>> not
>> call memset only for few of L4 protocols which are in the union here and
>> does not eat extra memory.
>> Anyway we're not on a very hot path.
>>
>> With that you'll be able to easily convert L4 proto checking 'if's to a
>> single
>> 'switch (proto)' statement and also clear the *ipv4_next_proto_mask
>> unconditionally.
>>
>> What do you think ?
>
> I fully agree and if you can have a separate patch it would be great.
OK. Good.
I'll prepare the patch as soon as this one will be merged.
>
>>
>> You could incorporate these changes to this patch or I could prepare the
>> separate patch on top of it.
>>
>> Anyway, for the current version:
>>
>> Acked-by: Ilya Maximets
>>
>
> Thanks!
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [ovs-dev, v2] netdev-dpdk: Memset rte_flow_item on a need basis.
Regards,
Asaf Penso
> -Original Message-
> From: Ilya Maximets
> Sent: Tuesday, February 5, 2019 1:46 PM
> To: Asaf Penso ; ovs-dev@openvswitch.org
> Cc: Roni Bar Yanai ; Stokes, Ian
> ; Finn Christensen
> Subject: Re: [ovs-dev,v2] netdev-dpdk: Memset rte_flow_item on a need
> basis.
>
> On 04.02.2019 19:14, Asaf Penso wrote:
> > In netdev_dpdk_add_rte_flow_offload function different rte_flow_item
> > are created as part of the pattern matching.
> >
> > For most of them, there is a check whether the wildcards are not zero.
> > In case of zero, nothing is being done with the rte_flow_item.
> >
> > Befor the wildcard check, and regardless of the result, the
> > rte_flow_item is being memset.
> >
> > The patch moves the memset function only if the condition of the
> > wildcard is true, thus saving cycles of memset if not needed.
> >
> > Signed-off-by: Asaf Penso
> > ---
>
> I thought about this part of code a bit. IMHO, we could create a union from
> the L4 items and clear them all at once. Like this:
>
> uint8_t proto = 0;
> struct flow_items {
> struct rte_flow_item_eth eth;
> struct rte_flow_item_vlan vlan;
> struct rte_flow_item_ipv4 ipv4;
> union {
> struct rte_flow_item_tcp tcp;
> struct rte_flow_item_udp udp;
> struct rte_flow_item_sctp sctp;
> struct rte_flow_item_icmp icmp;
> };
> } spec, mask;
> uint8_t *ipv4_next_proto_mask = _proto_id;
>
> memset(, 0, sizeof spec);
> memset(, 0, sizeof mask);
>
>
> Ethernet is a common case, userspace datapath always has exact match on
> vlan,
> IPv4 is also the common case, I think. So current code in most cases we'll not
> call memset only for few of L4 protocols which are in the union here and
> does not eat extra memory.
> Anyway we're not on a very hot path.
>
> With that you'll be able to easily convert L4 proto checking 'if's to a single
> 'switch (proto)' statement and also clear the *ipv4_next_proto_mask
> unconditionally.
>
> What do you think ?
I fully agree and if you can have a separate patch it would be great.
>
> You could incorporate these changes to this patch or I could prepare the
> separate patch on top of it.
>
> Anyway, for the current version:
>
> Acked-by: Ilya Maximets
>
Thanks!
> > v2 update coding-style compliant for sizeof operator
> > ---
> > lib/netdev-dpdk.c | 36 ++--
> > 1 file changed, 18 insertions(+), 18 deletions(-)
> >
> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> > 4bf0ca9..f07b10c 100644
> > --- a/lib/netdev-dpdk.c
> > +++ b/lib/netdev-dpdk.c
> > @@ -4570,18 +4570,18 @@ netdev_dpdk_add_rte_flow_offload(struct
> netdev *netdev,
> > /* Eth */
> > struct rte_flow_item_eth eth_spec;
> > struct rte_flow_item_eth eth_mask;
> > -memset(_spec, 0, sizeof(eth_spec));
> > -memset(_mask, 0, sizeof(eth_mask));
> > if (!eth_addr_is_zero(match->wc.masks.dl_src) ||
> > !eth_addr_is_zero(match->wc.masks.dl_dst)) {
> > -rte_memcpy(_spec.dst, >flow.dl_dst,
> sizeof(eth_spec.dst));
> > -rte_memcpy(_spec.src, >flow.dl_src,
> sizeof(eth_spec.src));
> > +memset(_spec, 0, sizeof eth_spec);
> > +memset(_mask, 0, sizeof eth_mask);
> > +rte_memcpy(_spec.dst, >flow.dl_dst, sizeof
> eth_spec.dst);
> > +rte_memcpy(_spec.src, >flow.dl_src, sizeof
> > + eth_spec.src);
> > eth_spec.type = match->flow.dl_type;
> >
> > rte_memcpy(_mask.dst, >wc.masks.dl_dst,
> > - sizeof(eth_mask.dst));
> > + sizeof eth_mask.dst);
> > rte_memcpy(_mask.src, >wc.masks.dl_src,
> > - sizeof(eth_mask.src));
> > + sizeof eth_mask.src);
> > eth_mask.type = match->wc.masks.dl_type;
> >
> > add_flow_pattern(, RTE_FLOW_ITEM_TYPE_ETH, @@
> > -4600,9 +4600,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev
> *netdev,
> > /* VLAN */
> > struct rte_flow_item_vlan vlan_spec;
> > struct rte_flow_item_vlan vlan_mask;
> > -memset(_spec, 0, sizeof(vlan_spec));
> > -memset(_mask, 0, sizeof(vlan_mask));
> > if (match->wc.masks.vlans[0].tci && match->flow.vlans[0].tci) {
> > +memset(_spec, 0, sizeof vlan_spec);
> > +memset(_mask, 0, sizeof vlan_mask);
> > vlan_spec.tci = match->flow.vlans[0].tci & ~htons(VLAN_CFI);
> > vlan_mask.tci = match->wc.masks.vlans[0].tci &
> > ~htons(VLAN_CFI);
> >
> > @@ -4617,9 +4617,9 @@ netdev_dpdk_add_rte_flow_offload(struct
> netdev *netdev,
> > uint8_t proto = 0;
> > struct rte_flow_item_ipv4 ipv4_spec;
> > struct rte_flow_item_ipv4 ipv4_mask;
> > -memset(_spec, 0, sizeof(ipv4_spec));
> > -memset(_mask, 0, sizeof(ipv4_mask));
> > if (match->flow.dl_type == htons(ETH_TYPE_IP)) {
> > +memset(_spec, 0, sizeof ipv4_spec);
> > +memset(_mask, 0, sizeof ipv4_mask);
> >
>
Re: [ovs-dev] [RFC 1/3] dpdk: Unify passing of DPDK EAL arguments.
Hi Ilya, Thanks for the patch and I think we knew about that pain when we exposed the very first parameter. I still remember Aaron struggling to find the best path forward. Time flies and here we are. The problem is that this change is not friendly from the community perspective to its users. That is like an exposed API which we should avoid break as much as possible. For instance, there are users (OpenStack) with support life cycle of 5+ years that cannot keep up with this kind of change but they expect to be able to update to newer OVS. One idea is to freeze whatever we have now and update the documentation to recommend the new way. We give like a couple OVS releases and only then ignore (or remove?) those parameters. IMO in the end we are moving the problem from one place to another because even with a single string, OVS users will be caught off guard when DPDK changes. Yes, less pain to OVS community in the sense that we don't have to add/remove/deprecate stuff, but it is still a bad user experience regardless, which is not what OVS is known for. Thanks, fbl On Fri, Feb 01, 2019 at 04:11:12PM +0300, Ilya Maximets wrote: > This patch deprecates the usage of current configuration knobs for > DPDK EAL arguments: > > - dpdk-alloc-mem > - dpdk-socket-mem > - dpdk-socket-limit > - dpdk-lcore-mask > - dpdk-hugepage-dir > - dpdk-extra > > All above configuration options replaced with single 'dpdk-options' > which has same format as old 'dpdk-extra', i.e. just a string with > all the DPDK EAL arguments. > > All the documentation about old configuration knobs removed. Users > could still use an old interface, but the deprecation warning will be > printed. In case 'dpdk-options' provided, values of old options will > be ignored. New users will start using new 'dpdk-options' as this is > the only documented way providing EAL arguments. > > Rationale: > > From release to release DPDK becomes more complex. New EAL arguments > appears, old arguments becomes deprecated. Some changes their meaning > and behaviour. It's not easy task to manage all this and current > code in OVS that tries to manage DPDK options is not flexible/extendable > enough even to track all the dependencies between options in DPDK 18.11. > For example, '--socket-mem' changed its meaning, '--legacy-mem' and > '--socket-limit' appeared. But '--legacy-mem' and '--socket-limit' > could not be used at the same time and, also, we want to provide > good default value for '--socket-limit' to keep the consistent > behaviour of OVS memory usage. And this will be worse in the future. > > Another point is that even now we have mutually exclusive database > configs in OVS and we have to handle them. i.e we're providing > 'dpdk-alloc-mem' and 'dpdk-socket-mem' at the same time. Additionally, > user allowed to configure same options in 'dpdk-extra'. So, we have > similar/equal options in a three places in ovsdb and we need to > choose which one to use. It's pretty easy for user to get into > inconsistent database configuration, because users could update > one field without removing others, and OVS has to resolve all the > conflicts somehow constructing the EAL args. But we do not know in > practice, if the resulted arguments are the arguments that user wanted > to see or just forget to remove the higher priority knob. > > Next point is that we have 'dpdk-lcore-mask' (-c) which is actually not > so user-friendly as '-l', but we have no option for it. Will we create > additional 'dpdk-lcore-list' ? I guess, no, because it's not really > important thing. But users will stuck with this not so user-friendly > masks. > > Another thing is that OVS is not really need to check the consistency > of the EAL arguments because DPDK could check it instead. DPDK will > always check the args and it will do it better. There is no real > need to duplicate this functionality. > > Keeping all the options in a single string 'dpdk-options' allows: > > * To have only one place for all the configs, i.e. it will be harder > for user to forget to remove something from the single string while > updating it. > * Not to check the consistency between different configs, DPDK could > check the cmdline itself. > * Not to document every DPDK EAL argument in OVS. We can just > describe few of them and point to DPDK docs for more information. > * OVS still able to provide some minimal default arguments. > Thanks to DPDK 18.11 we only need to specify an lcore. All other > arguments are not necessary. (We still also providing default > --socket-limit in case --socket-mem passed without it and without > --legacy-mem) > > Usage example: > > ovs-vsctl set Open_vSwitch . \ > other_config:dpdk-options="-l 1 -n 4 --socket-mem 1024,1024" > > Signed-off-by: Ilya Maximets > --- > Documentation/intro/install/dpdk.rst | 40 +--- > NEWS | 7 +- > lib/dpdk.c |
Re: [ovs-dev] [PATCH net-next V2 1/1] openvswitch: Declare ovs key structures using macros
On 2/4/2019 10:07 PM, David Miller wrote:
> From: Yi-Hung Wei
> Date: Mon, 4 Feb 2019 10:47:18 -0800
>
>> For example, to see how 'struct ovs_key_ipv6' is defined, now we need
>> to trace how OVS_KEY_IPV6_FIELDS is defined, and how OVS_KEY_FIELD_ARR
>> and OVS_KEY_FIELD defined. I think it makes the header file to be
>> more complicated.
> I completely agree.
>
> Unless this is totally unavoidable, I do not want to apply a patch
> which makes reading and auditing the networking code more difficult.
This technique is discussed for example in
https://stackoverflow.com/questions/6635851/real-world-use-of-x-macros,
and I found existing examples of using it in the kernel tree:
__BPF_FUNC_MAPPER in commit ebb676daa1a34 ("bpf: Print function name in
addition to function id")
__AAL_STAT_ITEMS and __SONET_ITEMS in commit 607ca46e97a1b ("UAPI:
(Scripted) Disintegrate include/linux"), the successor of commit
1da177e4c3f4 ("Linux-2.6.12-rc2"). I didn't dig deeper to the past.
I can agree it makes that H file a bit more complicated, but for sure
less than ## macros that are widely used.
However, I think the alternatives of generating such defines by some
scripts, or having the fields in more than one place are even worse, so
it is a kind of unavoidable.
Please reconsider regarding applying this patch.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [ovs-dev, v2] netdev-dpdk: Memset rte_flow_item on a need basis.
On 04.02.2019 19:14, Asaf Penso wrote:
> In netdev_dpdk_add_rte_flow_offload function different rte_flow_item are
> created as part of the pattern matching.
>
> For most of them, there is a check whether the wildcards are not zero.
> In case of zero, nothing is being done with the rte_flow_item.
>
> Befor the wildcard check, and regardless of the result, the
> rte_flow_item is being memset.
>
> The patch moves the memset function only if the condition of the
> wildcard is true, thus saving cycles of memset if not needed.
>
> Signed-off-by: Asaf Penso
> ---
I thought about this part of code a bit. IMHO, we could create
a union from the L4 items and clear them all at once. Like this:
uint8_t proto = 0;
struct flow_items {
struct rte_flow_item_eth eth;
struct rte_flow_item_vlan vlan;
struct rte_flow_item_ipv4 ipv4;
union {
struct rte_flow_item_tcp tcp;
struct rte_flow_item_udp udp;
struct rte_flow_item_sctp sctp;
struct rte_flow_item_icmp icmp;
};
} spec, mask;
uint8_t *ipv4_next_proto_mask = _proto_id;
memset(, 0, sizeof spec);
memset(, 0, sizeof mask);
Ethernet is a common case, userspace datapath always has exact match on vlan,
IPv4 is also the common case, I think. So current code in most cases we'll
not call memset only for few of L4 protocols which are in the union here and
does not eat extra memory.
Anyway we're not on a very hot path.
With that you'll be able to easily convert L4 proto checking 'if's to a single
'switch (proto)' statement and also clear the *ipv4_next_proto_mask
unconditionally.
What do you think ?
You could incorporate these changes to this patch or I could prepare the
separate
patch on top of it.
Anyway, for the current version:
Acked-by: Ilya Maximets
> v2 update coding-style compliant for sizeof operator
> ---
> lib/netdev-dpdk.c | 36 ++--
> 1 file changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 4bf0ca9..f07b10c 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -4570,18 +4570,18 @@ netdev_dpdk_add_rte_flow_offload(struct netdev
> *netdev,
> /* Eth */
> struct rte_flow_item_eth eth_spec;
> struct rte_flow_item_eth eth_mask;
> -memset(_spec, 0, sizeof(eth_spec));
> -memset(_mask, 0, sizeof(eth_mask));
> if (!eth_addr_is_zero(match->wc.masks.dl_src) ||
> !eth_addr_is_zero(match->wc.masks.dl_dst)) {
> -rte_memcpy(_spec.dst, >flow.dl_dst, sizeof(eth_spec.dst));
> -rte_memcpy(_spec.src, >flow.dl_src, sizeof(eth_spec.src));
> +memset(_spec, 0, sizeof eth_spec);
> +memset(_mask, 0, sizeof eth_mask);
> +rte_memcpy(_spec.dst, >flow.dl_dst, sizeof eth_spec.dst);
> +rte_memcpy(_spec.src, >flow.dl_src, sizeof eth_spec.src);
> eth_spec.type = match->flow.dl_type;
>
> rte_memcpy(_mask.dst, >wc.masks.dl_dst,
> - sizeof(eth_mask.dst));
> + sizeof eth_mask.dst);
> rte_memcpy(_mask.src, >wc.masks.dl_src,
> - sizeof(eth_mask.src));
> + sizeof eth_mask.src);
> eth_mask.type = match->wc.masks.dl_type;
>
> add_flow_pattern(, RTE_FLOW_ITEM_TYPE_ETH,
> @@ -4600,9 +4600,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
> /* VLAN */
> struct rte_flow_item_vlan vlan_spec;
> struct rte_flow_item_vlan vlan_mask;
> -memset(_spec, 0, sizeof(vlan_spec));
> -memset(_mask, 0, sizeof(vlan_mask));
> if (match->wc.masks.vlans[0].tci && match->flow.vlans[0].tci) {
> +memset(_spec, 0, sizeof vlan_spec);
> +memset(_mask, 0, sizeof vlan_mask);
> vlan_spec.tci = match->flow.vlans[0].tci & ~htons(VLAN_CFI);
> vlan_mask.tci = match->wc.masks.vlans[0].tci & ~htons(VLAN_CFI);
>
> @@ -4617,9 +4617,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
> uint8_t proto = 0;
> struct rte_flow_item_ipv4 ipv4_spec;
> struct rte_flow_item_ipv4 ipv4_mask;
> -memset(_spec, 0, sizeof(ipv4_spec));
> -memset(_mask, 0, sizeof(ipv4_mask));
> if (match->flow.dl_type == htons(ETH_TYPE_IP)) {
> +memset(_spec, 0, sizeof ipv4_spec);
> +memset(_mask, 0, sizeof ipv4_mask);
>
> ipv4_spec.hdr.type_of_service = match->flow.nw_tos;
> ipv4_spec.hdr.time_to_live= match->flow.nw_ttl;
> @@ -4662,9 +4662,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
>
> struct rte_flow_item_tcp tcp_spec;
> struct rte_flow_item_tcp tcp_mask;
> -memset(_spec, 0, sizeof(tcp_spec));
> -memset(_mask, 0, sizeof(tcp_mask));
> if (proto == IPPROTO_TCP) {
> +memset(_spec, 0, sizeof tcp_spec);
> +memset(_mask, 0, sizeof tcp_mask);
> tcp_spec.hdr.src_port = match->flow.tp_src;
> tcp_spec.hdr.dst_port =
