diff mbox series

[v2] fs: Improve eventpoll logging to stop indicting timerfd

Message ID 20210401021645.2609047-1-varmam@google.com (mailing list archive)
State New, archived
Headers show
Series [v2] fs: Improve eventpoll logging to stop indicting timerfd | expand

Commit Message

Manish Varma April 1, 2021, 2:16 a.m. UTC 
timerfd doesn't create any wakelocks, but eventpoll can.  When it does,
it names them after the underlying file descriptor, and since all
timerfd file descriptors are named "[timerfd]" (which saves memory on
systems like desktops with potentially many timerfd instances), all
wakesources created as a result of using the eventpoll-on-timerfd idiom
are called... "[timerfd]".

However, it becomes impossible to tell which "[timerfd]" wakesource is
affliated with which process and hence troubleshooting is difficult.

This change addresses this problem by changing the way eventpoll
wakesources are named:

1) the top-level per-process eventpoll wakesource is now named "epoll:P"
(instead of just "eventpoll"), where P, is the PID of the creating
process.
2) individual per-underlying-filedescriptor eventpoll wakesources are
now named "epollitemN:P.F", where N is a unique ID token and P is PID
of the creating process and F is the name of the underlying file
descriptor.

All together that should be splitted up into a change to eventpoll and
timerfd (or other file descriptors).

Co-developed-by: Kelly Rossmoyer <krossmo@google.com>
Signed-off-by: Kelly Rossmoyer <krossmo@google.com>
Signed-off-by: Manish Varma <varmam@google.com>
---
 fs/eventpoll.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

Comments

Al Viro April 1, 2021, 2:28 a.m. UTC | #1 
On Wed, Mar 31, 2021 at 07:16:45PM -0700, Manish Varma wrote:
> timerfd doesn't create any wakelocks, but eventpoll can.  When it does,
> it names them after the underlying file descriptor, and since all
> timerfd file descriptors are named "[timerfd]" (which saves memory on
> systems like desktops with potentially many timerfd instances), all
> wakesources created as a result of using the eventpoll-on-timerfd idiom
> are called... "[timerfd]".
> 
> However, it becomes impossible to tell which "[timerfd]" wakesource is
> affliated with which process and hence troubleshooting is difficult.
> 
> This change addresses this problem by changing the way eventpoll
> wakesources are named:
> 
> 1) the top-level per-process eventpoll wakesource is now named "epoll:P"
> (instead of just "eventpoll"), where P, is the PID of the creating
> process.
> 2) individual per-underlying-filedescriptor eventpoll wakesources are
> now named "epollitemN:P.F", where N is a unique ID token and P is PID
> of the creating process and F is the name of the underlying file
> descriptor.
> 
> All together that should be splitted up into a change to eventpoll and
> timerfd (or other file descriptors).

FWIW, it smells like a variant of wakeup_source_register() that would
take printf format + arguments would be a good idea.  I.e. something
like

> +		snprintf(buf, sizeof(buf), "epoll:%d", task_pid);
> +		epi->ep->ws = wakeup_source_register(NULL, buf);

		... = wakeup_source_register(NULL, "epoll:%d", task_pid);

etc.
kernel test robot April 1, 2021, 6:44 a.m. UTC | #2 
Hi Manish,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.12-rc5 next-20210331]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Manish-Varma/fs-Improve-eventpoll-logging-to-stop-indicting-timerfd/20210401-101954
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git d19cc4bfbff1ae72c3505a00fb8ce0d3fa519e6c
config: um-randconfig-r006-20210401 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/49789288695d81f196d99fadd89b5101b26bca8b
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Manish-Varma/fs-Improve-eventpoll-logging-to-stop-indicting-timerfd/20210401-101954
        git checkout 49789288695d81f196d99fadd89b5101b26bca8b
        # save the attached .config to linux build tree
        make W=1 ARCH=um 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   cc1: warning: arch/um/include/uapi: No such file or directory [-Wmissing-include-dirs]
   fs/eventpoll.c: In function 'ep_create_wakeup_source':
>> fs/eventpoll.c:1375:26: error: 'wakesource_create_id' undeclared (first use in this function); did you mean 'wakeup_source_create'?
    1375 |  id = atomic_inc_return(&wakesource_create_id);
         |                          ^~~~~~~~~~~~~~~~~~~~
         |                          wakeup_source_create
   fs/eventpoll.c:1375:26: note: each undeclared identifier is reported only once for each function it appears in


