Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 6 Jul 2020 14:24:13 +0200
From:   Jiri Olsa <jolsa@redhat.com>
To:     Alexey Budankov <alexey.budankov@linux.intel.com>
Cc:     Arnaldo Carvalho de Melo <acme@kernel.org>,
        Namhyung Kim <namhyung@kernel.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Andi Kleen <ak@linux.intel.com>,
        linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v9 02/15] tools/libperf: add properties to struct pollfd
 *entries objects
Message-ID: <20200706122413.GB3401866@krava>
References: <a4d5db4a-f25c-38dc-1c41-321a886cb122@linux.intel.com>
 <09bbbc85-7ef9-ff9f-9865-fce6a1a4e903@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <09bbbc85-7ef9-ff9f-9865-fce6a1a4e903@linux.intel.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Jul 03, 2020 at 10:41:45AM +0300, Alexey Budankov wrote:
> 
> Store boolean properties per struct pollfd *entries object in a
> bitmap of int size. Implement fdarray_prop__nonfilterable property
> to skip object from counting by fdarray_filter().

ok, I think can do it like this, few comments below

thanks,
jirka

> 
> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
> ---
>  tools/lib/api/fd/array.c                 | 17 +++++++++--------
>  tools/lib/api/fd/array.h                 | 18 +++++++++++++-----
>  tools/lib/perf/evlist.c                  | 10 +++++-----
>  tools/lib/perf/include/internal/evlist.h |  2 +-
>  tools/perf/tests/fdarray.c               |  2 +-
>  tools/perf/util/evlist.c                 |  2 +-
>  6 files changed, 30 insertions(+), 21 deletions(-)
> 
> diff --git a/tools/lib/api/fd/array.c b/tools/lib/api/fd/array.c
> index 89f9a2193c2d..a4223f8cb1ce 100644
> --- a/tools/lib/api/fd/array.c
> +++ b/tools/lib/api/fd/array.c
> @@ -12,31 +12,31 @@
>  void fdarray__init(struct fdarray *fda, int nr_autogrow)
>  {
>  	fda->entries	 = NULL;
> -	fda->priv	 = NULL;
> +	fda->prop	 = NULL;
>  	fda->nr		 = fda->nr_alloc = 0;
>  	fda->nr_autogrow = nr_autogrow;
>  }
>  
>  int fdarray__grow(struct fdarray *fda, int nr)
>  {
> -	void *priv;
> +	void *prop;
>  	int nr_alloc = fda->nr_alloc + nr;
> -	size_t psize = sizeof(fda->priv[0]) * nr_alloc;
> +	size_t psize = sizeof(fda->prop[0]) * nr_alloc;
>  	size_t size  = sizeof(struct pollfd) * nr_alloc;
>  	struct pollfd *entries = realloc(fda->entries, size);
>  
>  	if (entries == NULL)
>  		return -ENOMEM;
>  
> -	priv = realloc(fda->priv, psize);
> -	if (priv == NULL) {
> +	prop = realloc(fda->prop, psize);
> +	if (prop == NULL) {
>  		free(entries);
>  		return -ENOMEM;
>  	}
>  
>  	fda->nr_alloc = nr_alloc;
>  	fda->entries  = entries;
> -	fda->priv     = priv;
> +	fda->prop     = prop;
>  	return 0;
>  }
>  
> @@ -59,7 +59,7 @@ struct fdarray *fdarray__new(int nr_alloc, int nr_autogrow)
>  void fdarray__exit(struct fdarray *fda)
>  {
>  	free(fda->entries);
> -	free(fda->priv);
> +	free(fda->prop);
>  	fdarray__init(fda, 0);
>  }
>  
> @@ -69,7 +69,7 @@ void fdarray__delete(struct fdarray *fda)
>  	free(fda);
>  }
>  
> -int fdarray__add(struct fdarray *fda, int fd, short revents)
> +int fdarray__add(struct fdarray *fda, int fd, short revents, enum fdarray_props props)
>  {
>  	int pos = fda->nr;
>  
> @@ -79,6 +79,7 @@ int fdarray__add(struct fdarray *fda, int fd, short revents)
>  
>  	fda->entries[fda->nr].fd     = fd;
>  	fda->entries[fda->nr].events = revents;
> +	fda->prop[fda->nr].bits = props;
>  	fda->nr++;
>  	return pos;
>  }
> diff --git a/tools/lib/api/fd/array.h b/tools/lib/api/fd/array.h
> index b39557d1a88f..19b6a34aeea5 100644
> --- a/tools/lib/api/fd/array.h
> +++ b/tools/lib/api/fd/array.h
> @@ -21,10 +21,18 @@ struct fdarray {
>  	int	       nr_alloc;
>  	int	       nr_autogrow;
>  	struct pollfd *entries;
> -	union {
> -		int    idx;
> -		void   *ptr;
> -	} *priv;
> +	struct {
> +		union {
> +			int    idx;
> +			void   *ptr;
> +		} priv;
> +		int bits;
> +	} *prop;
> +};

why not keeping the 'priv' as a struct, like:

	struct {
		union {
			int    idx;
			void   *ptr;
		};
		unsigned int flags;
	} *priv;

I think we would have much less changes, also please rename bits
to flags and use some unsigned type for that

> +
> +enum fdarray_props {
> +	fdarray_prop__default	    = 0x00000000,
> +	fdarray_prop__nonfilterable = 0x00000001
>  };

s/fdarray_props/fdarray_flag/

>  
>  void fdarray__init(struct fdarray *fda, int nr_autogrow);
> @@ -33,7 +41,7 @@ void fdarray__exit(struct fdarray *fda);
>  struct fdarray *fdarray__new(int nr_alloc, int nr_autogrow);
>  void fdarray__delete(struct fdarray *fda);
>  
> -int fdarray__add(struct fdarray *fda, int fd, short revents);
> +int fdarray__add(struct fdarray *fda, int fd, short revents, enum fdarray_props props);
>  int fdarray__poll(struct fdarray *fda, int timeout);
>  int fdarray__filter(struct fdarray *fda, short revents,
>  		    void (*entry_destructor)(struct fdarray *fda, int fd, void *arg),
> diff --git a/tools/lib/perf/evlist.c b/tools/lib/perf/evlist.c
> index 6a875a0f01bb..25e76e458afb 100644

SNIP