GNOME Bugzilla – Bug 724363
[review] busy waiting while bringing up (taking down) device
Last modified: 2014-02-23 21:39:38 UTC
Hi, this branch was initially part of bug 719905. Currently, when bringing up/taking down a device, the code busy waits for the device IFF_UP flag to change. Doing this, it only looks at the cached device information, which does change while busy waiting.
Please see branch th/bgo724363_wait_device_bring_up
(In reply to comment #0) > Currently, when bringing up/taking down a device, the code busy waits for the > device IFF_UP flag to change. Doing this, it only looks at the cached device > information, which does change while busy waiting. he meant "which does NOT change"
Any particular reason to make the cache sync function a class function for NMDevice? It doesn't look like that's terribly useful here (since as the code is now, no device subclass overrides it) so maybe just drop that part until a subclass would need to override it? Any reason that nm_device_take_down() doesn't mirror nm_device_bring_up() for the "if (block ..." statement? bring_up() has "if (block && !device_is_up) {" but take_down() only has "if (block) {", where I'd expect "if (block && device_is_up) {". Otherwise, the code will always wait 200us even if the device is already down. Also, while we're here, maybe we should convert the "bringing up device" and "taking down device" info messages to debug level, they aren't really useful as info level.
IRC follow-up: For nm-platform I recommend name 'nm_platform_link_refresh' and for nm-device 'nm_device_refresh'. Feel free to improve on that.
(In reply to comment #3) (In reply to comment #4) Comments addressed and repushed branch
Looks good to me.
> core: fix waiting for bringing up/taking down device >- return ifindex ? nm_platform_link_is_up (ifindex) : TRUE; >+ return ifindex > 0 ? nm_platform_link_is_up (ifindex) : TRUE; these fixes seem unrelated to the rest of the patch. could you split them out? Also, it looks like take_down() needs the same fix?
(In reply to comment #7) > > core: fix waiting for bringing up/taking down device > > >- return ifindex ? nm_platform_link_is_up (ifindex) : TRUE; > >+ return ifindex > 0 ? nm_platform_link_is_up (ifindex) : TRUE; > > these fixes seem unrelated to the rest of the patch. could you split them out? > > Also, it looks like take_down() needs the same fix? Changed, and 3 commits pushed to master: 93e4e0f core: minor fix to ensure we call platform functions with positive ifindex dd2ce3d core: fix waiting for bringing up/taking down device 66f5256 core: add nm_platform_link_refresh() function to refresh the libnl cache for links