Message-ID: <163549826664.3523.4140191764737040064@kwain>
Date:   Fri, 29 Oct 2021 11:04:26 +0200
From:   Antoine Tenart <atenart@...nel.org>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     davem@...emloft.net, pabeni@...hat.com, gregkh@...uxfoundation.org,
        ebiederm@...ssion.com, stephen@...workplumber.org,
        herbert@...dor.apana.org.au, juri.lelli@...hat.com,
        netdev@...r.kernel.org, mhocko@...e.com
Subject: Re: [RFC PATCH net-next 8/9] net: delay device_del until run_todo

Quoting Antoine Tenart (2021-09-29 19:31:56)
> Quoting Jakub Kicinski (2021-09-29 15:31:26)
> > On Wed, 29 Sep 2021 10:26:35 +0200 Antoine Tenart wrote:
> 
> > > (While I did ran stress tests reading/writing attributes while
> > > unregistering devices, I think I missed an issue with the
> > > netdev_queue_default attributes; which hopefully can be fixed — if the
> > > whole idea is deemed acceptable).
> 
> I had a quick look about queue attributes, their removal should also be
> done in run_todo (that's easy). However the queues can be updated in
> flight (while holding the rtnl lock) and the error paths[1][2] do drain
> sysfs files (in kobject_put).
> 
> We can't release the rtnl lock here. It should be possible to delay this
> outside the rtnl lock (in the global workqueue) but as the kobject are
> embedded in the queues, we might need to have them live outside to allow
> async releases while a net device (and ->_rx/->_tx) is being freed[3].
> That adds to the complexity...
> 
> [1] https://elixir.bootlin.com/linux/latest/source/net/core/net-sysfs.c#L1662
> [2] https://elixir.bootlin.com/linux/latest/source/net/core/net-sysfs.c#L1067
> [3] Or having a dedicated workqueue and draining it.

I got back to this and while all other suggestions where easy enough to
get right, handling the queue sysfs files was not... The explanation is
the same for Tx and Rx queues.

Sysfs queue files are special: in addition to being created at device
registration and removed at unregistration, they also can be
reconfigured (added & removed) in-flight. This is done under the rtnl
lock. So for those sysfs files the trylock/restart construction also
helps in not hitting a deadlock while removing queues in-flight.

To make this work here, I had to delay the queue removal outside the
rtnl lock. As the queues are statically allocated in net_device->_tx, I
made them dynamically allocated to allow delaying their removal outside
the rtnl lock (in a workqueue for example).

This worked for allowing the removal of queues not to hit the ABBA
deadlock. (Extra logic to drain the queues before device removal might
be needed too). But this introduced an issue as naming collision between
queues was now possible (if we tried registering new queues while old
ones weren't unregistered yet). This is because the unregistration was
delayed, and for this to work the reconfiguration of queues should be
atomic under the rtnl lock; which is exactly what the changes to not hit
the ABBA deadlock requires... There are contradicting goals here.

This is not really fixable IMHO. First we would need to add a naming
collision logic for queues only, but then we don't have the same
two-step unregistration logic as we have for net devices. And failing on
queues reconfigurations for this doesn't seem acceptable to me (this has
a lot of implications, many "users" can request queues registration &
unregistration). Plus the complexity starts to be quite noticeable,
which doesn't help maintenance.

The above looks like a dead end to me. I tried several approaches to
better handle the queues in sysfs, but couldn't find a solution not
hitting an issue one way or another.

I have however a few other ideas, that may or may not be acceptable.
I'll start a dedicated answer to this thread to discuss those.

Thanks,
Antoine

Powered by blists - more mailing lists