lkml.org 
[lkml]   [2018]   [Jun]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v1 4/6] perf: Allow using AUX data in perf samples
On Tue, Jun 12, 2018 at 10:51:15AM +0300, Alexander Shishkin wrote:
> +static unsigned long perf_aux_sample_size(struct perf_event *event,
> + struct perf_sample_data *data,
> + size_t size)
> +{
> + struct perf_event *sampler = event->sample_event;
> + struct ring_buffer *rb;
> + int *disable_count;
> +
> + data->aux.size = 0;
> +
> + if (!sampler || READ_ONCE(sampler->state) != PERF_EVENT_STATE_ACTIVE)
> + goto out;
> +
> + if (READ_ONCE(sampler->oncpu) != smp_processor_id())
> + goto out;

Should those not be WARNs ? If we allow a configuration where that is
possible, we're doing it wrong I think.

> + /*
> + * Non-zero disable count here means that we, being the NMI
> + * context, are racing with pmu::add, pmu::del or address filter
> + * adjustment, which we want to avoid.
> + */
> + disable_count = this_cpu_ptr(sampler->pmu->pmu_disable_count);
> + if (*disable_count)
> + goto out;
> +
> + /* Re-enabled in perf_aux_sample_output() */
> + perf_pmu_disable(sampler->pmu);

This is disguisting.. and also broken I think. Imagine what happens
when the NMI hits in middle of perf_pmu_enable(), right where count
dropped to 0, but we've not yet done pmu->pmu_enable() yet.

Then we end up with a double disable and double enable.

> +
> + rb = ring_buffer_get(sampler);
> + if (!rb) {
> + perf_pmu_enable(sampler->pmu);
> + goto out;
> + }
> +
> + /* Restarted in perf_aux_sample_output() */
> + sampler->pmu->stop(sampler, PERF_EF_UPDATE);

More yuck...

You rreally should not be calling these pmu::methods, they're meant to
be used from _interrupt_ not NMI context. Using them like this is asking
for tons of trouble.

Why can't you just snapshot the current location and let the thing
'run' ?

> + data->aux.to = rb->aux_head;
> +
> + size = min(size, perf_aux_size(rb));
> +
> + if (data->aux.to < size)
> + data->aux.from = rb->aux_nr_pages * PAGE_SIZE + data->aux.to -
> + size;
> + else
> + data->aux.from = data->aux.to - size;
> + data->aux.size = ALIGN(size, sizeof(u64));
> + ring_buffer_put(rb);
> +
> +out:
> + return data->aux.size;
> +}
> +
> +static void perf_aux_sample_output(struct perf_event *event,
> + struct perf_output_handle *handle,
> + struct perf_sample_data *data)
> +{

> + ring_buffer_put(rb);
> + sampler->pmu->start(sampler, 0);
> +
> +out_enable:
> + perf_pmu_enable(sampler->pmu);

More of that... really yuck stuff.

> +}

\
 
 \ /
  Last update: 2018-06-14 22:21    [W:0.124 / U:0.268 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site