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:   Fri, 5 Jun 2020 13:38:34 +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 v7 01/13] tools/libperf: introduce notion of static
 polled file descriptors
Message-ID: <20200605113834.GC1404794@krava>
References: <5de4b954-24f0-1e8d-5a0d-7b12783b8218@linux.intel.com>
 <3c92a0ad-d7d3-4e78-f0b8-1d3a7122c69e@linux.intel.com>
 <20200605105051.GA1404794@krava>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200605105051.GA1404794@krava>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Jun 05, 2020 at 12:50:54PM +0200, Jiri Olsa wrote:
> On Wed, Jun 03, 2020 at 06:52:59PM +0300, Alexey Budankov wrote:
> > 
> > Implement adding of file descriptors by fdarray__add_stat() to
> > fix-sized (currently 1) stat_entries array located at struct fdarray.
> > Append added file descriptors to the array used by poll() syscall
> > during fdarray__poll() call. Copy poll() result of the added
> > descriptors from the array back to the storage for analysis.
> > 
> > Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
> > ---
> >  tools/lib/api/fd/array.c                 | 42 +++++++++++++++++++++++-
> >  tools/lib/api/fd/array.h                 |  7 ++++
> >  tools/lib/perf/evlist.c                  | 11 +++++++
> >  tools/lib/perf/include/internal/evlist.h |  2 ++
> >  4 files changed, 61 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/lib/api/fd/array.c b/tools/lib/api/fd/array.c
> > index 58d44d5eee31..b0027f2169c7 100644
> > --- a/tools/lib/api/fd/array.c
> > +++ b/tools/lib/api/fd/array.c
> > @@ -11,10 +11,16 @@
> >  
> >  void fdarray__init(struct fdarray *fda, int nr_autogrow)
> >  {
> > +	int i;
> > +
> >  	fda->entries	 = NULL;
> >  	fda->priv	 = NULL;
> >  	fda->nr		 = fda->nr_alloc = 0;
> >  	fda->nr_autogrow = nr_autogrow;
> > +
> > +	fda->nr_stat = 0;
> > +	for (i = 0; i < FDARRAY__STAT_ENTRIES_MAX; i++)
> > +		fda->stat_entries[i].fd = -1;
> >  }
> >  
> >  int fdarray__grow(struct fdarray *fda, int nr)
> > @@ -83,6 +89,20 @@ int fdarray__add(struct fdarray *fda, int fd, short revents)
> >  	return pos;
> >  }
> >  
> > +int fdarray__add_stat(struct fdarray *fda, int fd, short revents)
> > +{
> > +	int pos = fda->nr_stat;
> > +
> > +	if (pos >= FDARRAY__STAT_ENTRIES_MAX)
> > +		return -1;
> > +
> > +	fda->stat_entries[pos].fd = fd;
> > +	fda->stat_entries[pos].events = revents;
> > +	fda->nr_stat++;
> > +
> > +	return pos;
> > +}
> > +
> >  int fdarray__filter(struct fdarray *fda, short revents,
> >  		    void (*entry_destructor)(struct fdarray *fda, int fd, void *arg),
> >  		    void *arg)
> > @@ -113,7 +133,27 @@ int fdarray__filter(struct fdarray *fda, short revents,
> >  
> >  int fdarray__poll(struct fdarray *fda, int timeout)
> >  {
> > -	return poll(fda->entries, fda->nr, timeout);
> > +	int nr, i, pos, res;
> > +
> > +	nr = fda->nr;
> > +
> > +	for (i = 0; i < fda->nr_stat; i++) {
> > +		if (fda->stat_entries[i].fd != -1) {
> > +			pos = fdarray__add(fda, fda->stat_entries[i].fd,
> > +					   fda->stat_entries[i].events);
> 
> so every call to fdarray__poll will add whatever is
> in stat_entries to entries? how is it removed?
> 
> I think you should either follow what Adrian said
> and put 'static' descriptors early and check for
> filter number to match it as an 'quick fix'
> 
> or we should fix it for real and make it generic
> 
> so currently the interface is like this:
> 
>   pos1 = fdarray__add(a, fd1 ... );
>   pos2 = fdarray__add(a, fd2 ... );
>   pos3 = fdarray__add(a, fd2 ... );
> 
>   fdarray__poll(a);
> 
>   num = fdarray__filter(a, revents, destructor, arg);
> 
> when fdarray__filter removes some of the fds the 'pos1,pos2,pos3'
> indexes are not relevant anymore
> 
> how about we make the 'pos indexes' being stable by allocating
> separate object for each added descriptor and each poll call
> would create pollfd array from current objects, and entries
> would keep pointer to its pollfd entry
> 
>   struct fdentry *entry {
>        int              fd;
>        int              events;
>        struct pollfd   *pollfd;
>   }
> 
>   entry1 = fdarray__add(a, fd1 ...);
>   entry2 = fdarray__add(a, fd2 ...);
>   entry3 = fdarray__add(a, fd3 ...);
> 
>   fdarray__poll(a);
> 
>   struct pollfd *fdarray__entry_pollfd(a, entry1);
> 
> or smoething like that ;-)

maybe something like below (only compile tested)

jirka


---
diff --git a/tools/lib/api/fd/array.c b/tools/lib/api/fd/array.c
index 58d44d5eee31..f1effed3dde1 100644
--- a/tools/lib/api/fd/array.c
+++ b/tools/lib/api/fd/array.c
@@ -22,8 +22,8 @@ int fdarray__grow(struct fdarray *fda, int nr)
 	void *priv;
 	int nr_alloc = fda->nr_alloc + nr;
 	size_t psize = sizeof(fda->priv[0]) * nr_alloc;
-	size_t size  = sizeof(struct pollfd) * nr_alloc;
-	struct pollfd *entries = realloc(fda->entries, size);
+	size_t size  = sizeof(struct fdentry *) * nr_alloc;
+	struct fdentry **entries = realloc(fda->entries, size);
 
 	if (entries == NULL)
 		return -ENOMEM;
@@ -58,7 +58,12 @@ struct fdarray *fdarray__new(int nr_alloc, int nr_autogrow)
 
 void fdarray__exit(struct fdarray *fda)
 {
+	int i;
+
+	for (i = 0; i < fda->nr; i++)
+		free(fda->entries[i]);
 	free(fda->entries);
+	free(fda->pollfd);
 	free(fda->priv);
 	fdarray__init(fda, 0);
 }
@@ -69,18 +74,25 @@ void fdarray__delete(struct fdarray *fda)
 	free(fda);
 }
 
-int fdarray__add(struct fdarray *fda, int fd, short revents)
+struct fdentry *fdarray__add(struct fdarray *fda, int fd, short revents)
 {
-	int pos = fda->nr;
+	struct fdentry *entry;
 
 	if (fda->nr == fda->nr_alloc &&
 	    fdarray__grow(fda, fda->nr_autogrow) < 0)
-		return -ENOMEM;
+		return NULL;
+
+	entry = malloc(sizeof(*entry));
+	if (!entry)
+		return NULL;
+
+	entry->fd = fd;
+	entry->revents = revents;
+	entry->pollfd = NULL;
 
-	fda->entries[fda->nr].fd     = fd;
-	fda->entries[fda->nr].events = revents;
+	fda->entries[fda->nr] = entry;
 	fda->nr++;
-	return pos;
+	return entry;
 }
 
 int fdarray__filter(struct fdarray *fda, short revents,
@@ -93,7 +105,7 @@ int fdarray__filter(struct fdarray *fda, short revents,
 		return 0;
 
 	for (fd = 0; fd < fda->nr; ++fd) {
-		if (fda->entries[fd].revents & revents) {
+		if (fda->entries[fd]->revents & revents) {
 			if (entry_destructor)
 				entry_destructor(fda, fd, arg);
 
@@ -113,7 +125,22 @@ int fdarray__filter(struct fdarray *fda, short revents,
 
 int fdarray__poll(struct fdarray *fda, int timeout)
 {
-	return poll(fda->entries, fda->nr, timeout);
+	struct pollfd *pollfd = fda->pollfd;
+	int i;
+
+	pollfd = realloc(pollfd, sizeof(*pollfd) * fda->nr);
+	if (!pollfd)
+		return -ENOMEM;
+
+	fda->pollfd = pollfd;
+
+	for (i = 0; i < fda->nr; i++) {
+		pollfd[i].fd = fda->entries[i]->fd;
+		pollfd[i].revents = fda->entries[i]->revents;
+		fda->entries[i]->pollfd = &pollfd[i];
+	}
+
+	return poll(pollfd, fda->nr, timeout);
 }
 
 int fdarray__fprintf(struct fdarray *fda, FILE *fp)
@@ -121,7 +148,12 @@ int fdarray__fprintf(struct fdarray *fda, FILE *fp)
 	int fd, printed = fprintf(fp, "%d [ ", fda->nr);
 
 	for (fd = 0; fd < fda->nr; ++fd)
-		printed += fprintf(fp, "%s%d", fd ? ", " : "", fda->entries[fd].fd);
+		printed += fprintf(fp, "%s%d", fd ? ", " : "", fda->entries[fd]->fd);
 
 	return printed + fprintf(fp, " ]");
 }
+
+int fdentry__events(struct fdentry *entry)
+{
+	return entry->pollfd->revents;
+}
diff --git a/tools/lib/api/fd/array.h b/tools/lib/api/fd/array.h
index b39557d1a88f..5231ce047f2e 100644
--- a/tools/lib/api/fd/array.h
+++ b/tools/lib/api/fd/array.h
@@ -6,6 +6,12 @@
 
 struct pollfd;
 
+struct fdentry {
+	int		 fd;
+	int		 revents;
+	struct pollfd	*pollfd;
+};
+
 /**
  * struct fdarray: Array of file descriptors
  *
@@ -20,7 +26,10 @@ struct fdarray {
 	int	       nr;
 	int	       nr_alloc;
 	int	       nr_autogrow;
-	struct pollfd *entries;
+
+	struct fdentry	**entries;
+	struct pollfd	 *pollfd;
+
 	union {
 		int    idx;
 		void   *ptr;
@@ -33,7 +42,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);
+struct fdentry *fdarray__add(struct fdarray *fda, int fd, short revents);
 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),
@@ -41,6 +50,8 @@ int fdarray__filter(struct fdarray *fda, short revents,
 int fdarray__grow(struct fdarray *fda, int extra);
 int fdarray__fprintf(struct fdarray *fda, FILE *fp);
 
+int fdentry__events(struct fdentry *entry);
+
 static inline int fdarray__available_entries(struct fdarray *fda)
 {
 	return fda->nr_alloc - fda->nr;