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. > >> >> 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. > > Thanks, > Han > >> >> Best regards, Ilya Maximets. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
