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

[Peter Zijlstra]

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/