From: Hans de Goede <hdegoede@redhat.com>
To: Pavel Machek <pavel@ucw.cz>, Marc Kleine-Budde <mkl@pengutronix.de>
Cc: Andrea Righi <andrea.righi@canonical.com>,
Boqun Feng <boqun.feng@gmail.com>, Dan Murphy <dmurphy@ti.com>,
linux-leds@vger.kernel.org, linux-kernel@vger.kernel.org,
kernel@pengutronix.de, schuchmann@schleissheimer.de
Subject: Re: [PATCH] leds: trigger: fix potential deadlock with libata
Date: Sun, 7 Mar 2021 20:04:42 +0100 [thread overview]
Message-ID: <7a83931f-13bd-27c2-4050-4a21be74c49b@redhat.com> (raw)
In-Reply-To: <20210307161357.GA2933@duo.ucw.cz>
Hi,
On 3/7/21 5:13 PM, Pavel Machek wrote:
> Hi!
>
>>> --- 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()?
>
> Hans was pushing for this patch, perhaps he has some ideas...
I was not pushing for this particular fix, I was asking about a fix
for the lockdep identified potential deadlock.
And you replied that this was already fixed in your for-next branch
when I asked, so all in all, other then reporting the potential deadlock
(after it was already fixed) I have very little do to with this patch.
With that all said, I must say that I'm surprised that switching from
read_lock() to read_lock_irqsave() causes such a hefty penalty, so I
wonder what is really going on here. Using the irqsave version disables
interrupts, but AFAIK only on the current core and only for the duration
of the led_set_brightness() call(s) .
Is the system perhaps pinning IRQs to a specific CPU in combination with
a led_set_brightness() somehow taking much longer then it should?
Note that led_set_brightness() calls are not allowed to block, if they
block they should use the brightness_set_blocking callback in their
led_class_dev struct not the regular brightness_set callback. In which case
the LED-core will defer the actually setting of the LED to a workqueue.
So one thing which might be worthwhile to check is if any of the LED
drivers on the system in question are using the brightness_set callback,
where they should be using the blocking one.
Regards,
Hans
next prev parent reply other threads:[~2021-03-07 19:05 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-02 10:41 [PATCH] leds: trigger: fix potential deadlock with libata Andrea Righi
2020-11-25 12:46 ` Pavel Machek
2020-11-25 14:01 ` Boqun Feng
2020-11-25 14:15 ` Andrea Righi
2020-11-25 15:20 ` Andrea Righi
2021-03-06 20:39 ` Marc Kleine-Budde
2021-03-07 2:02 ` Boqun Feng
2021-03-07 7:14 ` Andrea Righi
2021-03-08 14:56 ` Marc Kleine-Budde
2021-03-08 15:05 ` Marc Kleine-Budde
2021-03-07 16:13 ` Pavel Machek
2021-03-07 19:04 ` Hans de Goede [this message]
2021-03-07 16:26 ` Pavel Machek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7a83931f-13bd-27c2-4050-4a21be74c49b@redhat.com \
--to=hdegoede@redhat.com \
--cc=andrea.righi@canonical.com \
--cc=boqun.feng@gmail.com \
--cc=dmurphy@ti.com \
--cc=kernel@pengutronix.de \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-leds@vger.kernel.org \
--cc=mkl@pengutronix.de \
--cc=pavel@ucw.cz \
--cc=schuchmann@schleissheimer.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).