Add reference tracking to netdev and release netdev reference in nondl on shutdown

[zhuyifei1999]

We are using onload in AF_XDP mode, and one thing we noticed was that as long as onload was loaded, shutdown / reboot would loop with console message:

unregister_netdevice: waiting for eth0 to become free. Usage count = 3
unregister_netdevice: waiting for eth0 to become free. Usage count = 3
unregister_netdevice: waiting for eth0 to become free. Usage count = 3

This is because module cleanup functions only get called in the delete_module syscall and not reboot. I added reference counting and fixed the issue by adding a reboot notifier that calls the appropriate cleanup functions necessary to release the netdev references.

[ligallag-amd]

Hi, thanks for your PR!

Overall, I think the netdev reference counting is a helpful addition. Once your upstream changes are accepted and have been pulled into out our sfc-linux-net repo, we will import the net driver with your changes this way.

As for your changes to the reboot notifier, it makes sense to me but I think there is an opportunity for this to race with the delete_module syscall (this is the only place I could find in the kernel source where mod->exit() is called). You have mentioned that shutdown shouldn't be concerned with this but I imagine the race would cause kernel warnings at the least. I'd like some of the other members of my team to weigh in on this.

[zhuyifei1999]

You have mentioned that shutdown shouldn't be concerned with this but I imagine the race would cause kernel warnings at the least.

I had a bit of consideration for this. Both cleanup functions hold RTNL lock:

onload/src/driver/linux_resource/nondl_driver.c

Lines 95 to 100 in 467ee38

	extern void efrm_nondl_unregister(void)
	{
	#ifdef EFHW_HAS_AF_XDP
	efrm_nondl_unregister_driver(&efrm_nondl_driver);
	#endif
	}

onload/src/driver/linux_resource/nondl_resource.c

Lines 90 to 109 in 467ee38

	/* Unregister a driver from the non-driverlink resource manager. */
	void efrm_nondl_unregister_driver(struct efrm_nondl_driver *driver)
	{
	struct efrm_nondl_device *device, *device_n;
	rtnl_lock();
	EFRM_ASSERT(nondl_driver == driver);
	list_for_each_entry_safe_reverse(device, device_n, &driver->devices,
	driver_node)
	efrm_nondl_del_device(device);
	BUG_ON(!list_empty(&driver->devices));
	nondl_driver = NULL;
	rtnl_unlock();
	}
	EXPORT_SYMBOL(efrm_nondl_unregister_driver);

onload/src/driver/linux_resource/nondl_resource.c

Lines 193 to 214 in 467ee38

	void efrm_nondl_shutdown(void)
	{
	struct efrm_nondl_device *device, *device_n;
	/* If we are being unloaded then any module which depends on
	* us should have been unloaded already, so our driver list
	* should be empty. If not then something went badly wrong.
	*
	* We still might have devices registered with us, though. If
	* so they need to be cleaned up. */
	rtnl_lock();
	BUG_ON(nondl_driver);
	list_for_each_entry_safe_reverse(device, device_n, &nondl_device_list, node)
	efrm_nondl_cleanup_netdev(device);
	BUG_ON(!list_empty(&nondl_device_list));
	rtnl_unlock();
	}

So data races aren't possible. Both functions are also in the format of

list_for_each_entry_safe_reverse(...)
   cleanup(...)
BUG_ON(!list_empty(...));

So both functions are essentially no-ops if they have already been called. The only place I see that it could cause crash / warn is efrm_nondl_unregister_driver calling EFRM_ASSERT(nondl_driver == driver);, which I just didn't bother fixing in the initial implementation, considering a normal shutdown PID 1 typically kills all processes before calling reboot syscall, so no one would be there to call delete_module syscall. And I'm not sure, if I were to fix this, what the best way to fix it would be.

[ligallag-amd]

Good points.

Regarding EFRM_ASSERT(nondl_driver == driver);, you could check if nondl_driver == NULL and exit early.

[zhuyifei1999]

Regarding EFRM_ASSERT(nondl_driver == driver);, you could check if nondl_driver == NULL and exit early.

Done.