public inbox for netdev@vger.kernel.org 
 help / color / mirror / Atom feed

From: xiakaixu <xiakaixu@huawei•com>
To: Peter Zijlstra <peterz@infradead•org>
Cc: <ast@plumgrid•com>, <davem@davemloft•net>, <acme@kernel•org>,
	<mingo@redhat•com>, <masami.hiramatsu.pt@hitachi•com>,
	<jolsa@kernel•org>, <daniel@iogearbox•net>, <wangnan0@huawei•com>,
	<linux-kernel@vger•kernel.org>, <pi3orama@163•com>,
	<hekuang@huawei•com>, <netdev@vger•kernel.org>
Subject: Re: [PATCH v6 3/4] bpf: Implement function bpf_perf_event_read() that get the selected hardware PMU conuter
Date: Wed, 5 Aug 2015 18:31:55 +0800	[thread overview]
Message-ID: <55C1E61B.1040808@huawei.com> (raw)
In-Reply-To: <20150805100425.GZ25159@twins.programming.kicks-ass.net>

于 2015/8/5 18:04, Peter Zijlstra 写道:
> On Tue, Aug 04, 2015 at 08:58:15AM +0000, Kaixu Xia wrote:
>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>> index 6251b53..726ca1b 100644
>> --- a/kernel/events/core.c
>> +++ b/kernel/events/core.c
>> @@ -8599,6 +8599,25 @@ struct perf_event_attr *perf_event_attrs(struct perf_event *event)
>>  	return &event->attr;
>>  }
>>  
>> +u64 perf_event_read_internal(struct perf_event *event)
> 
> Maybe: perf_event_read_local(), as this is this function only works for
> events active on the current CPU.
> 
>> +{
>> +	if (!event)
>> +		return -EINVAL;
>> +
>> +	if (unlikely(event->state != PERF_EVENT_STATE_ACTIVE))
>> +		return -EINVAL;
> 
> You can return perf_event_count() in that case.
> 
>> +
>> +	if (event->oncpu != raw_smp_processor_id() &&
> 
> That _must_ be smp_processor_id(). If that gives a warning (ie.
> preemption is not disabled or we're not affine to this one cpu) then the
> warning is valid.
> 
>> +	    event->ctx->task != current)
> 
> Write it like:
> 
> 	if (event->ctx->task != current &&
> 	    event->oncpu != smp_processor_id())
> 
> That way you'll not evaluate smp_processor_id() for current task events.
> 
>> +		return -EINVAL;
>> +
>> +	if (unlikely(event->attr.inherit))
>> +		return -EINVAL;
> 
> This should be in your accept function, inherited events should never
> get this far.
> 
> You need IRQs disabled while calling __perf_event_read(), did you test
> with lockdep enabled?

Thanks for your review! I've not tested it, will do that from now on.
> 
>> +	__perf_event_read(event);
>> +	return perf_event_count(event);
>> +}
> 
> Also, you probably want a WARN_ON(in_nmi()) there, this function is
> _NOT_ NMI safe.
> 
> 
> 
> .
> 

next prev parent reply	other threads:[~2015-08-05 10:31 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-08-04  8:58 [PATCH v6 0/4] bpf: Introduce the new ability of eBPF programs to access hardware PMU counter Kaixu Xia
2015-08-04  8:58 ` [PATCH v6 1/4] bpf: Make the bpf_prog_array_map more generic Kaixu Xia
2015-08-04  8:58 ` [PATCH v6 2/4] bpf: Add new bpf map type to store the pointer to struct perf_event Kaixu Xia
2015-08-04 17:43   ` Alexei Starovoitov
2015-08-05  9:41     ` Peter Zijlstra
2015-08-05  9:39   ` Peter Zijlstra
2015-08-05  9:43   ` Peter Zijlstra
2015-08-04  8:58 ` [PATCH v6 3/4] bpf: Implement function bpf_perf_event_read() that get the selected hardware PMU conuter Kaixu Xia
2015-08-04 17:55   ` Alexei Starovoitov
2015-08-05  2:09     ` xiakaixu
2015-08-05 10:04   ` Peter Zijlstra
2015-08-05 10:15     ` Peter Zijlstra
2015-08-05 16:00       ` Alexei Starovoitov
2015-08-05 10:31     ` xiakaixu [this message]
2015-08-05 13:53     ` Peter Zijlstra
2015-08-05 13:59       ` Peter Zijlstra
2015-08-05 16:08       ` Alexei Starovoitov
2015-08-05 16:21         ` Peter Zijlstra
2015-08-06  2:49       ` xiakaixu
2015-08-05 15:59     ` Alexei Starovoitov
2015-08-04  8:58 ` [PATCH v6 4/4] samples/bpf: example of get selected PMU counter value Kaixu Xia
2015-08-05 10:06 ` [PATCH v6 0/4] bpf: Introduce the new ability of eBPF programs to access hardware PMU counter Peter Zijlstra

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=55C1E61B.1040808@huawei.com \
    --to=xiakaixu@huawei$(echo .)com \
    --cc=acme@kernel$(echo .)org \
    --cc=ast@plumgrid$(echo .)com \
    --cc=daniel@iogearbox$(echo .)net \
    --cc=davem@davemloft$(echo .)net \
    --cc=hekuang@huawei$(echo .)com \
    --cc=jolsa@kernel$(echo .)org \
    --cc=linux-kernel@vger$(echo .)kernel.org \
    --cc=masami.hiramatsu.pt@hitachi$(echo .)com \
    --cc=mingo@redhat$(echo .)com \
    --cc=netdev@vger$(echo .)kernel.org \
    --cc=peterz@infradead$(echo .)org \
    --cc=pi3orama@163$(echo .)com \
    --cc=wangnan0@huawei$(echo .)com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox