Messages in this thread | | | Date | Thu, 14 Jun 2018 22:20:22 +0200 | From | Peter Zijlstra <> | Subject | Re: [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.
> +}
| |