LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v2] PM / wakeup: use seq_open() to show wakeup stats
@ 2018-03-05  8:47 Ganesh Mahendran
  2018-03-12  5:33 ` Ganesh Mahendran
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Ganesh Mahendran @ 2018-03-05  8:47 UTC (permalink / raw)
  To: rjw, pavel, len.brown, rafael, gregkh
  Cc: linux-pm, linux-kernel, 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 <opensource.ganesh@gmail.com>
----
v2: use srcu_read_lock instead of rcu_read_lock
---
 drivers/base/power/wakeup.c | 77 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 61 insertions(+), 16 deletions(-)

diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index ea01621..3bcab7d 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -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)
-- 
1.9.1

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] PM / wakeup: use seq_open() to show wakeup stats
  2018-03-05  8:47 [PATCH v2] PM / wakeup: use seq_open() to show wakeup stats Ganesh Mahendran
@ 2018-03-12  5:33 ` Ganesh Mahendran
  2018-03-13 16:39 ` Andy Shevchenko
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 12+ messages in thread
From: Ganesh Mahendran @ 2018-03-12  5:33 UTC (permalink / raw)
  To: Rafael J. Wysocki, Pavel Machek, Len Brown, Rafael J. Wysocki, Greg KH
  Cc: Linux PM, linux-kernel, Ganesh Mahendran

Hello, Rafael:

2018-03-05 16:47 GMT+08:00 Ganesh Mahendran <opensource.ganesh@gmail.com>:
> 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 <opensource.ganesh@gmail.com>
> ----
> v2: use srcu_read_lock instead of rcu_read_lock

How about the V2 patch?
If you have other concern, please let me know.

Thanks.

> ---
>  drivers/base/power/wakeup.c | 77 +++++++++++++++++++++++++++++++++++----------
>  1 file changed, 61 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index ea01621..3bcab7d 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -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)
> --
> 1.9.1
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] PM / wakeup: use seq_open() to show wakeup stats
  2018-03-05  8:47 [PATCH v2] PM / wakeup: use seq_open() to show wakeup stats Ganesh Mahendran
  2018-03-12  5:33 ` Ganesh Mahendran
@ 2018-03-13 16:39 ` Andy Shevchenko
  2018-03-14  1:33   ` Ganesh Mahendran
  2018-03-29  7:49 ` Ganesh Mahendran
  2018-03-30 10:25 ` Rafael J. Wysocki
  3 siblings, 1 reply; 12+ messages in thread
From: Andy Shevchenko @ 2018-03-13 16:39 UTC (permalink / raw)
  To: Ganesh Mahendran
  Cc: Rafael J. Wysocki, Pavel Machek, Brown, Len, Rafael J. Wysocki,
	Greg Kroah-Hartman, Linux PM, Linux Kernel Mailing List

On Mon, Mar 5, 2018 at 10:47 AM, Ganesh Mahendran
<opensource.ganesh@gmail.com> wrote:
> 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.

> +       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?

>  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));
>  }

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] PM / wakeup: use seq_open() to show wakeup stats
  2018-03-13 16:39 ` Andy Shevchenko
@ 2018-03-14  1:33   ` Ganesh Mahendran
  0 siblings, 0 replies; 12+ messages in thread
From: Ganesh Mahendran @ 2018-03-14  1:33 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Rafael J. Wysocki, Pavel Machek, Brown, Len, Rafael J. Wysocki,
	Greg Kroah-Hartman, Linux PM, Linux Kernel Mailing List

Hi, Andy

2018-03-14 0:39 GMT+08:00 Andy Shevchenko <andy.shevchenko@gmail.com>:
> On Mon, Mar 5, 2018 at 10:47 AM, Ganesh Mahendran
> <opensource.ganesh@gmail.com> wrote:
>> 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.
>
>> +       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.

>
>>  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));
>>  }
>
> --
> With Best Regards,
> Andy Shevchenko

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] PM / wakeup: use seq_open() to show wakeup stats
  2018-03-05  8:47 [PATCH v2] PM / wakeup: use seq_open() to show wakeup stats Ganesh Mahendran
  2018-03-12  5:33 ` Ganesh Mahendran
  2018-03-13 16:39 ` Andy Shevchenko
@ 2018-03-29  7:49 ` Ganesh Mahendran
  2018-03-29 21:54   ` Rafael J. Wysocki
  2018-03-30 10:25 ` Rafael J. Wysocki
  3 siblings, 1 reply; 12+ messages in thread
From: Ganesh Mahendran @ 2018-03-29  7:49 UTC (permalink / raw)
  To: Rafael J. Wysocki, Pavel Machek, Len Brown, Rafael J. Wysocki, Greg KH
  Cc: Linux PM, linux-kernel, Ganesh Mahendran

ping.

2018-03-05 16:47 GMT+08:00 Ganesh Mahendran <opensource.ganesh@gmail.com>:
> 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.

We have resolved the watchdog timeout issue by this patch.
Please help to review.

Thanks.

>
> Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
> ----
> v2: use srcu_read_lock instead of rcu_read_lock
> ---
>  drivers/base/power/wakeup.c | 77 +++++++++++++++++++++++++++++++++++----------
>  1 file changed, 61 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index ea01621..3bcab7d 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -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)
> --
> 1.9.1
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] PM / wakeup: use seq_open() to show wakeup stats
  2018-03-29  7:49 ` Ganesh Mahendran
