| Message ID | 20221202094532.2925-3-quic_aiquny@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | remoteproc: core: do pm relax when in | expand |
On Fri, Dec 02, 2022 at 05:45:32PM +0800, Maria Yu wrote: > Only the first detected crash needed to be handled, so change > to ordered workqueue to avoid unnecessary multi active work at > the same time. This will reduce the pm_relax unnecessary concurrency. > > Signed-off-by: Maria Yu <quic_aiquny@quicinc.com> > --- > drivers/remoteproc/remoteproc_core.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index c2d0af048c69..4b973eea10bb 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -2728,8 +2728,8 @@ static void __exit rproc_exit_panic(void) > > static int __init remoteproc_init(void) > { > - rproc_recovery_wq = alloc_workqueue("rproc_recovery_wq", > - WQ_UNBOUND | WQ_FREEZABLE, 0); > + rproc_recovery_wq = alloc_ordered_workqueue("rproc_recovery_wq", > + WQ_FREEZABLE, 0); There is an indentation issue with the second line and this patch doesn't compile: CC drivers/remoteproc/imx_dsp_rproc.o AR drivers/hwspinlock/built-in.a In file included from /home/mpoirier/work/remoteproc/kernel-review/include/linux/rhashtable-types.h:15, from /home/mpoirier/work/remoteproc/kernel-review/include/linux/ipc.h:7, from /home/mpoirier/work/remoteproc/kernel-review/include/uapi/linux/sem.h:5, from /home/mpoirier/work/remoteproc/kernel-review/include/linux/sem.h:5, from /home/mpoirier/work/remoteproc/kernel-review/include/linux/sched.h:15, from /home/mpoirier/work/remoteproc/kernel-review/include/linux/delay.h:23, from /home/mpoirier/work/remoteproc/kernel-review/drivers/remoteproc/remoteproc_core.c:19: /home/mpoirier/work/remoteproc/kernel-review/drivers/remoteproc/remoteproc_core.c: In function ‘remoteproc_init’: /home/mpoirier/work/remoteproc/kernel-review/drivers/remoteproc/remoteproc_core.c:2738:46: warning: too many arguments for format [-Wformat-extra-args] 2738 | rproc_recovery_wq = alloc_ordered_workqueue("rproc_recovery_wq", | ^~~~~~~~~~~~~~~~~~~ /home/mpoirier/work/remoteproc/kernel-review/include/linux/workqueue.h:419:18: note: in definition of macro ‘alloc_ordered_workqueue’ 419 | alloc_workqueue(fmt, WQ_UNBOUND | __WQ_ORDERED | \ | ^~~ Last but not least, please use the get_maintainer.pl script to make sure the right people are CC'ed on your patchsets. Thanks, Mathieu > if (!rproc_recovery_wq) { > pr_err("remoteproc: creation of rproc_recovery_wq failed\n"); > return -ENOMEM; > -- > 2.17.1 >
On Fri, Dec 02, 2022 at 05:45:32PM +0800, Maria Yu wrote: > Only the first detected crash needed to be handled, so change > to ordered workqueue to avoid unnecessary multi active work at > the same time. In cab8300b5621 ("remoteproc: Use unbounded workqueue for recovery work") Mukesh specifically said that it was required that multiple remoteproc instances should be allowed to recover concurrently. Is this no longer the case? Or am I perhaps misunderstanding the nuances of the different work queue models? > This will reduce the pm_relax unnecessary concurrency. I'm not sure I understand this sentence, unless I remove the word "pm_relax", was it added by mistake? If so, is the support for concurrent recovery really unnecessary? I know we have cases where we spend time in the recovery process just waiting for things to happen, so allowing recovery to run concurrent between instances sounds like a good idea. Regards, Bjorn > > Signed-off-by: Maria Yu <quic_aiquny@quicinc.com> > --- > drivers/remoteproc/remoteproc_core.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index c2d0af048c69..4b973eea10bb 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -2728,8 +2728,8 @@ static void __exit rproc_exit_panic(void) > > static int __init remoteproc_init(void) > { > - rproc_recovery_wq = alloc_workqueue("rproc_recovery_wq", > - WQ_UNBOUND | WQ_FREEZABLE, 0); > + rproc_recovery_wq = alloc_ordered_workqueue("rproc_recovery_wq", > + WQ_FREEZABLE, 0); > if (!rproc_recovery_wq) { > pr_err("remoteproc: creation of rproc_recovery_wq failed\n"); > return -ENOMEM; > -- > 2.17.1 >
On 12/3/2022 1:34 AM, Mathieu Poirier wrote: > On Fri, Dec 02, 2022 at 05:45:32PM +0800, Maria Yu wrote: >> Only the first detected crash needed to be handled, so change >> to ordered workqueue to avoid unnecessary multi active work at >> the same time. This will reduce the pm_relax unnecessary concurrency. >> >> Signed-off-by: Maria Yu <quic_aiquny@quicinc.com> >> --- >> drivers/remoteproc/remoteproc_core.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c >> index c2d0af048c69..4b973eea10bb 100644 >> --- a/drivers/remoteproc/remoteproc_core.c >> +++ b/drivers/remoteproc/remoteproc_core.c >> @@ -2728,8 +2728,8 @@ static void __exit rproc_exit_panic(void) >> >> static int __init remoteproc_init(void) >> { >> - rproc_recovery_wq = alloc_workqueue("rproc_recovery_wq", >> - WQ_UNBOUND | WQ_FREEZABLE, 0); >> + rproc_recovery_wq = alloc_ordered_workqueue("rproc_recovery_wq", >> + WQ_FREEZABLE, 0); > > There is an indentation issue with the second line and this patch doesn't > compile: > My Clang 14.0.7 didn't have such kind of compilation error. what's your CC version pls? Maybe I can have a try to reproduce. Anyway, I will double confirm if there is any difference with current patchset with my compile tested patchset as well. > CC drivers/remoteproc/imx_dsp_rproc.o > AR drivers/hwspinlock/built-in.a > In file included from /home/mpoirier/work/remoteproc/kernel-review/include/linux/rhashtable-types.h:15, > from /home/mpoirier/work/remoteproc/kernel-review/include/linux/ipc.h:7, > from /home/mpoirier/work/remoteproc/kernel-review/include/uapi/linux/sem.h:5, > from /home/mpoirier/work/remoteproc/kernel-review/include/linux/sem.h:5, > from /home/mpoirier/work/remoteproc/kernel-review/include/linux/sched.h:15, > from /home/mpoirier/work/remoteproc/kernel-review/include/linux/delay.h:23, > from /home/mpoirier/work/remoteproc/kernel-review/drivers/remoteproc/remoteproc_core.c:19: > /home/mpoirier/work/remoteproc/kernel-review/drivers/remoteproc/remoteproc_core.c: In function ‘remoteproc_init’: > /home/mpoirier/work/remoteproc/kernel-review/drivers/remoteproc/remoteproc_core.c:2738:46: warning: too many arguments for format [-Wformat-extra-args] > 2738 | rproc_recovery_wq = alloc_ordered_workqueue("rproc_recovery_wq", > | ^~~~~~~~~~~~~~~~~~~ > /home/mpoirier/work/remoteproc/kernel-review/include/linux/workqueue.h:419:18: note: in definition of macro ‘alloc_ordered_workqueue’ > 419 | alloc_workqueue(fmt, WQ_UNBOUND | __WQ_ORDERED | \ > | ^~~ > > Last but not least, please use the get_maintainer.pl script to make sure the > right people are CC'ed on your patchsets.get_maintainer.pl will be re-run for next patchset uploading. Thank you for reminder! > > Thanks, > Mathieu > >> if (!rproc_recovery_wq) { >> pr_err("remoteproc: creation of rproc_recovery_wq failed\n"); >> return -ENOMEM; >> -- >> 2.17.1 >>
On 12/3/2022 2:16 AM, Bjorn Andersson wrote: > On Fri, Dec 02, 2022 at 05:45:32PM +0800, Maria Yu wrote: >> Only the first detected crash needed to be handled, so change >> to ordered workqueue to avoid unnecessary multi active work at >> the same time. > > In cab8300b5621 ("remoteproc: Use unbounded workqueue for recovery > work") Mukesh specifically said that it was required that multiple > remoteproc instances should be allowed to recover concurrently. > > Is this no longer the case? Or am I perhaps misunderstanding the > nuances of the different work queue models? > >> This will reduce the pm_relax unnecessary concurrency. > > I'm not sure I understand this sentence, unless I remove the word > "pm_relax", was it added by mistake? > > > If so, is the support for concurrent recovery really unnecessary? > > I know we have cases where we spend time in the recovery process just > waiting for things to happen, so allowing recovery to run concurrent > between instances sounds like a good idea. > Agree with you. Allowing recovery to run concurrent for different subsystem still seems a good idea currently. Let's drop this change. > Regards, > Bjorn > >> >> Signed-off-by: Maria Yu <quic_aiquny@quicinc.com> >> --- >> drivers/remoteproc/remoteproc_core.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c >> index c2d0af048c69..4b973eea10bb 100644 >> --- a/drivers/remoteproc/remoteproc_core.c >> +++ b/drivers/remoteproc/remoteproc_core.c >> @@ -2728,8 +2728,8 @@ static void __exit rproc_exit_panic(void) >> >> static int __init remoteproc_init(void) >> { >> - rproc_recovery_wq = alloc_workqueue("rproc_recovery_wq", >> - WQ_UNBOUND | WQ_FREEZABLE, 0); >> + rproc_recovery_wq = alloc_ordered_workqueue("rproc_recovery_wq", >> + WQ_FREEZABLE, 0); >> if (!rproc_recovery_wq) { >> pr_err("remoteproc: creation of rproc_recovery_wq failed\n"); >> return -ENOMEM; >> -- >> 2.17.1 >>
On Tue, Dec 06, 2022 at 09:28:23AM +0800, Aiqun(Maria) Yu wrote: > On 12/3/2022 1:34 AM, Mathieu Poirier wrote: > > On Fri, Dec 02, 2022 at 05:45:32PM +0800, Maria Yu wrote: > > > Only the first detected crash needed to be handled, so change > > > to ordered workqueue to avoid unnecessary multi active work at > > > the same time. This will reduce the pm_relax unnecessary concurrency. > > > > > > Signed-off-by: Maria Yu <quic_aiquny@quicinc.com> > > > --- > > > drivers/remoteproc/remoteproc_core.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > > > index c2d0af048c69..4b973eea10bb 100644 > > > --- a/drivers/remoteproc/remoteproc_core.c > > > +++ b/drivers/remoteproc/remoteproc_core.c > > > @@ -2728,8 +2728,8 @@ static void __exit rproc_exit_panic(void) > > > static int __init remoteproc_init(void) > > > { > > > - rproc_recovery_wq = alloc_workqueue("rproc_recovery_wq", > > > - WQ_UNBOUND | WQ_FREEZABLE, 0); > > > + rproc_recovery_wq = alloc_ordered_workqueue("rproc_recovery_wq", > > > + WQ_FREEZABLE, 0); > > > > There is an indentation issue with the second line and this patch doesn't > > compile: > > > My Clang 14.0.7 didn't have such kind of compilation error. > what's your CC version pls? Maybe I can have a try to reproduce. I was either: arm-linux-gnueabihf-gcc 9.4.0 or aarch64-linux-gnu-gcc 9.4.0 I can't remember if I was compiling for 32 or 64 bit. > > Anyway, I will double confirm if there is any difference with current > patchset with my compile tested patchset as well. > > > CC drivers/remoteproc/imx_dsp_rproc.o > > AR drivers/hwspinlock/built-in.a > > In file included from /home/mpoirier/work/remoteproc/kernel-review/include/linux/rhashtable-types.h:15, > > from /home/mpoirier/work/remoteproc/kernel-review/include/linux/ipc.h:7, > > from /home/mpoirier/work/remoteproc/kernel-review/include/uapi/linux/sem.h:5, > > from /home/mpoirier/work/remoteproc/kernel-review/include/linux/sem.h:5, > > from /home/mpoirier/work/remoteproc/kernel-review/include/linux/sched.h:15, > > from /home/mpoirier/work/remoteproc/kernel-review/include/linux/delay.h:23, > > from /home/mpoirier/work/remoteproc/kernel-review/drivers/remoteproc/remoteproc_core.c:19: > > /home/mpoirier/work/remoteproc/kernel-review/drivers/remoteproc/remoteproc_core.c: In function ‘remoteproc_init’: > > /home/mpoirier/work/remoteproc/kernel-review/drivers/remoteproc/remoteproc_core.c:2738:46: warning: too many arguments for format [-Wformat-extra-args] > > 2738 | rproc_recovery_wq = alloc_ordered_workqueue("rproc_recovery_wq", > > | ^~~~~~~~~~~~~~~~~~~ > > /home/mpoirier/work/remoteproc/kernel-review/include/linux/workqueue.h:419:18: note: in definition of macro ‘alloc_ordered_workqueue’ > > 419 | alloc_workqueue(fmt, WQ_UNBOUND | __WQ_ORDERED | \ > > | ^~~ > > > > Last but not least, please use the get_maintainer.pl script to make sure the > > right people are CC'ed on your patchsets.get_maintainer.pl will be re-run for next patchset uploading. > Thank you for reminder! > > > > Thanks, > > Mathieu > > > > > if (!rproc_recovery_wq) { > > > pr_err("remoteproc: creation of rproc_recovery_wq failed\n"); > > > return -ENOMEM; > > > -- > > > 2.17.1 > > > > > > -- > Thx and BRs, > Aiqun(Maria) Yu
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index c2d0af048c69..4b973eea10bb 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -2728,8 +2728,8 @@ static void __exit rproc_exit_panic(void) static int __init remoteproc_init(void) { - rproc_recovery_wq = alloc_workqueue("rproc_recovery_wq", - WQ_UNBOUND | WQ_FREEZABLE, 0); + rproc_recovery_wq = alloc_ordered_workqueue("rproc_recovery_wq", + WQ_FREEZABLE, 0); if (!rproc_recovery_wq) { pr_err("remoteproc: creation of rproc_recovery_wq failed\n"); return -ENOMEM;
Only the first detected crash needed to be handled, so change to ordered workqueue to avoid unnecessary multi active work at the same time. This will reduce the pm_relax unnecessary concurrency. Signed-off-by: Maria Yu <quic_aiquny@quicinc.com> --- drivers/remoteproc/remoteproc_core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)