Re: [PATCH] Split executable and non-executable mmap tracking V2

From: Peter Zijlstra
Date: Mon May 17 2010 - 12:22:45 EST


On Mon, 2010-05-17 at 16:26 +0100, Eric B Munson wrote:
> This patch splits tracking of executable and non-executable mmaps.
> Executable mmaps are tracked normally and non-executable are
> tracked when --data is used.
>
> Signed-off-by: Anton Blanchard <anton@xxxxxxxxx>
>
> Updated code for stable perf ABI
> Signed-off-by: Eric B Munson <ebmunson@xxxxxxxxxx>
> ---
> Changes from V1:
> -Changed mmap_exec to mmap_data and left mmap as the executable mmap tracker
> to maintain backwards compatibility
> -Insert mmap_data at the end of the attr bit map


> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index c8e3754..05c1dd1 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -196,15 +196,16 @@ struct perf_event_attr {
> exclude_kernel : 1, /* ditto kernel */
> exclude_hv : 1, /* ditto hypervisor */
> exclude_idle : 1, /* don't count when idle */
> - mmap : 1, /* include mmap data */
> + mmap : 1, /* include exec mmap data*/
> comm : 1, /* include comm data */
> freq : 1, /* use freq, not period */
> inherit_stat : 1, /* per task counts */
> enable_on_exec : 1, /* next exec enables */
> task : 1, /* trace fork/exit */
> watermark : 1, /* wakeup_watermark */
> + mmap_data : 1, /* include mmap data */
>
> - __reserved_1 : 49;
> + __reserved_1 : 48;

That won't apply against the latest version.

> diff --git a/kernel/perf_event.c b/kernel/perf_event.c
> index 3d1552d..8ad6441 100644
> --- a/kernel/perf_event.c
> +++ b/kernel/perf_event.c
> @@ -1834,6 +1834,8 @@ static void free_event(struct perf_event *event)
> atomic_dec(&nr_events);
> if (event->attr.mmap)
> atomic_dec(&nr_mmap_events);
> + if (event->attr.mmap_data)
> + atomic_dec(&nr_mmap_events);
> if (event->attr.comm)
> atomic_dec(&nr_comm_events);
> if (event->attr.task)


> @@ -4641,6 +4655,8 @@ done:
> atomic_inc(&nr_events);
> if (event->attr.mmap)
> atomic_inc(&nr_mmap_events);
> + if (event->attr.mmap_data)
> + atomic_inc(&nr_mmap_events);
> if (event->attr.comm)
> atomic_inc(&nr_comm_events);
> if (event->attr.task)

Wouldn't you rather write:

if (event->attr.mmap || event->attr.mmap_data)

and avoid an atomic op?

> diff --git a/mm/mmap.c b/mm/mmap.c
> index 456ec6f..6ceee1d 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1781,6 +1781,7 @@ static int expand_downwards(struct vm_area_struct *vma,
> if (!error) {
> vma->vm_start = address;
> vma->vm_pgoff -= grow;
> + perf_event_mmap(vma);
> }
> }
> anon_vma_unlock(vma);

This wants to live in expand_stack(), or get replicated in
expand_upwards().


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/