Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1600139 > unrolled thread

Re: [PATCH v2 1/6] powerpc/perf: Define big-endian version of perf_mem_data_src

Started byMadhavan Srinivasan <maddy@linux.vnet.ibm.com>
First post2017-03-14 10:10 +0100
Last post2017-03-16 06:50 +0100
Articles 6 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 1/6] powerpc/perf: Define big-endian version of  perf_mem_data_src Madhavan Srinivasan <maddy@linux.vnet.ibm.com> - 2017-03-14 10:10 +0100
    Re: [PATCH v2 1/6] powerpc/perf: Define big-endian version of  perf_mem_data_src Peter Zijlstra <peterz@infradead.org> - 2017-03-14 14:00 +0100
      Re: [PATCH v2 1/6] powerpc/perf: Define big-endian version of perf_mem_data_src Michael Ellerman <mpe@ellerman.id.au> - 2017-03-15 07:30 +0100
        Re: [PATCH v2 1/6] powerpc/perf: Define big-endian version of  perf_mem_data_src Peter Zijlstra <peterz@infradead.org> - 2017-03-15 13:30 +0100
          Re: [PATCH v2 1/6] powerpc/perf: Define big-endian version of  perf_mem_data_src Madhavan Srinivasan <maddy@linux.vnet.ibm.com> - 2017-03-16 07:00 +0100
        Re: [PATCH v2 1/6] powerpc/perf: Define big-endian version of  perf_mem_data_src Madhavan Srinivasan <maddy@linux.vnet.ibm.com> - 2017-03-16 06:50 +0100

#1600139 — Re: [PATCH v2 1/6] powerpc/perf: Define big-endian version of perf_mem_data_src

FromMadhavan Srinivasan <maddy@linux.vnet.ibm.com>
Date2017-03-14 10:10 +0100
SubjectRe: [PATCH v2 1/6] powerpc/perf: Define big-endian version of perf_mem_data_src
Message-ID<tkQCS-6kd-21@gated-at.bofh.it>

On Monday 13 March 2017 06:20 PM, Peter Zijlstra wrote:
> On Mon, Mar 13, 2017 at 04:45:51PM +0530, Madhavan Srinivasan wrote:
>>>   - should you not have fixed this in the tool only? This patch
>>>     effectively breaks ABI on big-endian architectures.
>> IIUC, we are the first BE user for this feature
>> (Kindly correct me if I am wrong), so technically we
>> are not breaking ABI here :) .  But let me also look  at
>> the dynamic conversion part.
> Huh? PPC hasn't yet implemented this? Then why are you fixing it?

yes, PPC hasn't implemented this (until now).
And did not understand "Then why are you fixing it?"

Maddy
>

[toc] | [next] | [standalone]

#1600308

FromPeter Zijlstra <peterz@infradead.org>
Date2017-03-14 14:00 +0100
Message-ID<tkUdt-cW-55@gated-at.bofh.it>
In reply to#1600139
On Tue, Mar 14, 2017 at 02:31:51PM +0530, Madhavan Srinivasan wrote:

> >Huh? PPC hasn't yet implemented this? Then why are you fixing it?
> 
> yes, PPC hasn't implemented this (until now).

until now where?

> And did not understand "Then why are you fixing it?"

I see no implementation; so why are you poking at it.

[toc] | [prev] | [next] | [standalone]

#1601089 — Re: [PATCH v2 1/6] powerpc/perf: Define big-endian version of perf_mem_data_src

FromMichael Ellerman <mpe@ellerman.id.au>
Date2017-03-15 07:30 +0100
SubjectRe: [PATCH v2 1/6] powerpc/perf: Define big-endian version of perf_mem_data_src
Message-ID<tlaBz-3D3-5@gated-at.bofh.it>
In reply to#1600308
Hi Peter,

Peter Zijlstra <peterz@infradead.org> writes:
> On Tue, Mar 14, 2017 at 02:31:51PM +0530, Madhavan Srinivasan wrote:
>
>> >Huh? PPC hasn't yet implemented this? Then why are you fixing it?
>> 
>> yes, PPC hasn't implemented this (until now).
>
> until now where?

On powerpc there is currently no kernel support for filling the data_src
value with anything meaningful.

A user can still request PERF_SAMPLE_DATA_SRC (perf report -d), but they
just get the default value from perf_sample_data_init(), which is
PERF_MEM_NA.

Though even that is currently broken with a big endian perf tool.

>> And did not understand "Then why are you fixing it?"
>
> I see no implementation; so why are you poking at it.

Maddy has posted an implementation of the kernel part for powerpc in
patch 2 of this series, but maybe you're not on Cc?


Regardless of us wanting to do the kernel side on powerpc, the current
API is broken on big endian.

