PM / wakeup: use seq_open() to show wakeup stats

PATCH Superseded Ganesh Mahendran 4 participants 12 messages Mar 5, 2018 – Apr 25, 2018

Subsystems (4)

Driver core Suspend to RAM Hibernation Power management core

lore

Share & tools

lore

Ganesh Mahendran

author

2018-03-05 08:47 UTC #0 lore raw

PATCH v2 1/1 PM / wakeup: use seq_open() to show wakeup stats

single_open() interface requires that the whole output must fit into a single buffer. This will lead to timeout when system memory is not in a good situation.

This patch use seq_open() to show wakeup stats. This method need only one page, so timeout will not be observed.

Signed-off-by Ganesh Mahendran <[email protected]>

----
v2: use srcu_read_lock instead of rcu_read_lock

---
 drivers/base/power/wakeup.c | 77 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 61 insertions(+), 16 deletions(-)

drivers/base/power/wakeup.c +61 ~~-16~~

index ea01621..3bcab7d 100644
@@ -1029,32 +1029,77 @@ static int print_wakeup_source_stats(struct seq_file *m,
 	return 0;
 }
 
-/**
- * wakeup_sources_stats_show - Print wakeup sources statistics information.
- * @m: seq_file to print the statistics into.
- */
-static int wakeup_sources_stats_show(struct seq_file *m, void *unused)
+static void *wakeup_sources_stats_seq_start(struct seq_file *m,
+					loff_t *pos)
 {
 	struct wakeup_source *ws;
-	int srcuidx;
+	loff_t n = *pos;
+	int *srcuidx = m->private;
 
-	seq_puts(m, "name\t\tactive_count\tevent_count\twakeup_count\t"
-		"expire_count\tactive_since\ttotal_time\tmax_time\t"
-		"last_change\tprevent_suspend_time\n");
+	if (n == 0) {
+		seq_puts(m, "name\t\tactive_count\tevent_count\twakeup_count\t"
+			"expire_count\tactive_since\ttotal_time\tmax_time\t"
+			"last_change\tprevent_suspend_time\n");
+	}
 
-	srcuidx = srcu_read_lock(&wakeup_srcu);
-	list_for_each_entry_rcu(ws, &wakeup_sources, entry)
-		print_wakeup_source_stats(m, ws);
-	srcu_read_unlock(&wakeup_srcu, srcuidx);
+	*srcuidx = srcu_read_lock(&wakeup_srcu);
+	list_for_each_entry_rcu(ws, &wakeup_sources, entry) {
+		if (n-- > 0)
+			continue;
+		goto out;
+	}
+	ws = NULL;
+out:
+	return ws;
+}
+
+static void *wakeup_sources_stats_seq_next(struct seq_file *m,
+					void *v, loff_t *pos)
+{
+	struct wakeup_source *ws = v;
+	struct wakeup_source *next_ws = NULL;
+
+	++(*pos);
 
-	print_wakeup_source_stats(m, &deleted_ws);
+	list_for_each_entry_continue_rcu(ws, &wakeup_sources, entry) {
+		next_ws = ws;
+		break;
+	}
+
+	return next_ws;
+}
+
+static void wakeup_sources_stats_seq_stop(struct seq_file *m, void *v)
+{
+	int *srcuidx = m->private;
+
+	srcu_read_unlock(&wakeup_srcu, *srcuidx);
+}
+
+/**
+ * wakeup_sources_stats_seq_show - Print wakeup sources statistics information.
+ * @m: seq_file to print the statistics into.
+ * @v: wakeup_source of each iteration
+ */
+static int wakeup_sources_stats_seq_show(struct seq_file *m, void *v)
+{
+	struct wakeup_source *ws = v;
+
+	print_wakeup_source_stats(m, ws);
 
 	return 0;
 }
 
