[RFC PATCH v1 0/1] Support to use own workqueue for each LED

[RFC PATCH v1 0/1] Support to use own workqueue for each LED

[Arseniy Krasnov]

This allows to use own workqueue for each LED. This could be useful when we
have multiple LEDs which must work mutually (from user's point of view), for
example when complex animation must be played. Problem is that default wq -
'system_wq' does not guarantee order of callbacks execution, which control
brightness of every LED. So when userspace or pattern logic wants to change
brightness in one order, kworkers may do it in random way, thus breaking
smoothness of animation.

Here is example how to use this patch:

--- a/drivers/leds/leds-aw2013.c
+++ b/drivers/leds/leds-aw2013.c
@@ -264,11 +264,17 @@ static int aw2013_probe_dt(struct aw2013 *chip)
 	struct device_node *np = dev_of_node(&chip->client->dev), *child;
 	int count, ret = 0, i = 0;
 	struct aw2013_led *led;
+	struct workqueue_struct *wq;

 	count = of_get_available_child_count(np);
 	if (!count || count > AW2013_MAX_LEDS)
 		return -EINVAL;

+	wq = alloc_ordered_workqueue("aw2013_wq", 0);
+
+	if (!wq)
+		return -ENOMEM;
+
 	regmap_write(chip->regmap, AW2013_RSTR, AW2013_RSTR_RESET);

 	for_each_available_child_of_node(np, child) {
@@ -299,6 +305,7 @@ static int aw2013_probe_dt(struct aw2013 *chip)

 		led->cdev.brightness_set_blocking = aw2013_brightness_set;
 		led->cdev.blink_set = aw2013_blink_set;
+		led->cdev.set_brightness_wq = wq;

 		ret = devm_led_classdev_register_ext(&chip->client->dev,
 						     &led->cdev, &init_data);

E.g. special workqueue must be allocated and set for each LED during init of
'led_classdev'. Then later in 'led_init_core()', 'system_wq' won't be used for
such LEDs.

Arseniy Krasnov(1):
 drivers/leds/led-core.c | 8 ++++++--
 include/linux/leds.h    | 1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

-- 
2.35.0

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

[Arseniy Krasnov]

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(-)

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
-- 
2.35.0

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 522 bytes --]

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
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Arseniy Krasnov]

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

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1007 bytes --]

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

-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Arseniy Krasnov]

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
> 

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

[Dmitry Rokosov]

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.

-- 
Thank you,
Dmitry

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

[Dmitry Rokosov]

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.

-- 
Thank you,
Dmitry