Re: [PATCH net 2/3] nexthop: Make nexthop bucket dump more efficient

[David Ahern <dsahern@kernel.org>]

On 8/8/23 1:52 AM, Ido Schimmel wrote:
> rtm_dump_nexthop_bucket_nh() is used to dump nexthop buckets belonging
> to a specific resilient nexthop group. The function returns a positive
> return code (the skb length) upon both success and failure.
> 
> The above behavior is problematic. When a complete nexthop bucket dump
> is requested, the function that walks the different nexthops treats the
> non-zero return code as an error. This causes buckets belonging to
> different resilient nexthop groups to be dumped using different buffers
> even if they can all fit in the same buffer:
> 
>  # ip link add name dummy1 up type dummy
>  # ip nexthop add id 1 dev dummy1
>  # ip nexthop add id 10 group 1 type resilient buckets 1
>  # ip nexthop add id 20 group 1 type resilient buckets 1
>  # strace -e recvmsg -s 0 ip nexthop bucket
>  [...]
>  recvmsg(3, {msg_name={sa_family=AF_NETLINK, nl_pid=0, nl_groups=00000000}, msg_namelen=12, msg_iov=[...], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 64
>  id 10 index 0 idle_time 10.27 nhid 1
>  [...]
>  recvmsg(3, {msg_name={sa_family=AF_NETLINK, nl_pid=0, nl_groups=00000000}, msg_namelen=12, msg_iov=[...], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 64
>  id 20 index 0 idle_time 6.44 nhid 1
>  [...]
> 
> Fix by only returning a non-zero return code when an error occurred and
> restarting the dump from the bucket index we failed to fill in. This
> allows buckets belonging to different resilient nexthop groups to be
> dumped using the same buffer:
> 
>  # ip link add name dummy1 up type dummy
>  # ip nexthop add id 1 dev dummy1
>  # ip nexthop add id 10 group 1 type resilient buckets 1
>  # ip nexthop add id 20 group 1 type resilient buckets 1
>  # strace -e recvmsg -s 0 ip nexthop bucket
>  [...]
>  recvmsg(3, {msg_name={sa_family=AF_NETLINK, nl_pid=0, nl_groups=00000000}, msg_namelen=12, msg_iov=[...], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 128
>  id 10 index 0 idle_time 30.21 nhid 1
>  id 20 index 0 idle_time 26.7 nhid 1
>  [...]
> 
> While this change is more of a performance improvement change than an
> actual bug fix, it is a prerequisite for a subsequent patch that does
> fix a bug.
> 
> Fixes: 8a1bbabb034d ("nexthop: Add netlink handlers for bucket dump")
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> Reviewed-by: Petr Machata <petrm@nvidia.com>
> ---
>  net/ipv4/nexthop.c | 16 +++++-----------
>  1 file changed, 5 insertions(+), 11 deletions(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>