Re: ledtrig-cpu: Limit to 4 CPUs

Re: ledtrig-cpu: Limit to 4 CPUs

[Jacek Anaszewski]

Hi Pavel,

On 9/19/20 11:38 AM, Pavel Machek wrote:
> commit 318681d3e019e39354cc6c2155a7fd1bb8e8084d
> Author: Pavel Machek <pavel@ucw.cz>
> Date:   Sat Sep 19 11:34:58 2020 +0200
> 
>      ledtrig-cpu: Limit to 4 CPUs
>      
>      Some machines have thousands of CPUs... and trigger mechanisms was not
>      really meant for thousands of triggers. I doubt anyone uses this
>      trigger on many-CPU machine; but if they do, they'll need to do it
>      properly.
>      
>      Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> diff --git a/drivers/leds/trigger/ledtrig-cpu.c b/drivers/leds/trigger/ledtrig-cpu.c
> index 869976d1b734..b7e00b09b137 100644
> --- a/drivers/leds/trigger/ledtrig-cpu.c
> +++ b/drivers/leds/trigger/ledtrig-cpu.c
> @@ -2,14 +2,18 @@
>   /*
>    * ledtrig-cpu.c - LED trigger based on CPU activity
>    *
> - * This LED trigger will be registered for each possible CPU and named as
> - * cpu0, cpu1, cpu2, cpu3, etc.
> + * This LED trigger will be registered for first four CPUs and named
> + * as cpu0, cpu1, cpu2, cpu3. There's additional trigger called cpu that
> + * is on when any CPU is active.
> + *
> + * If you want support for arbitrary number of CPUs, make it one trigger,
> + * with additional sysfs file selecting which CPU to watch.
>    *
>    * It can be bound to any LED just like other triggers using either a
>    * board file or via sysfs interface.
>    *
>    * An API named ledtrig_cpu is exported for any user, who want to add CPU
> - * activity indication in their code
> + * activity indication in their code.
>    *
>    * Copyright 2011 Linus Walleij <linus.walleij@linaro.org>
>    * Copyright 2011 - 2012 Bryan Wu <bryan.wu@canonical.com>
> @@ -145,6 +149,9 @@ static int __init ledtrig_cpu_init(void)
>   	for_each_possible_cpu(cpu) {
>   		struct led_trigger_cpu *trig = &per_cpu(cpu_trig, cpu);
>   
> +		if (cpu > 4)

NACK. The workaround for this trigger was implemented for a reason -
to make it working on platforms with arbitrary number of logical cpus.
I've got 8, so I am discriminated now. Not saying, that it precludes
trigger registration with no single line of warning.
Regardless of that - you have no guarantee that you're not breaking
anyone - "I doubt" is not a sufficient argument.

> +			continue;
> +
>   		snprintf(trig->name, MAX_NAME_LEN, "cpu%d", cpu);
>   
>   		led_trigger_register_simple(trig->name, &trig->_trig);
> 

-- 
Best regards,
Jacek Anaszewski

Re: ledtrig-cpu: Limit to 4 CPUs

[Marek Behun]

On Sun, 20 Sep 2020 16:15:09 +0200
Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:

> Hi Pavel,
> 
> On 9/19/20 11:38 AM, Pavel Machek wrote:
> > commit 318681d3e019e39354cc6c2155a7fd1bb8e8084d
> > Author: Pavel Machek <pavel@ucw.cz>
> > Date:   Sat Sep 19 11:34:58 2020 +0200
> > 
> >      ledtrig-cpu: Limit to 4 CPUs
> >      
> >      Some machines have thousands of CPUs... and trigger mechanisms was not
> >      really meant for thousands of triggers. I doubt anyone uses this
> >      trigger on many-CPU machine; but if they do, they'll need to do it
> >      properly.
> >      
> >      Signed-off-by: Pavel Machek <pavel@ucw.cz>
> > 
> > diff --git a/drivers/leds/trigger/ledtrig-cpu.c b/drivers/leds/trigger/ledtrig-cpu.c
> > index 869976d1b734..b7e00b09b137 100644
> > --- a/drivers/leds/trigger/ledtrig-cpu.c
> > +++ b/drivers/leds/trigger/ledtrig-cpu.c
> > @@ -2,14 +2,18 @@
> >   /*
> >    * ledtrig-cpu.c - LED trigger based on CPU activity
> >    *
> > - * This LED trigger will be registered for each possible CPU and named as
> > - * cpu0, cpu1, cpu2, cpu3, etc.
> > + * This LED trigger will be registered for first four CPUs and named
> > + * as cpu0, cpu1, cpu2, cpu3. There's additional trigger called cpu that
> > + * is on when any CPU is active.
> > + *
> > + * If you want support for arbitrary number of CPUs, make it one trigger,
> > + * with additional sysfs file selecting which CPU to watch.
> >    *
> >    * It can be bound to any LED just like other triggers using either a
> >    * board file or via sysfs interface.
> >    *
> >    * An API named ledtrig_cpu is exported for any user, who want to add CPU
> > - * activity indication in their code
> > + * activity indication in their code.
> >    *
> >    * Copyright 2011 Linus Walleij <linus.walleij@linaro.org>
> >    * Copyright 2011 - 2012 Bryan Wu <bryan.wu@canonical.com>
> > @@ -145,6 +149,9 @@ static int __init ledtrig_cpu_init(void)
> >   	for_each_possible_cpu(cpu) {
> >   		struct led_trigger_cpu *trig = &per_cpu(cpu_trig, cpu);
> >   
> > +		if (cpu > 4)  
> 
> NACK. The workaround for this trigger was implemented for a reason -
> to make it working on platforms with arbitrary number of logical cpus.
> I've got 8, so I am discriminated now. Not saying, that it precludes
> trigger registration with no single line of warning.
> Regardless of that - you have no guarantee that you're not breaking
> anyone - "I doubt" is not a sufficient argument.
> 

If that is the case Jacek, I would try 16 and then see if people
complain. Do you really think that someone sets a specific LED to
trigger on activity on CPU id > 16?

If you do not agree, then I think we should implement a "cpu" trigger
where the cpu ID (or maybe mask of multiple CPUs) is configurable via
another sysfs file. And then declare current cpu trigger (with names
"cpu%d") as legacy.

Marek

Re: ledtrig-cpu: Limit to 4 CPUs

[Jacek Anaszewski]

On 9/20/20 5:39 PM, Marek Behun wrote:
> On Sun, 20 Sep 2020 16:15:09 +0200
> Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
> 
>> Hi Pavel,
>>
>> On 9/19/20 11:38 AM, Pavel Machek wrote:
>>> commit 318681d3e019e39354cc6c2155a7fd1bb8e8084d
>>> Author: Pavel Machek <pavel@ucw.cz>
>>> Date:   Sat Sep 19 11:34:58 2020 +0200
>>>
>>>       ledtrig-cpu: Limit to 4 CPUs
>>>       
>>>       Some machines have thousands of CPUs... and trigger mechanisms was not
>>>       really meant for thousands of triggers. I doubt anyone uses this
>>>       trigger on many-CPU machine; but if they do, they'll need to do it
>>>       properly.
>>>       
>>>       Signed-off-by: Pavel Machek <pavel@ucw.cz>
>>>
>>> diff --git a/drivers/leds/trigger/ledtrig-cpu.c b/drivers/leds/trigger/ledtrig-cpu.c
>>> index 869976d1b734..b7e00b09b137 100644
>>> --- a/drivers/leds/trigger/ledtrig-cpu.c
>>> +++ b/drivers/leds/trigger/ledtrig-cpu.c
>>> @@ -2,14 +2,18 @@
>>>    /*
>>>     * ledtrig-cpu.c - LED trigger based on CPU activity
>>>     *
>>> - * This LED trigger will be registered for each possible CPU and named as
>>> - * cpu0, cpu1, cpu2, cpu3, etc.
>>> + * This LED trigger will be registered for first four CPUs and named
>>> + * as cpu0, cpu1, cpu2, cpu3. There's additional trigger called cpu that
>>> + * is on when any CPU is active.
>>> + *
>>> + * If you want support for arbitrary number of CPUs, make it one trigger,
>>> + * with additional sysfs file selecting which CPU to watch.
>>>     *
>>>     * It can be bound to any LED just like other triggers using either a
>>>     * board file or via sysfs interface.
>>>     *
>>>     * An API named ledtrig_cpu is exported for any user, who want to add CPU
>>> - * activity indication in their code
>>> + * activity indication in their code.
>>>     *
>>>     * Copyright 2011 Linus Walleij <linus.walleij@linaro.org>
>>>     * Copyright 2011 - 2012 Bryan Wu <bryan.wu@canonical.com>
>>> @@ -145,6 +149,9 @@ static int __init ledtrig_cpu_init(void)
>>>    	for_each_possible_cpu(cpu) {
>>>    		struct led_trigger_cpu *trig = &per_cpu(cpu_trig, cpu);
>>>    
>>> +		if (cpu > 4)
>>
>> NACK. The workaround for this trigger was implemented for a reason -
>> to make it working on platforms with arbitrary number of logical cpus.
>> I've got 8, so I am discriminated now. Not saying, that it precludes
>> trigger registration with no single line of warning.
>> Regardless of that - you have no guarantee that you're not breaking
>> anyone - "I doubt" is not a sufficient argument.
>>
> 
> If that is the case Jacek, I would try 16 and then see if people
> complain. Do you really think that someone sets a specific LED to
> trigger on activity on CPU id > 16?

I have an access to the machine with 80 cpus, so I could once
get surprised not being able to find cpuN triggers not being
listed among available triggers.

And say that I have a solution where I install 80 userspace LEDs
(drivers/leds/uleds.c) and register them on each cpuN triggers to get
notifications on how cpus work.

> If you do not agree, then I think we should implement a "cpu" trigger
> where the cpu ID (or maybe mask of multiple CPUs) is configurable via
> another sysfs file. And then declare current cpu trigger (with names
> "cpu%d") as legacy.

Yes, we can do that, and even mark the cpu trigger as legacy but we
cannot prevent people from using it if that was present in kernel
for many years.

-- 
Best regards,
Jacek Anaszewski

Re: ledtrig-cpu: Limit to 4 CPUs

[Marek Behun]

On Sun, 20 Sep 2020 18:55:28 +0200
Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:

> On 9/20/20 5:39 PM, Marek Behun wrote:
> > On Sun, 20 Sep 2020 16:15:09 +0200
> > Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
> >   
> >> Hi Pavel,
> >>
> >> On 9/19/20 11:38 AM, Pavel Machek wrote:  
> >>> commit 318681d3e019e39354cc6c2155a7fd1bb8e8084d
> >>> Author: Pavel Machek <pavel@ucw.cz>
> >>> Date:   Sat Sep 19 11:34:58 2020 +0200
> >>>
> >>>       ledtrig-cpu: Limit to 4 CPUs
> >>>       
> >>>       Some machines have thousands of CPUs... and trigger mechanisms was not
> >>>       really meant for thousands of triggers. I doubt anyone uses this
> >>>       trigger on many-CPU machine; but if they do, they'll need to do it
> >>>       properly.
> >>>       
> >>>       Signed-off-by: Pavel Machek <pavel@ucw.cz>
> >>>
> >>> diff --git a/drivers/leds/trigger/ledtrig-cpu.c b/drivers/leds/trigger/ledtrig-cpu.c
> >>> index 869976d1b734..b7e00b09b137 100644
> >>> --- a/drivers/leds/trigger/ledtrig-cpu.c
> >>> +++ b/drivers/leds/trigger/ledtrig-cpu.c
> >>> @@ -2,14 +2,18 @@
> >>>    /*
> >>>     * ledtrig-cpu.c - LED trigger based on CPU activity
> >>>     *
> >>> - * This LED trigger will be registered for each possible CPU and named as
> >>> - * cpu0, cpu1, cpu2, cpu3, etc.
> >>> + * This LED trigger will be registered for first four CPUs and named
> >>> + * as cpu0, cpu1, cpu2, cpu3. There's additional trigger called cpu that
> >>> + * is on when any CPU is active.
> >>> + *
> >>> + * If you want support for arbitrary number of CPUs, make it one trigger,
> >>> + * with additional sysfs file selecting which CPU to watch.
> >>>     *
> >>>     * It can be bound to any LED just like other triggers using either a
> >>>     * board file or via sysfs interface.
> >>>     *
> >>>     * An API named ledtrig_cpu is exported for any user, who want to add CPU
> >>> - * activity indication in their code
> >>> + * activity indication in their code.
> >>>     *
> >>>     * Copyright 2011 Linus Walleij <linus.walleij@linaro.org>
> >>>     * Copyright 2011 - 2012 Bryan Wu <bryan.wu@canonical.com>
> >>> @@ -145,6 +149,9 @@ static int __init ledtrig_cpu_init(void)
> >>>    	for_each_possible_cpu(cpu) {
> >>>    		struct led_trigger_cpu *trig = &per_cpu(cpu_trig, cpu);
> >>>    
> >>> +		if (cpu > 4)  
> >>
> >> NACK. The workaround for this trigger was implemented for a reason -
> >> to make it working on platforms with arbitrary number of logical cpus.
> >> I've got 8, so I am discriminated now. Not saying, that it precludes
> >> trigger registration with no single line of warning.
> >> Regardless of that - you have no guarantee that you're not breaking
> >> anyone - "I doubt" is not a sufficient argument.
> >>  
> > 
> > If that is the case Jacek, I would try 16 and then see if people
> > complain. Do you really think that someone sets a specific LED to
> > trigger on activity on CPU id > 16?  
> 
> I have an access to the machine with 80 cpus, so I could once
> get surprised not being able to find cpuN triggers not being
> listed among available triggers.
> 
> And say that I have a solution where I install 80 userspace LEDs
> (drivers/leds/uleds.c) and register them on each cpuN triggers to get
> notifications on how cpus work.

Hi Jacek,

I understand (and Pavel does for sure too) that many people
currently have that possibility, that they have access to machines with
many CPUs and many LEDs. We also understand that currently it is
possible for users to set 1847th LED to trigger on activity on CPU ID
1337. What we are suggesting is that practically no one uses this, and
for those 10 people who do, well it would be better for them to migrate
to new ABI than for kernel developers having forever maintain this
legacy ABI.

Legacy drivers get removed from kernel from time to time, if no one
uses them. So I think Pavel's proposal (although I may not agree with
the limit 4) has some merit. If we try this, and someone complains, we
can then discuss. If we don't try, we may never know.

Marek

> > If you do not agree, then I think we should implement a "cpu" trigger
> > where the cpu ID (or maybe mask of multiple CPUs) is configurable via
> > another sysfs file. And then declare current cpu trigger (with names
> > "cpu%d") as legacy.  
> 
> Yes, we can do that, and even mark the cpu trigger as legacy but we
> cannot prevent people from using it if that was present in kernel
> for many years.
> 

Re: ledtrig-cpu: Limit to 4 CPUs

[Jacek Anaszewski]

On 9/20/20 7:33 PM, Marek Behun wrote:
> On Sun, 20 Sep 2020 18:55:28 +0200
> Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
> 
>> On 9/20/20 5:39 PM, Marek Behun wrote:
>>> On Sun, 20 Sep 2020 16:15:09 +0200
>>> Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
>>>    
>>>> Hi Pavel,
>>>>
>>>> On 9/19/20 11:38 AM, Pavel Machek wrote:
>>>>> commit 318681d3e019e39354cc6c2155a7fd1bb8e8084d
>>>>> Author: Pavel Machek <pavel@ucw.cz>
>>>>> Date:   Sat Sep 19 11:34:58 2020 +0200
>>>>>
>>>>>        ledtrig-cpu: Limit to 4 CPUs
>>>>>        
>>>>>        Some machines have thousands of CPUs... and trigger mechanisms was not
>>>>>        really meant for thousands of triggers. I doubt anyone uses this
>>>>>        trigger on many-CPU machine; but if they do, they'll need to do it
>>>>>        properly.
>>>>>        
>>>>>        Signed-off-by: Pavel Machek <pavel@ucw.cz>
>>>>>
>>>>> diff --git a/drivers/leds/trigger/ledtrig-cpu.c b/drivers/leds/trigger/ledtrig-cpu.c
>>>>> index 869976d1b734..b7e00b09b137 100644
>>>>> --- a/drivers/leds/trigger/ledtrig-cpu.c
>>>>> +++ b/drivers/leds/trigger/ledtrig-cpu.c
>>>>> @@ -2,14 +2,18 @@
>>>>>     /*
>>>>>      * ledtrig-cpu.c - LED trigger based on CPU activity
>>>>>      *
>>>>> - * This LED trigger will be registered for each possible CPU and named as
>>>>> - * cpu0, cpu1, cpu2, cpu3, etc.
>>>>> + * This LED trigger will be registered for first four CPUs and named
>>>>> + * as cpu0, cpu1, cpu2, cpu3. There's additional trigger called cpu that
>>>>> + * is on when any CPU is active.
>>>>> + *
>>>>> + * If you want support for arbitrary number of CPUs, make it one trigger,
>>>>> + * with additional sysfs file selecting which CPU to watch.
>>>>>      *
>>>>>      * It can be bound to any LED just like other triggers using either a
>>>>>      * board file or via sysfs interface.
>>>>>      *
>>>>>      * An API named ledtrig_cpu is exported for any user, who want to add CPU
>>>>> - * activity indication in their code
>>>>> + * activity indication in their code.
>>>>>      *
>>>>>      * Copyright 2011 Linus Walleij <linus.walleij@linaro.org>
>>>>>      * Copyright 2011 - 2012 Bryan Wu <bryan.wu@canonical.com>
>>>>> @@ -145,6 +149,9 @@ static int __init ledtrig_cpu_init(void)
>>>>>     	for_each_possible_cpu(cpu) {
>>>>>     		struct led_trigger_cpu *trig = &per_cpu(cpu_trig, cpu);
>>>>>     
>>>>> +		if (cpu > 4)
>>>>
>>>> NACK. The workaround for this trigger was implemented for a reason -
>>>> to make it working on platforms with arbitrary number of logical cpus.
>>>> I've got 8, so I am discriminated now. Not saying, that it precludes
>>>> trigger registration with no single line of warning.
>>>> Regardless of that - you have no guarantee that you're not breaking
>>>> anyone - "I doubt" is not a sufficient argument.
>>>>   
>>>
>>> If that is the case Jacek, I would try 16 and then see if people
>>> complain. Do you really think that someone sets a specific LED to
>>> trigger on activity on CPU id > 16?
>>
>> I have an access to the machine with 80 cpus, so I could once
>> get surprised not being able to find cpuN triggers not being
>> listed among available triggers.
>>
>> And say that I have a solution where I install 80 userspace LEDs
>> (drivers/leds/uleds.c) and register them on each cpuN triggers to get
>> notifications on how cpus work.
> 
> Hi Jacek,
> 
> I understand (and Pavel does for sure too) that many people
> currently have that possibility, that they have access to machines with
> many CPUs and many LEDs. We also understand that currently it is
> possible for users to set 1847th LED to trigger on activity on CPU ID
> 1337. What we are suggesting is that practically no one uses this, and
> for those 10 people who do, well it would be better for them to migrate
> to new ABI than for kernel developers having forever maintain this
> legacy ABI.
> 
> Legacy drivers get removed from kernel from time to time, if no one
> uses them. So I think Pavel's proposal (although I may not agree with
> the limit 4) has some merit. If we try this, and someone complains, we
> can then discuss. If we don't try, we may never know.

Just go ahead without my ack. I just wanted not to let it go without
any discussion. At least we leave a trace...

>>> If you do not agree, then I think we should implement a "cpu" trigger
>>> where the cpu ID (or maybe mask of multiple CPUs) is configurable via
>>> another sysfs file. And then declare current cpu trigger (with names
>>> "cpu%d") as legacy.
>>
>> Yes, we can do that, and even mark the cpu trigger as legacy but we
>> cannot prevent people from using it if that was present in kernel
>> for many years.
>>
> 

-- 
Best regards,
Jacek Anaszewski

Re: ledtrig-cpu: Limit to 4 CPUs

[Pavel Machek]

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

Hi!

> >    *
> >    * It can be bound to any LED just like other triggers using either a
> >    * board file or via sysfs interface.
> >    *
> >    * An API named ledtrig_cpu is exported for any user, who want to add CPU
> > - * activity indication in their code
> > + * activity indication in their code.
> >    *
> >    * Copyright 2011 Linus Walleij <linus.walleij@linaro.org>
> >    * Copyright 2011 - 2012 Bryan Wu <bryan.wu@canonical.com>
> > @@ -145,6 +149,9 @@ static int __init ledtrig_cpu_init(void)
> >   	for_each_possible_cpu(cpu) {
> >   		struct led_trigger_cpu *trig = &per_cpu(cpu_trig, cpu);
> > +		if (cpu > 4)
> 
> NACK. The workaround for this trigger was implemented for a reason -
> to make it working on platforms with arbitrary number of logical cpus.
> I've got 8, so I am discriminated now. Not saying, that it precludes
> trigger registration with no single line of warning.

Can I get details of your setup?

What CPU type that is, and how are you mapping CPU activity to LEDs?

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

Re: ledtrig-cpu: Limit to 4 CPUs

[Jacek Anaszewski]

On 9/20/20 8:34 PM, Pavel Machek wrote:
> Hi!
> 
>>>     *
>>>     * It can be bound to any LED just like other triggers using either a
>>>     * board file or via sysfs interface.
>>>     *
>>>     * An API named ledtrig_cpu is exported for any user, who want to add CPU
>>> - * activity indication in their code
>>> + * activity indication in their code.
>>>     *
>>>     * Copyright 2011 Linus Walleij <linus.walleij@linaro.org>
>>>     * Copyright 2011 - 2012 Bryan Wu <bryan.wu@canonical.com>
>>> @@ -145,6 +149,9 @@ static int __init ledtrig_cpu_init(void)
>>>    	for_each_possible_cpu(cpu) {
>>>    		struct led_trigger_cpu *trig = &per_cpu(cpu_trig, cpu);
>>> +		if (cpu > 4)
>>
>> NACK. The workaround for this trigger was implemented for a reason -
>> to make it working on platforms with arbitrary number of logical cpus.
>> I've got 8, so I am discriminated now. Not saying, that it precludes
>> trigger registration with no single line of warning.
> 
> Can I get details of your setup?

I don't use this trigger, but I can imagine that someone does.

> What CPU type that is, and how are you mapping CPU activity to LEDs?

The type of CPU is irrelevant here. What is important is the fact
that with this trigger it is possible to visually monitor CPU core
online state. Probably it would be good to ask the author of that
trigger about his use case.

We've had also another patch in 2017 adding "cpu" trigger to
ledtrig-cpu.c that expressed number of online cores in a function of
brightness.

The commit message said:

"This is particularly useful on tiny linux boards with more CPU cores
than LED pins."

So apparently there are still users thereof.

As I've checked it now it has a bug, as it assumes max brightness to be
always LED_FULL, so that will need a fix.

I have spoken up, because I don't get the reason for your patch.
This driver was reworked year ago to remove PAGE_SIZE limit,
and I even applied it to my for-next tree, but that was at
the time of handling maintainership to yourself, and you
seem to not have picked that commit.

Was that intentional? We had even Greg's ack [0].

[0] https://lkml.org/lkml/2019/9/30/397

-- 
Best regards,
Jacek Anaszewski

Re: ledtrig-cpu: Limit to 4 CPUs

[Pavel Machek]

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

Hi!

> >Can I get details of your setup?
> 
> I don't use this trigger, but I can imagine that someone does.

Well, if someone exists, we can increase the limit, or convince them
to change their setup.

> >What CPU type that is, and how are you mapping CPU activity to LEDs?
> 
> The type of CPU is irrelevant here. What is important is the fact
> that with this trigger it is possible to visually monitor CPU core
> online state. Probably it would be good to ask the author of that
> trigger about his use case.

It is relevant -- cpu trigger never worked on x86. I had patch fixing
it, but got pushback.

> I have spoken up, because I don't get the reason for your patch.
> This driver was reworked year ago to remove PAGE_SIZE limit,
> and I even applied it to my for-next tree, but that was at
> the time of handling maintainership to yourself, and you
> seem to not have picked that commit.
> 
> Was that intentional? We had even Greg's ack [0].

I checked, and I believe the commit is in:

#ifdef CONFIG_LEDS_TRIGGERS
static BIN_ATTR(trigger, 0644, led_trigger_read, led_trigger_write,
0);
static struct bin_attribute *led_trigger_bin_attrs[] = {

So.. no, it is not causing kernel crashes or something. But it is
example of bad interface, and _that_ is causing problems. (And yes, if
I realized it is simply possible to limit it, maybe the BIN_ATTR
conversion would not be neccessary...)

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: ledtrig-cpu: Limit to 4 CPUs

[Jacek Anaszewski]

Hi Pavel,

On 9/22/20 12:42 AM, Pavel Machek wrote:
> Hi!
> 
>>> Can I get details of your setup?
>>
>> I don't use this trigger, but I can imagine that someone does.
> 
> Well, if someone exists, we can increase the limit, or convince them
> to change their setup.

Linux is used in many commercial projects and each such change generates
a cost, so this is not a sheer matter of convincing someone.

>>> What CPU type that is, and how are you mapping CPU activity to LEDs?
>>
>> The type of CPU is irrelevant here. What is important is the fact
>> that with this trigger it is possible to visually monitor CPU core
>> online state. Probably it would be good to ask the author of that
>> trigger about his use case.
> 
> It is relevant -- cpu trigger never worked on x86. I had patch fixing
> it, but got pushback.

You mean literally x86 (32-bit)? Because I checked yesterday on my
x86_64 and it worked just fine, i.e. changing cpu online state
generated events on all userspace LEDs I registered for each cpuN
trigger.

>> I have spoken up, because I don't get the reason for your patch.
>> This driver was reworked year ago to remove PAGE_SIZE limit,
>> and I even applied it to my for-next tree, but that was at
>> the time of handling maintainership to yourself, and you
>> seem to not have picked that commit.
>>
>> Was that intentional? We had even Greg's ack [0].
> 
> I checked, and I believe the commit is in:

Indeed, I blindly sought the changeset in git log for ledtrig-cpu.c
file.

> #ifdef CONFIG_LEDS_TRIGGERS
> static BIN_ATTR(trigger, 0644, led_trigger_read, led_trigger_write,
> 0);
> static struct bin_attribute *led_trigger_bin_attrs[] = {
> 
> So.. no, it is not causing kernel crashes or something. But it is
> example of bad interface, and _that_ is causing problems. (And yes, if
> I realized it is simply possible to limit it, maybe the BIN_ATTR
> conversion would not be neccessary...)

The limitation you proposed breaks the trigger on many plafforms.

-- 
Best regards,
Jacek Anaszewski

Re: ledtrig-cpu: Limit to 4 CPUs

[Jacek Anaszewski]

On 9/22/20 10:41 PM, Jacek Anaszewski wrote:
> Hi Pavel,
> 
> On 9/22/20 12:42 AM, Pavel Machek wrote:
>> Hi!
>>
>>>> Can I get details of your setup?
>>>
>>> I don't use this trigger, but I can imagine that someone does.
>>
>> Well, if someone exists, we can increase the limit, or convince them
>> to change their setup.
> 
> Linux is used in many commercial projects and each such change generates
> a cost, so this is not a sheer matter of convincing someone.
> 
>>>> What CPU type that is, and how are you mapping CPU activity to LEDs?
>>>
>>> The type of CPU is irrelevant here. What is important is the fact
>>> that with this trigger it is possible to visually monitor CPU core
>>> online state. Probably it would be good to ask the author of that
>>> trigger about his use case.
>>
>> It is relevant -- cpu trigger never worked on x86. I had patch fixing
>> it, but got pushback.
> 
> You mean literally x86 (32-bit)? Because I checked yesterday on my
> x86_64 and it worked just fine, i.e. changing cpu online state
> generated events on all userspace LEDs I registered for each cpuN
> trigger.
> 
>>> I have spoken up, because I don't get the reason for your patch.
>>> This driver was reworked year ago to remove PAGE_SIZE limit,
>>> and I even applied it to my for-next tree, but that was at
>>> the time of handling maintainership to yourself, and you
>>> seem to not have picked that commit.
>>>
>>> Was that intentional? We had even Greg's ack [0].
>>
>> I checked, and I believe the commit is in:
> 
> Indeed, I blindly sought the changeset in git log for ledtrig-cpu.c
> file.
> 
>> #ifdef CONFIG_LEDS_TRIGGERS
>> static BIN_ATTR(trigger, 0644, led_trigger_read, led_trigger_write,
>> 0);
>> static struct bin_attribute *led_trigger_bin_attrs[] = {
>>
>> So.. no, it is not causing kernel crashes or something. But it is
>> example of bad interface, and _that_ is causing problems. (And yes, if
>> I realized it is simply possible to limit it, maybe the BIN_ATTR
>> conversion would not be neccessary...)
> 
> The limitation you proposed breaks the trigger on many plafforms.

Actually it precludes its use.

I still see the patch in your linux-next, so I reserve myself the
right to comment on your pull request.

-- 
Best regards,
Jacek Anaszewski

Re: ledtrig-cpu: Limit to 4 CPUs

[Pavel Machek]

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

Hi!

> >>So.. no, it is not causing kernel crashes or something. But it is
> >>example of bad interface, and _that_ is causing problems. (And yes, if
> >>I realized it is simply possible to limit it, maybe the BIN_ATTR
> >>conversion would not be neccessary...)
> >
> >The limitation you proposed breaks the trigger on many plafforms.
> 
> Actually it precludes its use.
> 
> I still see the patch in your linux-next, so I reserve myself the
> right to comment on your pull request.

You are free to comment on anything.

I believe probability someone uses that with more than 4 CPUs is <
5%. Probability that someone uses it with more than 100 CPUs is << 1%
I'd say. Systems just don't have that many LEDs. I'll take the risk.

If I broke someone's real, existing setup, I'll raise the limit.

(With exception of uled setups. In such cases, I'll just laugh.)

If you know or can find out someone using it with more than 4 CPUs, I
can obviously raise the limit before the merge.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: ledtrig-cpu: Limit to 4 CPUs

[Jacek Anaszewski]

On 9/25/20 11:40 AM, Pavel Machek wrote:
> Hi!
> 
>>>> So.. no, it is not causing kernel crashes or something. But it is
>>>> example of bad interface, and _that_ is causing problems. (And yes, if
>>>> I realized it is simply possible to limit it, maybe the BIN_ATTR
>>>> conversion would not be neccessary...)
>>>
>>> The limitation you proposed breaks the trigger on many plafforms.
>>
>> Actually it precludes its use.
>>
>> I still see the patch in your linux-next, so I reserve myself the
>> right to comment on your pull request.
> 
> You are free to comment on anything.
> 
> I believe probability someone uses that with more than 4 CPUs is <
> 5%. 

So you even didn't bother to check:

$ git grep "default-trigger = \"cpu[4-9]"
arch/arm/boot/dts/vexpress-v2m-rs1.dtsi: 
linux,default-trigger = "cpu4";
arch/arm/boot/dts/vexpress-v2m-rs1.dtsi: 
linux,default-trigger = "cpu5";
arch/arm/boot/dts/vexpress-v2m.dtsi: 
linux,default-trigger = "cpu4";
arch/arm/boot/dts/vexpress-v2m.dtsi: 
linux,default-trigger = "cpu5";

cpus are enumerated starting from 0, so there are more reasons for which
your patch is broken:

1. There are mainline users.
2. You claim that you limit trigger use to 4 cpus, while the number is
    actually 5, due to your condition:
	+		if (cpu > 4)
	+			continue;
3. For platforms exceeding the limit the number of triggers registered
    would not match the number all available cpus, for no obvious reason.
    Better solution would be to prevent use of the trigger entirely
    in such cases, which would need only to alter first instruction in
    ledtrig_cpu_init(), which currently is:

	BUILD_BUG_ON(CONFIG_NR_CPUS > 9999);

However I am not in favor of this approach since after removing
PAGE_LIMIT size on triggers file, the trigger doesn't longer cause
problems even with hypothetical 1000 cpus.

The correct approach would be to create new trigger with better
interface and then advise people switching to it.

> Probability that someone uses it with more than 100 CPUs is << 1%
> I'd say. Systems just don't have that many LEDs. I'll take the risk.
> 
> If I broke someone's real, existing setup, I'll raise the limit.

Is this professional approach - throw a potential bug at users and
check if it will hit them? :-) And for no reason - you're not fixing
anything.

> (With exception of uled setups. In such cases, I'll just laugh.)
> 
> If you know or can find out someone using it with more than 4 CPUs, I
> can obviously raise the limit before the merge.

-- 
Best regards,
Jacek Anaszewski

Re: ledtrig-cpu: Limit to 4 CPUs

[Pavel Machek]

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

Hi!

> > I believe probability someone uses that with more than 4 CPUs is <
> > 5%.
> 
> So you even didn't bother to check:
> 
> $ git grep "default-trigger = \"cpu[4-9]"
> arch/arm/boot/dts/vexpress-v2m-rs1.dtsi: linux,default-trigger = "cpu4";
> arch/arm/boot/dts/vexpress-v2m-rs1.dtsi: linux,default-trigger = "cpu5";
> arch/arm/boot/dts/vexpress-v2m.dtsi: linux,default-trigger = "cpu4";
> arch/arm/boot/dts/vexpress-v2m.dtsi: linux,default-trigger = "cpu5";
> 
> cpus are enumerated starting from 0, so there are more reasons for which
> your patch is broken:
> 
> 1. There are mainline users.
> 2. You claim that you limit trigger use to 4 cpus, while the number is
>    actually 5, due to your condition:
> 	+		if (cpu > 4)
> 	+			continue;

Ok, fixed.

> 3. For platforms exceeding the limit the number of triggers registered
>    would not match the number all available cpus, for no obvious reason.
>    Better solution would be to prevent use of the trigger entirely
>    in such cases, which would need only to alter first instruction in
>    ledtrig_cpu_init(), which currently is:
> 
> 	BUILD_BUG_ON(CONFIG_NR_CPUS > 9999);

Hmm. If I do that I'll get complains from various build bots...

But I might do dependency in Kconfig...

> The correct approach would be to create new trigger with better
> interface and then advise people switching to it.

Patch would be accepted.

> > Probability that someone uses it with more than 100 CPUs is << 1%
> > I'd say. Systems just don't have that many LEDs. I'll take the risk.
> > 
> > If I broke someone's real, existing setup, I'll raise the limit.
> 
> Is this professional approach - throw a potential bug at users and
> check if it will hit them? :-) And for no reason - you're not fixing
> anything.

I'm sorry I failed to meet your expectations.

I raised limit to 8.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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