Re: [PATCH 10/73] libperf: Add perf_mmap struct

From: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
To: Jiri Olsa <jolsa@kernel.org>
Cc: lkml <linux-kernel@vger.kernel.org>,
	Ingo Molnar <mingo@kernel.org>,
	Namhyung Kim <namhyung@kernel.org>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Peter Zijlstra <a.p.zijlstra@chello.nl>,
	Michael Petlan <mpetlan@redhat.com>
Subject: Re: [PATCH 10/73] libperf: Add perf_mmap struct
Date: Mon, 23 Sep 2019 12:05:33 -0300	[thread overview]
Message-ID: <20190923150533.GG16544@kernel.org> (raw)
In-Reply-To: <20190913132355.21634-11-jolsa@kernel.org>

Em Fri, Sep 13, 2019 at 03:22:52PM +0200, Jiri Olsa escreveu:
> Add the perf_mmap to libperf.
> 
> The definition is added into:
> 
>   include/internal/mmap.h
> 
> which is not to be included by users, but shared
> within perf and libperf.

> diff --git a/tools/perf/lib/include/internal/mmap.h b/tools/perf/lib/include/internal/mmap.h
> new file mode 100644
> index 000000000000..8d10559dee49
> --- /dev/null
> +++ b/tools/perf/lib/include/internal/mmap.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __LIBPERF_INTERNAL_MMAP_H
> +#define __LIBPERF_INTERNAL_MMAP_H
> +
> +#include <linux/refcount.h>
> +#include <linux/compiler.h>
> +#include <stdlib.h>
> +#include <stdbool.h>

So you're doing this with high granularity, cool! But then you should
take care not to add unnecessary stuff here, i.e. these four headers are
not necessary at this point in the series, I'm removing them and adding
as they become necessary.

- Arnaldo

> +/**
> + * struct perf_mmap - perf's ring buffer mmap details
> + *
> + * @refcnt - e.g. code using PERF_EVENT_IOC_SET_OUTPUT to share this
> + */
> +struct perf_mmap {
> +	void		*base;
> +};
> +
> +#endif /* __LIBPERF_INTERNAL_MMAP_H */
> diff --git a/tools/perf/util/mmap.c b/tools/perf/util/mmap.c
> index f3b7c8b0fa90..76190b2edd78 100644
> --- a/tools/perf/util/mmap.c
> +++ b/tools/perf/util/mmap.c
> @@ -31,7 +31,7 @@ size_t perf_mmap__mmap_len(struct mmap *map)
>  static union perf_event *perf_mmap__read(struct mmap *map,
>  					 u64 *startp, u64 end)
>  {
> -	unsigned char *data = map->base + page_size;
> +	unsigned char *data = map->core.base + page_size;
>  	union perf_event *event = NULL;
>  	int diff = end - *startp;
>  
> @@ -116,7 +116,7 @@ void perf_mmap__get(struct mmap *map)
>  
>  void perf_mmap__put(struct mmap *map)
>  {
> -	BUG_ON(map->base && refcount_read(&map->refcnt) == 0);
> +	BUG_ON(map->core.base && refcount_read(&map->refcnt) == 0);
>  
>  	if (refcount_dec_and_test(&map->refcnt))
>  		perf_mmap__munmap(map);
> @@ -317,9 +317,9 @@ void perf_mmap__munmap(struct mmap *map)
>  		munmap(map->data, perf_mmap__mmap_len(map));
>  		map->data = NULL;
>  	}
> -	if (map->base != NULL) {
> -		munmap(map->base, perf_mmap__mmap_len(map));
> -		map->base = NULL;
> +	if (map->core.base != NULL) {
> +		munmap(map->core.base, perf_mmap__mmap_len(map));
> +		map->core.base = NULL;
>  		map->fd = -1;
>  		refcount_set(&map->refcnt, 0);
>  	}
> @@ -370,12 +370,12 @@ int perf_mmap__mmap(struct mmap *map, struct mmap_params *mp, int fd, int cpu)
>  	refcount_set(&map->refcnt, 2);
>  	map->prev = 0;
>  	map->mask = mp->mask;
> -	map->base = mmap(NULL, perf_mmap__mmap_len(map), mp->prot,
> +	map->core.base = mmap(NULL, perf_mmap__mmap_len(map), mp->prot,
>  			 MAP_SHARED, fd, 0);
> -	if (map->base == MAP_FAILED) {
> +	if (map->core.base == MAP_FAILED) {
>  		pr_debug2("failed to mmap perf event ring buffer, error %d\n",
>  			  errno);
> -		map->base = NULL;
> +		map->core.base = NULL;
>  		return -1;
>  	}
>  	map->fd = fd;
> @@ -399,7 +399,7 @@ int perf_mmap__mmap(struct mmap *map, struct mmap_params *mp, int fd, int cpu)
>  	}
>  
>  	if (auxtrace_mmap__mmap(&map->auxtrace_mmap,
> -				&mp->auxtrace_mp, map->base, fd))
> +				&mp->auxtrace_mp, map->core.base, fd))
>  		return -1;
>  
>  	return perf_mmap__aio_mmap(map, mp);
> @@ -444,7 +444,7 @@ static int __perf_mmap__read_init(struct mmap *md)
>  {
>  	u64 head = perf_mmap__read_head(md);
>  	u64 old = md->prev;
> -	unsigned char *data = md->base + page_size;
> +	unsigned char *data = md->core.base + page_size;
>  	unsigned long size;
>  
>  	md->start = md->overwrite ? head : old;
> @@ -489,7 +489,7 @@ int perf_mmap__push(struct mmap *md, void *to,
>  		    int push(struct mmap *map, void *to, void *buf, size_t size))
>  {
>  	u64 head = perf_mmap__read_head(md);
> -	unsigned char *data = md->base + page_size;
> +	unsigned char *data = md->core.base + page_size;
>  	unsigned long size;
>  	void *buf;
>  	int rc = 0;
> diff --git a/tools/perf/util/mmap.h b/tools/perf/util/mmap.h
> index 01524608a984..9028b0e8a0ed 100644
> --- a/tools/perf/util/mmap.h
> +++ b/tools/perf/util/mmap.h
> @@ -1,6 +1,7 @@
>  #ifndef __PERF_MMAP_H
>  #define __PERF_MMAP_H 1
>  
> +#include <internal/mmap.h>
>  #include <linux/compiler.h>
>  #include <linux/refcount.h>
>  #include <linux/types.h>
> @@ -20,7 +21,7 @@ struct aiocb;
>   * @refcnt - e.g. code using PERF_EVENT_IOC_SET_OUTPUT to share this
>   */
>  struct mmap {
> -	void		 *base;
> +	struct perf_mmap	core;
>  	int		 mask;
>  	int		 fd;
>  	int		 cpu;
> @@ -88,12 +89,12 @@ void perf_mmap__consume(struct mmap *map);
>  
>  static inline u64 perf_mmap__read_head(struct mmap *mm)
>  {
> -	return ring_buffer_read_head(mm->base);
> +	return ring_buffer_read_head(mm->core.base);
>  }
>  
>  static inline void perf_mmap__write_tail(struct mmap *md, u64 tail)
>  {
> -	ring_buffer_write_tail(md->base, tail);
> +	ring_buffer_write_tail(md->core.base, tail);
>  }
>  
>  union perf_event *perf_mmap__read_forward(struct mmap *map);
> -- 
> 2.21.0

-- 

- Arnaldo