That's because in the kernel the PERF_MEM_NA value is constructed using
shifts:

  /* TLB access */
  #define PERF_MEM_TLB_NA		0x01 /* not available */
  ...
  #define PERF_MEM_TLB_SHIFT	26
  
  #define PERF_MEM_S(a, s) \
  	(((__u64)PERF_MEM_##a##_##s) << PERF_MEM_##a##_SHIFT)
  
  #define PERF_MEM_NA (PERF_MEM_S(OP, NA)   |\
  		    PERF_MEM_S(LVL, NA)   |\
  		    PERF_MEM_S(SNOOP, NA) |\
  		    PERF_MEM_S(LOCK, NA)  |\
  		    PERF_MEM_S(TLB, NA))

Which works out as:

  ((0x01 << 0) | (0x01 << 5) | (0x01 << 19) | (0x01 << 24) | (0x01 << 26))


Which means the PERF_MEM_NA value comes out of the kernel as 0x5080021
in CPU endian.

But then in the perf tool, the code uses the bitfields to inspect the
value, and currently the bitfields are defined using little endian
ordering.

So eg. in perf_mem__tlb_scnprintf() we see:
  data_src->val = 0x5080021
             op = 0x0
            lvl = 0x0
          snoop = 0x0
           lock = 0x0
           dtlb = 0x0
           rsvd = 0x5080021


So this patch does what I think is the minimal fix, of changing the
definition of the bitfields to match the values that are already
exported by the kernel on big endian. And it makes no change on little
endian.

cheers

[toc] | [prev] | [next] | [standalone]

#1601360

FromPeter Zijlstra <peterz@infradead.org>
Date2017-03-15 13:30 +0100
Message-ID<tlgdY-7yR-15@gated-at.bofh.it>
In reply to#1601089
On Wed, Mar 15, 2017 at 05:20:15PM +1100, Michael Ellerman wrote:

> > I see no implementation; so why are you poking at it.
> 
> Maddy has posted an implementation of the kernel part for powerpc in
> patch 2 of this series, but maybe you're not on Cc?

I am not indeed. That and a completely inadequate Changelog have lead to
great confusion.

[toc] | [prev] | [next] | [standalone]

#1601964

FromMadhavan Srinivasan <maddy@linux.vnet.ibm.com>
Date2017-03-16 07:00 +0100
Message-ID<tlwC5-2c1-13@gated-at.bofh.it>
In reply to#1601360

On Wednesday 15 March 2017 05:53 PM, Peter Zijlstra wrote:
> On Wed, Mar 15, 2017 at 05:20:15PM +1100, Michael Ellerman wrote:
>
>>> I see no implementation; so why are you poking at it.
>> Maddy has posted an implementation of the kernel part for powerpc in
>> patch 2 of this series, but maybe you're not on Cc?
> I am not indeed. That and a completely inadequate Changelog have lead to
> great confusion.

Yes. my bad. I will send out a v3 today and will CC. Also will add
ellerman's explanation to the commit message.

Sorry for the confusion.

Maddy

[toc] | [prev] | [next] | [standalone]

#1601957

FromMadhavan Srinivasan <maddy@linux.vnet.ibm.com>
Date2017-03-16 06:50 +0100
Message-ID<tlwsp-27G-5@gated-at.bofh.it>
In reply to#1601089

On Wednesday 15 March 2017 11:50 AM, Michael Ellerman wrote:
> Hi Peter,
>
> Peter Zijlstra <peterz@infradead.org> writes:
>> On Tue, Mar 14, 2017 at 02:31:51PM +0530, Madhavan Srinivasan wrote:
>>
>>>> Huh? PPC hasn't yet implemented this? Then why are you fixing it?
>>> yes, PPC hasn't implemented this (until now).
>> until now where?
> On powerpc there is currently no kernel support for filling the data_src
> value with anything meaningful.
>
> A user can still request PERF_SAMPLE_DATA_SRC (perf report -d), but they
> just get the default value from perf_sample_data_init(), which is
> PERF_MEM_NA.
>
> Though even that is currently broken with a big endian perf tool.
>
>>> And did not understand "Then why are you fixing it?"
>> I see no implementation; so why are you poking at it.
> Maddy has posted an implementation of the kernel part for powerpc in
> patch 2 of this series, but maybe you're not on Cc?

Sorry, was out yesterday.

Yes my bad. I CCed lkml and ppcdev and took the emails
from get_maintainer script and added to each file.

I will send out a v3 with peterz and others in all patch.

>
> Regardless of us wanting to do the kernel side on powerpc, the current
> API is broken on big endian.
>
> That's because in the kernel the PERF_MEM_NA value is constructed using
> shifts:
>
>    /* TLB access */
>    #define PERF_MEM_TLB_NA		0x01 /* not available */
>    ...
>    #define PERF_MEM_TLB_SHIFT	26
>    
>    #define PERF_MEM_S(a, s) \
>    	(((__u64)PERF_MEM_##a##_##s) << PERF_MEM_##a##_SHIFT)
>    
>    #define PERF_MEM_NA (PERF_MEM_S(OP, NA)   |\
>    		    PERF_MEM_S(LVL, NA)   |\
>    		    PERF_MEM_S(SNOOP, NA) |\
>    		    PERF_MEM_S(LOCK, NA)  |\
>    		    PERF_MEM_S(TLB, NA))
>
> Which works out as:
>
>    ((0x01 << 0) | (0x01 << 5) | (0x01 << 19) | (0x01 << 24) | (0x01 << 26))
>
>
> Which means the PERF_MEM_NA value comes out of the kernel as 0x5080021
> in CPU endian.
>
> But then in the perf tool, the code uses the bitfields to inspect the
> value, and currently the bitfields are defined using little endian
> ordering.
>
> So eg. in perf_mem__tlb_scnprintf() we see:
>    data_src->val = 0x5080021
>               op = 0x0
>              lvl = 0x0
>            snoop = 0x0
>             lock = 0x0
>             dtlb = 0x0
>             rsvd = 0x5080021
>
>
> So this patch does what I think is the minimal fix, of changing the
> definition of the bitfields to match the values that are already
> exported by the kernel on big endian. And it makes no change on little
> endian.

Thanks for the detailed explanation. I will add this to the patch
commit message in the v3.

Maddy

>
> cheers
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web