On Sat, Mar 06, 2021 at 09:39:54PM +0100, Marc Kleine-Budde wrote:
> Hello *,
>
> On 02.11.2020 11:41:52, Andrea Righi wrote:
> > We have the following potential deadlock condition:
> >
> > ========================================================
> > WARNING: possible irq lock inversion dependency detected
> > 5.10.0-rc2+ #25 Not tainted
> > --------------------------------------------------------
> > swapper/3/0 just changed the state of lock:
> > ffff8880063bd618 (&host->lock){-...}-{2:2}, at: ata_bmdma_interrupt+0x27/0x200
> > but this lock took another, HARDIRQ-READ-unsafe lock in the past:
> > (&trig->leddev_list_lock){.+.?}-{2:2}
> >
> > and interrupts could create inverse lock ordering between them.
>
> [...]
>
> > ---
> > drivers/leds/led-triggers.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> > index 91da90cfb11d..16d1a93a10a8 100644
> > --- a/drivers/leds/led-triggers.c
> > +++ b/drivers/leds/led-triggers.c
> > @@ -378,14 +378,15 @@ void led_trigger_event(struct led_trigger *trig,
> > enum led_brightness brightness)
> > {
> > struct led_classdev *led_cdev;
> > + unsigned long flags;
> >
> > if (!trig)
> > return;
> >
> > - read_lock(&trig->leddev_list_lock);
> > + read_lock_irqsave(&trig->leddev_list_lock, flags);
> > list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list)
> > led_set_brightness(led_cdev, brightness);
> > - read_unlock(&trig->leddev_list_lock);
> > + read_unlock_irqrestore(&trig->leddev_list_lock, flags);
> > }
> > EXPORT_SYMBOL_GPL(led_trigger_event);
>
> meanwhile this patch hit v5.10.x stable and caused a performance
> degradation on our use case:
>
> It's an embedded ARM system, 4x Cortex A53, with an SPI attached CAN
> controller. CAN stands for Controller Area Network and here used to
> connect to some automotive equipment. Over CAN an ISOTP (a CAN-specific
> Transport Protocol) transfer is running. With this patch, we see CAN
> frames delayed for ~6ms, the usual gap between CAN frames is 240µs.
>
> Reverting this patch, restores the old performance.
>
> What is the best way to solve this dilemma? Identify the critical path
> in our use case? Is there a way we can get around the irqsave in
> led_trigger_event()?
>
Probably, we can change from rwlock to rcu here, POC code as follow,
only compile tested. Marc, could you see whether this help the
performance on your platform? Please note that I haven't test it in a
running kernel and I'm not that familir with led subsystem, so use it
with caution ;-)
(While at it, I think maybe we miss the leddev_list_lock in net/mac80211
in the patch)
Regards,
Boqun
------->8
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 4e7b78a84149..ae68ccab6cc9 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -171,10 +171,12 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
/* Remove any existing trigger */
if (led_cdev->trigger) {
- write_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags);
- list_del(&led_cdev->trig_list);
- write_unlock_irqrestore(&led_cdev->trigger->leddev_list_lock,
+ spin_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags);
+ list_del_rcu(&led_cdev->trig_list);
+ spin_unlock_irqrestore(&led_cdev->trigger->leddev_list_lock,
flags);
+ /* Wait for the readers gone */
+ synchronize_rcu();
cancel_work_sync(&led_cdev->set_brightness_work);
led_stop_software_blink(led_cdev);
if (led_cdev->trigger->deactivate)
@@ -186,9 +188,9 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
led_set_brightness(led_cdev, LED_OFF);
}
if (trig) {
- write_lock_irqsave(&trig->leddev_list_lock, flags);
- list_add_tail(&led_cdev->trig_list, &trig->led_cdevs);
- write_unlock_irqrestore(&trig->leddev_list_lock, flags);
+ spin_lock_irqsave(&trig->leddev_list_lock, flags);
+ list_add_tail_rcu(&led_cdev->trig_list, &trig->led_cdevs);
+ spin_unlock_irqrestore(&trig->leddev_list_lock, flags);
led_cdev->trigger = trig;
if (trig->activate)
@@ -223,9 +225,12 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
trig->deactivate(led_cdev);
err_activate:
- write_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags);
- list_del(&led_cdev->trig_list);
- write_unlock_irqrestore(&led_cdev->trigger->leddev_list_lock, flags);
+ spin_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags);
+ list_del_rcu(&led_cdev->trig_list);
+ spin_unlock_irqrestore(&led_cdev->trigger->leddev_list_lock, flags);
+
+ /* XXX could use call_rcu() here? */
+ synchronize_rcu();
led_cdev->trigger = NULL;
led_cdev->trigger_data = NULL;
led_set_brightness(led_cdev, LED_OFF);
@@ -285,7 +290,7 @@ int led_trigger_register(struct led_trigger *trig)
struct led_classdev *led_cdev;
struct led_trigger *_trig;
- rwlock_init(&trig->leddev_list_lock);
+ spin_lock_init(&trig->leddev_list_lock);
INIT_LIST_HEAD(&trig->led_cdevs);
down_write(&triggers_list_lock);
@@ -378,15 +383,14 @@ void led_trigger_event(struct led_trigger *trig,
enum led_brightness brightness)
{
struct led_classdev *led_cdev;
- unsigned long flags;
if (!trig)
return;
- read_lock_irqsave(&trig->leddev_list_lock, flags);
- list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list)
+ rcu_read_lock();
+ list_for_each_entry_rcu(led_cdev, &trig->led_cdevs, trig_list)
led_set_brightness(led_cdev, brightness);
- read_unlock_irqrestore(&trig->leddev_list_lock, flags);
+ rcu_read_unlock();
}
EXPORT_SYMBOL_GPL(led_trigger_event);
@@ -397,20 +401,19 @@ static void led_trigger_blink_setup(struct led_trigger *trig,
int invert)
{
struct led_classdev *led_cdev;
- unsigned long flags;
if (!trig)
return;
- read_lock_irqsave(&trig->leddev_list_lock, flags);
- list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list) {
+ rcu_read_lock();
+ list_for_each_entry_rcu(led_cdev, &trig->led_cdevs, trig_list) {
if (oneshot)
led_blink_set_oneshot(led_cdev, delay_on, delay_off,
invert);
else
led_blink_set(led_cdev, delay_on, delay_off);
}
- read_unlock_irqrestore(&trig->leddev_list_lock, flags);
+ rcu_read_unlock();
}
void led_trigger_blink(struct led_trigger *trig,
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 6a8d6409c993..5acc0e8a9f18 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -356,7 +356,7 @@ struct led_trigger {
struct led_hw_trigger_type *trigger_type;
/* LEDs under control by this trigger (for simple triggers) */
- rwlock_t leddev_list_lock;
+ spinlock_t leddev_list_lock;
struct list_head led_cdevs;
/* Link to next registered trigger */
diff --git a/net/mac80211/led.c b/net/mac80211/led.c
index b275c8853074..5ec5070fe210 100644
--- a/net/mac80211/led.c
+++ b/net/mac80211/led.c
@@ -283,10 +283,10 @@ static void tpt_trig_timer(struct timer_list *t)
}
}
- read_lock(&local->tpt_led.leddev_list_lock);
+ rcu_read_lock();
list_for_each_entry(led_cdev, &local->tpt_led.led_cdevs, trig_list)
led_blink_set(led_cdev, &on, &off);
- read_unlock(&local->tpt_led.leddev_list_lock);
+ rcu_read_unlock();
}
const char *
@@ -349,10 +349,10 @@ static void ieee80211_stop_tpt_led_trig(struct ieee80211_local *local)
tpt_trig->running = false;
del_timer_sync(&tpt_trig->timer);
- read_lock(&local->tpt_led.leddev_list_lock);
+ rcu_read_lock();
list_for_each_entry(led_cdev, &local->tpt_led.led_cdevs, trig_list)
led_set_brightness(led_cdev, LED_OFF);
- read_unlock(&local->tpt_led.leddev_list_lock);
+ rcu_read_unlock();
}
void ieee80211_mod_tpt_led_trig(struct ieee80211_local *local,
