* ledtrig netdev: what is the purpose of spinlock usage? @ 2020-10-29 6:00 Marek Behún 2020-10-29 17:45 ` Pavel Machek 0 siblings, 1 reply; 6+ messages in thread From: Marek Behún @ 2020-10-29 6:00 UTC (permalink / raw) To: Ben Whitten; +Cc: linux-leds Hi Ben (or Pavel or whomever can explain this), since you are the original author of netdev LED trigger, I guess this question should go to you. Why are spinlocks used as locks in the netdev trigger code? Is this for performance? Would it be a drastic performance hit to use mutexes? The reason why I am asking is that I am designing an API for transparent HW offload of LED triggers. (Some LEDs, eg. LEDs on ethernet PHYs, can blink on rx/tx activity themselves). You can find this at https://git.kernel.org/pub/scm/linux/kernel/git/kabel/linux.git/log/?h=leds-trigger-hw-offload The current approach for the netdev trigger is that the set_baseline_state function calls trigger_offload() method of the LED classdev. But the whole set_baseline_state function is called from within spinlock, and so when the trigger_offload method calls something that can sleep (MDIO bus communication, for example), kernel complains: BUG: scheduling while atomic Thanks. Marek ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: ledtrig netdev: what is the purpose of spinlock usage? 2020-10-29 6:00 ledtrig netdev: what is the purpose of spinlock usage? Marek Behún @ 2020-10-29 17:45 ` Pavel Machek 2020-10-29 18:13 ` Marek Behún 0 siblings, 1 reply; 6+ messages in thread From: Pavel Machek @ 2020-10-29 17:45 UTC (permalink / raw) To: Marek Behún; +Cc: Ben Whitten, linux-leds [-- Attachment #1: Type: text/plain, Size: 486 bytes --] Hi! > since you are the original author of netdev LED trigger, I guess this > question should go to you. Why are spinlocks used as locks in the > netdev trigger code? Is this for performance? Would it be a drastic > performance hit to use mutexes? Triggers can be called from interrupt context, IIRC, and many simple LEDs can be operated from interrupt context, too. Thus you need spinlocks... Best regards, Pavel -- http://www.livejournal.com/~pavelmachek [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 195 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: ledtrig netdev: what is the purpose of spinlock usage? 2020-10-29 17:45 ` Pavel Machek @ 2020-10-29 18:13 ` Marek Behún 2020-10-29 21:32 ` Ben Whitten 2020-10-29 22:49 ` Jacek Anaszewski 0 siblings, 2 replies; 6+ messages in thread From: Marek Behún @ 2020-10-29 18:13 UTC (permalink / raw) To: Pavel Machek; +Cc: Ben Whitten, linux-leds On Thu, 29 Oct 2020 18:45:29 +0100 Pavel Machek <pavel@ucw.cz> wrote: > Hi! > > > since you are the original author of netdev LED trigger, I guess this > > question should go to you. Why are spinlocks used as locks in the > > netdev trigger code? Is this for performance? Would it be a drastic > > performance hit to use mutexes? > > Triggers can be called from interrupt context, IIRC, and many simple > LEDs can be operated from interrupt context, too. > > Thus you need spinlocks... > > Best regards, > Pavel Pavel, the set_baseline_state function in netdev trigger is guarded by a spinlock only when reading/writing device_name attribute and in netdev notify callback. netdev_trig_notify can for example put the device away on NETDEV_UNREGISTER event, and if someone was reading/writing the device_name at the same time netdev_trig_notify is manipulating netdevice pointer, it could break. So my guess for the lock is that it is used because of this. It is possible that netdev_trig_notify can be called from interrupt context, I will have to look into this. Anyway for the trigger_offload() method to be able to communicate with PHYs I need the set_baseline_state function not to be called from within spinlock. Otherwise the drivers implementing this method would get too complicated. Would it be allright if on netdev event (link up, link down, netdev changename, netdev unregister) the set_baseline_state was called from a work, instead of directly from the spinlock? I will send a patch proposing and explaining this. Marek ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: ledtrig netdev: what is the purpose of spinlock usage? 2020-10-29 18:13 ` Marek Behún @ 2020-10-29 21:32 ` Ben Whitten 2020-10-29 22:49 ` Jacek Anaszewski 1 sibling, 0 replies; 6+ messages in thread From: Ben Whitten @ 2020-10-29 21:32 UTC (permalink / raw) To: Marek Behún; +Cc: Pavel Machek, linux-leds Hi, On Thu, 29 Oct 2020 at 18:13, Marek Behún <kabel@kernel.org> wrote: > > On Thu, 29 Oct 2020 18:45:29 +0100 > Pavel Machek <pavel@ucw.cz> wrote: > > > Hi! > > > > > since you are the original author of netdev LED trigger, I guess this > > > question should go to you. Why are spinlocks used as locks in the > > > netdev trigger code? Is this for performance? Would it be a drastic > > > performance hit to use mutexes? > > > > Triggers can be called from interrupt context, IIRC, and many simple > > LEDs can be operated from interrupt context, too. > > > > Thus you need spinlocks... > > > > Best regards, > > Pavel > > Pavel, > > the set_baseline_state function in netdev trigger is guarded by a > spinlock only when reading/writing device_name attribute and in > netdev notify callback. > > netdev_trig_notify can for example put the device away on > NETDEV_UNREGISTER event, and if someone was reading/writing the > device_name at the same time netdev_trig_notify is manipulating > netdevice pointer, it could break. So my guess for the lock is that it > is used because of this. > > It is possible that netdev_trig_notify can be called from interrupt > context, I will have to look into this. IIRC that is what I was seeing on my platform and I used the same locking mechanism for consistency, though my memory is hazy. > > Anyway for the trigger_offload() method to be able to communicate with > PHYs I need the set_baseline_state function not to be called from > within spinlock. Otherwise the drivers implementing this method would > get too complicated. Would it be allright if on netdev event (link up, > link down, netdev changename, netdev unregister) the set_baseline_state > was called from a work, instead of directly from the spinlock? > > I will send a patch proposing and explaining this. Go for it, patches welcome! Sounds interesting. > > Marek ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: ledtrig netdev: what is the purpose of spinlock usage? 2020-10-29 18:13 ` Marek Behún 2020-10-29 21:32 ` Ben Whitten @ 2020-10-29 22:49 ` Jacek Anaszewski 2020-10-29 23:14 ` Marek Behún 1 sibling, 1 reply; 6+ messages in thread From: Jacek Anaszewski @ 2020-10-29 22:49 UTC (permalink / raw) To: Marek Behún, Pavel Machek; +Cc: Ben Whitten, linux-leds Hi Marek, On 10/29/20 7:13 PM, Marek Behún wrote: > On Thu, 29 Oct 2020 18:45:29 +0100 > Pavel Machek <pavel@ucw.cz> wrote: > >> Hi! >> >>> since you are the original author of netdev LED trigger, I guess this >>> question should go to you. Why are spinlocks used as locks in the >>> netdev trigger code? Is this for performance? Would it be a drastic >>> performance hit to use mutexes? >> >> Triggers can be called from interrupt context, IIRC, and many simple >> LEDs can be operated from interrupt context, too. >> >> Thus you need spinlocks... >> >> Best regards, >> Pavel > > Pavel, > > the set_baseline_state function in netdev trigger is guarded by a > spinlock only when reading/writing device_name attribute and in > netdev notify callback. > > netdev_trig_notify can for example put the device away on > NETDEV_UNREGISTER event, and if someone was reading/writing the > device_name at the same time netdev_trig_notify is manipulating > netdevice pointer, it could break. So my guess for the lock is that it > is used because of this. > > It is possible that netdev_trig_notify can be called from interrupt > context, I will have to look into this. > > Anyway for the trigger_offload() method to be able to communicate with > PHYs I need the set_baseline_state function not to be called from > within spinlock. Otherwise the drivers implementing this method would > get too complicated. Would it be allright if on netdev event (link up, > link down, netdev changename, netdev unregister) the set_baseline_state > was called from a work, instead of directly from the spinlock? > > I will send a patch proposing and explaining this. register_netdevice_notifier() registers raw notifier chain, whose callbacks are not called from atomic context and there are no restrictions on callbacks. See include/linux/notifier.h. So it looks like the spin_lock_bh() can be safely changed to mutex_lock(). -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: ledtrig netdev: what is the purpose of spinlock usage? 2020-10-29 22:49 ` Jacek Anaszewski @ 2020-10-29 23:14 ` Marek Behún 0 siblings, 0 replies; 6+ messages in thread From: Marek Behún @ 2020-10-29 23:14 UTC (permalink / raw) To: Jacek Anaszewski; +Cc: Pavel Machek, Ben Whitten, linux-leds On Thu, 29 Oct 2020 23:49:19 +0100 Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote: > register_netdevice_notifier() registers raw notifier chain, > whose callbacks are not called from atomic context and there are > no restrictions on callbacks. See include/linux/notifier.h. > > So it looks like the spin_lock_bh() can be safely changed to > mutex_lock(). > Niiiiceeee, this simplifies things much. Thank you, Jacek :) ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-10-29 23:14 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-10-29 6:00 ledtrig netdev: what is the purpose of spinlock usage? Marek Behún 2020-10-29 17:45 ` Pavel Machek 2020-10-29 18:13 ` Marek Behún 2020-10-29 21:32 ` Ben Whitten 2020-10-29 22:49 ` Jacek Anaszewski 2020-10-29 23:14 ` Marek Behún
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).