linux-kernel - Re: [PATCH 1/2] ARM: dts: imx6q: add pmu interrupt-affinity

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <5f35af2bc4b5c97d69ff9ab1c71da1f7@agner.ch>
Date:   Mon, 21 Jan 2019 10:39:58 +0100
From:   Stefan Agner <stefan@...er.ch>
To:     Lucas Stach <l.stach@...gutronix.de>, will.deacon@....com,
        mark.rutland@....com
Cc:     shawnguo@...nel.org, s.hauer@...gutronix.de,
        max.krummenacher@...adex.com, marcel.ziswiler@...adex.com,
        linux-kernel@...r.kernel.org, linux-imx@....com,
        kernel@...gutronix.de, fabio.estevam@....com, dev@...henker.ch,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 1/2] ARM: dts: imx6q: add pmu interrupt-affinity

[adding Will/Mark]

On 18.01.2019 16:41, Stefan Agner wrote:
> On 18.01.2019 15:12, Lucas Stach wrote:
>> Am Freitag, den 18.01.2019, 14:59 +0100 schrieb Stefan Agner:
>>> Explicitly specify interrupt affinity to avoid HW perfevents
>>> need to guess. This avoids the following error upon boot:
>>>   hw perfevents: no interrupt-affinity property for /pmu, guessing.
>>>
>> But then it isn't correct either AFAICS. On i.MX6 all the PMU IRQs are
>> ORed together into a single SPI, instead of each core dealing with its
>> own PPI. So pretending that there are more IRQs with affinity to each
>> core isn't the right thing to do, no?
>>
> 
> Oh I see, we only have a single interrupt in the i.MX 6 case.
> 
> I agree, this patches are wrong.
> 
> Hm, but why does hw perf then think it needs to guess? Doesn't seem hard
> to guess right if there is only one choice...
> 
> We probably need to do something like this?
> 
> --- a/drivers/perf/arm_pmu_platform.c
> +++ b/drivers/perf/arm_pmu_platform.c
> @@ -122,7 +122,8 @@ static int pmu_parse_irqs(struct arm_pmu *pmu)
>                         return pmu_parse_percpu_irq(pmu, irq);
>         }
>  
> -       if (nr_cpu_ids != 1 && !pmu_has_irq_affinity(pdev->dev.of_node))
> {
> +       if ((nr_cpu_ids != 1 || num_irqs != 1) &&
> +           !pmu_has_irq_affinity(pdev->dev.of_node)) {
>                 pr_warn("no interrupt-affinity property for %pOF,
> guessing.\n",
>                         pdev->dev.of_node);
>         }

Yeah, I see, it was on Friday... if anything, it should be:

-       if (nr_cpu_ids != 1 && !pmu_has_irq_affinity(pdev->dev.of_node))
{
+       if (nr_cpu_ids != 1 && num_irqs != 1 &&
+           !pmu_has_irq_affinity(pdev->dev.of_node)) {

However, I realized that arm_pmu_platform actually currently only
assigns the one IRQ to the first CPU. This leads to perf not working if
a process is scheduled on another CPU then the first.

I could reproduce this. PID 763 is a long running task.

root@...ibri-imx6-05051054:~# taskset -p 0x1 763
pid 763's current affinity mask: 3
pid 763's new affinity mask: 1


root@...ibri-imx6-05051054:~# perf stat -e cpu-cycles -p 763
^C
 Performance counter stats for process id '763':

           7581021      cpu-cycles

       1.222248490 seconds time elapsed


root@...ibri-imx6-05051054:~# taskset -p 0x2 763
pid 763's current affinity mask: 1
pid 763's new affinity mask: 2


root@...ibri-imx6-05051054:~# perf stat -e cpu-cycles -p 763
^C
 Performance counter stats for process id '763':

     <not counted>      cpu-cycles                                      
             (0.00%)

       1.050253575 seconds time elapsed

I guess we need to tell the PMU driver that a single IRQ should be used
for all CPUs?

--
Stefan



> 
> 
>> Regards,
>> Lucas
>>
>>> Specifying all four CPUs shows no aversive effects on i.MX 6Dual
>>> SoCs.
>>>
>>> > Signed-off-by: Stefan Agner <stefan@...er.ch>
>>> ---
>>>  arch/arm/boot/dts/imx6q.dtsi | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
>>> index 8381d24eff7d..d2c1977c8b16 100644
>>> --- a/arch/arm/boot/dts/imx6q.dtsi
>>> +++ b/arch/arm/boot/dts/imx6q.dtsi
>>> @@ -537,6 +537,13 @@
>>> >  			<0x28 0x0000000c>; /* DCIC2_MUX_CTL */
>>>  };
>>>  
>>> +&pmu {
>>> > > +	interrupt-affinity = <&{/cpus/cpu@0}>,
>>> > > +			     <&{/cpus/cpu@1}>,
>>> > > +			     <&{/cpus/cpu@2}>,
>>> > > +			     <&{/cpus/cpu@3}>;
>>> +};
>>> +
>>>  &vpu {
>>> >  	compatible = "fsl,imx6q-vpu", "cnm,coda960";
>>>  };