Re: [PATCH net-next v2] ipv4: fib: Replay events when registering FIB notifier

On Tue, Nov 01, 2016 at 07:19:59AM -0700, Eric Dumazet wrote:

On Tue, 2016-11-01 at 00:57 +0200, Ido Schimmel wrote:

quoted

On Mon, Oct 31, 2016 at 02:24:06PM -0700, Eric Dumazet wrote:

quoted

quoted

How well will this work for large FIB tables ?

Holding rtnl while sending thousands of skb will prevent consumers to
make progress ?

Can you please clarify what do you mean by "while sending thousands of
skb"? This patch doesn't generate notifications to user space, but
instead invokes notification routines inside the kernel. I probably
misunderstood you.

Are you suggesting this be done using RCU instead? Well, there are a
couple of reasons why I took RTNL here:

No, I do not believe RCU is wanted here, in control path where we might
sleep anyway.

quoted

1) The FIB notification chain is blocking, so listeners are expected to
be able to sleep. This isn't possible if we use RCU. Note that this
chain is mainly useful for drivers that reflect the FIB table into a
capable device and hardware operations usually involve sleeping.

2) The insertion of a single route is done with RTNL held. I didn't want
to differentiate between both cases. This property is really useful for
listeners, as they don't need to worry about locking in writer-side.
Access to data structs is serialized by RTNL.

My concern was that for large iterations, you might hold RTNL and/or
current cpu for hundred of ms or even seconds...

I understand your concern, but I think it's helpful to look at the users
of this API. It was only recently introduced [1] because nobody needed
it beside switch drivers that reflect the FIB table and I believe it'll
stay that way. Currently, only mlxsw and rocker use it.

Now, in these use cases when register_fib_notifier() is called the
switch ports are still not present in the system, so we really only have
a few routes used for management. Similarly, when
unregister_fib_notifier() is called, the switch ports are already gone
and most FIBs were flushed due to NETDEV_UNREGISTER, so again we only
have a handful of FIBs to iterate over.

Does that sound reasonable to you?

1. https://www.spinics.net/lists/netdev/msg397444.html