@ 2018-03-29 21:54   ` Rafael J. Wysocki
  0 siblings, 0 replies; 12+ messages in thread
From: Rafael J. Wysocki @ 2018-03-29 21:54 UTC (permalink / raw)
  To: Ganesh Mahendran
  Cc: Pavel Machek, Len Brown, Rafael J. Wysocki, Greg KH, Linux PM,
	linux-kernel

On Thursday, March 29, 2018 9:49:43 AM CEST Ganesh Mahendran wrote:
> ping.
> 
> 2018-03-05 16:47 GMT+08:00 Ganesh Mahendran <opensource.ganesh@gmail.com>:
> > 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.
> 
> 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!

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] PM / wakeup: use seq_open() to show wakeup stats
  2018-03-05  8:47 [PATCH v2] PM / wakeup: use seq_open() to show wakeup stats Ganesh Mahendran
                   ` (2 preceding siblings ...)
  2018-03-29  7:49 ` Ganesh Mahendran
@ 2018-03-30 10:25 ` Rafael J. Wysocki
  2018-03-30 11:00   ` Geert Uytterhoeven
  2018-04-02  1:31   ` Ganesh Mahendran
  3 siblings, 2 replies; 12+ messages in thread
From: Rafael J. Wysocki @ 2018-03-30 10:25 UTC (permalink / raw)
  To: Ganesh Mahendran; +Cc: pavel, len.brown, rafael, gregkh, linux-pm, linux-kernel

On Monday, March 5, 2018 9:47:46 AM CEST Ganesh Mahendran wrote:
> 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 <opensource.ganesh@gmail.com>
> ----
> v2: use srcu_read_lock instead of rcu_read_lock
> ---
>  drivers/base/power/wakeup.c | 77 +++++++++++++++++++++++++++++++++++----------
>  1 file changed, 61 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index ea01621..3bcab7d 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -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;
> +}

Please clean up the above at least.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] PM / wakeup: use seq_open() to show wakeup stats
  2018-03-30 10:25 ` Rafael J. Wysocki
@ 2018-03-30 11:00   ` Geert Uytterhoeven
  2018-04-02  1:33     ` Ganesh Mahendran
  2018-04-02  1:31   ` Ganesh Mahendran
  1 sibling, 1 reply; 12+ messages in thread
From: Geert Uytterhoeven @ 2018-03-30 11:00 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Ganesh Mahendran, Pavel Machek, Len Brown, Rafael J. Wysocki,
	Greg KH, Linux PM list, Linux Kernel Mailing List

On Fri, Mar 30, 2018 at 12:25 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Monday, March 5, 2018 9:47:46 AM CEST Ganesh Mahendran wrote:
>> 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 <opensource.ganesh@gmail.com>
>> ----
>> v2: use srcu_read_lock instead of rcu_read_lock
>> ---
>>  drivers/base/power/wakeup.c | 77 +++++++++++++++++++++++++++++++++++----------
>>  1 file changed, 61 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
>> index ea01621..3bcab7d 100644
>> --- a/drivers/base/power/wakeup.c
>> +++ b/drivers/base/power/wakeup.c
>> @@ -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;
>> +}
>
> Please clean up the above at least.
>
> 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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] PM / wakeup: use seq_open() to show wakeup stats
  2018-03-30 10:25 ` Rafael J. Wysocki
  2018-03-30 11:00   ` Geert Uytterhoeven
@ 2018-04-02  1:31   ` Ganesh Mahendran
  1 sibling, 0 replies; 12+ messages in thread
From: Ganesh Mahendran @ 2018-04-02  1:31 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Pavel Machek, Len Brown, Rafael J. Wysocki, Greg KH, Linux PM,
	linux-kernel

2018-03-30 18:25 GMT+08:00 Rafael J. Wysocki <rjw@rjwysocki.net>:
> On Monday, March 5, 2018 9:47:46 AM CEST Ganesh Mahendran wrote:
>> 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 <opensource.ganesh@gmail.com>
>> ----
>> v2: use srcu_read_lock instead of rcu_read_lock
>> ---
>>  drivers/base/power/wakeup.c | 77 +++++++++++++++++++++++++++++++++++----------
>>  1 file changed, 61 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
>> index ea01621..3bcab7d 100644
>> --- a/drivers/base/power/wakeup.c
>> +++ b/drivers/base/power/wakeup.c
>> @@ -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;
>> +}
>
> 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.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] PM / wakeup: use seq_open() to show wakeup stats
  2018-03-30 11:00   ` Geert Uytterhoeven
