Messages in this thread | | | Date | Mon, 14 Mar 2016 13:38:37 +0100 | From | Peter Zijlstra <> | Subject | Re: [PATCH v2 2/5] perf: Free aux pages in unmap path |
| |
On Fri, Mar 04, 2016 at 03:42:46PM +0200, Alexander Shishkin wrote: > @@ -4649,10 +4679,22 @@ static void perf_mmap_close(struct vm_area_struct *vma) > */ > if (rb_has_aux(rb) && vma->vm_pgoff == rb->aux_pgoff && > atomic_dec_and_mutex_lock(&rb->aux_mmap_count, &event->mmap_mutex)) { > + /* > + * Stop all aux events that are writing to this here buffer, > + * so that we can free its aux pages and corresponding pmu > + * data. Note that after rb::aux_mmap_count dropped to zero, > + * they won't start any more (see perf_aux_output_begin()). > + */ > + perf_pmu_output_stop(event);
So to me it seems like we're interested in rb, we don't particularly care about @event in this case.
> + > + /* now it's safe to free the pages */ > atomic_long_sub(rb->aux_nr_pages, &mmap_user->locked_vm); > vma->vm_mm->pinned_vm -= rb->aux_mmap_locked; > > + /* this has to be the last one */ > rb_free_aux(rb); > + WARN_ON_ONCE(atomic_read(&rb->aux_refcount)); > + > mutex_unlock(&event->mmap_mutex); > } >
> +static void __perf_event_output_stop(struct perf_event *event, void *data) > +{ > + struct perf_event *parent = event->parent; > + struct remote_output *ro = data; > + struct ring_buffer *rb = ro->rb; > + > + if (!has_aux(event)) > + return; > +
if (!parent) parent = event;
> + if (rcu_dereference(event->rb) == rb) s/event/parent/
> + ro->err = __perf_event_stop(event);
> + else if (parent && rcu_dereference(parent->rb) == rb) > + ro->err = __perf_event_stop(event);
and these can go.. However..
> +} > + > +static int __perf_pmu_output_stop(void *info) > +{ > + struct perf_event *event = info; > + struct pmu *pmu = event->pmu; > + struct perf_cpu_context *cpuctx = get_cpu_ptr(pmu->pmu_cpu_context); > + struct remote_output ro = { > + .rb = event->rb, > + }; > + > + rcu_read_lock(); > + perf_event_aux_ctx(&cpuctx->ctx, __perf_event_output_stop, &ro); > + if (cpuctx->task_ctx) > + perf_event_aux_ctx(cpuctx->task_ctx, __perf_event_output_stop, > + &ro); > + rcu_read_unlock(); > + > + return ro.err; > +} > + > +static void perf_pmu_output_stop(struct perf_event *event) > +{ > + int cpu, err; > + > + /* better be thorough */ > + get_online_cpus(); > +restart: > + for_each_online_cpu(cpu) { > + err = cpu_function_call(cpu, __perf_pmu_output_stop, event); > + if (err) > + goto restart; > + } > + put_online_cpus(); > +}
This seems wildly overkill, could we not iterate rb->event_list like we do for the normal buffer?
Sure, we need to IPI for each event found, but that seems better than unconditionally sending IPIs to all CPUs.
| |