Current algorithm of ovsdb_datum_union looks like this:
for-each atom in b:
if not bin_search(a, atom):
push(a, clone(atom))
quicksort(a)
So, the complexity looks like this:
Nb * log2(Na) + Nb + (Na + Nb) * log2(Na + Nb)
Comparisons clones Comparisons for quicksort
for search
ovsdb_datum_union() is heavily used in database transactions while
new element is added to a set. For example, if new logical switch
port is added to a logical switch in OVN. This is a very common
use case where CMS adds one new port to an existing switch that
already has, let's say, 100 ports. For this case ovsdb-server will
have to perform:
1 * log2(100) + 1 clone + 101 * log2(101)
Comparisons Comparisons for
for search quicksort.
~7 1 ~707
Roughly 714 comparisons of atoms and 1 clone.
Since binary search can give us position, where new atom should go
(it's the 'low' index after the search completion) for free, the
logic can be re-worked like this:
copied = 0
for-each atom in b:
desired_position = bin_search(a, atom)
push(result, a[ copied : desired_position - 1 ])
copied = desired_position
push(result, clone(atom))
push(result, a[ copied : Na ])
swap(a, result)
Complexity of this schema:
Nb * log2(Na) + Nb + Na
Comparisons clones memory copy on push
for search
'swap' is just a swap of a few pointers. 'push' is not a 'clone',
but a simple memory copy of 'union ovsdb_atom'.
In general, this schema substitutes complexity of a quicksort
with complexity of a memory copy of Na atom structures, where we're
not even copying strings that these atoms are pointing to.
Complexity in the example above goes down from 714 comparisons
to 7 comparisons and memcpy of 100 * sizeof (union ovsdb_atom) bytes.
General complexity of a memory copy should always be lower than
complexity of a quicksort, especially because these copies usually
performed in bulk, so this new schema should work faster for any input.
All in all, this change allows to execute several times more
transactions per second for transactions that adds new entries to sets.
Alternatively, union can be implemented as a linear merge of two
sorted arrays, but this will result in O(Na) comparisons, which
is more than Nb * log2(Na) in common case, since Na is usually
far bigger than Nb. Linear merge will also mean per-atom memory
copies instead of copying in bulk.
'replace' functionality of ovsdb_datum_union() had no users, so it
just removed. But it can easily be added back if needed in the future.
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Acked-by: Han Zhou <hzhou@ovn.org>
Acked-by: Mark D. Gray <mark.d.gray@redhat.com>
I would have found these useful for the OVN tests. The {in} operator
is the same as {<=}, but it's still useful to have the alternate syntax
because most of the time we think of set inclusion separately from
set subsets. The {not-in} operator is different from any existing
operator though.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Ilya Maximets <i.maximets@ovn.org>
Valgrind reported:
2491: database commands -- negative checks
==19245== 36 (32 direct, 4 indirect) bytes in 1 blocks are definitely lost in loss record 36 of 53
==19245== at 0x4C2FD5F: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==19245== by 0x431AB4: xrealloc (util.c:149)
==19245== by 0x41656D: ovsdb_datum_reallocate (ovsdb-data.c:1883)
==19245== by 0x41656D: ovsdb_datum_union (ovsdb-data.c:1961)
==19245== by 0x4107B2: cmd_add (db-ctl-base.c:1494)
==19245== by 0x406E2E: do_vsctl (ovs-vsctl.c:2626)
==19245== by 0x406E2E: main (ovs-vsctl.c:183)
==19252== 16 bytes in 1 blocks are definitely lost in loss record 9 of 52
==19252== at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==19252== by 0x430F74: xmalloc (util.c:138)
==19252== by 0x414D07: clone_atoms (ovsdb-data.c:990)
==19252== by 0x4153F6: ovsdb_datum_clone (ovsdb-data.c:1012)
==19252== by 0x4104D3: cmd_remove (db-ctl-base.c:1564)
==19252== by 0x406E2E: do_vsctl (ovs-vsctl.c:2626)
==19252== by 0x406E2E: main (ovs-vsctl.c:183)
This patch fixes them.
Acked-by: William Tu <u9012063@gmail.com>
Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Tables and columns may be abbreviated to unique prefixes, but until
now the error messages have just said there's more than one match.
This commit makes the error messages list the possibilities.
Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Memory leak occured in case specified key was not found in table
record.
Signed-off-by: Damijan Skvarc <damjan.skvarc@gmail.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Let the caller decide how to handle the error. Prepare for using the
parser in ovn-nbctl daemon mode.
Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Let the caller handle the error. Needed for ovn-nbctl daemon mode.
Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Return the error via the context instead of calling ctl_fatal() so that
the caller can decide how to handle it.
Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Traditionally, for boolean variables we use boolean values.
Lets keep to that tradition.
Hopefully, using false with a bool works with gcc 6.3.1;
I use both recent versions of gcc (7.3) and older
versions (4.x), but did not see the issue found in
165c1f0649af commit.
Cc: Ian Stokes<ian.stokes@intel.com>
Fixes: 165c1f0649af ("db-ctl-base: Fix compilation warnings.")
Signed-off-by: Darrell Ball <dlu998@gmail.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
This commit fixes uninitialized variable warnings in functions
cmd_create() and cmd_get() when compiling with gcc 6.3.1 and -Werror
by initializing variables 'symbol' and 'new' to NULL.
Cc: Alex Wang <alexw@nicira.com>
Fixes: 07ff77ccb82a ("db-ctl-base: Make common database command code into library.")
Signed-off-by: Ian Stokes <ian.stokes@intel.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Return the error via the context instead of calling ctl_fatal() so that
the caller can decide how to handle it.
Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Return the error via the context instead of calling ctl_fatal() so that
the caller can decide how to handle it.
Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Return the error via the context instead of calling ctl_fatal() so that
the caller can decide how to handle it.
Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Return the error via the context instead of calling ctl_fatal() so that
the caller can decide how to handle it.
Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Release resources now that we are returning to the caller on error.
Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Release resources now that we are returning to the caller on error.
Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Release resources now that we are returning to the caller on error.
Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Propagate the error via the context for the caller to handle it.
Result of applying the following semantic patch (Coccinelle):
@@
expression s;
@@
- die_if_error(s);
+ ctx->error = s;
+ if (ctx->error) {
+ return;
+ }
Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Prepare for the command handlers (pre_cmd_*() cmd_*() functions) to
report errors by storing them in the context.
Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Return the error message to the caller instead of reporting it and dying
so that the caller can handle the error without terminating the process
if needed.
Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Return the error message to the caller instead of reporting it and dying
so that the caller can handle the error without terminating the process
if needed.
Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Return the error message to the caller instead of reporting it and dying
so that the caller can handle the error without terminating the process
if needed.
Also, we no longer return the column as it was not used by any of
existing callers.
Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Return the error message to the caller instead of reporting it and dying
so that the caller can handle the error without terminating the process
if needed.
Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Return the error message to the caller instead of reporting it and dying
so that the caller can handle the error without terminating the process
if needed.
Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Return the error message to the caller instead of reporting it and dying
so that the caller can handle the error without terminating the process
if needed.
Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Signal that multiple rows match the record identifier via a new output
parameter instead of reporting the problem and dying, so that the caller
can handle the error without terminating the process if needed.
Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Return the error message to the caller instead of reporting it and dying
so that the caller can handle the error without terminating the process
if needed.
Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Return the error message to the caller instead of reporting it and dying
so that the caller can handle the error without terminating the process
if needed.
Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Return the error message to the caller instead of reporting it and dying
so that the caller can handle the error without terminating the process
if needed.
Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Return the error message to the caller instead of reporting it and dying
so that the caller can handle the error without terminating the process
if needed.
Also, rename the function as it is no longer a typical predicate, so
that the users don't assume that the result is passed in return value.
Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Return the error message to the caller instead of reporting it and dying
so that the caller can handle the error without terminating the process
if needed.
Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Return the error message to the caller instead of reporting it and dying
so that the caller can handle the error without terminating the process
if needed.
Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
These utilities logged the command very early, before parsing the options
or the command. This meant that logging options (like --log-file or
-vsyslog:off) weren't considered for the purpose of logging the command.
This fixes the problem.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Justin Pettit <jpettit@ovn.org>
Several OVS structs contain embedded named unions, like this:
struct {
...
union {
...
} u;
};
C11 standardized a feature that many compilers already implemented
anyway, where an embedded union may be unnamed, like this:
struct {
...
union {
...
};
};
This is more convenient because it allows the programmer to omit "u."
in many places. OVS already used this feature in several places. This
commit embraces it in several others.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Justin Pettit <jpettit@ovn.org>
Tested-by: Alin Gabriel Serdean <aserdean@ovn.org>
Acked-by: Alin Gabriel Serdean <aserdean@ovn.org>
The ovn-nbctl, ovn-sbctl, and ovs-vsctl manpages are inconsistent in
their "Database Commands" section when it comes to referring to what
database tables exist. This commit amends this by making each *ctl
manpage reference the corresponding database manpage instead.
To aid in having a more handy list, the --help text of ovn-nbctl,
ovn-sbctl, and ovs-vsctl have been modified to list the available
tables. This is also referenced in the manpages for those applications.
Signed-off-by: Mark Michelson <mmichels@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
In C++, 'class' is a keyword. If this is used as the name for a field,
then C++ compilers can get confused about the context and fail to
compile references to such fields. Rename the field to 'class_' to
avoid this issue.
Signed-off-by: Joe Stringer <joe@ovn.org>
Acked-by: Ben Pfaff <blp@ovn.org>
Shadowing is when a variable with a given name in an inner scope hides a
different variable with the same name in a surrounding scope. This is
generally undesirable because it can confuse programmers. This commit
eliminates most of it.
Found with -Wshadow=local in GCC 7. The repo is not really ready to enable
this option by default because of a few cases that are harder to fix, and
harmless, such as nested use of CMAP_FOR_EACH.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Andy Zhou <azhou@ovn.org>
If a particular column is supposed to be reached by following a reference
from a UUID column, then that really needs to happen; if there's no
reference, then we're probably starting from a row in the wrong table.
This fixes an assertion failure in command "ovs-vsctl list netflow br0",
if bridge br0 without any netflows.
$ovs-vsctl list netflow br0
ovs-vsctl: lib/ovsdb-idl.c:2407: assertion column_idx < class->n_columns failed
in ovsdb_idl_read()
Aborted
Fixes: 3f5b5f7b4115 ("db-ctl-base: Always support all tables in schema.")
Signed-off-by: Zhipeng Lu <lu.zhipeng@zte.com.cn>
Signed-off-by: Ben Pfaff <blp@ovn.org>
This is like the --ovs option to ovn-trace, but it applies to every flow
dumped, so it has different applications.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Andy Zhou <azhou@ovn.org>
This renames get_row() to ctl_get_row() and makes it public. It's
unfortunate that it adds a cast, but getting rid of redundant code seems
worth it to me.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Andy Zhou <azhou@ovn.org>
This allows commands like "ovn-sbctl lflow-list abcdef" to find a
datapath that has external-ids:logical-switch=abcdef12-3456-...
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Andy Zhou <azhou@ovn.org>
This will be used in an upcoming commit to allow Datapath_Binding records
in the OVN southbound database to be identified based on external-ids:name
and other map values.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Andy Zhou <azhou@ovn.org>
The 'table' field is redundant because the required 'column' field
implies the table that the column is a part of.
This simplifies the users and makes it harder to get these things wrong.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Andy Zhou <azhou@ovn.org>
This will have another caller in an upcoming commit.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Andy Zhou <azhou@ovn.org>
Acked-by: Russell Bryant <russell@ovn.org>
Until now, uuid_is_partial_string() returned the number of characters at
the beginning of a string that were the beginning of a valid UUID. This
is useful, but all of the callers actually wanted to get a value of 0 if
the string contained a character that was invalid for a UUID. This makes
that change.
Examples:
"123" previously yielded 3 and still does.
"xyzzy" previously yielded 0 and still does.
"123xyzzy" previously yielded 3, now yields 0.
"e66250bb-9531-491b-b9c3-5385cabb0080" previously yielded 36, still does.
"e66250bb-9531-491b-b9c3-5385cabb0080xyzzy" previously yielded 36, now 0.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Andy Zhou <azhou@ovn.org>
Acked-by: Russell Bryant <russell@ovn.org>
This makes it easier to type ovs-vsctl, ovn-sbctl, ovn-nbctl, and vtep-ctl
commands without cut-and-paste.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Andy Zhou <azhou@ovn.org>
