On 11/2/23 07:26, Han Zhou wrote: > > > On Wed, Nov 1, 2023 at 11:25 AM Ilya Maximets <i.maxim...@ovn.org > <mailto: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> <mailto: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> >> >> > <mailto: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> <http://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.
Looks fine. Nit: man pages are usually referenced with a number, i.e. 'See ovs-nb(5)'. > > OVN v23.09.0 - 15 Sep 2023 > -------------------------- > -------------------------------------------------------------- > > I can add it in v2 or when merging if there are no other comments. Sure. I see Ales posted some comments. > > Thanks, > Han > >> > >> > Thanks, >> > Han >> > >> >> >> >> Best regards, Ilya Maximets. >> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
