diff mbox series

[RFC,v1,1/1] leds: support to use own workqueue for each LED

Message ID 33d05330-7c52-e873-bf32-209d40c77632@sberdevices.ru
State New
Headers
Series [RFC,v1,1/1] leds: support to use own workqueue for each LED |

Commit Message

Arseniy Krasnov Oct. 30, 2022, 6:07 a.m. UTC 
  This allows to set own workqueue for each LED. This may be useful, because
default 'system_wq' does not guarantee execution order of each work_struct,
thus for several brightness update requests (for multiple leds), real
brightness switch could be in random order.

Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
---
 drivers/leds/led-core.c | 8 ++++++--
 include/linux/leds.h    | 1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

-- 
2.35.0

Comments

Pavel Machek Oct. 30, 2022, 12:20 p.m. UTC | #1 
On Sun 2022-10-30 06:07:05, Arseniy Krasnov wrote:
> This allows to set own workqueue for each LED. This may be useful, because
> default 'system_wq' does not guarantee execution order of each work_struct,
> thus for several brightness update requests (for multiple leds), real
> brightness switch could be in random order.

So.. what?

Even if execution order is switched, human eye will not be able to
tell the difference.
								Pavel
Arseniy Krasnov Oct. 30, 2022, 2:01 p.m. UTC | #2 
On 30.10.2022 15:20, Pavel Machek wrote:
> On Sun 2022-10-30 06:07:05, Arseniy Krasnov wrote:
>> This allows to set own workqueue for each LED. This may be useful, because
>> default 'system_wq' does not guarantee execution order of each work_struct,
>> thus for several brightness update requests (for multiple leds), real
>> brightness switch could be in random order.
> 
> So.. what?
> 
> Even if execution order is switched, human eye will not be able to
> tell the difference.
Hello,

Problem arises on one of our boards where we have 14 triples of leds(each
triple contains R G B). Test case is to play complex animation on all leds:
smooth switch from on RGB state to another. Sometimes there are glitches in
this process - divergence from expectable RGB state. We fixed this by using
ordered workqueue.

Thanks, Arseniy

> 								Pavel
Pavel Machek Oct. 30, 2022, 8:15 p.m. UTC | #3 
Hi!

> >> This allows to set own workqueue for each LED. This may be useful, because
> >> default 'system_wq' does not guarantee execution order of each work_struct,
> >> thus for several brightness update requests (for multiple leds), real
> >> brightness switch could be in random order.
> > 
> > So.. what?
> > 
> > Even if execution order is switched, human eye will not be able to
> > tell the difference.
> Hello,
> 
> Problem arises on one of our boards where we have 14 triples of leds(each
> triple contains R G B). Test case is to play complex animation on all leds:
> smooth switch from on RGB state to another. Sometimes there are glitches in
> this process - divergence from expectable RGB state. We fixed this by using
> ordered workqueue.

Are there other solutions possible? Like batch and always apply _all_
the updates you have queued from your the worker code?

Best regards,
								Pavel
Arseniy Krasnov Oct. 31, 2022, 7:01 a.m. UTC | #4 
On 30.10.2022 23:15, Pavel Machek wrote:
> Hi!
> 
>>>> This allows to set own workqueue for each LED. This may be useful, because
>>>> default 'system_wq' does not guarantee execution order of each work_struct,
>>>> thus for several brightness update requests (for multiple leds), real
>>>> brightness switch could be in random order.
>>>
>>> So.. what?
>>>
>>> Even if execution order is switched, human eye will not be able to
>>> tell the difference.
>> Hello,
>>
>> Problem arises on one of our boards where we have 14 triples of leds(each
>> triple contains R G B). Test case is to play complex animation on all leds:
>> smooth switch from on RGB state to another. Sometimes there are glitches in
>> this process - divergence from expectable RGB state. We fixed this by using
>> ordered workqueue.
> 
> Are there other solutions possible? Like batch and always apply _all_
> the updates you have queued from your the worker code?

