On Tue, Nov 7, 2023 at 6:12 PM Han Zhou <[email protected]> wrote: > > > > On Tue, Nov 7, 2023 at 8:06 AM Dumitru Ceara <[email protected]> wrote: > > > > On 11/6/23 08:19, Han Zhou wrote: > > > On Sun, Nov 5, 2023 at 10:59 PM Ales Musil <[email protected]> wrote: > > >> > > >> > > >> > > >> On Sat, Nov 4, 2023 at 5:45 AM Han Zhou <[email protected]> wrote: > > >>> > > >>> > > >>> > > >>> On Fri, Nov 3, 2023 at 1:08 AM Ales Musil <[email protected]> wrote: > > >>>> > > >>>> > > >>>> > > >>>> On Tue, Oct 24, 2023 at 9:36 PM Han Zhou <[email protected]> wrote: > > >>>>> > > >>>>> Enhance MAC_Binding aging to allow CIDR-based threshold > > > configurations. > > >>>>> This enables distinct threshold settings for different IP ranges, > > >>>>> applying the longest prefix matching for overlapping ranges. > > >>>>> > > >>>>> A common use case involves setting a default threshold for all IPs, > > > while > > >>>>> disabling aging for a specific range and potentially excluding a > > > subset > > >>>>> within that range. > > >>>>> > > >>>>> Signed-off-by: Han Zhou <[email protected]> > > >>>>> --- > > >>>> > > >>>> > > >>>> Hi Han, > > >>>> > > >>>> thank you for the patch, I have a couple of comments down below. > > >>> > > >>> Thanks for your review! > > >>> > > > > > > Hi Han, > > > > Thanks for the patch! Not a very in-depth review but I left a bunch of > > minor comments and shared my opinion on the configuration format. > > > > Thanks for your review! > > > Regards, > > Dumitru > > > > >>>> > > >>>>> > > >>>>> northd/aging.c | 297 > > > +++++++++++++++++++++++++++++++++++++++++++++--- > > >>>>> northd/aging.h | 3 + > > >>>>> northd/northd.c | 11 +- > > >>>>> ovn-nb.xml | 63 +++++++++- > > >>>>> tests/ovn.at | 60 ++++++++++ > > >>>>> 5 files changed, 413 insertions(+), 21 deletions(-) > > >>>>> > > >>>>> diff --git a/northd/aging.c b/northd/aging.c > > >>>>> index f626c72c8ca3..e5868211a63b 100644 > > >>>>> --- a/northd/aging.c > > >>>>> +++ b/northd/aging.c > > >>>>> @@ -47,12 +47,253 @@ aging_waker_schedule_next_wake(struct > > > aging_waker *waker, int64_t next_wake_ms) > > >>>>> } > > >>>>> } > > >>>>> > > >>>>> +struct threshold_entry { > > >>>>> + union { > > >>>>> + ovs_be32 ipv4; > > >>>>> + struct in6_addr ipv6; > > >>>>> + } prefix; > > >>>>> + bool is_v4; > > >>>> > > >>>> > > >>>> We can avoid the is_v4 whatsover by storing the address as mapped v4 > > > in 'in6_addr', I saw the concern and we can still have separate arrays for > > > v4 and v6. The parsing IMO becomes much simpler if we use the mapped v4 for > > > both see down below. > > >>>> > > >>> > > >>> I did consider using in6_addr, but it would make the > > > find_threshold_for_ip() slightly more tedious because for every v4 entry, > > > even if stored in a separate array, we will still need to call > > > in6_addr_get_mapped_ipv4() to convert it. While using the union we can > > > directly access what we need (ipv4 or ipv6). > > >> > > >> > > >> We wouldn't have to actually call in6_addr_get_mapped_ipv4() for the v4 > > > variant, that's why I suggested that approach. We can adjust the mask and > > > then use the same logic as for ipv6 (entry->plen += 96 should do the trick). > > >> > > > > > > Understood, and my reply below was about that: > > >>> The search may be simplified a little in find_threshold_for_ip() with > > > adjusted prefix for v4, only if we combine the two arrays to a single > > > array. Otherwise we will need to have a function to abstract and reuse the > > > ipv6 matching logic, while the ipv4 matching is really just 3 lines of code > > > :) > > > > > >>> > > >>> The is_v4 member was needed when using a single array, but not necessary > > > any more when storing v4 and v6 entries separately, but I kept it just for > > > convenience, so that I don't need to use another arg in > > > parse_threshold_entry() to return the AF type. Using in6_addr can avoid the > > > extra arg, too, but similarly, we would have to always call the macro > > > IN6_IS_ADDR_V4MAPPED, which doesn't seem cleaner than the is_v4 field to > > > me. Using ip46_parse_cidr does save several lines of code, but that code > > > snippet itself is quite small. So with all these being considered, I think > > > either approach doesn't really make too much difference. I am totally fine > > > to change it if you still think a single in6_addr is better than the union. > > >>> > > >>>>> > > >>>>> + unsigned int plen; > > >>>>> + unsigned int threshold; > > >>>> > > >>>> > > >>>> I personally prefer sized integers. > > >>>> > > >>> I think it may be better to match the type of related helper functions. > > >>> For plen, it is ip[v6]_parse_cidr(). > > >>> For threshold, it is str_to_uint(), or even smap_get_uint() returns > > > unsigned int. > > >>> > > >>>>> > > >>>>> +}; > > >>>>> + > > >>>>> +/* Contains CIDR-based aging threshold configuration parsed from > > >>>>> + * "Logical_Router:options:mac_binding_age_threshold". > > >>>>> + * > > >>>>> + * This struct is also used for non-CIDR-based threshold, e.g. the > > > ones from > > >>>>> + * "NB_Global:other_config:fdb_age_threshold" for the common > > > aging_context > > >>>>> + * interface. > > >>>>> + * > > >>>>> + * - The arrays `v4_entries` and `v6_entries` are populated with > > > parsed entries > > >>>>> + * for IPv4 and IPv6 CIDRs, respectively, along with their > > > associated > > >>>>> + * thresholds. Entries within these arrays are sorted by prefix > > > length, > > >>>>> + * starting with the longest. > > >>>>> + * > > >>>>> + * - If a threshold is provided without an accompanying prefix, it's > > > captured > > >>>>> + * in `default_threshold`. In cases with multiple unprefixed > > > thresholds, > > >>>>> + * `default_threshold` will only store the last one. */ > > >>>>> +struct threshold_config { > > >>>>> + struct threshold_entry *v4_entries; > > >>>>> + int n_v4_entries; > > > > Nit: size_t > > > Ack > > > >>>>> + struct threshold_entry *v6_entries; > > >>>>> + int n_v6_entries; > > > > Nit: size_t > > > Ack > > > >>>>> + unsigned int default_threshold; > > >>>>> +}; > > >>>>> + > > >>>>> +static int > > >>>>> +compare_entries_by_prefix_length(const void *a, const void *b) > > >>>>> +{ > > >>>>> + const struct threshold_entry *entry_a = a; > > >>>>> + const struct threshold_entry *entry_b = b; > > >>>>> + > > >>>>> + return entry_b->plen - entry_a->plen; > > >>>>> +} > > >>>>> + > > >>>>> +/* Parse an ENTRY in the threshold option, with the format: > > >>>>> + * [CIDR:]THRESHOLD > > >>>>> + * > > >>>>> + * Returns true if successful, false if failed. */ > > >>>>> +static bool > > >>>>> +parse_threshold_entry(const char *str, struct threshold_entry *entry) > > >>>>> +{ > > >>>>> + char *colon_ptr; > > >>>>> + unsigned int value; > > >>>>> + const char *threshold_str; > > >>>>> + > > >>>>> + colon_ptr = strrchr(str, ':'); > > >>>> > > >>>> > > >>>> I find this a bit confusing and probably error prone, could we use > > > different delimiters for the threshold? I guess we could change it to > > > 'cidr;threshold,cidr;threshold' WDYT? This would allow us to use > > > 'strtok_r()' here and we could spare the multiple copies and replace with a > > > single one. > > >>>> > > >>> I thought about that, too, but decided to use ":" instead of ";" because > > > "A:B" looks more natural to represent "A has a value B", and ";" is more > > > like a delimiter to separate two different elements, just like ",". So I > > > feel that ":" might be more friendly to end users. I'd love to hear > > > opinions from more people. > > >>> If ";" or something else turned out to be a better option for the end > > > user, we can use strtok_r() here but I think it shouldn't be a concern if > > > we can't use it. > > > > IMO ":" is better. But that's just personal preference. > > > OK > > > >>> > > >>>>> > > >>>>> + if (!colon_ptr) { > > >>>>> + threshold_str = str; > > >>>>> + entry->plen = 0; > > >>>>> + } else { > > >>>>> + threshold_str = colon_ptr + 1; > > >>>>> + } > > >>>>> + > > >>>>> + if (!str_to_uint(threshold_str, 10, &value)) { > > >>>>> + return false; > > >>>>> + } > > >>>>> + entry->threshold = value; > > >>>>> + > > >>>>> + if (!colon_ptr) { > > >>>>> + return true; > > >>>>> + } > > >>>>> + > > >>>>> + /* ":" was found, so parse the string before ":" as a cidr. */ > > >>>>> + char ip_cidr[128]; > > >>>>> + ovs_strzcpy(ip_cidr, str, MIN(colon_ptr - str + 1, sizeof > > > ip_cidr)); > > >>>>> + char *error = ip_parse_cidr(ip_cidr, &entry->prefix.ipv4, > > > &entry->plen); > > >>>> > > >>>> > > >>>> As mentioned earlier by storing it in 'in6_addr' we could use > > > 'ip46_parse_cidr()' which would simplify the logic. We can also adjust the > > > prefix for v4 so the search is the same for both. > > >>> > > >>> The search may be simplified a little in find_threshold_for_ip() with > > > adjusted prefix for v4, only if we combine the two arrays to a single > > > array. Otherwise we will need to have a function to abstract and reuse the > > > ipv6 matching logic, while the ipv4 matching is really just 3 lines of code > > > :) > > >>> But if you prefer to use a single array for both v4 and v6, I can try > > > that in v2. > > >>> > > >>>> > > >>>>> > > >>>>> + if (!error) { > > >>>>> + entry->is_v4 = true; > > >>>>> + return true; > > >>>>> + } > > >>>>> + free(error); > > >>>>> + error = ipv6_parse_cidr(ip_cidr, &entry->prefix.ipv6, > > > &entry->plen); > > > > If I'm not wrong this is ambiguous for cases when the cidr length is not > > present. > > > > Maybe we should add a test for an (explicit and implicit) /128 config. > > > Both ip_parse_cidr and ipv6_parse_cidr accept different formats including addresses without mask/length. There are actually lots of combinations but I think it should belong to unit tests of those functions instead of testing them everywhere when they are used. > I did include random examples in the test case including an IPv4 address without mask and I can add one IPv6 without mask, too, but still, it is not supposed to be a thorough test for those CIDR parsing functions. > > > >>>>> + if (!error) { > > >>>>> + entry->is_v4 = false; > > >>>>> + return true; > > >>>>> + } > > >>>>> + free(error); > > >>>>> + return false; > > >>>>> +} > > >>>>> + > > >>>>> +static void > > >>>>> +threshold_config_destroy(struct threshold_config *config) > > >>>>> +{ > > >>>>> + free(config->v4_entries); > > >>>>> + free(config->v6_entries); > > >>>>> + config->v4_entries = config->v6_entries = NULL; > > >>>>> + config->n_v4_entries = config->n_v6_entries = 0; > > >>>>> + config->default_threshold = 0; > > >>>>> +} > > >>>>> + > > >>>>> +/* Parse the threshold option string, which has the format: > > >>>>> + * ENTRY[;ENTRY[...]] > > >>>>> + * > > >>>>> + * For the exact format of ENTRY, refer to the function > > >>>>> + * `parse_threshold_entry`. > > >>>>> + * > > >>>>> + * The parsed data is populated to the struct threshold_config. > > >>>>> + * See the comments of struct threshold_config for details. > > >>>>> + * > > >>>>> + * Return Values: > > >>>>> + * - Returns `false` if the input does not match the expected format. > > >>>>> + * Consequently, no entries will be populated. > > >>>>> + * - Returns `true` upon successful parsing. The caller is > > > responsible for > > >>>>> + * releasing the allocated memory by calling > > > threshold_config_destroy. */ > > >>>>> +static bool > > >>>>> +parse_aging_threshold(const char *opt, > > >>>>> + struct threshold_config *config) > > >>>>> +{ > > >>>>> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > > >>>>> + struct threshold_entry e; > > >>>>> + const char *start = opt, *end; > > >>>>> + memset(config, 0, sizeof *config); > > >>>>> + > > >>>>> + while (start && *start) { > > >>>>> + end = strchr(start, ';'); > > >>>> > > >>>> > > >>>> We should use 'strtok_r()' this again allows us to avoid the constant > > > copies replaced by single one. > > >>> > > >>> Make sense. I will change this in v2. > > >>> > > >>>> > > >>>>> > > >>>>> + if (end) { > > >>>>> + /* Big enough for any <ipv6>/<ipv6 mask>:<threshold> */ > > >>>>> + char buf[128]; > > >>>>> + ovs_strzcpy(buf, start, MIN(end - start + 1, sizeof > > > buf)); > > >>>>> + if (!parse_threshold_entry(buf, &e)) { > > >>>>> + VLOG_WARN_RL(&rl, "Parsing aging threshold '%s' > > > failed.", buf); > > >>>>> + goto err; > > >>>>> + } > > >>>>> + start = end + 1; > > >>>>> + } else { > > >>>>> + if (!parse_threshold_entry(start, &e)) { > > >>>>> + VLOG_WARN_RL(&rl, "Parsing aging threshold '%s' > > > failed.", > > >>>>> + start); > > >>>>> + goto err; > > >>>>> + } > > >>>>> + start += strlen(start); > > >>>>> + } > > >>>>> + if (!e.plen) { > > >>>>> + config->default_threshold = e.threshold; > > >>>>> + } else if (e.is_v4) { > > >>>>> + config->n_v4_entries++; > > >>>>> + config->v4_entries = xrealloc(config->v4_entries, > > >>>>> + config->n_v4_entries * > > > sizeof e); > > >>>>> + config->v4_entries[config->n_v4_entries - 1] = e; > > > > Nit: Maybe use x2nrealloc()? > > I opted not to use x2nrealloc since it's merely a short configuration string (without much thinking). Using x2nrealloc could give the impression to the code reader that a large number of entries are anticipated. But, I am really fine either way. > > Best regards, > Han > > > > > >>>>> + } else { > > >>>>> + config->n_v6_entries++; > > >>>>> + config->v6_entries = xrealloc(config->v6_entries, > > >>>>> + config->n_v6_entries * > > > sizeof e); > > >>>>> + config->v6_entries[config->n_v6_entries - 1] = e; > > >>>>> + } > > >>>>> + } > > >>>>> + > > >>>>> + if (config->n_v4_entries > 0) { > > >>>>> + qsort(config->v4_entries, config->n_v4_entries, sizeof e, > > >>>>> + compare_entries_by_prefix_length); > > >>>>> + } > > >>>>> + > > >>>>> + if (config->n_v6_entries > 0) { > > >>>>> + qsort(config->v6_entries, config->n_v6_entries, sizeof e, > > >>>>> + compare_entries_by_prefix_length); > > >>>>> + } > > >>>>> + > > >>>>> + return true; > > >>>>> + > > >>>>> +err: > > >>>>> + threshold_config_destroy(config); > > >>>>> + return false; > > >>>>> +} > > >>>>> + > > >>>>> +static unsigned int > > >>>>> +find_threshold_for_ip(const char *ip_str, > > >>>>> + const struct threshold_config *config) > > >>>>> +{ > > >>>>> + if (!ip_str) { > > >>>>> + return config->default_threshold; > > >>>>> + } > > >>>>> + > > >>>>> + ovs_be32 ipv4; > > >>>>> + struct in6_addr ipv6; > > >>>>> + if (ip_parse(ip_str, &ipv4)) { > > >>>>> + for (int i = 0; i < config->n_v4_entries; i++) { > > >>>>> + ovs_be32 masked_ip = ipv4 & > > >>>>> + be32_prefix_mask(config->v4_entries[i].plen); > > >>>>> + if (masked_ip == config->v4_entries[i].prefix.ipv4) { > > >>>>> + return config->v4_entries[i].threshold; > > >>>>> + } > > >>>>> + } > > >>>>> + } else if (ipv6_parse(ip_str, &ipv6)) { > > >>>>> + for (int i = 0; i < config->n_v6_entries; i++) { > > >>>>> + struct in6_addr v6_mask = > > >>>>> + ipv6_create_mask(config->v6_entries[i].plen); > > >>>>> + struct in6_addr masked_ip = ipv6_addr_bitand(&ipv6, > > > &v6_mask); > > >>>>> + if (ipv6_addr_equals(&masked_ip, > > >>>>> + > > > &config->v6_entries[i].prefix.ipv6)) { > > >>>>> + return config->v6_entries[i].threshold; > > >>>>> + } > > >>>>> + } > > >>>>> + } > > >>>>> + return config->default_threshold; > > >>>>> +} > > >>>>> + > > >>>>> +/* Parse the threshold option string (see the comment of the function > > >>>>> + * parse_aging_threshold), and returns the smallest threshold. */ > > >>>>> +unsigned int > > >>>>> +min_mac_binding_age_threshold(const char *opt) > > >>>>> +{ > > >>>>> + struct threshold_config config; > > >>>>> + if (!parse_aging_threshold(opt, &config)) { > > >>>>> + return 0; > > >>>>> + } > > >>>>> + > > >>>>> + unsigned int threshold = UINT_MAX; > > >>>>> + unsigned int t; > > >>>>> + > > >>>>> + for (int i = 0; i < config.n_v4_entries; i++) { > > >>>>> + t = config.v4_entries[i].threshold; > > >>>>> + if (t && t < threshold) { > > >>>>> + threshold = t; > > >>>>> + } > > >>>>> + } > > >>>>> + > > >>>>> + for (int i = 0; i < config.n_v6_entries; i++) { > > >>>>> + t = config.v6_entries[i].threshold; > > >>>>> + if (t && t < threshold) { > > >>>>> + threshold = t; > > >>>>> + } > > >>>>> + } > > >>>>> + > > >>>>> + t = config.default_threshold; > > >>>>> + if (t && t < threshold) { > > >>>>> + threshold = t; > > >>>>> + } > > >>>>> + > > >>>>> + threshold_config_destroy(&config); > > >>>>> + > > >>>>> + return threshold == UINT_MAX ? 0 : threshold; > > >>>>> +} > > >>>>> + > > >>>>> struct aging_context { > > >>>>> int64_t next_wake_ms; > > >>>>> int64_t time_wall_now; > > >>>>> uint32_t removal_limit; > > >>>>> uint32_t n_removed; > > >>>>> - uint64_t threshold; > > >>>>> + struct threshold_config *threshold; > > >>>>> }; > > >>>>> > > >>>>> static struct aging_context > > >>>>> @@ -63,13 +304,14 @@ aging_context_init(uint32_t removal_limit) > > >>>>> .time_wall_now = time_wall_msec(), > > >>>>> .removal_limit = removal_limit, > > >>>>> .n_removed = 0, > > >>>>> - .threshold = 0, > > >>>>> + .threshold = NULL, > > >>>>> }; > > >>>>> return ctx; > > >>>>> } > > >>>>> > > >>>>> static void > > >>>>> -aging_context_set_threshold(struct aging_context *ctx, uint64_t > > > threshold) > > >>>>> +aging_context_set_threshold(struct aging_context *ctx, > > >>>>> + struct threshold_config *threshold) > > >>>>> { > > >>>>> ctx->threshold = threshold; > > >>>>> } > > >>>>> @@ -81,19 +323,27 @@ aging_context_is_at_limit(struct aging_context > > > *ctx) > > >>>>> } > > >>>>> > > >>>>> static bool > > >>>>> -aging_context_handle_timestamp(struct aging_context *ctx, int64_t > > > timestamp) > > >>>>> +aging_context_handle_timestamp(struct aging_context *ctx, int64_t > > > timestamp, > > >>>>> + const char *ip) > > >>>>> { > > >>>>> int64_t elapsed = ctx->time_wall_now - timestamp; > > >>>>> if (elapsed < 0) { > > >>>>> return false; > > >>>>> } > > >>>>> > > >>>>> - if (elapsed >= ctx->threshold) { > > >>>>> + ovs_assert(ctx->threshold); > > >>>>> + uint64_t threshold = 1000 * find_threshold_for_ip(ip, > > > ctx->threshold); > > >>>>> + > > >>>>> + if (!threshold) { > > >>>>> + return false; > > >>>>> + } > > >>>>> + > > >>>>> + if (elapsed >= threshold) { > > >>>>> ctx->n_removed++; > > >>>>> return true; > > >>>>> } > > >>>>> > > >>>>> - ctx->next_wake_ms = MIN(ctx->next_wake_ms, (ctx->threshold - > > > elapsed)); > > >>>>> + ctx->next_wake_ms = MIN(ctx->next_wake_ms, (threshold - > > > elapsed)); > > >>>>> return false; > > >>>>> } > > >>>>> > > >>>>> @@ -121,13 +371,18 @@ mac_binding_aging_run_for_datapath(const struct > > > sbrec_datapath_binding *dp, > > >>>>> return; > > >>>>> } > > >>>>> > > >>>>> + if (!ctx->threshold->n_v4_entries && > > > !ctx->threshold->n_v6_entries > > >>>>> + && !ctx->threshold->default_threshold) { > > >>>>> + return; > > >>>>> + } > > >>>>> + > > >>>>> struct sbrec_mac_binding *mb_index_row = > > >>>>> sbrec_mac_binding_index_init_row(mb_by_datapath); > > >>>>> sbrec_mac_binding_index_set_datapath(mb_index_row, dp); > > >>>>> > > >>>>> const struct sbrec_mac_binding *mb; > > >>>>> SBREC_MAC_BINDING_FOR_EACH_EQUAL (mb, mb_index_row, > > > mb_by_datapath) { > > >>>>> - if (aging_context_handle_timestamp(ctx, mb->timestamp)) { > > >>>>> + if (aging_context_handle_timestamp(ctx, mb->timestamp, > > > mb->ip)) { > > >>>>> sbrec_mac_binding_delete(mb); > > >>>>> if (aging_context_is_at_limit(ctx)) { > > >>>>> break; > > >>>>> @@ -166,13 +421,20 @@ en_mac_binding_aging_run(struct engine_node > > > *node, void *data OVS_UNUSED) > > >>>>> continue; > > >>>>> } > > >>>>> > > >>>>> - uint64_t threshold = smap_get_uint(&od->nbr->options, > > >>>>> - > > > "mac_binding_age_threshold", 0); > > >>>>> - aging_context_set_threshold(&ctx, threshold * 1000); > > >>>>> + struct threshold_config threshold_config; > > >>>>> + if (!parse_aging_threshold(smap_get(&od->nbr->options, > > >>>>> + > > > "mac_binding_age_threshold"), > > >>>>> + &threshold_config)) { > > >>>>> + return; > > >>>>> + } > > >>>>> + > > >>>>> + aging_context_set_threshold(&ctx, &threshold_config); > > >>>>> > > >>>>> mac_binding_aging_run_for_datapath(od->sb, > > >>>>> > > > sbrec_mac_binding_by_datapath, > > >>>>> &ctx); > > >>>>> + threshold_config_destroy(&threshold_config); > > >>>>> + > > >>>>> if (aging_context_is_at_limit(&ctx)) { > > >>>>> /* Schedule the next run after specified delay. */ > > >>>>> ctx.next_wake_ms = AGING_BULK_REMOVAL_DELAY_MSEC; > > >>>>> @@ -255,7 +517,7 @@ fdb_run_for_datapath(const struct > > > sbrec_datapath_binding *dp, > > >>>>> > > >>>>> const struct sbrec_fdb *fdb; > > >>>>> SBREC_FDB_FOR_EACH_EQUAL (fdb, fdb_index_row, fdb_by_dp_key) { > > >>>>> - if (aging_context_handle_timestamp(ctx, fdb->timestamp)) { > > >>>>> + if (aging_context_handle_timestamp(ctx, fdb->timestamp, > > > NULL)) { > > >>>>> sbrec_fdb_delete(fdb); > > >>>>> if (aging_context_is_at_limit(ctx)) { > > >>>>> break; > > >>>>> @@ -293,11 +555,16 @@ en_fdb_aging_run(struct engine_node *node, void > > > *data OVS_UNUSED) > > >>>>> continue; > > >>>>> } > > >>>>> > > >>>>> - uint64_t threshold = > > >>>>> - smap_get_uint(&od->nbs->other_config, > > > "fdb_age_threshold", 0); > > >>>>> - aging_context_set_threshold(&ctx, threshold * 1000); > > >>>>> - > > >>>>> + struct threshold_config threshold_config; > > >>>>> + if (!parse_aging_threshold(smap_get(&od->nbs->other_config, > > >>>>> + "fdb_age_threshold"), > > >>>>> + &threshold_config)) { > > >>>> > > >>>> > > >>>> This would mean that fdb aging would accept the same arguments, but > > > that doesn't seem right. We should set only the default value instead of > > > the whole parsing. > > >>>> > > >>> Calling parse_aging_threshold here is just a convenient way to > > > initialize the other fields of the structure while setting the default > > > value. I agree it may be just more clear to explicitly initialize the > > > threshold_config and set the default value by calling smap_get_uint(). I > > > will do that in v2. > > >>> > > >>> I will wait for the consensus on the CIDR option's format and the > > > separate v.s. combined array before sending v2. > > >> > > >> > > >> I definitely don't want to block this patch on something that is more or > > > less cosmetic change so let's see what others think. > > >> > > > The option format may be more important since it is user facing. With an > > > example, we need to choose between: > > > > > > A: 192.168.0.0/16:300;192.168.10.0/24:0;fe80::/10:600;1200 > > > > > > or > > > > > > B: 192.168.0.0/16;300,192.168.10.0/24;0,fe80::/10;600,1200 > > > > > > Thanks, > > > Han > > > > > >>> > > >>> > > >>> Thanks, > > >>> Han > > >>> > > >>>>> > > >>>>> + return; > > >>>>> + } > > >>>>> + aging_context_set_threshold(&ctx, &threshold_config); > > >>>>> fdb_run_for_datapath(od->sb, sbrec_fdb_by_dp_key, &ctx); > > >>>>> + threshold_config_destroy(&threshold_config); > > >>>>> + > > >>>>> if (aging_context_is_at_limit(&ctx)) { > > >>>>> /* Schedule the next run after specified delay. */ > > >>>>> ctx.next_wake_ms = AGING_BULK_REMOVAL_DELAY_MSEC; > > >>>>> diff --git a/northd/aging.h b/northd/aging.h > > >>>>> index 650990e244aa..2cfe2fd44457 100644 > > >>>>> --- a/northd/aging.h > > >>>>> +++ b/northd/aging.h > > >>>>> @@ -30,6 +30,9 @@ void *en_mac_binding_aging_waker_init(struct > > > engine_node *node, > > >>>>> struct engine_arg *arg); > > >>>>> void en_mac_binding_aging_waker_cleanup(void *data); > > >>>>> > > >>>>> +/* The MAC binding aging helper function. */ > > >>>>> +unsigned int min_mac_binding_age_threshold(const char *opt); > > >>>>> + > > >>>>> /* The FDB aging node functions. */ > > >>>>> void en_fdb_aging_run(struct engine_node *node, void *data); > > >>>>> void *en_fdb_aging_init(struct engine_node *node, struct engine_arg > > > *arg); > > >>>>> diff --git a/northd/northd.c b/northd/northd.c > > >>>>> index f8b046d83e85..4bc898982cf2 100644 > > >>>>> --- a/northd/northd.c > > >>>>> +++ b/northd/northd.c > > >>>>> @@ -17,6 +17,7 @@ > > >>>>> #include <stdlib.h> > > >>>>> #include <stdio.h> > > >>>>> > > >>>>> +#include "aging.h" > > >>>>> #include "debug.h" > > >>>>> #include "bitmap.h" > > >>>>> #include "coverage.h" > > >>>>> @@ -1166,8 +1167,14 @@ ovn_datapath_update_external_ids(struct > > > ovn_datapath *od) > > >>>>> smap_add(&ids, "always_learn_from_arp_request", "false"); > > >>>>> } > > >>>>> > > >>>>> - uint32_t age_threshold = smap_get_uint(&od->nbr->options, > > >>>>> - > > > "mac_binding_age_threshold", 0); > > >>>>> + /* For timestamp refreshing, the smallest threshold of the > > > option is > > >>>>> + * set to SB to make sure all entries are refreshed in time. > > >>>>> + * XXX: This approach simplifies processing in > > > ovn-controller, but it > > >>>>> + * may be enhanced, if necessary, to parse the complete > > > CIDR-based > > >>>>> + * threshold configurations to SB to reduce unnecessary > > > refreshes. */ > > >>>>> + uint32_t age_threshold = min_mac_binding_age_threshold( > > >>>>> + smap_get(&od->nbr->options, > > >>>>> + > > > "mac_binding_age_threshold")); > > >>>>> if (age_threshold) { > > >>>>> smap_add_format(&ids, "mac_binding_age_threshold", > > >>>>> "%u", age_threshold); > > >>>>> diff --git a/ovn-nb.xml b/ovn-nb.xml > > >>>>> index 1de0c30416ce..80733273484e 100644 > > >>>>> --- a/ovn-nb.xml > > >>>>> +++ b/ovn-nb.xml > > >>>>> @@ -2686,10 +2686,65 @@ or > > >>>>> </column> > > >>>>> > > >>>>> <column name="options" key="mac_binding_age_threshold" > > >>>>> - type='{"type": "integer", "minInteger": 0, > > > "maxInteger": 4294967295}'> > > >>>>> - MAC binding aging <code>threshold</code> value in seconds. > > > MAC binding > > >>>>> - exceeding this timeout will be automatically removed. The > > > value > > >>>>> - defaults to 0, which means disabled. > > >>>>> + type='{"type": "string"}'> > > >>>>> + <p> > > >>>>> + Specifies the MAC binding aging thresholds based on > > > CIDRs, with the > > >>>>> + format: > > > <var>entry</var>[<code>;</code><var>entry</var>[...]], > > >>>>> + where each <var>entry</var> has the format: > > >>>>> + [<var>cidr</var><code>:</code>]<var>threshold</var> > > >>>>> + </p> > > >>>>> + > > >>>>> + <ul> > > >>>>> + <li> > > >>>>> + <var>cidr</var>: Can be either an IPv4 or IPv6 CIDR. > > >>>>> + </li> > > >>>>> + <li> > > >>>>> + <var>threshold</var>: Threshold value in seconds. MAC > > > bindings with > > >>>>> + IP addresses matching the specified CIDR that exceed > > > this timeout > > >>>>> + will be automatically removed. > > >>>>> + </li> > > >>>>> + </ul> > > >>>>> + > > >>>>> + <p> > > >>>>> + If an <var>entry</var> is provided without an CIDR (just > > > the > > >>>>> + threshold value), it specifies the default threshold for > > > MAC > > >>>>> + bindings that don't match any of the given CIDRs. If > > > there are > > >>>>> + multiple default threshold entries in the option, the > > > behavior is > > >>>>> + undefined. > > >>>>> + </p> > > >>>>> + > > >>>>> + <p> > > >>>>> + If there are multiple CIDRs matching a MAC binding IP, > > > the one with > > >>>>> + the longest prefix length takes effect. If there are > > > multiple > > >>>>> + entries with the same CIDR in the option, the behavior is > > >>>>> + undefined. > > >>>>> + </p> > > >>>>> + > > >>>>> + <p> > > >>>>> + If no matching CIDR is found for a MAC binding IP, and > > > no default > > >>>>> + threshold is specified, the behavior defaults to the > > > original: the > > >>>>> + binding will not be removed based on age. > > >>>>> + </p> > > >>>>> + > > >>>>> + <p> > > >>>>> + The value can also default to an empty string, which > > > means that the > > >>>>> + aging threshold is disabled. Any string not in the above > > > format is > > >>>>> + regarded as invalid and the aging is disabled. > > >>>>> + </p> > > >>>>> + > > >>>>> + <p> > > >>>>> + Example: > > >>>>> + <code> > > > 192.168.0.0/16:300;192.168.10.0/24:0;fe80::/10:600;1200</code> > > >>>>> + </p> > > >>>>> + > > >>>>> + <p> > > >>>>> + This sets a threshold of 300 seconds for MAC bindings > > > with IP > > >>>>> + addresses in the 192.168.0.0/16 range, excluding the > > > 192.168.1.0/24 > > >>>>> + range (for which the aging is disabled), a threshold of > > > 600 seconds > > >>>>> + for MAC bindings with IP addresses in the fe80::/10 IPv6 > > > range, and > > >>>>> + a default threshold of 1200 seconds for all other MAC > > > bindings. > > >>>>> + </p> > > >>>>> + > > >>>>> </column> > > >>>>> </group> > > >>>>> > > >>>>> diff --git a/tests/ovn.at b/tests/ovn.at > > >>>>> index 637d92bedbc7..07b5c1ec57fc 100644 > > >>>>> --- a/tests/ovn.at > > >>>>> +++ b/tests/ovn.at > > >>>>> @@ -34676,6 +34676,66 @@ OVS_WAIT_UNTIL([ > > >>>>> > > >>>>> AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=79 --no-stats | > > > strip_cookie], [0], []) > > >>>>> > > >>>>> +# Test CIDR-based threshold configuration > > >>>>> +check ovn-nbctl set logical_router gw > > > options:mac_binding_age_threshold=" > > > 192.168.10.0/255.255.255.0:2;192.168.10.64/26:0;192.168.10.20:0" > > >>>>> +check ovn-nbctl --wait=sb sync > > >>>>> +uuid=$(fetch_column datapath _uuid external_ids:name=gw) > > >>>>> +AT_CHECK([ovn-sbctl get datapath $uuid > > > external_ids:mac_binding_age_threshold], [0], [dnl > > >>>>> +"2" > > >>>>> +]) > > >>>>> + > > >>>>> +# Send GARP to populate MAC binding table records > > >>>>> +send_garp hv1 ext1 10 # belong to 192.168.10.0/24 > > >>>>> +send_garp hv2 ext2 20 # belong to 192.168.10.20/32 > > >>>>> +send_garp hv2 ext2 65 # belong to 192.168.10.64/26 > > >>>>> + > > >>>>> +OVS_WAIT_UNTIL([ovn-sbctl list mac_binding | grep -q > > > "192.168.10.10"]) > > >>>>> +OVS_WAIT_UNTIL([ovn-sbctl list mac_binding | grep -q > > > "192.168.10.20"]) > > >>>>> +OVS_WAIT_UNTIL([ovn-sbctl list mac_binding | grep -q > > > "192.168.10.65"]) > > >>>>> + > > >>>>> +OVS_WAIT_UNTIL([ > > >>>>> + test "0" = "$(ovn-sbctl list mac_binding | grep -c > > > '192.168.10.10')" > > >>>>> +]) > > >>>>> +# The other two should remain because the corresponding prefixes > > > have threshold 0 > > >>>>> +AT_CHECK([ovn-sbctl list mac_binding | grep -q "192.168.10.20"]) > > >>>>> +AT_CHECK([ovn-sbctl list mac_binding | grep -q "192.168.10.65"]) > > >>>>> +check ovn-sbctl --all destroy mac_binding > > >>>>> + > > >>>>> +# Set the aging threshold mixed with IPv6 prefixes and default > > > threshold > > >>>>> +check ovn-nbctl set logical_router gw > > > options:mac_binding_age_threshold="1; 192.168.10.64/26:0;ff00:1234::/32:888" > > >>>>> +check ovn-nbctl --wait=sb sync > > >>>>> +uuid=$(fetch_column datapath _uuid external_ids:name=gw) > > >>>>> +AT_CHECK([ovn-sbctl get datapath $uuid > > > external_ids:mac_binding_age_threshold], [0], [dnl > > >>>>> +"1" > > >>>>> +]) > > >>>>> + > > >>>>> +# Send GARP to populate MAC binding table records > > >>>>> +send_garp hv1 ext1 10 # belong to default > > >>>>> +send_garp hv2 ext2 65 # belong to 192.168.10.64/26 > > >>>>> + > > >>>>> +OVS_WAIT_UNTIL([ovn-sbctl list mac_binding | grep -q > > > "192.168.10.10"]) > > >>>>> +OVS_WAIT_UNTIL([ovn-sbctl list mac_binding | grep -q > > > "192.168.10.65"]) > > >>>>> + > > >>>>> +OVS_WAIT_UNTIL([ > > >>>>> + test "0" = "$(ovn-sbctl list mac_binding | grep -c > > > '192.168.10.10')" > > >>>>> +]) > > >>>>> +AT_CHECK([ovn-sbctl list mac_binding | grep -q "192.168.10.65"]) > > >>>>> +check ovn-sbctl --all destroy mac_binding > > >>>>> + > > >>>>> +# Set the aging threshold with invalid format > > >>>>> +check ovn-nbctl set logical_router gw > > > options:mac_binding_age_threshold="1;abc/26:0" > > >>>>> +check ovn-nbctl --wait=sb sync > > >>>>> +uuid=$(fetch_column datapath _uuid external_ids:name=gw) > > >>>>> +AT_CHECK([ovn-sbctl get datapath $uuid > > > external_ids:mac_binding_age_threshold], [1], [ignore], [ignore]) > > >>>>> + > > >>>>> +# Send GARP to populate MAC binding table records > > >>>>> +send_garp hv1 ext1 10 > > >>>>> + > > >>>>> +OVS_WAIT_UNTIL([ovn-sbctl list mac_binding | grep -q > > > "192.168.10.10"]) > > >>>>> +# The record is not deleted > > >>>>> +sleep 5 > > >>>>> +AT_CHECK([ovn-sbctl list mac_binding | grep -q "192.168.10.10"]) > > >>>>> + > > >>>>> OVN_CLEANUP([hv1], [hv2]) > > >>>>> AT_CLEANUP > > >>>>> ]) > > >>>>> -- > > >>>>> 2.38.1 > > >>>>> > > >>>>> _______________________________________________ > > >>>>> dev mailing list > > >>>>> [email protected] > > >>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >>>>> > > >>>> > > >>>> Thanks, > > >>>> Ales > > >>>> > > >>>> -- > > >>>> > > >>>> Ales Musil > > >>>> > > >>>> Senior Software Engineer - OVN Core > > >>>> > > >>>> Red Hat EMEA > > >>>> > > >>>> [email protected] > > >> > > >> > > >> Thanks, > > >> Ales > > >> -- > > >> > > >> Ales Musil > > >> > > >> Senior Software Engineer - OVN Core > > >> > > >> Red Hat EMEA > > >> > > >> [email protected] > > > _______________________________________________ > > > dev mailing list > > > [email protected] > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
I just sent v2. PTAL: https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ Thanks, Han _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