vim +1375 fs/eventpoll.c

  1357	
  1358	static int ep_create_wakeup_source(struct epitem *epi)
  1359	{
  1360		struct name_snapshot n;
  1361		struct wakeup_source *ws;
  1362		pid_t task_pid;
  1363		char buf[64];
  1364		int id;
  1365	
  1366		task_pid = task_pid_nr(current);
  1367	
  1368		if (!epi->ep->ws) {
  1369			snprintf(buf, sizeof(buf), "epoll:%d", task_pid);
  1370			epi->ep->ws = wakeup_source_register(NULL, buf);
  1371			if (!epi->ep->ws)
  1372				return -ENOMEM;
  1373		}
  1374	
> 1375		id = atomic_inc_return(&wakesource_create_id);
  1376		take_dentry_name_snapshot(&n, epi->ffd.file->f_path.dentry);
  1377		snprintf(buf, sizeof(buf), "epollitem%d:%d.%s", id, task_pid, n.name.name);
  1378		ws = wakeup_source_register(NULL, buf);
  1379		release_dentry_name_snapshot(&n);
  1380	
  1381		if (!ws)
  1382			return -ENOMEM;
  1383		rcu_assign_pointer(epi->ws, ws);
  1384	
  1385		return 0;
  1386	}
  1387	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Manish Varma April 1, 2021, 11:57 p.m. UTC | #3 
Hi Al,

On Wed, Mar 31, 2021 at 7:29 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Wed, Mar 31, 2021 at 07:16:45PM -0700, Manish Varma wrote:
> > timerfd doesn't create any wakelocks, but eventpoll can.  When it does,
> > it names them after the underlying file descriptor, and since all
> > timerfd file descriptors are named "[timerfd]" (which saves memory on
> > systems like desktops with potentially many timerfd instances), all
> > wakesources created as a result of using the eventpoll-on-timerfd idiom
> > are called... "[timerfd]".
> >
> > However, it becomes impossible to tell which "[timerfd]" wakesource is
> > affliated with which process and hence troubleshooting is difficult.
> >
> > This change addresses this problem by changing the way eventpoll
> > wakesources are named:
> >
> > 1) the top-level per-process eventpoll wakesource is now named "epoll:P"
> > (instead of just "eventpoll"), where P, is the PID of the creating
> > process.
> > 2) individual per-underlying-filedescriptor eventpoll wakesources are
> > now named "epollitemN:P.F", where N is a unique ID token and P is PID
> > of the creating process and F is the name of the underlying file
> > descriptor.
> >
> > All together that should be splitted up into a change to eventpoll and
> > timerfd (or other file descriptors).
>
> FWIW, it smells like a variant of wakeup_source_register() that would
> take printf format + arguments would be a good idea.  I.e. something
> like
>
> > +             snprintf(buf, sizeof(buf), "epoll:%d", task_pid);
> > +             epi->ep->ws = wakeup_source_register(NULL, buf);
>
>                 ... = wakeup_source_register(NULL, "epoll:%d", task_pid);
>
> etc.

Noted. I will fix this in the v3 patch.

Thanks,
Manish
diff mbox series

Patch

diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 7df8c0fa462b..8d3369a02633 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -297,6 +297,7 @@  static LIST_HEAD(tfile_check_list);
 
 static long long_zero;
 static long long_max = LONG_MAX;
+static atomic_t wakesource_create_id  = ATOMIC_INIT(0);
 
 struct ctl_table epoll_table[] = {
 	{
@@ -1451,15 +1452,23 @@  static int ep_create_wakeup_source(struct epitem *epi)
 {
 	struct name_snapshot n;
 	struct wakeup_source *ws;
+	pid_t task_pid;
+	char buf[64];
+	int id;
+
+	task_pid = task_pid_nr(current);
 
 	if (!epi->ep->ws) {
-		epi->ep->ws = wakeup_source_register(NULL, "eventpoll");
+		snprintf(buf, sizeof(buf), "epoll:%d", task_pid);
+		epi->ep->ws = wakeup_source_register(NULL, buf);
 		if (!epi->ep->ws)
 			return -ENOMEM;
 	}
 
+	id = atomic_inc_return(&wakesource_create_id);
 	take_dentry_name_snapshot(&n, epi->ffd.file->f_path.dentry);
-	ws = wakeup_source_register(NULL, n.name.name);
+	snprintf(buf, sizeof(buf), "epollitem%d:%d.%s", id, task_pid, n.name.name);
+	ws = wakeup_source_register(NULL, buf);
 	release_dentry_name_snapshot(&n);
 
 	if (!ws)