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

Groups > linux.kernel > #1220476 > unrolled thread

Re: [PATCH v6 1/2] perf,kvm/powerpc: Add kvm_perf.h for powerpc

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 v6 1/2] perf,kvm/powerpc: Add kvm_perf.h for powerpc Hemant Kumar <hemant@linux.vnet.ibm.com> - 2015-09-08 07:00 +0200
    Re: [PATCH v6 1/2] perf,kvm/powerpc: Add kvm_perf.h for powerpc Michael Ellerman <mpe@ellerman.id.au> - 2015-09-08 10:40 +0200
      Re: [PATCH v6 1/2] perf,kvm/powerpc: Add kvm_perf.h for powerpc Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-09-08 17:20 +0200

#1220476 — Re: [PATCH v6 1/2] perf,kvm/powerpc: Add kvm_perf.h for powerpc

On 09/07/2015 10:40 AM, Michael Ellerman wrote:
> On Fri, 2015-09-04 at 17:51 -0300, Arnaldo Carvalho de Melo wrote:
>> Em Tue, Sep 01, 2015 at 12:18:47PM +0530, Hemant Kumar escreveu:
>>>> Should I try to process the 5 together, applying thest two first?
>>   
>>> Yes, this patchset needs to be applied before applying the other patchset,
>>> since there is a direct dependency on these two for the tooling part to
>>> work.
>>   
>>>> I see there are no acks from powerpc arch maintainers, how should we
>>>> proceed here? If there are no problems with the arch bits, and if it is
>>>> just to enable the tooling part, again, should I process the 5 as just
>>>> one series?
>>   
>>> The reason to split the earlier patchset into two was to separate the
>>> tooling/perf/ and arch/powerpc/ side patches, as asked by Michael..
>>   
>>> Here is the link to that discussion :
>>> http://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg86916.html
>>   
>>> If Michael is ok with the patches, you can process all the 5 patches
>>> together. Michael?
>> Michael?
> I'm not particularly happy with it.
>
> Can we at least remove this hunk from the uapi header:
>
> +/* This is to shut the compiler up */
> +#define KVM_ENTRY_TRACE ""
> +#define KVM_EXIT_TRACE ""
> +#define KVM_EXIT_REASON ""
>

Agreed, I didn't like this too, but I kept this because of the generic
perf userspace code that looks for KVM_{ENTRY,EXIT}_TRACE and
KVM_EXIT_REASON. We can remove this and put this hunk in the
userspace side.

Arnaldo,
Can we remove the dependency on uapi altogether (also suggested
by Scott) because it doesn't seem to fulfill much purpose? Rather,
hardcode the events in the userspace completely (since, tracepoint
event names are unlikely to change) ? Some of what is being done
by x86 already in kvm-stat.c where its defining kvm_events_tp[] and
its not using the macros, rather, the tracepoints directly. Macros are
only being used in builtin-kvm.c where the tracepoint names are
matched with KVM_{ENTRY,EXIT}_TRACE and when we are looking
for the key KVM_EXIT_REASON.

-- 
Thanks,
Hemant Kumar

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1220601

On Tue, 2015-09-08 at 10:26 +0530, Hemant Kumar wrote:
> 
> On 09/07/2015 10:40 AM, Michael Ellerman wrote:
> > On Fri, 2015-09-04 at 17:51 -0300, Arnaldo Carvalho de Melo wrote:
> >> Em Tue, Sep 01, 2015 at 12:18:47PM +0530, Hemant Kumar escreveu:
> >>>> Should I try to process the 5 together, applying thest two first?
> >>   
> >>> Yes, this patchset needs to be applied before applying the other patchset,
> >>> since there is a direct dependency on these two for the tooling part to
> >>> work.
> >>   
> >>>> I see there are no acks from powerpc arch maintainers, how should we
> >>>> proceed here? If there are no problems with the arch bits, and if it is
> >>>> just to enable the tooling part, again, should I process the 5 as just
> >>>> one series?
> >>   
> >>> The reason to split the earlier patchset into two was to separate the
> >>> tooling/perf/ and arch/powerpc/ side patches, as asked by Michael..
> >>   
> >>> Here is the link to that discussion :
> >>> http://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg86916.html
> >>   
> >>> If Michael is ok with the patches, you can process all the 5 patches
> >>> together. Michael?
> >> Michael?
> > I'm not particularly happy with it.
> >
> > Can we at least remove this hunk from the uapi header:
> >
> > +/* This is to shut the compiler up */
> > +#define KVM_ENTRY_TRACE ""
> > +#define KVM_EXIT_TRACE ""
> > +#define KVM_EXIT_REASON ""
> >
> 
> Agreed, I didn't like this too, but I kept this because of the generic
> perf userspace code that looks for KVM_{ENTRY,EXIT}_TRACE and
> KVM_EXIT_REASON. We can remove this and put this hunk in the
> userspace side.

