| Message ID | 20200919093833.GA14326@duo.ucw.cz |
|---|---|
| State | New |
| Headers | show |
| Series | ledtrig-cpu: Limit to 4 CPUs | expand |
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); >
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
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.
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. >
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
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
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
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
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
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.
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
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.
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
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) + continue; + snprintf(trig->name, MAX_NAME_LEN, "cpu%d", cpu); led_trigger_register_simple(trig->name, &trig->_trig);
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>