[ovs-dev] [RFC v2] bridge: Allow manual notifications about interfaces' updates.

[Ben Pfaff]

On Tue, Nov 05, 2019 at 09:40:26PM +0100, Ilya Maximets wrote:
> On 05.11.2019 20:20, Ben Pfaff wrote:
> > On Mon, Nov 04, 2019 at 02:22:34PM +0100, Ilya Maximets wrote:
> > > Sometimes interface updates could happen in a way ifnotifier is not
> > > able to catch.  For example some heavy operations (device reset) in
> > > netdev-dpdk could require re-applying of the bridge configuration.
> > > 
> > > For this purpose new manual notifier introduced. Its function
> > > 'if_notifier_manual_report()' could be called directly by the code
> > > that aware about changes.  This new notifier is thread safe.
> > > 
> > > Signed-off-by: Ilya Maximets <i.maximets at ovn.org>
> > > ---
> > > 
> > > Sending this as RFC that might be useful in context of the following patch:
> > > https://mail.openvswitch.org/pipermail/ovs-dev/2019-October/364155.html
> > > 
> > > It will allow us to not introduce heavy dpdk notifier just to update
> > > one sequence number on RTE_ETH_EVENT_INTR_RESET events. We could also
> > > use it to report other changes from DPDK, e.g. LSC interrupts.
> > 
> > I *think* I understand what's going on here.  It's that the proposed
> > dpdk-notifier
> > (https://mail.openvswitch.org/pipermail/ovs-dev/2019-October/364154.html)
> > takes more locks, etc., so it's going to be slow.  This one doesn't so
> > it's cheaper.  This one, however, will only work if the OVS code that
> > makes the device changes calls into it, whereas the other one gets
> > notified by DPDK itself.
> 
> Both implementations doesn't receive notifications from DPDK itself.
> dpdk-notifier is just a more complex variant of the same thing with
> dynamic allocations and list of notifiers.  In practice, the only
> way to trigger it is to call dpdk_notifierr_report_link() from the
> OVS code.
> 
> The part that really receives DPDK events is 'dpdk_eth_event_callback()'
> from https://mail.openvswitch.org/pipermail/ovs-dev/2019-October/364155.html
> 
> There is a possibility to make everything super right (not sure).
> This should look like this:
> 
>   * Allow more than one type of notifiers. i.e. introduce
>     ifnotifier-provider.h, make a common interface in ifnotifier.{c,h}.
>     Move code of the existing notifiers to ifnotifier-{linux,bsd}.{c.h}
>     and make them both implement ifnotifier with type='system'.
> 
>   * Introduce dpdk_notifier (analogue of rtnetlink_notifier) that will
>     subscribe itself to DPDK events with rte_eth_dev_callback_register()
>     and receive actual DPDK events via callback.
> 
>   * Introduce ifnotifier-dpdk.{c,h} that will register itself as ifnotifier
>     with type='dpdk'.  Subscribe ifnotifier-dpdk to dpdk_notifier with
>     dpdk_notifier_create().
> 
>   * bridge.c should subscribe itself to all registered types of ifnotifiers
>     to increase 'ifaces_changed' sequence number.
> 
>   * netdev-dpdk.c should subscribe to dpdk_notifier to receive DPDK
>     events and perform required actions on devices.
> 
>   * Something else to resolve issues with DPDK intialization.
>     (we can't register dpdk callbacks before dpdk initialization that
>      could happen way after bridge initialization)
> 
> I'm not sure if it's really possible to properly implement, but it looks
> like an over-engineering anyway.
> 
> BTW, I'm not sure if any of our solutions are good enough.

OK.  Thanks for all the details.

Given that we have three solutions, of which it's possible none may be
"good enough", I favor the simplest (the "manual" one).

> > I think that this one could avoid introducing a mutex by using an atomic
> > pointer, but I don't know whether that's worthwhile.
> > 
> 
> The case is that we need to be sure that 'ifaces_changed' sequence number
> will not be destroyed while (before, actually) we're using it.
> 
>    Main thread       Notification thread
> 
>    create seq
>    set cb <-- func
>                      notification appeared
>                      func = read cb
>                      func()
>    set cb <-- NULL
>    destroy seq
>                      seq_change() // change of destroyed seq will lead to crash.
> 
> 
> So, the mutex ensures that callback is still registered while we're
> executing it.

Ah, OK.  That makes sense.