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

Groups > linux.kernel > #1356419 > unrolled thread

Re: [PATCH V5 02/10] perf/amd/iommu: Consolidate and move perf_event_amd_iommu header

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 V5 02/10] perf/amd/iommu: Consolidate and move  perf_event_amd_iommu header Peter Zijlstra <peterz@infradead.org> - 2016-03-12 14:30 +0100
    Re: [PATCH V5 02/10] perf/amd/iommu: Consolidate and move  perf_event_amd_iommu header Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> - 2016-03-14 06:30 +0100
      Re: [PATCH V5 02/10] perf/amd/iommu: Consolidate and move  perf_event_amd_iommu header Peter Zijlstra <peterz@infradead.org> - 2016-03-14 11:00 +0100
        Re: [PATCH V5 02/10] perf/amd/iommu: Consolidate and move  perf_event_amd_iommu header Borislav Petkov <bp@alien8.de> - 2016-03-14 15:30 +0100
          Re: [PATCH V5 02/10] perf/amd/iommu: Consolidate and move  perf_event_amd_iommu header Peter Zijlstra <peterz@infradead.org> - 2016-03-14 17:40 +0100
            Re: [PATCH V5 02/10] perf/amd/iommu: Consolidate and move  perf_event_amd_iommu header Peter Zijlstra <peterz@infradead.org> - 2016-03-15 09:50 +0100
            Re: [PATCH V5 02/10] perf/amd/iommu: Consolidate and move  perf_event_amd_iommu header Borislav Petkov <bp@alien8.de> - 2016-03-15 11:50 +0100
              Re: [PATCH V5 02/10] perf/amd/iommu: Consolidate and move  perf_event_amd_iommu header Peter Zijlstra <peterz@infradead.org> - 2016-03-15 12:00 +0100

#1356419 — Re: [PATCH V5 02/10] perf/amd/iommu: Consolidate and move perf_event_amd_iommu header

On Tue, Feb 23, 2016 at 08:12:36AM -0600, Suravee Suthikulpanit wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> First, this patch move arch/x86/events/amd/iommu.h to
> arch/x86/include/asm/perf/amd/iommu.h so that we easily include
> it in both perf-amd-iommu and amd-iommu drivers.
> 
> Then, we consolidate declaration of AMD IOMMU performance counter
> APIs into one file.

These seem two independent thingies; should this therefore not be 2
patches?

> Reviewed-by: Joerg Roedel <jroedel@suse.de>
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> ---
>  arch/x86/events/amd/iommu.c           |  2 +-
>  arch/x86/events/amd/iommu.h           | 40 ---------------------------------
>  arch/x86/include/asm/perf/amd/iommu.h | 42 +++++++++++++++++++++++++++++++++++

That seems somewhat excessive. Not only do you create
arch/x86/include/asm/perf/ you then put another directory on top of
that.

[toc] | [next] | [standalone]

#1356947

Hi,

On 03/12/2016 08:22 PM, Peter Zijlstra wrote:
> On Tue, Feb 23, 2016 at 08:12:36AM -0600, Suravee Suthikulpanit wrote:
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> First, this patch move arch/x86/events/amd/iommu.h to
>> arch/x86/include/asm/perf/amd/iommu.h so that we easily include
>> it in both perf-amd-iommu and amd-iommu drivers.
>>
>> Then, we consolidate declaration of AMD IOMMU performance counter
>> APIs into one file.
>
> These seem two independent thingies; should this therefore not be 2
> patches?
>
>> Reviewed-by: Joerg Roedel <jroedel@suse.de>
>> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>> ---
>>   arch/x86/events/amd/iommu.c           |  2 +-
>>   arch/x86/events/amd/iommu.h           | 40 ---------------------------------
>>   arch/x86/include/asm/perf/amd/iommu.h | 42 +++++++++++++++++++++++++++++++++++
>
> That seems somewhat excessive. Not only do you create
> arch/x86/include/asm/perf/ you then put another directory on top of
> that.
>

The original header files (arch/x86/events/amd/iommu.h and 
drivers/iommu/amd_iommu_proto.h) has duplicate function declarations. 
So, with the new header file being in the 
arch/x86/include/asm/perf/amd/iommu.h, we can just have one function 
declaration.

So, you just want to separate the file moving part and the part that 
removes of the duplication?

Thanks,
Suravee

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

#1357109

