Re: [PATCH v3] fs: Improve eventpoll logging to stop indicting timerfd

[Manish Varma]

Hello Alexander and Thomas,

Please share if you have any further feedback on this patch, or if
there's any other action required from my end to before this gets
merged.

Thanks,
Manish

On Thu, Apr 1, 2021 at 10:57 PM Manish Varma <varmam@xxxxxxxxxx> 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).
>
> Reported-by: kernel test robot <lkp@xxxxxxxxx>
> Co-developed-by: Kelly Rossmoyer <krossmo@xxxxxxxxxx>
> Signed-off-by: Kelly Rossmoyer <krossmo@xxxxxxxxxx>
> Signed-off-by: Manish Varma <varmam@xxxxxxxxxx>
> ---
>  drivers/base/power/wakeup.c | 10 ++++++++--
>  fs/eventpoll.c              | 10 ++++++++--
>  include/linux/pm_wakeup.h   |  4 ++--
>  3 files changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index 01057f640233..3628536c67a5 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -216,13 +216,19 @@ EXPORT_SYMBOL_GPL(wakeup_source_remove);
>  /**
>   * wakeup_source_register - Create wakeup source and add it to the list.
>   * @dev: Device this wakeup source is associated with (or NULL if virtual).
> - * @name: Name of the wakeup source to register.
> + * @fmt: format string for the wakeup source name
>   */
>  struct wakeup_source *wakeup_source_register(struct device *dev,
> -                                            const char *name)
> +                                            const char *fmt, ...)
>  {
>         struct wakeup_source *ws;
>         int ret;
> +       char name[128];
> +       va_list args;
> +
> +       va_start(args, fmt);
> +       vsnprintf(name, sizeof(name), fmt, args);
> +       va_end(args);
>
>         ws = wakeup_source_create(name);
>         if (ws) {
> diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> index 7df8c0fa462b..7c35987a8887 100644
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
> @@ -312,6 +312,7 @@ struct ctl_table epoll_table[] = {
>  };
>  #endif /* CONFIG_SYSCTL */
>
> +static atomic_t wakesource_create_id  = ATOMIC_INIT(0);
>  static const struct file_operations eventpoll_fops;
>
>  static inline int is_file_epoll(struct file *f)
> @@ -1451,15 +1452,20 @@ static int ep_create_wakeup_source(struct epitem *epi)
>  {
>         struct name_snapshot n;
>         struct wakeup_source *ws;
> +       pid_t task_pid;
> +       int id;
> +
> +       task_pid = task_pid_nr(current);
>
>         if (!epi->ep->ws) {
> -               epi->ep->ws = wakeup_source_register(NULL, "eventpoll");
> +               epi->ep->ws = wakeup_source_register(NULL, "epoll:%d", task_pid);
>                 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);
> +       ws = wakeup_source_register(NULL, "epollitem%d:%d.%s", id, task_pid, n.name.name);
>         release_dentry_name_snapshot(&n);
>
>         if (!ws)
> diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h
> index aa3da6611533..cb91c84f6f08 100644
> --- a/include/linux/pm_wakeup.h
> +++ b/include/linux/pm_wakeup.h
> @@ -95,7 +95,7 @@ extern void wakeup_source_destroy(struct wakeup_source *ws);
>  extern void wakeup_source_add(struct wakeup_source *ws);
>  extern void wakeup_source_remove(struct wakeup_source *ws);
>  extern struct wakeup_source *wakeup_source_register(struct device *dev,
> -                                                   const char *name);
> +                                                   const char *fmt, ...);
>  extern void wakeup_source_unregister(struct wakeup_source *ws);
>  extern int wakeup_sources_read_lock(void);
>  extern void wakeup_sources_read_unlock(int idx);
> @@ -137,7 +137,7 @@ static inline void wakeup_source_add(struct wakeup_source *ws) {}
>  static inline void wakeup_source_remove(struct wakeup_source *ws) {}
>
>  static inline struct wakeup_source *wakeup_source_register(struct device *dev,
> -                                                          const char *name)
> +                                                          const char *fmt, ...)
>  {
>         return NULL;
>  }
> --
> 2.31.0.208.g409f899ff0-goog
>


-- 
Manish Varma | Software Engineer | varmam@xxxxxxxxxx | 650-686-0858