IIUC You, it is possible to do this if brightness update requests are performed using
write to "brightness" file in /sys/class/led/. But if pattern trigger mode is used(in my
case) - I can't synchronize these requests as they are created internally in kernel on
timer tick.

Thanks, Arseniy
> 
> Best regards,
> 								Pavel
>
Dmitry Rokosov Oct. 31, 2022, 2:47 p.m. UTC | #5 
Hello Pavel and Arseniy,

Please find my thoughts below.

On Mon, Oct 31, 2022 at 10:01:28AM +0300, Arseniy Krasnov wrote:
> On 30.10.2022 23:15, Pavel Machek wrote:
> > Hi!
> > 
> >>>> This allows to set own workqueue for each LED. This may be useful, because
> >>>> default 'system_wq' does not guarantee execution order of each work_struct,
> >>>> thus for several brightness update requests (for multiple leds), real
> >>>> brightness switch could be in random order.
> >>>
> >>> So.. what?
> >>>
> >>> Even if execution order is switched, human eye will not be able to
> >>> tell the difference.
> >> Hello,
> >>
> >> Problem arises on one of our boards where we have 14 triples of leds(each
> >> triple contains R G B). Test case is to play complex animation on all leds:
> >> smooth switch from on RGB state to another. Sometimes there are glitches in
> >> this process - divergence from expectable RGB state. We fixed this by using
> >> ordered workqueue.
> > 
> > Are there other solutions possible? Like batch and always apply _all_
> > the updates you have queued from your the worker code?
> 
> IIUC You, it is possible to do this if brightness update requests are performed using
> write to "brightness" file in /sys/class/led/. But if pattern trigger mode is used(in my
> case) - I can't synchronize these requests as they are created internally in kernel on
> timer tick.

Even more, system_wq is used when you push brightness changing requests
to sysfs node, and it could be re-ordered as well. In other words, from
queue perspective sysfs iface and trigger iface have the same behavior.

Also we can be faced with another big problem here: let's imagine you have
I2C based LED controller driver. Usually, in such drivers you're stuck
to the one driver owned mutex, which protects I2C transactions from each
other.

When you change brightness very often (let's say a hundred thousand times
per minute) you schedule many workers to system_wq. Due to system_wq is
multicore and unordered it creates many kworkers. Each kworker stucks on
the driver mutex and goes to TASK_UNINTERRUPTIBLE state. It affects Load
Average value so much. On the our device LA maximum could reach 30-35
units due to such idle kworkers.

I'm not sure custom workqueue initialization from specific HW driver is
a good solution... But it's much better than nothing.

Pavel, please share your thoughts about above problems? Maybe you have
more advanced and scalable solution idea, I would appreciate if you
could share it with us.
Dmitry Rokosov Nov. 9, 2022, 6:01 p.m. UTC | #6 
Hello Pavel,

Do you have any feedback/thoughts about this one?
The problem looks very bad, Load Average value should not grow up due to
any userspace triggered led animation. I suppose we stronlgy need to
fixup this behavior for sure.

Appreciate any help!