On Mon, Mar 14, 2016 at 12:26:00PM +0700, Suravee Suthikulpanit wrote:
> Hi,
> 
> On 03/12/2016 08:22 PM, Peter Zijlstra wrote:
> >On Tue, Feb 23, 2016 at 08:12:36AM -0600, Suravee Suthikulpanit wrote:
> >>From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> >>
> >>First, this patch move arch/x86/events/amd/iommu.h to
> >>arch/x86/include/asm/perf/amd/iommu.h so that we easily include
> >>it in both perf-amd-iommu and amd-iommu drivers.
> >>
> >>Then, we consolidate declaration of AMD IOMMU performance counter
> >>APIs into one file.
> >
> >These seem two independent thingies; should this therefore not be 2
> >patches?
> >
> >>Reviewed-by: Joerg Roedel <jroedel@suse.de>
> >>Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> >>---
> >>  arch/x86/events/amd/iommu.c           |  2 +-
> >>  arch/x86/events/amd/iommu.h           | 40 ---------------------------------
> >>  arch/x86/include/asm/perf/amd/iommu.h | 42 +++++++++++++++++++++++++++++++++++
> >
> >That seems somewhat excessive. Not only do you create
> >arch/x86/include/asm/perf/ you then put another directory on top of
> >that.
> >
> 
> The original header files (arch/x86/events/amd/iommu.h and
> drivers/iommu/amd_iommu_proto.h) has duplicate function declarations. So,
> with the new header file being in the arch/x86/include/asm/perf/amd/iommu.h,
> we can just have one function declaration.
> 
> So, you just want to separate the file moving part and the part that removes
> of the duplication?

I'm fine with a new header, it just seems putting it in a two deep
direcotry hierarchy of its own that seems excessive.

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

#1357278

On Mon, Mar 14, 2016 at 08:37:02PM +0700, Suravee Suthikulpanit wrote:
> Basically, we are trying to match the current Perf hierarchy for AMD IOMMU
> (arch/x86/events/amd/iommu.c). I can put it into
> arch/x86/include/asm/perf_amd_iommu.h. What would you prefer?

Yeah, I was going to say the same thing - match the hierarchy so that
there are no confusions between paths. Makes sense to me.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

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

#1357403

On Mon, Mar 14, 2016 at 03:19:45PM +0100, Borislav Petkov wrote:
> On Mon, Mar 14, 2016 at 08:37:02PM +0700, Suravee Suthikulpanit wrote:
> > Basically, we are trying to match the current Perf hierarchy for AMD IOMMU
> > (arch/x86/events/amd/iommu.c). I can put it into
> > arch/x86/include/asm/perf_amd_iommu.h. What would you prefer?
> 
> Yeah, I was going to say the same thing - match the hierarchy so that
> there are no confusions between paths. Makes sense to me.

Well there's still the 'perf' vs' events' thing, but also what other
files did you want to put there?

For now I think I prefer a filename without extra directories; we can
always move files about if there's more use later.

Also, since its being used by both events/amd/iommu.c and
drivers/iommu/amd_iommu.c you can also chose a name in the latter
namespace.

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

#1357907

On Tue, Mar 15, 2016 at 07:39:31AM +0700, Suravee Suthikulpanit wrote:
> What if I just merge the newly introduced arch/x86/include/perf/amd/iommu.h
> into the include/linux/amd-iommu.h? I do not see the point of having to
> separate things out into two files.
> 

Works for me. Thanks!

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

#1357981

On Tue, Mar 15, 2016 at 07:39:31AM +0700, Suravee Suthikulpanit wrote:
> What if I just merge the newly introduced arch/x86/include/perf/amd/iommu.h
> into the include/linux/amd-iommu.h? I do not see the point of having to
> separate things out into two files.

Except that this header has x86-specific stuff and include/linux/ is
arch-agnostic.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

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

#1357985

On Tue, Mar 15, 2016 at 11:40:17AM +0100, Borislav Petkov wrote:
> On Tue, Mar 15, 2016 at 07:39:31AM +0700, Suravee Suthikulpanit wrote:
> > What if I just merge the newly introduced arch/x86/include/perf/amd/iommu.h
> > into the include/linux/amd-iommu.h? I do not see the point of having to
> > separate things out into two files.
> 
> Except that this header has x86-specific stuff and include/linux/ is
> arch-agnostic.

Which would suggest that header is placed wrong, because I would expect
the amd-iommu to really be rather x86 specific. Or is AMD making ARM
parts for which this is useful too?

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web