On 2014/1/15 18:24, Robert Richter wrote:
> On 15.01.14 10:02:44, Weng Meiling wrote:
>> On 2014/1/14 23:05, Robert Richter wrote:
>>> @@ -94,6 +98,11 @@ static int op_create_counter(int cpu, int event)
>>>
>>> per_cpu(perf_events, cpu)[event] = pevent;
>>>
>>> + /* sync perf_events with overflow handler: */
>>> + smp_wmb();
>>> +
>>> + perf_event_enable(pevent);
>>> +
>>
>> Should this step go before the if check:pevent->state !=
>> PERF_EVENT_STATE_ACTIVE ?
>> Because the attr->disabled is true, So after the
>> perf_event_create_kernel_counter
>> the pevent->state is not PERF_EVENT_STATE_ACTIVE.
>
> Right, the check is a problem. We need to move it after the event was
> enabled. On error, we need to NULL the event, see below.
>
> -Robert
>
> ---
> drivers/oprofile/oprofile_perf.c | 27 +++++++++++++++++++--------
> 1 file changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/oprofile/oprofile_perf.c
> b/drivers/oprofile/oprofile_perf.c
> index d5b2732..9dfb236 100644
> --- a/drivers/oprofile/oprofile_perf.c
> +++ b/drivers/oprofile/oprofile_perf.c
> @@ -38,6 +38,9 @@ static void op_overflow_handler(struct perf_event *event,
> int id;
> u32 cpu = smp_processor_id();
>
> + /* sync perf_events with op_create_counter(): */
> + smp_rmb();
> +
> for (id = 0; id < num_counters; ++id)
> if (per_cpu(perf_events, cpu)[id] == event)
> break;
> @@ -68,6 +71,7 @@ static void op_perf_setup(void)
> attr->config = counter_config[i].event;
> attr->sample_period = counter_config[i].count;
> attr->pinned = 1;
> + attr->disabled = 1;
> }
> }
>
> @@ -85,16 +89,23 @@ static int op_create_counter(int cpu, int event)
> if (IS_ERR(pevent))
> return PTR_ERR(pevent);
>
> - if (pevent->state != PERF_EVENT_STATE_ACTIVE) {
> - perf_event_release_kernel(pevent);
> - pr_warning("oprofile: failed to enable event %d "
> - "on CPU %d\n", event, cpu);
> - return -EBUSY;
> - }
> -
> per_cpu(perf_events, cpu)[event] = pevent;
>
> - return 0;
> + /* sync perf_events with overflow handler: */
> + smp_wmb();
> +
> + perf_event_enable(pevent);
> +
> + if (pevent->state == PERF_EVENT_STATE_ACTIVE)
> + return 0;
> +
> + perf_event_release_kernel(pevent);
> + per_cpu(perf_events, cpu)[event] = NULL;
> +
> + pr_warning("oprofile: failed to enable event %d on CPU %d\n",
> + event, cpu);
> +
> + return -EBUSY;
> }
>
> static void op_destroy_counter(int cpu, int event)
>
OK, I'll test the patch, and send the result as soon as possible.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
