* Re: [PATCH -fixes] drivers: perf: Fix smp_processor_id() use in preemptible code
2024-08-26 16:52 [PATCH -fixes] drivers: perf: Fix smp_processor_id() use in preemptible code Alexandre Ghiti
@ 2024-08-26 16:58 ` Anup Patel
2024-08-26 17:31 ` Nam Cao
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Anup Patel @ 2024-08-26 16:58 UTC (permalink / raw)
To: Alexandre Ghiti
Cc: Atish Patra, Will Deacon, Mark Rutland, Paul Walmsley,
Palmer Dabbelt, Albert Ou, linux-riscv, linux-arm-kernel,
linux-kernel, Nam Cao
On Mon, Aug 26, 2024 at 10:22 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
>
> As reported in [1], the use of smp_processor_id() in
> pmu_sbi_device_probe() must be protected by disabling the preemption, so
> simple use get_cpu()/put_cpu() instead.
>
> Reported-by: Nam Cao <namcao@linutronix.de>
> Closes: https://lore.kernel.org/linux-riscv/20240820074925.ReMKUPP3@linutronix.de/ [1]
> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
LGTM.
Reviewed-by: Anup Patel <anup@brainfault.org>
Regards,
Anup
> ---
> drivers/perf/riscv_pmu_sbi.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 31a17a56eb3b..25b1b699b3e2 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -1373,11 +1373,15 @@ static int pmu_sbi_device_probe(struct platform_device *pdev)
>
> /* SBI PMU Snapsphot is only available in SBI v2.0 */
> if (sbi_v2_available) {
> + int cpu;
> +
> ret = pmu_sbi_snapshot_alloc(pmu);
> if (ret)
> goto out_unregister;
>
> - ret = pmu_sbi_snapshot_setup(pmu, smp_processor_id());
> + cpu = get_cpu();
> +
> + ret = pmu_sbi_snapshot_setup(pmu, cpu);
> if (ret) {
> /* Snapshot is an optional feature. Continue if not available */
> pmu_sbi_snapshot_free(pmu);
> @@ -1391,6 +1395,7 @@ static int pmu_sbi_device_probe(struct platform_device *pdev)
> */
> static_branch_enable(&sbi_pmu_snapshot_available);
> }
> + put_cpu();
> }
>
> register_sysctl("kernel", sbi_pmu_sysctl_table);
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH -fixes] drivers: perf: Fix smp_processor_id() use in preemptible code
2024-08-26 16:52 [PATCH -fixes] drivers: perf: Fix smp_processor_id() use in preemptible code Alexandre Ghiti
2024-08-26 16:58 ` Anup Patel
@ 2024-08-26 17:31 ` Nam Cao
2024-08-26 17:37 ` Alexandre Ghiti
2024-08-26 19:35 ` Andrea Parri
2024-08-27 12:53 ` Will Deacon
3 siblings, 1 reply; 9+ messages in thread
From: Nam Cao @ 2024-08-26 17:31 UTC (permalink / raw)
To: Alexandre Ghiti
Cc: Atish Patra, Anup Patel, Will Deacon, Mark Rutland, Paul Walmsley,
Palmer Dabbelt, Albert Ou, linux-riscv, linux-arm-kernel,
linux-kernel
On Mon, Aug 26, 2024 at 06:52:10PM +0200, Alexandre Ghiti wrote:
> As reported in [1], the use of smp_processor_id() in
> pmu_sbi_device_probe() must be protected by disabling the preemption, so
> simple use get_cpu()/put_cpu() instead.
>
> Reported-by: Nam Cao <namcao@linutronix.de>
> Closes: https://lore.kernel.org/linux-riscv/20240820074925.ReMKUPP3@linutronix.de/ [1]
> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
Tested-by: Nam Cao <namcao@linutronix.de>
I think this also needs:
Fixes: a8625217a054 ("drivers/perf: riscv: Implement SBI PMU snapshot function")
Best regards,
Nam
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH -fixes] drivers: perf: Fix smp_processor_id() use in preemptible code
2024-08-26 17:31 ` Nam Cao
@ 2024-08-26 17:37 ` Alexandre Ghiti
0 siblings, 0 replies; 9+ messages in thread
From: Alexandre Ghiti @ 2024-08-26 17:37 UTC (permalink / raw)
To: Nam Cao, Alexandre Ghiti
Cc: Atish Patra, Anup Patel, Will Deacon, Mark Rutland, Paul Walmsley,
Palmer Dabbelt, Albert Ou, linux-riscv, linux-arm-kernel,
linux-kernel
On 26/08/2024 19:31, Nam Cao wrote:
> On Mon, Aug 26, 2024 at 06:52:10PM +0200, Alexandre Ghiti wrote:
>> As reported in [1], the use of smp_processor_id() in
>> pmu_sbi_device_probe() must be protected by disabling the preemption, so
>> simple use get_cpu()/put_cpu() instead.
>>
>> Reported-by: Nam Cao <namcao@linutronix.de>
>> Closes: https://lore.kernel.org/linux-riscv/20240820074925.ReMKUPP3@linutronix.de/ [1]
>> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> Tested-by: Nam Cao <namcao@linutronix.de>
>
> I think this also needs:
> Fixes: a8625217a054 ("drivers/perf: riscv: Implement SBI PMU snapshot function")
Indeed, thanks for the tag!
>
> Best regards,
> Nam
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH -fixes] drivers: perf: Fix smp_processor_id() use in preemptible code
2024-08-26 16:52 [PATCH -fixes] drivers: perf: Fix smp_processor_id() use in preemptible code Alexandre Ghiti
2024-08-26 16:58 ` Anup Patel
2024-08-26 17:31 ` Nam Cao
@ 2024-08-26 19:35 ` Andrea Parri
2024-08-27 6:06 ` Alexandre Ghiti
2024-08-27 12:53 ` Will Deacon
3 siblings, 1 reply; 9+ messages in thread
From: Andrea Parri @ 2024-08-26 19:35 UTC (permalink / raw)
To: Alexandre Ghiti
Cc: Atish Patra, Anup Patel, Will Deacon, Mark Rutland, Paul Walmsley,
Palmer Dabbelt, Albert Ou, linux-riscv, linux-arm-kernel,
linux-kernel, Nam Cao
On Mon, Aug 26, 2024 at 06:52:10PM +0200, Alexandre Ghiti wrote:
> As reported in [1], the use of smp_processor_id() in
> pmu_sbi_device_probe() must be protected by disabling the preemption, so
> simple use get_cpu()/put_cpu() instead.
simply
> Reported-by: Nam Cao <namcao@linutronix.de>
Reported-by: Andrea Parri <parri.andrea@gmail.com>
> Closes: https://lore.kernel.org/linux-riscv/20240820074925.ReMKUPP3@linutronix.de/ [1]
> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
Tested-by: Andrea Parri <parri.andrea@gmail.com>
Andrea
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH -fixes] drivers: perf: Fix smp_processor_id() use in preemptible code
2024-08-26 19:35 ` Andrea Parri
@ 2024-08-27 6:06 ` Alexandre Ghiti
0 siblings, 0 replies; 9+ messages in thread
From: Alexandre Ghiti @ 2024-08-27 6:06 UTC (permalink / raw)
To: Andrea Parri
Cc: Atish Patra, Anup Patel, Will Deacon, Mark Rutland, Paul Walmsley,
Palmer Dabbelt, Albert Ou, linux-riscv, linux-arm-kernel,
linux-kernel, Nam Cao
Hi Andrea,
On Mon, Aug 26, 2024 at 9:36 PM Andrea Parri <parri.andrea@gmail.com> wrote:
>
> On Mon, Aug 26, 2024 at 06:52:10PM +0200, Alexandre Ghiti wrote:
> > As reported in [1], the use of smp_processor_id() in
> > pmu_sbi_device_probe() must be protected by disabling the preemption, so
> > simple use get_cpu()/put_cpu() instead.
>
> simply
>
>
> > Reported-by: Nam Cao <namcao@linutronix.de>
>
> Reported-by: Andrea Parri <parri.andrea@gmail.com>
I added it at first but since there is no link to a bug report,
checkpatch complains so I removed it. But I have just checked the
documentation which states:
"The tag should be followed by a Closes: tag pointing to the report,
unless the report is not available on the web"
So I'm sorry about that, it won't happen again.
Thanks,
Alex
>
>
> > Closes: https://lore.kernel.org/linux-riscv/20240820074925.ReMKUPP3@linutronix.de/ [1]
> > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
>
> Tested-by: Andrea Parri <parri.andrea@gmail.com>
>
> Andrea
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH -fixes] drivers: perf: Fix smp_processor_id() use in preemptible code
2024-08-26 16:52 [PATCH -fixes] drivers: perf: Fix smp_processor_id() use in preemptible code Alexandre Ghiti
` (2 preceding siblings ...)
2024-08-26 19:35 ` Andrea Parri
@ 2024-08-27 12:53 ` Will Deacon
2024-08-28 12:36 ` Alexandre Ghiti
3 siblings, 1 reply; 9+ messages in thread
From: Will Deacon @ 2024-08-27 12:53 UTC (permalink / raw)
To: Alexandre Ghiti
Cc: Atish Patra, Anup Patel, Mark Rutland, Paul Walmsley,
Palmer Dabbelt, Albert Ou, linux-riscv, linux-arm-kernel,
linux-kernel, Nam Cao
On Mon, Aug 26, 2024 at 06:52:10PM +0200, Alexandre Ghiti wrote:
> As reported in [1], the use of smp_processor_id() in
> pmu_sbi_device_probe() must be protected by disabling the preemption, so
> simple use get_cpu()/put_cpu() instead.
>
> Reported-by: Nam Cao <namcao@linutronix.de>
> Closes: https://lore.kernel.org/linux-riscv/20240820074925.ReMKUPP3@linutronix.de/ [1]
> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> ---
> drivers/perf/riscv_pmu_sbi.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 31a17a56eb3b..25b1b699b3e2 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -1373,11 +1373,15 @@ static int pmu_sbi_device_probe(struct platform_device *pdev)
>
> /* SBI PMU Snapsphot is only available in SBI v2.0 */
> if (sbi_v2_available) {
> + int cpu;
> +
> ret = pmu_sbi_snapshot_alloc(pmu);
> if (ret)
> goto out_unregister;
>
> - ret = pmu_sbi_snapshot_setup(pmu, smp_processor_id());
> + cpu = get_cpu();
> +
> + ret = pmu_sbi_snapshot_setup(pmu, cpu);
> if (ret) {
> /* Snapshot is an optional feature. Continue if not available */
> pmu_sbi_snapshot_free(pmu);
> @@ -1391,6 +1395,7 @@ static int pmu_sbi_device_probe(struct platform_device *pdev)
> */
> static_branch_enable(&sbi_pmu_snapshot_available);
> }
> + put_cpu();
Are you sure it's safe to enable the static key with preemption disabled?
I thought that could block on a mutex.
Will
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH -fixes] drivers: perf: Fix smp_processor_id() use in preemptible code
2024-08-27 12:53 ` Will Deacon
@ 2024-08-28 12:36 ` Alexandre Ghiti
2024-08-28 17:13 ` Atish Patra
0 siblings, 1 reply; 9+ messages in thread
From: Alexandre Ghiti @ 2024-08-28 12:36 UTC (permalink / raw)
To: Will Deacon
Cc: Atish Patra, Anup Patel, Mark Rutland, Paul Walmsley,
Palmer Dabbelt, Albert Ou, linux-riscv, linux-arm-kernel,
linux-kernel, Nam Cao
Hi Will,
On Tue, Aug 27, 2024 at 2:53 PM Will Deacon <will@kernel.org> wrote:
>
> On Mon, Aug 26, 2024 at 06:52:10PM +0200, Alexandre Ghiti wrote:
> > As reported in [1], the use of smp_processor_id() in
> > pmu_sbi_device_probe() must be protected by disabling the preemption, so
> > simple use get_cpu()/put_cpu() instead.
> >
> > Reported-by: Nam Cao <namcao@linutronix.de>
> > Closes: https://lore.kernel.org/linux-riscv/20240820074925.ReMKUPP3@linutronix.de/ [1]
> > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > ---
> > drivers/perf/riscv_pmu_sbi.c | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> > index 31a17a56eb3b..25b1b699b3e2 100644
> > --- a/drivers/perf/riscv_pmu_sbi.c
> > +++ b/drivers/perf/riscv_pmu_sbi.c
> > @@ -1373,11 +1373,15 @@ static int pmu_sbi_device_probe(struct platform_device *pdev)
> >
> > /* SBI PMU Snapsphot is only available in SBI v2.0 */
> > if (sbi_v2_available) {
> > + int cpu;
> > +
> > ret = pmu_sbi_snapshot_alloc(pmu);
> > if (ret)
> > goto out_unregister;
> >
> > - ret = pmu_sbi_snapshot_setup(pmu, smp_processor_id());
> > + cpu = get_cpu();
> > +
> > + ret = pmu_sbi_snapshot_setup(pmu, cpu);
> > if (ret) {
> > /* Snapshot is an optional feature. Continue if not available */
> > pmu_sbi_snapshot_free(pmu);
> > @@ -1391,6 +1395,7 @@ static int pmu_sbi_device_probe(struct platform_device *pdev)
> > */
> > static_branch_enable(&sbi_pmu_snapshot_available);
> > }
> > + put_cpu();
>
> Are you sure it's safe to enable the static key with preemption disabled?
> I thought that could block on a mutex.
Yep, it seems you're right, thanks for jumping in.
I'm discussing with Atish how to fix that differently, I'll be back
with another version very soon.
Thanks again,
Alex
>
> Will
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH -fixes] drivers: perf: Fix smp_processor_id() use in preemptible code
2024-08-28 12:36 ` Alexandre Ghiti
@ 2024-08-28 17:13 ` Atish Patra
0 siblings, 0 replies; 9+ messages in thread
From: Atish Patra @ 2024-08-28 17:13 UTC (permalink / raw)
To: Alexandre Ghiti, Will Deacon
Cc: Atish Patra, Anup Patel, Mark Rutland, Paul Walmsley,
Palmer Dabbelt, Albert Ou, linux-riscv, linux-arm-kernel,
linux-kernel, Nam Cao
On 8/28/24 5:36 AM, Alexandre Ghiti wrote:
> Hi Will,
>
> On Tue, Aug 27, 2024 at 2:53 PM Will Deacon <will@kernel.org> wrote:
>>
>> On Mon, Aug 26, 2024 at 06:52:10PM +0200, Alexandre Ghiti wrote:
>>> As reported in [1], the use of smp_processor_id() in
>>> pmu_sbi_device_probe() must be protected by disabling the preemption, so
>>> simple use get_cpu()/put_cpu() instead.
>>>
>>> Reported-by: Nam Cao <namcao@linutronix.de>
>>> Closes: https://lore.kernel.org/linux-riscv/20240820074925.ReMKUPP3@linutronix.de/ [1]
>>> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
>>> ---
>>> drivers/perf/riscv_pmu_sbi.c | 7 ++++++-
>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
>>> index 31a17a56eb3b..25b1b699b3e2 100644
>>> --- a/drivers/perf/riscv_pmu_sbi.c
>>> +++ b/drivers/perf/riscv_pmu_sbi.c
>>> @@ -1373,11 +1373,15 @@ static int pmu_sbi_device_probe(struct platform_device *pdev)
>>>
>>> /* SBI PMU Snapsphot is only available in SBI v2.0 */
>>> if (sbi_v2_available) {
>>> + int cpu;
>>> +
>>> ret = pmu_sbi_snapshot_alloc(pmu);
>>> if (ret)
>>> goto out_unregister;
>>>
>>> - ret = pmu_sbi_snapshot_setup(pmu, smp_processor_id());
>>> + cpu = get_cpu();
>>> +
>>> + ret = pmu_sbi_snapshot_setup(pmu, cpu);
>>> if (ret) {
>>> /* Snapshot is an optional feature. Continue if not available */
>>> pmu_sbi_snapshot_free(pmu);
>>> @@ -1391,6 +1395,7 @@ static int pmu_sbi_device_probe(struct platform_device *pdev)
>>> */
>>> static_branch_enable(&sbi_pmu_snapshot_available);
>>> }
>>> + put_cpu();
>>
>> Are you sure it's safe to enable the static key with preemption disabled?
>> I thought that could block on a mutex.
>
Thanks Will for pointing that out.
> Yep, it seems you're right, thanks for jumping in.
>
> I'm discussing with Atish how to fix that differently, I'll be back
> with another version very soon.
>
Looking at the driver core framework code, I am wondering if a probe
function can be preempted to run on a different cpu. If it can only be
preempted by higher priority kernel threads or interrupts but is
guaranteed to run on the same cpu again, we can just use the
raw_smp_processor_id.
However, if there is no guarantee then we can just invoke
get_cpu/put_cpu around pmu_sbi_snapshot_setup.
> Thanks again,
>
> Alex
>
>>
>> Will
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 9+ messages in thread