Date: Fri, 8 Apr 2016 14:33:03 -0300
From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: "Wangnan (F)" <wangnan0@huawei.com>
Cc: pi3orama@163.com, linux-kernel@vger.kernel.org, lizefan@huawei.com,
        Arnaldo Carvalho de Melo <acme@redhat.com>,
        Jiri Olsa <jolsa@kernel.org>
Subject: Re: [PATCH 1/4] perf trace: Improve error message when receive
 non-tracepoint events
Message-ID: <20160408173302.GQ5945@kernel.org>
References: <1460128045-97310-1-git-send-email-wangnan0@huawei.com>
 <1460128045-97310-2-git-send-email-wangnan0@huawei.com>
 <20160408152257.GO5945@kernel.org>
 <5707D879.8030406@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <5707D879.8030406@huawei.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2902
Lines: 78

Em Sat, Apr 09, 2016 at 12:12:41AM +0800, Wangnan (F) escreveu:
> 
> 
> On 2016/4/8 23:22, Arnaldo Carvalho de Melo wrote:
> >Em Fri, Apr 08, 2016 at 03:07:22PM +0000, Wang Nan escreveu:
> >>Before this patch, strange error message is provided if passed a
> >>non-tracepoint event to 'perf trace':
> >>
> >>  # perf trace -a  --ev cycles sleep 1
> >>  Failed to set filter "common_pid != 27500" on event cycles with 22 (Invalid argument)
> >>
> >>This is because 'perf trace' accepts all valid events during cmdline
> >>parsing, but in fact user can only provide tracepoints, because it
> >>needs filter.
> >>
> >>This patch validate evlist, report error earlier:
> >>
> >>  # ./perf trace -a  --ev cycles sleep 1
> >>  Only support tracepoint events!
> >Humm, perhaps we should instead refrain from setting filters to non
> >tracepoint events? I.e. I don't see why we whouldn't support, say,
> >software events...
> >
> >/me trying some now, i.e.:
> >
> >   # trace --ev minor-faults --no-syscalls
> >
> >But it has some issues...
> >
> >- Arnaldo
> 
> We already have commit fdf14720fbd02 ("perf tools: Only set filter for
> tracepoints events") so you won't see the ugly error message again.

Ok
 
> However, I think parsing non-tracepoint events in 'perf trace' is still
> a challange. We never support it in 'perf trace' and I'm not too much
> sure who will need this feature and how to use them, and why he/she can't
> use 'perf record' instead.

Well, it works already, the issue I had with 'trace --ev minor-faults'
is that we start with a freq=0, if we do:

  # trace --ev minor-faults/freq=1234/

We set up it in a way that we get the events:

  { sample_period, sample_freq }   1234

  # trace --ev minor-faults/freq=1234/ -e nanosleep usleep 1
  18446744073709.551 (         ): minor-faults/freq=1234/:)
  18446744073709.551 (         ): minor-faults/freq=1234/:)
  18446744073709.551 (         ): minor-faults/freq=1234/:)
  18446744073709.551 (         ): minor-faults/freq=1234/:)
  18446744073709.551 (         ): minor-faults/freq=1234/:)
     0.345 ( 0.058 ms): usleep/24424 nanosleep(rqtp: 0x7ffc39aca490) = 0
  #

But we're not setting up the sample_type, another minor issue, will fix.

In general I think we should not artificially limit what can be done
with one of the tools, trying to leave policy to the user, and being
able to have sampling events mixed with strace-like formatted syscall
entry+exit, tracepoints and other kinds of events like minor-faults, etc
looks sensible.

We can even make the default for PERF_[SH]W_EVENTS to be resolve the
symbol, like we wo in 'perf script'.

And if a frequency is not provided for a sampling event, set a default,
like 'perf record' and 'top' do, its just that the default one for 'perf
trace' now is 0, making me thinkg it was busted in a more serious
fashion.

Now to test Millian's 'perf trace --call-chain' patch...

- Arnaldo