+static const struct seq_operations wakeup_sources_stats_seq_ops = {
+	.start = wakeup_sources_stats_seq_start,
+	.next  = wakeup_sources_stats_seq_next,
+	.stop  = wakeup_sources_stats_seq_stop,
+	.show  = wakeup_sources_stats_seq_show,
+};
+
 static int wakeup_sources_stats_open(struct inode *inode, struct file *file)
 {
-	return single_open(file, wakeup_sources_stats_show, NULL);
+	return seq_open_private(file, &wakeup_sources_stats_seq_ops, sizeof(int));
 }
 
 static const struct file_operations wakeup_sources_stats_fops = {

@@ -1062,7 +1107,7 @@ static int wakeup_sources_stats_open(struct inode *inode, struct file *file)
 	.open = wakeup_sources_stats_open,
 	.read = seq_read,
 	.llseek = seq_lseek,
-	.release = single_release,
+	.release = seq_release_private,
 };
 
 static int __init wakeup_sources_debugfs_init(void)

Signature · 1 line

1.9.1

Andy Shevchenko

2018-03-13 16:39 UTC #2 lore raw

In reply to Ganesh Mahendran

drivers/base/power/wakeup.c static int print_wakeup_source_stats(struct seq_file *m, · line 1043

Quoted 12 lines · Ganesh Mahendran

single_open() interface requires that the whole output must fit into a single buffer. This will lead to timeout when system memory is not in a good situation.
This patch use seq_open() to show wakeup stats. This method need only one page, so timeout will not be observed.
drivers/base/power/wakeup.c +5 -0
+       if (n == 0) {
+               seq_puts(m, "name\t\tactive_count\tevent_count\twakeup_count\t"
+                       "expire_count\tactive_since\ttotal_time\tmax_time\t"
+                       "last_change\tprevent_suspend_time\n");
+       }

Can't you do this once at ->open() stage, for example?

Quoted 5 lines

static int wakeup_sources_stats_open(struct inode *inode, struct file *file) {
drivers/base/power/wakeup.c +1 -1
-       return single_open(file, wakeup_sources_stats_show, NULL);
+       return seq_open_private(file, &wakeup_sources_stats_seq_ops, sizeof(int));
 }

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Ganesh Mahendran

author

2018-03-14 01:33 UTC #3 lore raw

Hi, Andy

2018-03-14 0:39 GMT+08:00 Andy Shevchenko <[email protected]>:

Quoted 9 lines

Quoted 6 lines · Ganesh Mahendran
single_open() interface requires that the whole output must fit into a single buffer. This will lead to timeout when system memory is not in a good situation.
This patch use seq_open() to show wakeup stats. This method need only one page, so timeout will not be observed.

Quoted 7 lines

drivers/base/power/wakeup.c static int print_wakeup_source_stats(struct seq_file *m, · line 1043
Quoted 5 lines
drivers/base/power/wakeup.c +5 -0
+       if (n == 0) {
+               seq_puts(m, "name\t\tactive_count\tevent_count\twakeup_count\t"
+                       "expire_count\tactive_since\ttotal_time\tmax_time\t"
+                       "last_change\tprevent_suspend_time\n");
+       }
Can't you do this once at ->open() stage, for example?

We can not put this at ->open(). Because in seq_open(), the buffer is not ready,
the seq buffer is allocated in seq_read().

Thanks.

Quoted 10 lines

drivers/base/power/wakeup.c static int print_wakeup_source_stats(struct seq_file *m, · line 1103
Quoted 6 lines
static int wakeup_sources_stats_open(struct inode *inode, struct file *file) {
drivers/base/power/wakeup.c +1 -1
-       return single_open(file, wakeup_sources_stats_show, NULL);
+       return seq_open_private(file, &wakeup_sources_stats_seq_ops, sizeof(int));
 }
--
With Best Regards,
Andy Shevchenko

Rafael J. Wysocki

2018-03-29 21:54 UTC #5 lore raw

Quoted 7 lines · CEST Ganesh Mahendran

ping.
2018-03-05 16:47 GMT+08:00 Ganesh Mahendran <[email protected]>:
Quoted 4 lines
single_open() interface requires that the whole output must fit into a single buffer. This will lead to timeout when system memory is not in a good situation.

Quoted 5 lines

Quoted 2 lines
This patch use seq_open() to show wakeup stats. This method need only one page, so timeout will not be observed.
We have resolved the watchdog timeout issue by this patch. Please help to review.

Sorry for the delay.

I'll have a look tomorrow if possible.

Thanks!

Rafael J. Wysocki

2018-03-30 10:25 UTC #6 lore raw

In reply to Ganesh Mahendran

Quoted 48 lines · CEST Ganesh Mahendran

single_open() interface requires that the whole output must fit into a single buffer. This will lead to timeout when system memory is not in a good situation.

This patch use seq_open() to show wakeup stats. This method need only one page, so timeout will not be observed.

Signed-off-by Ganesh Mahendran <[email protected]>

----
v2: use srcu_read_lock instead of rcu_read_lock

---
 drivers/base/power/wakeup.c | 77 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 61 insertions(+), 16 deletions(-)

drivers/base/power/wakeup.c +9 ~~-13~~

index ea01621..3bcab7d 100644
@@ -1029,32 +1029,77 @@ static int print_wakeup_source_stats(struct seq_file *m,
 	return 0;
 }
 
-/**
- * wakeup_sources_stats_show - Print wakeup sources statistics information.
- * @m: seq_file to print the statistics into.
- */
-static int wakeup_sources_stats_show(struct seq_file *m, void *unused)
+static void *wakeup_sources_stats_seq_start(struct seq_file *m,
+					loff_t *pos)
 {
 	struct wakeup_source *ws;
-	int srcuidx;
+	loff_t n = *pos;
+	int *srcuidx = m->private;
 
-	seq_puts(m, "name\t\tactive_count\tevent_count\twakeup_count\t"
-		"expire_count\tactive_since\ttotal_time\tmax_time\t"
-		"last_change\tprevent_suspend_time\n");
+	if (n == 0) {
+		seq_puts(m, "name\t\tactive_count\tevent_count\twakeup_count\t"
+			"expire_count\tactive_since\ttotal_time\tmax_time\t"
+			"last_change\tprevent_suspend_time\n");
+	}
 
-	srcuidx = srcu_read_lock(&wakeup_srcu);
-	list_for_each_entry_rcu(ws, &wakeup_sources, entry)
-		print_wakeup_source_stats(m, ws);
-	srcu_read_unlock(&wakeup_srcu, srcuidx);

drivers/base/power/wakeup.c static int print_wakeup_source_stats(struct seq_file *m, · line 1054

Quoted 10 lines

drivers/base/power/wakeup.c +10 -0

+	*srcuidx = srcu_read_lock(&wakeup_srcu);
+	list_for_each_entry_rcu(ws, &wakeup_sources, entry) {
+		if (n-- > 0)
+			continue;
+		goto out;
+	}
+	ws = NULL;
+out:
+	return ws;
+}

Please clean up the above at least.

If I'm not mistaken, you don't need the label and the goto here.

Geert Uytterhoeven

2018-03-30 11:00 UTC #7 lore raw

Quoted 62 lines · Rafael J. Wysocki

Quoted 48 lines · CEST Ganesh Mahendran

single_open() interface requires that the whole output must fit into a single buffer. This will lead to timeout when system memory is not in a good situation.

This patch use seq_open() to show wakeup stats. This method need only one page, so timeout will not be observed.

Signed-off-by Ganesh Mahendran <[email protected]>

----
v2: use srcu_read_lock instead of rcu_read_lock

---
 drivers/base/power/wakeup.c | 77 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 61 insertions(+), 16 deletions(-)

drivers/base/power/wakeup.c +9 ~~-13~~

index ea01621..3bcab7d 100644
@@ -1029,32 +1029,77 @@ static int print_wakeup_source_stats(struct seq_file *m,
      return 0;
 }
 
-/**
- * wakeup_sources_stats_show - Print wakeup sources statistics information.
- * @m: seq_file to print the statistics into.
- */
-static int wakeup_sources_stats_show(struct seq_file *m, void *unused)
+static void *wakeup_sources_stats_seq_start(struct seq_file *m,
+                                     loff_t *pos)
 {
      struct wakeup_source *ws;
-     int srcuidx;
+     loff_t n = *pos;
+     int *srcuidx = m->private;
 
-     seq_puts(m, "name\t\tactive_count\tevent_count\twakeup_count\t"
-             "expire_count\tactive_since\ttotal_time\tmax_time\t"
-             "last_change\tprevent_suspend_time\n");
+     if (n == 0) {
+             seq_puts(m, "name\t\tactive_count\tevent_count\twakeup_count\t"
+                     "expire_count\tactive_since\ttotal_time\tmax_time\t"
+                     "last_change\tprevent_suspend_time\n");
+     }
 
-     srcuidx = srcu_read_lock(&wakeup_srcu);
-     list_for_each_entry_rcu(ws, &wakeup_sources, entry)
-             print_wakeup_source_stats(m, ws);
-     srcu_read_unlock(&wakeup_srcu, srcuidx);

drivers/base/power/wakeup.c static int print_wakeup_source_stats(struct seq_file *m, · line 1054

Quoted 10 lines

drivers/base/power/wakeup.c +10 -0

+     *srcuidx = srcu_read_lock(&wakeup_srcu);
+     list_for_each_entry_rcu(ws, &wakeup_sources, entry) {
+             if (n-- > 0)
+                     continue;
+             goto out;
+     }
+     ws = NULL;
+out:
+     return ws;
+}

Please clean up the above at least.

Quoted 1 line

If I'm not mistaken, you don't need the label and the goto here.

The continue is also not needed, if the test condition is inverted.

Gr{oetje,eeting}s,

Geert

Signature · 5 lines

Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Ganesh Mahendran

author

2018-04-02 01:31 UTC #11 lore raw

In reply to Rafael J. Wysocki

2018-03-30 18:25 GMT+08:00 Rafael J. Wysocki <[email protected]>:

Quoted 45 lines · CEST Ganesh Mahendran

Quoted 44 lines

single_open() interface requires that the whole output must fit into a single buffer. This will lead to timeout when system memory is not in a good situation.

This patch use seq_open() to show wakeup stats. This method need only one page, so timeout will not be observed.

Signed-off-by Ganesh Mahendran <[email protected]>

----
v2: use srcu_read_lock instead of rcu_read_lock

---
 drivers/base/power/wakeup.c | 77 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 61 insertions(+), 16 deletions(-)

drivers/base/power/wakeup.c +9 -9

index ea01621..3bcab7d 100644
@@ -1029,32 +1029,77 @@ static int print_wakeup_source_stats(struct seq_file *m,
      return 0;
 }
 
-/**
- * wakeup_sources_stats_show - Print wakeup sources statistics information.
- * @m: seq_file to print the statistics into.
- */
-static int wakeup_sources_stats_show(struct seq_file *m, void *unused)
+static void *wakeup_sources_stats_seq_start(struct seq_file *m,
+                                     loff_t *pos)
 {
      struct wakeup_source *ws;
-     int srcuidx;
+     loff_t n = *pos;
+     int *srcuidx = m->private;
 
-     seq_puts(m, "name\t\tactive_count\tevent_count\twakeup_count\t"
-             "expire_count\tactive_since\ttotal_time\tmax_time\t"
-             "last_change\tprevent_suspend_time\n");
+     if (n == 0) {
+             seq_puts(m, "name\t\tactive_count\tevent_count\twakeup_count\t"
+                     "expire_count\tactive_since\ttotal_time\tmax_time\t"
+                     "last_change\tprevent_suspend_time\n");
+     }

Quoted 16 lines

Quoted 4 lines

drivers/base/power/wakeup.c +0 -4

-     srcuidx = srcu_read_lock(&wakeup_srcu);
-     list_for_each_entry_rcu(ws, &wakeup_sources, entry)
-             print_wakeup_source_stats(m, ws);
-     srcu_read_unlock(&wakeup_srcu, srcuidx);

drivers/base/power/wakeup.c static int print_wakeup_source_stats(struct seq_file *m, · line 1054

Quoted 10 lines

drivers/base/power/wakeup.c +10 -0

+     *srcuidx = srcu_read_lock(&wakeup_srcu);
+     list_for_each_entry_rcu(ws, &wakeup_sources, entry) {
+             if (n-- > 0)
+                     continue;
+             goto out;
+     }
+     ws = NULL;
+out:
+     return ws;
+}

Please clean up the above at least.

Hi, Rafael

When length of file "wakeup_sources" is larger than 1 page, wakeup_sources_stats_seq_start()
may be called more then 1 time if the user space wants to read all of the file. So we need to locate to last read item, if it is not the first time to read the file.

We can see the same logic in kmemleak_seq_start().

Thanks.

Quoted 3 lines

If I'm not mistaken, you don't need the label and the goto here.

Ganesh Mahendran

author

2018-04-02 01:33 UTC #8 lore raw

In reply to Geert Uytterhoeven

2018-03-30 19:00 GMT+08:00 Geert Uytterhoeven <[email protected]>:

Quoted 63 lines · Rafael J. Wysocki

Quoted 62 lines · CEST Ganesh Mahendran

Quoted 48 lines

single_open() interface requires that the whole output must fit into a single buffer. This will lead to timeout when system memory is not in a good situation.

This patch use seq_open() to show wakeup stats. This method need only one page, so timeout will not be observed.

Signed-off-by Ganesh Mahendran <[email protected]>

----
v2: use srcu_read_lock instead of rcu_read_lock

---
 drivers/base/power/wakeup.c | 77 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 61 insertions(+), 16 deletions(-)

drivers/base/power/wakeup.c +9 ~~-13~~

index ea01621..3bcab7d 100644
@@ -1029,32 +1029,77 @@ static int print_wakeup_source_stats(struct seq_file *m,
      return 0;
 }
 
-/**
- * wakeup_sources_stats_show - Print wakeup sources statistics information.
- * @m: seq_file to print the statistics into.
- */
-static int wakeup_sources_stats_show(struct seq_file *m, void *unused)
+static void *wakeup_sources_stats_seq_start(struct seq_file *m,
+                                     loff_t *pos)
 {
      struct wakeup_source *ws;
-     int srcuidx;
+     loff_t n = *pos;
+     int *srcuidx = m->private;
 
-     seq_puts(m, "name\t\tactive_count\tevent_count\twakeup_count\t"
-             "expire_count\tactive_since\ttotal_time\tmax_time\t"
-             "last_change\tprevent_suspend_time\n");
+     if (n == 0) {
+             seq_puts(m, "name\t\tactive_count\tevent_count\twakeup_count\t"
+                     "expire_count\tactive_since\ttotal_time\tmax_time\t"
+                     "last_change\tprevent_suspend_time\n");
+     }
 
-     srcuidx = srcu_read_lock(&wakeup_srcu);
-     list_for_each_entry_rcu(ws, &wakeup_sources, entry)
-             print_wakeup_source_stats(m, ws);
-     srcu_read_unlock(&wakeup_srcu, srcuidx);

drivers/base/power/wakeup.c static int print_wakeup_source_stats(struct seq_file *m, · line 1054

Quoted 10 lines

drivers/base/power/wakeup.c +10 -0

+     *srcuidx = srcu_read_lock(&wakeup_srcu);
+     list_for_each_entry_rcu(ws, &wakeup_sources, entry) {
+             if (n-- > 0)
+                     continue;
+             goto out;
+     }
+     ws = NULL;
+out:
+     return ws;
+}

Please clean up the above at least.

Quoted 3 lines

Quoted 1 line
If I'm not mistaken, you don't need the label and the goto here.
The continue is also not needed, if the test condition is inverted.

Hi, Geert

We need to locate to the last read item. What is your suggestion here?

Thanks.

Quoted 11 lines

Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Geert Uytterhoeven

2018-04-02 06:46 UTC #9 lore raw

Hi Ganesh,

Quoted 37 lines · Ganesh Mahendran

2018-03-30 19:00 GMT+08:00 Geert Uytterhoeven <[email protected]>:
Quoted 36 lines · Rafael J. Wysocki
Quoted 35 lines · CEST Ganesh Mahendran
Quoted 21 lines
single_open() interface requires that the whole output must fit into a single buffer. This will lead to timeout when system memory is not in a good situation.
This patch use seq_open() to show wakeup stats. This method need only one page, so timeout will not be observed.
drivers/base/power/wakeup.c +2 -5
@@ -1029,32 +1029,77 @@ static int print_wakeup_source_stats(struct seq_file *m,
      return 0;
 }
 
-/**
- * wakeup_sources_stats_show - Print wakeup sources statistics information.
- * @m: seq_file to print the statistics into.
- */
-static int wakeup_sources_stats_show(struct seq_file *m, void *unused)
+static void *wakeup_sources_stats_seq_start(struct seq_file *m,
+                                     loff_t *pos)
 {
drivers/base/power/wakeup.c static int print_wakeup_source_stats(struct seq_file *m, · line 1044
Quoted 10 lines
drivers/base/power/wakeup.c +9 -0
 
+     list_for_each_entry_rcu(ws, &wakeup_sources, entry) {
+             if (n-- > 0)
+                     continue;
+             goto out;
+     }
+     ws = NULL;
+out:
+     return ws;
+}
Please clean up the above at least.

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
If I'm not mistaken, you don't need the label and the goto here.
The continue is also not needed, if the test condition is inverted.
Hi, Geert
We need to locate to the last read item. What is your suggestion here?

I didn't mean to get rid of that logic, but to reorganize the code to make it simpler:

        list_for_each_entry_rcu(ws, &wakeup_sources, entry) {
                if (n-- <= 0)
                        return ws;
        }

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Ganesh Mahendran

author

2018-04-25 11:01 UTC #10 lore raw

2018-04-02 14:46 GMT+08:00 Geert Uytterhoeven <[email protected]>:

Quoted 49 lines

Hi Ganesh,
Quoted 37 lines · Ganesh Mahendran
2018-03-30 19:00 GMT+08:00 Geert Uytterhoeven <[email protected]>:
Quoted 36 lines · Rafael J. Wysocki
Quoted 35 lines · CEST Ganesh Mahendran
Quoted 21 lines
single_open() interface requires that the whole output must fit into a single buffer. This will lead to timeout when system memory is not in a good situation.
This patch use seq_open() to show wakeup stats. This method need only one page, so timeout will not be observed.
drivers/base/power/wakeup.c +2 -5
@@ -1029,32 +1029,77 @@ static int print_wakeup_source_stats(struct seq_file *m,
      return 0;
 }
 
-/**
- * wakeup_sources_stats_show - Print wakeup sources statistics information.
- * @m: seq_file to print the statistics into.
- */
-static int wakeup_sources_stats_show(struct seq_file *m, void *unused)
+static void *wakeup_sources_stats_seq_start(struct seq_file *m,
+                                     loff_t *pos)
 {
drivers/base/power/wakeup.c static int print_wakeup_source_stats(struct seq_file *m, · line 1044
Quoted 10 lines
drivers/base/power/wakeup.c +9 -0
 
+     list_for_each_entry_rcu(ws, &wakeup_sources, entry) {
+             if (n-- > 0)
+                     continue;
+             goto out;
+     }
+     ws = NULL;
+out:
+     return ws;
+}
Please clean up the above at least.
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
If I'm not mistaken, you don't need the label and the goto here.
The continue is also not needed, if the test condition is inverted.
Hi, Geert
We need to locate to the last read item. What is your suggestion here?

Quoted 7 lines

I didn't mean to get rid of that logic, but to reorganize the code to make it simpler:
        list_for_each_entry_rcu(ws, &wakeup_sources, entry) {
                if (n-- <= 0)
                        return ws;
        }

I send a v3 patch.
Thanks for your review.

Quoted 11 lines

Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds