lkml.org 
[lkml]   [2019]   [Nov]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v3 1/9] perf tools: add parse events append error
Thanks! This is part of the v5 patch set, specifically:
https://lkml.org/lkml/2019/10/30/1001

Let me know if there's anything else blocking this. Thanks,
Ian

On Mon, Oct 28, 2019 at 2:36 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Mon, Oct 28, 2019 at 02:06:24PM -0700, Ian Rogers wrote:
> > On Mon, Oct 28, 2019 at 12:32 PM Jiri Olsa <jolsa@redhat.com> wrote:
> > >
> > > On Fri, Oct 25, 2019 at 08:14:36AM -0700, Ian Rogers wrote:
> > > > On Fri, Oct 25, 2019 at 12:58 AM Jiri Olsa <jolsa@redhat.com> wrote:
> > > > >
> > > > > On Thu, Oct 24, 2019 at 12:01:54PM -0700, Ian Rogers wrote:
> > > > > > Parse event error handling may overwrite one error string with another
> > > > > > creating memory leaks and masking errors. Introduce a helper routine
> > > > > > that appends error messages and avoids the memory leak.
> > > > > >
> > > > > > A reproduction of this problem can be seen with:
> > > > > > perf stat -e c/c/
> > > > > > After this change this produces:
> > > > > > event syntax error: 'c/c/'
> > > > > > \___ unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: Cannot find PMU `c'. Missing kernel support?)(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,pc,in_tx,edge,any,offcore_rsp,in_tx_cp,ldlat,inv,umask,frontend,cmask,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))
> > > > >
> > > > >
> > > > > hum... I'd argue that the previous state was better:
> > > > >
> > > > > [jolsa@krava perf]$ ./perf stat -e c/c/
> > > > > event syntax error: 'c/c/'
> > > > > \___ unknown term
> > > > >
> > > > >
> > > > > jirka
> > > >
> > > > I am agnostic. We can either have the previous state or the new state,
> > > > I'm keen to resolve the memory leak. Another alternative is to warn
> > > > that multiple errors have occurred before dropping or printing the
> > > > previous error. As the code is shared in memory places the approach
> > > > taken here was to try to not conceal anything that could potentially
> > > > be useful. Given this, is the preference to keep the status quo
> > > > without any warning?
> > >
> > > if the other alternative is string above, yes.. but perhaps
> > > keeping just the first error would be the best way?
> > >
> > > here it seems to be the:
> > > "Cannot find PMU `c'. Missing kernel support?)(help: valid..."
> >
> > I think this is a reasonable idea. I'd propose doing it as an
> > additional patch, the purpose of this patch is to avoid a possible
> > memory leak. I can write the patch and base it on this series.
> > To resolve the issue, I'd add an extra first error to the struct
> > parse_events_error. All callers would need to be responsible for
> > cleaning this up when present, which is why I'd rather not make it
> > part of this patch.
> > Does this sound reasonable?
>
> yep, sounds good
>
> jirka
>

\
 
 \ /
  Last update: 2019-11-04 21:38    [W:0.195 / U:0.976 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site