* 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).