On Wed, Nov 1, 2023 at 11:25 AM Ilya Maximets <i.maxim...@ovn.org> wrote: > > On 10/26/23 18:51, Han Zhou wrote: > > > > > > On Wed, Oct 25, 2023 at 11:15 AM Ilya Maximets <i.maxim...@ovn.org <mailto:i.maxim...@ovn.org>> wrote: > >> > >> On 10/24/23 21:35, Han Zhou 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 <hz...@ovn.org <mailto:hz...@ovn.org>> > >> > --- > >> > northd/aging.c | 297 +++++++++++++++++++++++++++++++++++++++++++++--- > >> > northd/aging.h | 3 + > >> > northd/northd.c | 11 +- > >> > ovn-nb.xml | 63 +++++++++- > >> > tests/ovn.at <http://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; > >> > >> Hi, Han. Thanks for the patch! > >> > >> Not a full review, but I wonder if it would be cleaner to replace > >> all the structure members above with a single 'struct in6_addr prefix;' > >> and store ipv4 addresses as ipv6-mapped ipv4. This will allow to use > >> a single array for storing the entries as well. May save some lines > >> of code. > >> > >> What do you think? > > > > Thanks Ilya for the review. In fact using a common in6_addr in a single array was my first attempt, but then I realized that if there are both IPv4 and IPv6 entries in the array, for each mac-binding checking, say if it is IPv4, it may have to go through all the IPv6 entries unnecessarily. If there are a huge amount of MAC-bindings to check, the time may be doubled. Probably this doesn't have a big impact, but using two arrays didn't seem too many lines of code, so I went with the current *safe* approach. Let me know if you have a strong preference for the alternative, and I am happy to change it. > > That makes sense. > > Though, if performance here is a concern, we probably should have an > actual I-P for MAC binding/FDB aging. It should be possible to keep > track of current entries in a heap or a skiplist based on the expected > removal time and adjust whenever actual Sb tables change and not walk > over the whole table each time. > > Another idea to keep in mind is to use a classifier instead of array, > but this will only make sense with a large number of different CIDRs > in the configuration, otherwise the overhead of the classifier will > likely be higher than simple array traversal. > > With the lack of I-P, I think, it's OK to have the entries in separate > arrays for now. > Cool. I'd say performance is probably not a big concern right now, while I was also trying to avoid adding an obvious performance penalty. I agree that when performance becomes a real concern we should do I-P.
> > > >> > >> Also, this patch, probably, needs a NEWS entry. > > > > I was wondering if this change is big enough to add one. Now I will add it since you brought up :). > > It's a new user-facing API. Should be in the NEWS, IMO. > OK, how about the below change: ------------------------------------------------------------ diff --git a/NEWS b/NEWS index 425dfe0a84e1..6352990ae8f5 100644 --- a/NEWS +++ b/NEWS @@ -1,5 +1,7 @@ Post v23.09.0 ------------- + - Support CIDR based MAC binding aging threshold. See man ovn-nb for + 'mac_binding_age_threshold' for more details. OVN v23.09.0 - 15 Sep 2023 -------------------------- -------------------------------------------------------------- I can add it in v2 or when merging if there are no other comments. Thanks, Han > > > > Thanks, > > Han > > > >> > >> Best regards, Ilya Maximets. > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
