On 8/6/25 6:07 PM, Dima Chumak wrote:
> On 2025-08-01 10:43 PM, Ilya Maximets wrote:
>> On 7/23/25 3:12 PM, Dima Chumak via dev wrote:
>>> For route lookup based on the destination address a single routing table
>>> is sufficient. For a more advanced routing when a lookup needs to take
>>> into consideration other parameters, such as a source address, a
>>> multi-table lookup is needed.
>>>
>>> This change introduces infrastructure for using multiple routing
>>> tables that can be added dynamically, as a pre-step towards importing
>>> non-default routing tables from kernel.
>>>
>>> Signed-off-by: Dima Chumak <[email protected]>
>>> ---
>>> lib/netdev-dummy.c | 6 +-
>>> lib/ovs-router.c | 135 ++++++++++++++++++++++++++++++++---------
>>> lib/ovs-router.h | 13 +++-
>>> lib/route-table-bsd.c | 3 +-
>>> lib/route-table-stub.c | 3 +-
>>> lib/route-table.c | 4 +-
>>> 6 files changed, 124 insertions(+), 40 deletions(-)
>>>
<snip>
>>> @@ -669,18 +735,19 @@ ovs_router_lookup_cmd(struct unixctl_conn *conn, int
>>> argc,
>>> }
>>>
>>> void
>>> -ovs_router_flush(void)
>>> +ovs_router_flush(bool flush_all)
>>> {
>>> - struct ovs_router_entry *rt;
>>> + struct clsmap_node *node;
>>>
>>> ovs_mutex_lock(&mutex);
>>> - classifier_defer(&cls);
>>> - CLS_FOR_EACH(rt, cr, &cls) {
>>> - if (rt->priority == rt->plen || rt->local) {
>>> - rt_entry_delete__(&rt->cr);
>>> + HMAP_FOR_EACH_POP (node, hash_node, &clsmap) {
>>> + cls_destroy(&node->cls, flush_all);
>>> + if (!node->cls.n_rules) {
>>> + classifier_destroy(&node->cls);
>>> + free(node);
>>> }
>>
>> This feels like there will be a memory leak in case there are still rules in
>> this classifier. It also feels strange that the function named cls_destroy
>> doesn't actually destroy the classifier and can even leave some rules in it.
>>
>
> Probably something like cls_flush() would be a better name to indicate
> that some entries may not be deleted?
Yeah, cls_flush might be better.
>
> I tried to preserve the current behaviour that we have for standard
> tables, where user added entries are not flushed, and apply the same
> logic to custom tables as well.
It makes sense, yes.
Note: I was looking at the code on the current main again and it seems
like there is a time window when we remove all the routes but didn't yet
dump them back. During that time the tunnel route lookup can fail and
some packets can be dropped, I suppose. It's not a problem of this set,
but we may need to eventually split the function into two separate ones:
one to make the rules invisible in the next version, then re-dump all
the routes adding them as the next version and then second function to
advance the version and delete all the old rules. This should be a proper
way of handling the route updates atomically as it is done for OpenFlow
rules in the actual ofproto classifier. But it probably should be a
separate change. Writing it down here just to have it documented.
>
> The intention behind flush_all argument is to enable flushing
> everything, including user entries, on ovs shutdown to avoid leaks that
> address sanitizer reported for some router related unit tests.
>
> At the moment ASAN check doesn't show any leaks but I can add a more
> specific unit test to check potential leaks for user added routes and
> rules, what do you think?
Sure, some more tests would be good to have.
>
> P.S. For all other comments they're all clear to me and I can start
> preparing V3 with the changes. Thanks.
Thanks!
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