On Mon, Oct 31, 2022 at 05:47:04PM +0300, Dmitry Rokosov wrote:
> Hello Pavel and Arseniy,
> 
> Please find my thoughts below.
> 
> On Mon, Oct 31, 2022 at 10:01:28AM +0300, Arseniy Krasnov wrote:
> > On 30.10.2022 23:15, Pavel Machek wrote:
> > > Hi!
> > > 
> > >>>> This allows to set own workqueue for each LED. This may be useful, because
> > >>>> default 'system_wq' does not guarantee execution order of each work_struct,
> > >>>> thus for several brightness update requests (for multiple leds), real
> > >>>> brightness switch could be in random order.
> > >>>
> > >>> So.. what?
> > >>>
> > >>> Even if execution order is switched, human eye will not be able to
> > >>> tell the difference.
> > >> Hello,
> > >>
> > >> Problem arises on one of our boards where we have 14 triples of leds(each
> > >> triple contains R G B). Test case is to play complex animation on all leds:
> > >> smooth switch from on RGB state to another. Sometimes there are glitches in
> > >> this process - divergence from expectable RGB state. We fixed this by using
> > >> ordered workqueue.
> > > 
> > > Are there other solutions possible? Like batch and always apply _all_
> > > the updates you have queued from your the worker code?
> > 
> > IIUC You, it is possible to do this if brightness update requests are performed using
> > write to "brightness" file in /sys/class/led/. But if pattern trigger mode is used(in my
> > case) - I can't synchronize these requests as they are created internally in kernel on
> > timer tick.
> 
> Even more, system_wq is used when you push brightness changing requests
> to sysfs node, and it could be re-ordered as well. In other words, from
> queue perspective sysfs iface and trigger iface have the same behavior.
> 
> Also we can be faced with another big problem here: let's imagine you have
> I2C based LED controller driver. Usually, in such drivers you're stuck
> to the one driver owned mutex, which protects I2C transactions from each
> other.
> 
> When you change brightness very often (let's say a hundred thousand times
> per minute) you schedule many workers to system_wq. Due to system_wq is
> multicore and unordered it creates many kworkers. Each kworker stucks on
> the driver mutex and goes to TASK_UNINTERRUPTIBLE state. It affects Load
> Average value so much. On the our device LA maximum could reach 30-35
> units due to such idle kworkers.
> 
> I'm not sure custom workqueue initialization from specific HW driver is
> a good solution... But it's much better than nothing.
> 
> Pavel, please share your thoughts about above problems? Maybe you have
> more advanced and scalable solution idea, I would appreciate if you
> could share it with us.
diff mbox series

Patch

diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 4a97cb745788..e8ecf332793a 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -188,6 +188,9 @@  static void led_blink_setup(struct led_classdev *led_cdev,
 
 void led_init_core(struct led_classdev *led_cdev)
 {
+	if (!led_cdev->set_brightness_wq)
+		led_cdev->set_brightness_wq = system_wq;
+
 	INIT_WORK(&led_cdev->set_brightness_work, set_brightness_delayed);
 
 	timer_setup(&led_cdev->blink_timer, led_timer_function, 0);
@@ -252,7 +255,8 @@  void led_set_brightness(struct led_classdev *led_cdev, unsigned int brightness)
 		 */
 		if (!brightness) {
 			set_bit(LED_BLINK_DISABLE, &led_cdev->work_flags);
-			schedule_work(&led_cdev->set_brightness_work);
+			queue_work(led_cdev->set_brightness_wq,
+				   &led_cdev->set_brightness_work);
 		} else {
 			set_bit(LED_BLINK_BRIGHTNESS_CHANGE,
 				&led_cdev->work_flags);
@@ -273,7 +277,7 @@  void led_set_brightness_nopm(struct led_classdev *led_cdev, unsigned int value)
 
 	/* If brightness setting can sleep, delegate it to a work queue task */
 	led_cdev->delayed_set_value = value;
-	schedule_work(&led_cdev->set_brightness_work);
+	queue_work(led_cdev->set_brightness_wq, &led_cdev->set_brightness_work);
 }
 EXPORT_SYMBOL_GPL(led_set_brightness_nopm);
 
diff --git a/include/linux/leds.h b/include/linux/leds.h
index ba4861ec73d3..1121bae21fed 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -140,6 +140,7 @@  struct led_classdev {
 	void			(*flash_resume)(struct led_classdev *led_cdev);
 
 	struct work_struct	set_brightness_work;
+	struct workqueue_struct *set_brightness_wq;
 	int			delayed_set_value;
 
 #ifdef CONFIG_LEDS_TRIGGERS