* [PATCH v3] fs: Improve eventpoll logging to stop indicting timerfd
@ 2021-04-02 5:57 Manish Varma
2021-06-21 22:06 ` Manish Varma
0 siblings, 1 reply; 3+ messages in thread
From: Manish Varma @ 2021-04-02 5:57 UTC (permalink / raw)
To: Alexander Viro, Thomas Gleixner
Cc: linux-fsdevel, linux-kernel, kernel-team, Manish Varma,
kernel test robot, Kelly Rossmoyer
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@intel.com>
Co-developed-by: Kelly Rossmoyer <krossmo@google.com>
Signed-off-by: Kelly Rossmoyer <krossmo@google.com>
Signed-off-by: Manish Varma <varmam@google.com>
---
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
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v3] fs: Improve eventpoll logging to stop indicting timerfd
2021-04-02 5:57 [PATCH v3] fs: Improve eventpoll logging to stop indicting timerfd Manish Varma
@ 2021-06-21 22:06 ` Manish Varma
2022-01-10 12:44 ` Manish Varma
0 siblings, 1 reply; 3+ messages in thread
From: Manish Varma @ 2021-06-21 22:06 UTC (permalink / raw)
To: Alexander Viro, Thomas Gleixner
Cc: linux-fsdevel, linux-kernel, kernel-team, kernel test robot,
Kelly Rossmoyer
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@google.com> 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@intel.com>
> Co-developed-by: Kelly Rossmoyer <krossmo@google.com>
> Signed-off-by: Kelly Rossmoyer <krossmo@google.com>
> Signed-off-by: Manish Varma <varmam@google.com>
> ---
> 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@google.com | 650-686-0858
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v3] fs: Improve eventpoll logging to stop indicting timerfd
2021-06-21 22:06 ` Manish Varma
@ 2022-01-10 12:44 ` Manish Varma
0 siblings, 0 replies; 3+ messages in thread
From: Manish Varma @ 2022-01-10 12:44 UTC (permalink / raw)
To: Alexander Viro, Thomas Gleixner
Cc: linux-fsdevel, linux-kernel, kernel-team, kernel test robot,
Kelly Rossmoyer, Vijay Nayak
Hello Alexander and Thomas,
Friendly ping if you could share feedback (if any) to get this patch
merged.
Thanks,
Manish
On Tue, Jun 22, 2021 at 3:36 AM Manish Varma <varmam@google.com> wrote:
>
> 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@google.com> 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@intel.com>
> > Co-developed-by: Kelly Rossmoyer <krossmo@google.com>
> > Signed-off-by: Kelly Rossmoyer <krossmo@google.com>
> > Signed-off-by: Manish Varma <varmam@google.com>
> > ---
> > 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@google.com | 650-686-0858
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-01-10 12:45 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-02 5:57 [PATCH v3] fs: Improve eventpoll logging to stop indicting timerfd Manish Varma
2021-06-21 22:06 ` Manish Varma
2022-01-10 12:44 ` Manish Varma
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).