@ 2018-04-02  1:33     ` Ganesh Mahendran
  2018-04-02  6:46       ` Geert Uytterhoeven
  0 siblings, 1 reply; 12+ messages in thread
From: Ganesh Mahendran @ 2018-04-02  1:33 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Rafael J. Wysocki, Pavel Machek, Len Brown, Rafael J. Wysocki,
	Greg KH, Linux PM list, Linux Kernel Mailing List

2018-03-30 19:00 GMT+08:00 Geert Uytterhoeven <geert@linux-m68k.org>:
> On Fri, Mar 30, 2018 at 12:25 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Monday, March 5, 2018 9:47:46 AM CEST Ganesh Mahendran wrote:
>>> 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 <opensource.ganesh@gmail.com>
>>> ----
>>> v2: use srcu_read_lock instead of rcu_read_lock
>>> ---
>>>  drivers/base/power/wakeup.c | 77 +++++++++++++++++++++++++++++++++++----------
>>>  1 file changed, 61 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
>>> index ea01621..3bcab7d 100644
>>> --- a/drivers/base/power/wakeup.c
>>> +++ b/drivers/base/power/wakeup.c
>>> @@ -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;
>>> +}
>>
>> Please clean up the above at least.
>>
>> 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.

>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] PM / wakeup: use seq_open() to show wakeup stats
  2018-04-02  1:33     ` Ganesh Mahendran
@ 2018-04-02  6:46       ` Geert Uytterhoeven
  2018-04-25 11:01         ` Ganesh Mahendran
  0 siblings, 1 reply; 12+ messages in thread
From: Geert Uytterhoeven @ 2018-04-02  6:46 UTC (permalink / raw)
  To: Ganesh Mahendran
  Cc: Rafael J. Wysocki, Pavel Machek, Len Brown, Rafael J. Wysocki,
	Greg KH, Linux PM list, Linux Kernel Mailing List

Hi Ganesh,

On Mon, Apr 2, 2018 at 3:33 AM, Ganesh Mahendran
<opensource.ganesh@gmail.com> wrote:
> 2018-03-30 19:00 GMT+08:00 Geert Uytterhoeven <geert@linux-m68k.org>:
>> On Fri, Mar 30, 2018 at 12:25 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>> On Monday, March 5, 2018 9:47:46 AM CEST Ganesh Mahendran wrote:
>>>> 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.

>>>> --- a/drivers/base/power/wakeup.c
>>>> +++ b/drivers/base/power/wakeup.c
>>>> @@ -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)
>>>>  {

>>>> +     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.
>>
>> 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 -- geert@linux-m68k.org

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] PM / wakeup: use seq_open() to show wakeup stats
  2018-04-02  6:46       ` Geert Uytterhoeven
@ 2018-04-25 11:01         ` Ganesh Mahendran
  0 siblings, 0 replies; 12+ messages in thread
From: Ganesh Mahendran @ 2018-04-25 11:01 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Rafael J. Wysocki, Pavel Machek, Len Brown, Rafael J. Wysocki,
	Greg KH, Linux PM list, Linux Kernel Mailing List

2018-04-02 14:46 GMT+08:00 Geert Uytterhoeven <geert@linux-m68k.org>:
> Hi Ganesh,
>
> On Mon, Apr 2, 2018 at 3:33 AM, Ganesh Mahendran
> <opensource.ganesh@gmail.com> wrote:
>> 2018-03-30 19:00 GMT+08:00 Geert Uytterhoeven <geert@linux-m68k.org>:
>>> On Fri, Mar 30, 2018 at 12:25 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>>> On Monday, March 5, 2018 9:47:46 AM CEST Ganesh Mahendran wrote:
>>>>> 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.
>
>>>>> --- a/drivers/base/power/wakeup.c
>>>>> +++ b/drivers/base/power/wakeup.c
>>>>> @@ -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)
>>>>>  {
>
>>>>> +     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.
>>>
>>> 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;
>         }

I send a v3 patch.
Thanks for your review.

>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2018-04-25 11:01 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-05  8:47 [PATCH v2] PM / wakeup: use seq_open() to show wakeup stats Ganesh Mahendran
2018-03-12  5:33 ` Ganesh Mahendran
2018-03-13 16:39 ` Andy Shevchenko
2018-03-14  1:33   ` Ganesh Mahendran
2018-03-29  7:49 ` Ganesh Mahendran
2018-03-29 21:54   ` Rafael J. Wysocki
2018-03-30 10:25 ` Rafael J. Wysocki
2018-03-30 11:00   ` Geert Uytterhoeven
2018-04-02  1:33     ` Ganesh Mahendran
2018-04-02  6:46       ` Geert Uytterhoeven
2018-04-25 11:01         ` Ganesh Mahendran
2018-04-02  1:31   ` Ganesh Mahendran

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).