Yes please.

> Arnaldo,
> Can we remove the dependency on uapi altogether (also suggested
> by Scott) because it doesn't seem to fulfill much purpose? Rather,
> hardcode the events in the userspace completely (since, tracepoint
> event names are unlikely to change) ? Some of what is being done
> by x86 already in kvm-stat.c where its defining kvm_events_tp[] and
> its not using the macros, rather, the tracepoints directly. Macros are
> only being used in builtin-kvm.c where the tracepoint names are
> matched with KVM_{ENTRY,EXIT}_TRACE and when we are looking
> for the key KVM_EXIT_REASON.

That would certainly make me a lot happier with it.

Also think about what would happen if the tracepoint names *did* change. The
perf code would want to try and work with both the old and new names, so at
that point you'd end up hard coding the names in the perf code anyway.

cheers



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1220924

Em Tue, Sep 08, 2015 at 06:33:45PM +1000, Michael Ellerman escreveu:
> On Tue, 2015-09-08 at 10:26 +0530, Hemant Kumar wrote:
> > 
> > On 09/07/2015 10:40 AM, Michael Ellerman wrote:
> > > On Fri, 2015-09-04 at 17:51 -0300, Arnaldo Carvalho de Melo wrote:
> > >> Em Tue, Sep 01, 2015 at 12:18:47PM +0530, Hemant Kumar escreveu:
> > >>>> Should I try to process the 5 together, applying thest two first?
> > >>   
> > >>> Yes, this patchset needs to be applied before applying the other patchset,
> > >>> since there is a direct dependency on these two for the tooling part to
> > >>> work.
> > >>   
> > >>>> I see there are no acks from powerpc arch maintainers, how should we
> > >>>> proceed here? If there are no problems with the arch bits, and if it is
> > >>>> just to enable the tooling part, again, should I process the 5 as just
> > >>>> one series?
> > >>   
> > >>> The reason to split the earlier patchset into two was to separate the
> > >>> tooling/perf/ and arch/powerpc/ side patches, as asked by Michael..
> > >>   
> > >>> Here is the link to that discussion :
> > >>> http://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg86916.html
> > >>   
> > >>> If Michael is ok with the patches, you can process all the 5 patches
> > >>> together. Michael?
> > >> Michael?
> > > I'm not particularly happy with it.
> > >
> > > Can we at least remove this hunk from the uapi header:
> > >
> > > +/* This is to shut the compiler up */
> > > +#define KVM_ENTRY_TRACE ""
> > > +#define KVM_EXIT_TRACE ""
> > > +#define KVM_EXIT_REASON ""
> > >
> > 
> > Agreed, I didn't like this too, but I kept this because of the generic
> > perf userspace code that looks for KVM_{ENTRY,EXIT}_TRACE and
> > KVM_EXIT_REASON. We can remove this and put this hunk in the
> > userspace side.
> 
> Yes please.
> 
> > Arnaldo,
> > Can we remove the dependency on uapi altogether (also suggested
> > by Scott) because it doesn't seem to fulfill much purpose? Rather,
> > hardcode the events in the userspace completely (since, tracepoint
> > event names are unlikely to change) ? Some of what is being done
> > by x86 already in kvm-stat.c where its defining kvm_events_tp[] and
> > its not using the macros, rather, the tracepoints directly. Macros are
> > only being used in builtin-kvm.c where the tracepoint names are
> > matched with KVM_{ENTRY,EXIT}_TRACE and when we are looking
> > for the key KVM_EXIT_REASON.
> 
> That would certainly make me a lot happier with it.
> 
> Also think about what would happen if the tracepoint names *did* change. The
> perf code would want to try and work with both the old and new names, so at
> that point you'd end up hard coding the names in the perf code anyway.

Yeah, seems like we have a plan :-)

- Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web