On 2/9/21 12:26 PM, gmingchen(陈供明) wrote:
Hi Dumitru,
Thanks for review.
On 2021/2/8, 6:19 PM, "Dumitru Ceara" <[email protected]> wrote:
On 2/7/21 1:25 PM, gmingchen(陈供明) wrote:
> From: Gongming Chen <[email protected]>
Hi Gongming,
First of all, thanks for the contribution!
This is not a full review, just some comments for now.
It seems that there's a memory leak with your patch applied.
AddressSanitizer reports:
Direct leak of 12 byte(s) in 1 object(s) allocated from:
#0 0x49644d in malloc
(/home/runner/work/ovn/ovn/ovn-20.12.90/_build/sub/tests/ovstest+0x49644d)
#1 0x538604 in xmalloc
/home/runner/work/ovn/ovn/ovs_src/lib/util.c:138:15
#2 0x62636f in expr_const_sets_add
/home/runner/work/ovn/ovn/ovn-20.12.90/_build/sub/../../lib/expr.c:1237:18
#3 0x4cf8f6 in create_addr_sets
/home/runner/work/ovn/ovn/ovn-20.12.90/_build/sub/../../tests/test-ovn.c:230:5
#4 0x4cf8f6 in test_parse_expr__
/home/runner/work/ovn/ovn/ovn-20.12.90/_build/sub/../../tests/test-ovn.c:296:5
#5 0x4dfd04 in ovs_cmdl_run_command__
/home/runner/work/ovn/ovn/ovs_src/lib/command-line.c:247:17
#6 0x4c6810 in test_ovn_main
/home/runner/work/ovn/ovn/ovn-20.12.90/_build/sub/../../tests/test-ovn.c:1635:5
#7 0x4c6810 in ovstest_wrapper_test_ovn_main__
/home/runner/work/ovn/ovn/ovn-20.12.90/_build/sub/../../tests/test-ovn.c:1638:1
#8 0x4dfd04 in ovs_cmdl_run_command__
/home/runner/work/ovn/ovn/ovs_src/lib/command-line.c:247:17
#9 0x4c5fe3 in main
/home/runner/work/ovn/ovn/ovn-20.12.90/_build/sub/../../tests/ovstest.c:150:9
#10 0x7f71aa6ddbf6 in __libc_start_main
(/lib/x86_64-linux-gnu/libc.so.6+0x21bf6)
Full reports are available in the ovsrobot OVN github CI run
artifacts:
https://github.com/ovsrobot/ovn/actions/runs/545416492
Just a note, if you push the branch to your own fork it will
trigger the
github action to run CI and the "linux clang test asan" job will
also
enable AddressSanitizer.
Thanks, there are really missing the free ip_data.ip, resulting in a
memory leak.
Ok.
There are also a few style related issues (e.g., sizeof args),
please
see the guidelines here:
https://github.com/ovn-org/ovn/blob/master/Documentation/internals/contributing/coding-style.rst
Sorry, does this mean that
ip_r_data->ip = xmalloc(4 * sizeof(uint32_t))
should be replaced with
ip_r_data->ip = xmalloc(4 * sizeof ip_r_data->ip)?
Actually:
sizeof *ip_r_data->ip
>
> In the ovn security group, each host ip corresponds to at least
4 flow
> tables (different connection states). As the scale of hosts
using the
> security group increases, the ovs security group flow table will
> increase sharply, especially when it is applied the remote group
> feature in OpenStack.
>
> This patch merges ipv4 addresses with wildcard masks, and
replaces this
> ipv4 addresses with the merged ip/mask. This will greatly
reduce the
> entries in the ovs security group flow table, especially when
the host
> size is large. After being used in a production environment,
the number
> of ovs flow tables will be reduced by at least 50% in most
scenarios,
> when the remote group in OpenStack is applied.
I think it would be great to describe the algorithm here, in the
commit
log, but also in the comments in the code.
You are right, I will resubmit and add the description of the
algorithm to the
commit log and the code comments.
Cool, thanks.
>
> Analysis in the simplest scenario, a network 1.1.1.0/24
network, enable
> the OpenStack security group remote group feature, create 253
virtual
> machine ports(1.1.1.2-1.1.1.254).
>
> Only focus on the number of ip addresses, in the table=44 table:
> ./configure --disable-combine-ipv4:
> 1.1.1.2-1.1.1.254(253 flow meters) * 4(connection status) *
> 1(local net of localport) = 1012
>
> ./configure --enable-combine-ipv4(default):
> 1.1.1.2/31
> 1.1.1.4/30
> 1.1.1.8/29
> 1.1.1.16/28
> 1.1.1.32/27
> 1.1.1.64/26
> 1.1.1.128/26
> 1.1.1.192/27
> 1.1.1.224/28
> 1.1.1.240/29
> 1.1.1.248/30
> 1.1.1.252/31
> 1.1.1.254
> 13 flow tables * 4(connection status) * 1(local net of
localport) = 52
>
> Reduced from 1012 flow meters to 52, a 19.4 times reduction.
>
> Some scenes are similar to the following:
> 1.1.1.2, 1.1.1.6
> After the combine:
> 1.1.1.2/255.255.255.251
> This will slightly increase the difficulty of finding the flow
table
> corresponding to a single address.
> such as:
> ovs-ofctl dump-flows br-int | grep 1.1.1.6
> The result is empty.
> 1.1.1.6 will match 1.1.1.2/255.255.255.251
Would it make sense and potentially make the life of the users
easier by
also adding a utility to automatically do the IP/MASK match?
I'm thinking of something like:
$ ovs-ofctl dump-flows br-int | ovn-match-ip 1.1.1.6
[..]
I totally agree with you, adding a utility similar to ovn-match-ip
will be more
optimized and easier to troubleshoot.
I will resubmit and add an ovn-match-ip utility.
Nice, thanks.
I didn't look at the implementation in detail but I'm surprised
by the
following:
- change test-ovn.c to define $set1 = {1.1.1.2, .. 1.1.1.254},
basically
the example you gave above.
- then run:
$ echo 'ip4.src == $set1' | ./tests/ovstest test-ovn expr-to-flows
ip,nw_src=1.1.1.2/31
ip,nw_src=1.1.1.4
$
Shouldn't this output actually match the 13 entries in your
sample output?
I guess you forgot to modify the 3 in test-ovn.c
expr_const_sets_add(addr_sets, "set1", addrs1, 3, true) to 253.
Oh you're right, my bad, sorry about that. What about adding this as
an explicit test in ovn.at?
I also tested it, and the output is the same as the 13 entries in the
above example:
$ echo'ip4.src == $set1' | ./tests/ovstest test-ovn expr-to-flows
ip,nw_src=1.1.1.64/26
ip,nw_src=1.1.1.128/26
ip,nw_src=1.1.1.252/31
ip,nw_src=1.1.1.248/30
ip,nw_src=1.1.1.2/31
ip,nw_src=1.1.1.32/27
ip,nw_src=1.1.1.240/29
ip,nw_src=1.1.1.254
ip,nw_src=1.1.1.8/29
ip,nw_src=1.1.1.224/28
ip,nw_src=1.1.1.4/30
ip,nw_src=1.1.1.16/28
ip,nw_src=1.1.1.192/27
$
>
> Signed-off-by: Gongming Chen <[email protected]>
> ---
> configure.ac | 1 +
> lib/expr.c | 217 +++++++++++++++++++++++++++++++++++
> m4/ovn.m4 | 21 ++++
> tests/atlocal.in | 1 +
> tests/ovn.at | 286
+++++++++++++++++++++++++++++++++--------------
> 5 files changed, 443 insertions(+), 83 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index b2d084318..6dc51a5cc 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -131,6 +131,7 @@ OVS_LIBTOOL_VERSIONS
> OVS_CHECK_CXX
> AX_FUNC_POSIX_MEMALIGN
> OVN_CHECK_UNBOUND
> +OVN_CHECK_COMBINE_IPV4
Is there a specific reason to not have this enabled by default? It
would be great if we could avoid HAVE_COMBINE_IPV4 ifdefs in the
code
and tests.
In fact, it is enabled by default.
Previously, because there is no utility like ovn-match-ip, disable the
feature is more helpful for troubleshooting, when the scale is small.
After adding the ovn-match-ip utility, I think it does not matter whether
there is an option to disable this feature.
Then I think HAVE_COMBINE_IPV4 ifdefs can be removed.
Great, that's what I was hoping for.
>
> OVS_CHECK_INCLUDE_NEXT([stdio.h string.h])
> AC_CONFIG_FILES([lib/libovn.sym])
> diff --git a/lib/expr.c b/lib/expr.c
> index 796e88ac7..0b6dee3b3 100644
> --- a/lib/expr.c
> +++ b/lib/expr.c
> @@ -1030,6 +1030,194 @@ expr_constant_set_destroy(struct
expr_constant_set *cs)
> }
> }
>
> +struct ip_v
> +{
> + uint32_t *ip;
> + uint32_t size;
> +};
> +
> +struct ip_r
> +{
> + uint32_t *ip;
> + uint32_t *mask;
> + uint32_t used;
> + uint32_t size;
> + bool *masked;
> +};
> +
> +/* Macro to check ip return data and xrealloc. */
In my opinion it would be clearer if this would be a function.
I agree with you, I will replace this macro with function.
> +#define CHECK_REALLOC_IP_R_DATA(IP_R_DATA) \
> + if (IP_R_DATA->used >= IP_R_DATA->size) { \
Probably best to wrap IP_R_DATA as "(IP_R_DATA)"
Yes, that would be better.
> + IP_R_DATA->ip = xrealloc(IP_R_DATA->ip, \
> + 2 * IP_R_DATA->size * sizeof(uint32_t)); \
> + IP_R_DATA->mask = xrealloc(IP_R_DATA->mask, \
> + 2 * IP_R_DATA->size * sizeof(uint32_t)); \
> + IP_R_DATA->masked = xrealloc(IP_R_DATA->masked, \
> + 2 * IP_R_DATA->size * sizeof(bool)); \
> + IP_R_DATA->size = IP_R_DATA->size * 2; \
> + }
> +
> +static void
> +combine_ipv4_in_mask(struct ip_v *ip_data, struct ip_r
*ip_r_data){
Some comments describing the algorithm would help.
I'll wait for your feedback on all the above before continuing
the review.
Thanks,
Dumitru
In addition to the commit log, I will also add a brief description of
the algorithm here.
Thanks,
Gongming
Great, thanks again!
Dumitru
