Hi Dumitru, Sorry to reply you so late. The ovn-match-ip utility has been submitted, and using ovn-match-ip is much more convenient than before. Therefore, the disable-combine-ipv4 option has also been removed The code and commit log also briefly describe and organize the algorithm. The test in this example has been added as 'ip4.src == $set5'.
Thanks, Gongming Chen On 2021/2/10, 4:20 AM, "Dumitru Ceara" <[email protected]> wrote: Resending as my previous try didn't make it on the mailing list. On 2/9/21 1:06 PM, Dumitru Ceara wrote: > 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
