Re: [PATCH net-next v6 04/11] ipv4: Dump route exceptions if requested

[David Ahern <dsahern@gmail.com>]

On 6/20/19 1:01 PM, Stefano Brivio wrote:
>>> diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
>>> index 94e5d83db4db..03f51e5192e5 100644
>>> --- a/net/ipv4/fib_trie.c
>>> +++ b/net/ipv4/fib_trie.c
>>> @@ -2078,28 +2078,51 @@ void fib_free_table(struct fib_table *tb)
>>>  	call_rcu(&tb->rcu, __trie_free_rcu);
>>>  }
>>>  
>>> +static int fib_dump_fnhe_from_leaf(struct fib_alias *fa, struct sk_buff *skb,
>>> +				   struct netlink_callback *cb,
>>> +				   int *fa_index, int fa_start)
>>> +{
>>> +	struct fib_info *fi = fa->fa_info;
>>> +	int nhsel;
>>> +
>>> +	if (!fi || fi->fib_flags & RTNH_F_DEAD)
>>> +		return 0;
>>> +
>>> +	for (nhsel = 0; nhsel < fib_info_num_path(fi); nhsel++) {
>>> +		int err;
>>> +
>>> +		err = fnhe_dump_buckets(fa, nhsel, skb, cb, fa_index, fa_start);
>>> +		if (err)
>>> +			return err;
>>> +	}
>>> +
>>> +	return 0;
>>> +}  
>>
>> fib_info would be the better argument to pass in to the fnhe dump, and
> 
> ...we need to pass the table ID to rt_fill_info(). Sure, I can pass
> that explicitly, but doing so kind of tells me I'm not passing the
> right argument, with sufficient information. What do you think?

I think that is preferable to passing fib_alias.

> 
>> I think the loop over where the bucket is should be in route.c as well.
>> So how about fib_info_dump_fnhe() as the helper exposed from route.c,
>> and it does the loop over nexthops and calls fnhe_dump_buckets.
> 
> Yes, I could do that conveniently if I'm passing a fib_info there. I'm
> stlll undecided if it's worth it, I guess I don't really have a
> preference.
> 
>> As for the loop, you could fill an skb without finishing a bucket inside
>> of a nexthop so you need top track which nexthop is current as well.
> 
> I think this is not a problem, and also checked that selftests trigger
> this. Buckets are transparent to the counter for partial dumps (s_fa),
> they are just an arbitrary grouping from that perspective, just like
> items on the chain for the same bucket.
> 
> Take this example, s_i values in [], s_fa values in ():
> 
>   node (fa) #1 [1]
>     nexthop #1
>     bucket #1 -> #0 in chain (1)
>     bucket #2 -> #0 in chain (2) -> #1 in chain (3) -> #2 in chain (4)
>     bucket #3 -> #0 in chain (5) -> #1 in chain (6)
> 
>     nexthop #2
>     bucket #1 -> #0 in chain (7) -> #1 in chain (8)
>     bucket #2 -> #0 in chain (9)
>   --
>   node (fa) #2 [2]
>     nexthop #1
>     bucket #1 -> #0 in chain (1) -> #1 in chain (2)
>     bucket #2 -> #0 in chain (3)
> 
> 
> If I stop at (3), (4), (7) for "node #1", or at (2) for "node #2", it
> doesn't really matter, because nexthops and buckets are always
> traversed in the same way (walking flattens all that).
> 
> For IPv4, I could even drop the in-tree/in-node distinction (s_i/s_fa).
> But accounting becomes terribly inconvenient though, and it would be
> inconsistent with what I needed to do for IPv6 (skip/skip_in_node): we
> have 'sernum' there, which is used to mark what node we need to restart
> from in case of changes. Within a node, however, I can't make any
> assumptions like that, so if the fib6 tree changes, I'll restart from
> the beginning of the node (see discussion with Martin on v1).
> 
> My idea would be to keep it like it is at the moment, and later make it
> as "accurate" as it is on IPv6, introducing something like 'sernum'. If
> we start with this, it will be more convenient to do that later.
> 

ok, if you have it covered. can you add that description above to the
commit message. Be good to capture how it is covered.