Re: [PATCH] lockdep: Avoid /proc/lockdep & lock_stat infinite output

2007-10-09 Thread Tim Pepper 
On Tue 09 Oct at 13:14:49 +0200 [EMAIL PROTECTED] said:
> FWIW I had to do Tim's bits too. Just moving all output from the start
> into the show method didn't fix it.

Yes.  The way the original lockdep_proc.c code was doing its pointers
around its seq data was definitely wrong, regardless of the output
outside of _show().

> -static void *l_start(struct seq_file *m, loff_t *pos)
> +static void *l_next(struct seq_file *m, void *v, loff_t *pos)
>  {
> - struct lock_class *class = m->private;
> -
> - if (>lock_entry == all_lock_classes.next)
> - seq_printf(m, "all lock classes:\n");
> -
> - return class;
> + (*pos)++;
> + return l_start(m, pos);
>  }

Isn't this is going to make l_start/l_next() approach O(n^2) when they can
be more like O(n)?  It appears to be the case that _next() will always
get a valid *v, so you can just step immediately to the next element.
The _start() seems to be the only place where you'd actually need to
search based on *pos.

The below moves the headers out of the _start() functions, but by using
SEQ_START_TOKEN (as appears to be the trend in other seq_file users)
to differentiate.  This means *pos==0 then is the header and *pos==1..n
are the lock class elements 0..(n-1), which again appears to be what
others are doing.



Both /proc/lockdep and /proc/lock_stat output may loop infinitely.

When a read() requests an amount of data smaller than the amount of data
that the seq_file's foo_show() outputs, the output starts looping and
outputs the "stuck" element's data infinitely.  There may be multiple
sequential calls to foo_start(), foo_next()/foo_show(), and foo_stop()
for a single open with sequential read of the file.  The _start() does not
have to start with the 0th element and _show() might be called multiple
times in a row for the same element for a given open/read of the seq_file.

Also header output should not be happening in _start().  All output should
be in _show(), which SEQ_START_TOKEN is meant to help.  Having output in
_start() may also negatively impact seq_file's seq_read() and traverse()
accounting.

Signed-off-by: Tim Pepper <[EMAIL PROTECTED]>
Cc: Peter Zijlstra <[EMAIL PROTECTED]>
Cc: Ingo Molnar <[EMAIL PROTECTED]>
Cc: Al Viro <[EMAIL PROTECTED]>

---

Compared to my previous version this now also has output only happening
in _show().  Compared to Peter's version with output only in _show(),
this is more efficient in its _next().


--- linux-2.6.23-rc9.orig/kernel/lockdep_proc.c
+++ linux-2.6.23-rc9/kernel/lockdep_proc.c
@@ -25,28 +25,38 @@
 
 static void *l_next(struct seq_file *m, void *v, loff_t *pos)
 {
-   struct lock_class *class = v;
+   struct lock_class *class;
 
(*pos)++;
 
+   if (v == SEQ_START_TOKEN)
+   class = m->private;
+   else {
+   class = v;
+
+   if (class->lock_entry.next != _lock_classes)
+   class = list_entry(class->lock_entry.next,
+  struct lock_class, lock_entry);
+   else
+   class = NULL;
+   }
-   if (class->lock_entry.next != _lock_classes)
-   class = list_entry(class->lock_entry.next, struct lock_class,
- lock_entry);
-   else
-   class = NULL;
-   m->private = class;
 
return class;
 }
 
 static void *l_start(struct seq_file *m, loff_t *pos)
 {
-   struct lock_class *class = m->private;
+   struct lock_class *class;
+   loff_t i = 0;
 
-   if (>lock_entry == all_lock_classes.next)
-   seq_printf(m, "all lock classes:\n");
+   if (*pos == 0)
+   return SEQ_START_TOKEN;
 
+   list_for_each_entry(class, _lock_classes, lock_entry) {
+   if (++i == *pos)
+   return class;
+   }
+   return NULL;
-   return class;
 }
 
 static void l_stop(struct seq_file *m, void *v)
@@ -101,10 +111,15 @@ static void print_name(struct seq_file *
 static int l_show(struct seq_file *m, void *v)
 {
unsigned long nr_forward_deps, nr_backward_deps;
-   struct lock_class *class = m->private;
+   struct lock_class *class = v;
struct lock_list *entry;
char c1, c2, c3, c4;
 
+   if (v == SEQ_START_TOKEN) {
+   seq_printf(m, "all lock classes:\n");
+   return 0;
+   }
+
seq_printf(m, "%p", class->key);
 #ifdef CONFIG_DEBUG_LOCKDEP
seq_printf(m, " OPS:%8ld", class->ops);
@@ -523,10 +538,11 @@ static void *ls_start(struct seq_file *m
 {
struct lock_stat_seq *data = m->private;
 
-   if (data->iter == data->stats)
-   seq_header(m);
+   if (*pos == 0)
+   return SEQ_START_TOKEN;
 
-   if (data->iter == data->iter_end)
+   data->iter = data->stats + *pos;
+   if (data->iter >= data->iter_end)
data->iter = NULL;

Re: [PATCH] lockdep: Avoid /proc/lockdep & lock_stat infinite output

2007-10-09 Thread Peter Zijlstra 
On Tue, 2007-10-09 at 02:30 +0100, Al Viro wrote:
> On Mon, Oct 08, 2007 at 06:15:51PM -0700, Tim Pepper wrote:
> > 
> > When a read() requests an amount of data smaller than the amount of data
> > that the seq_file's foo_show() outputs, the output starts looping and
> > outputs the "stuck" element's data infinitely.  There may be multiple
> > sequential calls to foo_start(), foo_next()/foo_show(), and foo_stop()
> > for a single open with sequential read of the file.  The _start() does not
> > have to start with the 0th element and _show() might be called multiple
> > times in a row for the same element for a given open/read of the seq_file.
> >  
> >  static void *l_start(struct seq_file *m, loff_t *pos)
> >  {
> > -   struct lock_class *class = m->private;
> > +   struct lock_class *class;
> > +   loff_t i = 0;
> >  
> > -   if (>lock_entry == all_lock_classes.next)
> > +   if (*pos == 0)
> > seq_printf(m, "all lock classes:\n");
> 
> Do not generate output outside of ->show() and you won't have these
> problems.  That's where your infinite output crap comes from.
> 
> IOW, NAK - fix the underlying problem.


FWIW I had to do Tim's bits too. Just moving all output from the start
into the show method didn't fix it.

Signed-off-by: Tim Pepper <[EMAIL PROTECTED]>
Signed-off-by: Peter Zijlstra <[EMAIL PROTECTED]>
---
 kernel/lockdep_proc.c |   69 +++---
 1 file changed, 33 insertions(+), 36 deletions(-)

Index: linux-2.6/kernel/lockdep_proc.c
===
--- linux-2.6.orig/kernel/lockdep_proc.c
+++ linux-2.6/kernel/lockdep_proc.c
@@ -23,32 +23,25 @@
 
 #include "lockdep_internals.h"
 
-static void *l_next(struct seq_file *m, void *v, loff_t *pos)
+static void *l_start(struct seq_file *m, loff_t *pos)
 {
-   struct lock_class *class = v;
-
-   (*pos)++;
-
-   if (class->lock_entry.next != _lock_classes)
-   class = list_entry(class->lock_entry.next, struct lock_class,
- lock_entry);
-   else
-   class = NULL;
-   m->private = class;
+   struct lock_class *class;
+   int i = 0;
 
-   return class;
+   list_for_each_entry(class, _lock_classes, lock_entry) {
+   if (i++ == *pos)
+   return class;
+   }
+   return NULL;
 }
 
-static void *l_start(struct seq_file *m, loff_t *pos)
+static void *l_next(struct seq_file *m, void *v, loff_t *pos)
 {
-   struct lock_class *class = m->private;
-
-   if (>lock_entry == all_lock_classes.next)
-   seq_printf(m, "all lock classes:\n");
-
-   return class;
+   (*pos)++;
+   return l_start(m, pos);
 }
 
+
 static void l_stop(struct seq_file *m, void *v)
 {
 }
@@ -101,10 +94,16 @@ static void print_name(struct seq_file *
 static int l_show(struct seq_file *m, void *v)
 {
unsigned long nr_forward_deps, nr_backward_deps;
-   struct lock_class *class = m->private;
+   struct lock_class *class = v;
struct lock_list *entry;
char c1, c2, c3, c4;
 
+   if (WARN_ON(class == NULL))
+   return 0;
+
+   if (>lock_entry == all_lock_classes.next)
+   seq_printf(m, "all lock classes:\n");
+
seq_printf(m, "%p", class->key);
 #ifdef CONFIG_DEBUG_LOCKDEP
seq_printf(m, " OPS:%8ld", class->ops);
@@ -522,28 +521,19 @@ static void seq_header(struct seq_file *
 static void *ls_start(struct seq_file *m, loff_t *pos)
 {
struct lock_stat_seq *data = m->private;
+   struct lock_stat_data *iter;
 
-   if (data->iter == data->stats)
-   seq_header(m);
-
-   if (data->iter == data->iter_end)
-   data->iter = NULL;
+   iter = data->iter + *pos;
+   if (iter >= data->iter_end)
+   iter = NULL;
 
-   return data->iter;
+   return iter;
 }
 
 static void *ls_next(struct seq_file *m, void *v, loff_t *pos)
 {
-   struct lock_stat_seq *data = m->private;
-
(*pos)++;
-
-   data->iter = v;
-   data->iter++;
-   if (data->iter == data->iter_end)
-   data->iter = NULL;
-
-   return data->iter;
+   return ls_start(m, pos);
 }
 
 static void ls_stop(struct seq_file *m, void *v)
@@ -553,8 +543,15 @@ static void ls_stop(struct seq_file *m, 
 static int ls_show(struct seq_file *m, void *v)
 {
struct lock_stat_seq *data = m->private;
+   struct lock_stat_data *iter = v;
+
+   if (WARN_ON(iter == NULL))
+   return 0;
+
+   if (iter == data->iter)
+   seq_header(m);
 
-   seq_stats(m, data->iter);
+   seq_stats(m, iter);
return 0;
 }
 



signature.asc
Description: This is a digitally signed message part

Re: [PATCH] lockdep: Avoid /proc/lockdep lock_stat infinite output

2007-10-09 Thread Peter Zijlstra 
On Tue, 2007-10-09 at 02:30 +0100, Al Viro wrote:
 On Mon, Oct 08, 2007 at 06:15:51PM -0700, Tim Pepper wrote:
  
  When a read() requests an amount of data smaller than the amount of data
  that the seq_file's foo_show() outputs, the output starts looping and
  outputs the stuck element's data infinitely.  There may be multiple
  sequential calls to foo_start(), foo_next()/foo_show(), and foo_stop()
  for a single open with sequential read of the file.  The _start() does not
  have to start with the 0th element and _show() might be called multiple
  times in a row for the same element for a given open/read of the seq_file.
   
   static void *l_start(struct seq_file *m, loff_t *pos)
   {
  -   struct lock_class *class = m-private;
  +   struct lock_class *class;
  +   loff_t i = 0;
   
  -   if (class-lock_entry == all_lock_classes.next)
  +   if (*pos == 0)
  seq_printf(m, all lock classes:\n);
 
 Do not generate output outside of -show() and you won't have these
 problems.  That's where your infinite output crap comes from.
 
 IOW, NAK - fix the underlying problem.


FWIW I had to do Tim's bits too. Just moving all output from the start
into the show method didn't fix it.

Signed-off-by: Tim Pepper [EMAIL PROTECTED]
Signed-off-by: Peter Zijlstra [EMAIL PROTECTED]
---
 kernel/lockdep_proc.c |   69 +++---
 1 file changed, 33 insertions(+), 36 deletions(-)

Index: linux-2.6/kernel/lockdep_proc.c
===
--- linux-2.6.orig/kernel/lockdep_proc.c
+++ linux-2.6/kernel/lockdep_proc.c
@@ -23,32 +23,25 @@
 
 #include lockdep_internals.h
 
-static void *l_next(struct seq_file *m, void *v, loff_t *pos)
+static void *l_start(struct seq_file *m, loff_t *pos)
 {
-   struct lock_class *class = v;
-
-   (*pos)++;
-
-   if (class-lock_entry.next != all_lock_classes)
-   class = list_entry(class-lock_entry.next, struct lock_class,
- lock_entry);
-   else
-   class = NULL;
-   m-private = class;
+   struct lock_class *class;
+   int i = 0;
 
-   return class;
+   list_for_each_entry(class, all_lock_classes, lock_entry) {
+   if (i++ == *pos)
+   return class;
+   }
+   return NULL;
 }
 
-static void *l_start(struct seq_file *m, loff_t *pos)
+static void *l_next(struct seq_file *m, void *v, loff_t *pos)
 {
-   struct lock_class *class = m-private;
-
-   if (class-lock_entry == all_lock_classes.next)
-   seq_printf(m, all lock classes:\n);
-
-   return class;
+   (*pos)++;
+   return l_start(m, pos);
 }
 
+
 static void l_stop(struct seq_file *m, void *v)
 {
 }
@@ -101,10 +94,16 @@ static void print_name(struct seq_file *
 static int l_show(struct seq_file *m, void *v)
 {
unsigned long nr_forward_deps, nr_backward_deps;
-   struct lock_class *class = m-private;
+   struct lock_class *class = v;
struct lock_list *entry;
char c1, c2, c3, c4;
 
+   if (WARN_ON(class == NULL))
+   return 0;
+
+   if (class-lock_entry == all_lock_classes.next)
+   seq_printf(m, all lock classes:\n);
+
seq_printf(m, %p, class-key);
 #ifdef CONFIG_DEBUG_LOCKDEP
seq_printf(m,  OPS:%8ld, class-ops);
@@ -522,28 +521,19 @@ static void seq_header(struct seq_file *
 static void *ls_start(struct seq_file *m, loff_t *pos)
 {
struct lock_stat_seq *data = m-private;
+   struct lock_stat_data *iter;
 
-   if (data-iter == data-stats)
-   seq_header(m);
-
-   if (data-iter == data-iter_end)
-   data-iter = NULL;
+   iter = data-iter + *pos;
+   if (iter = data-iter_end)
+   iter = NULL;
 
-   return data-iter;
+   return iter;
 }
 
 static void *ls_next(struct seq_file *m, void *v, loff_t *pos)
 {
-   struct lock_stat_seq *data = m-private;
-
(*pos)++;
-
-   data-iter = v;
-   data-iter++;
-   if (data-iter == data-iter_end)
-   data-iter = NULL;
-
-   return data-iter;
+   return ls_start(m, pos);
 }
 
 static void ls_stop(struct seq_file *m, void *v)
@@ -553,8 +543,15 @@ static void ls_stop(struct seq_file *m, 
 static int ls_show(struct seq_file *m, void *v)
 {
struct lock_stat_seq *data = m-private;
+   struct lock_stat_data *iter = v;
+
+   if (WARN_ON(iter == NULL))
+   return 0;
+
+   if (iter == data-iter)
+   seq_header(m);
 
-   seq_stats(m, data-iter);
+   seq_stats(m, iter);
return 0;
 }
 



signature.asc
Description: This is a digitally signed message part

Re: [PATCH] lockdep: Avoid /proc/lockdep lock_stat infinite output

2007-10-09 Thread Tim Pepper 
On Tue 09 Oct at 13:14:49 +0200 [EMAIL PROTECTED] said:
 FWIW I had to do Tim's bits too. Just moving all output from the start
 into the show method didn't fix it.

Yes.  The way the original lockdep_proc.c code was doing its pointers
around its seq data was definitely wrong, regardless of the output
outside of _show().

 -static void *l_start(struct seq_file *m, loff_t *pos)
 +static void *l_next(struct seq_file *m, void *v, loff_t *pos)
  {
 - struct lock_class *class = m-private;
 -
 - if (class-lock_entry == all_lock_classes.next)
 - seq_printf(m, all lock classes:\n);
 -
 - return class;
 + (*pos)++;
 + return l_start(m, pos);
  }

Isn't this is going to make l_start/l_next() approach O(n^2) when they can
be more like O(n)?  It appears to be the case that _next() will always
get a valid *v, so you can just step immediately to the next element.
The _start() seems to be the only place where you'd actually need to
search based on *pos.

The below moves the headers out of the _start() functions, but by using
SEQ_START_TOKEN (as appears to be the trend in other seq_file users)
to differentiate.  This means *pos==0 then is the header and *pos==1..n
are the lock class elements 0..(n-1), which again appears to be what
others are doing.



Both /proc/lockdep and /proc/lock_stat output may loop infinitely.

When a read() requests an amount of data smaller than the amount of data
that the seq_file's foo_show() outputs, the output starts looping and
outputs the stuck element's data infinitely.  There may be multiple
sequential calls to foo_start(), foo_next()/foo_show(), and foo_stop()
for a single open with sequential read of the file.  The _start() does not
have to start with the 0th element and _show() might be called multiple
times in a row for the same element for a given open/read of the seq_file.

Also header output should not be happening in _start().  All output should
be in _show(), which SEQ_START_TOKEN is meant to help.  Having output in
_start() may also negatively impact seq_file's seq_read() and traverse()
accounting.

Signed-off-by: Tim Pepper [EMAIL PROTECTED]
Cc: Peter Zijlstra [EMAIL PROTECTED]
Cc: Ingo Molnar [EMAIL PROTECTED]
Cc: Al Viro [EMAIL PROTECTED]

---

Compared to my previous version this now also has output only happening
in _show().  Compared to Peter's version with output only in _show(),
this is more efficient in its _next().


--- linux-2.6.23-rc9.orig/kernel/lockdep_proc.c
+++ linux-2.6.23-rc9/kernel/lockdep_proc.c
@@ -25,28 +25,38 @@
 
 static void *l_next(struct seq_file *m, void *v, loff_t *pos)
 {
-   struct lock_class *class = v;
+   struct lock_class *class;
 
(*pos)++;
 
+   if (v == SEQ_START_TOKEN)
+   class = m-private;
+   else {
+   class = v;
+
+   if (class-lock_entry.next != all_lock_classes)
+   class = list_entry(class-lock_entry.next,
+  struct lock_class, lock_entry);
+   else
+   class = NULL;
+   }
-   if (class-lock_entry.next != all_lock_classes)
-   class = list_entry(class-lock_entry.next, struct lock_class,
- lock_entry);
-   else
-   class = NULL;
-   m-private = class;
 
return class;
 }
 
 static void *l_start(struct seq_file *m, loff_t *pos)
 {
-   struct lock_class *class = m-private;
+   struct lock_class *class;
+   loff_t i = 0;
 
-   if (class-lock_entry == all_lock_classes.next)
-   seq_printf(m, all lock classes:\n);
+   if (*pos == 0)
+   return SEQ_START_TOKEN;
 
+   list_for_each_entry(class, all_lock_classes, lock_entry) {
+   if (++i == *pos)
+   return class;
+   }
+   return NULL;
-   return class;
 }
 
 static void l_stop(struct seq_file *m, void *v)
@@ -101,10 +111,15 @@ static void print_name(struct seq_file *
 static int l_show(struct seq_file *m, void *v)
 {
unsigned long nr_forward_deps, nr_backward_deps;
-   struct lock_class *class = m-private;
+   struct lock_class *class = v;
struct lock_list *entry;
char c1, c2, c3, c4;
 
+   if (v == SEQ_START_TOKEN) {
+   seq_printf(m, all lock classes:\n);
+   return 0;
+   }
+
seq_printf(m, %p, class-key);
 #ifdef CONFIG_DEBUG_LOCKDEP
seq_printf(m,  OPS:%8ld, class-ops);
@@ -523,10 +538,11 @@ static void *ls_start(struct seq_file *m
 {
struct lock_stat_seq *data = m-private;
 
-   if (data-iter == data-stats)
-   seq_header(m);
+   if (*pos == 0)
+   return SEQ_START_TOKEN;
 
-   if (data-iter == data-iter_end)
+   data-iter = data-stats + *pos;
+   if (data-iter = data-iter_end)
data-iter = NULL;
 
return data-iter;
@@ -538,8

Re: [PATCH] lockdep: Avoid /proc/lockdep & lock_stat infinite output

2007-10-08 Thread Tim Pepper 
On Tue 09 Oct at 02:30:11 +0100 [EMAIL PROTECTED] said:
> On Mon, Oct 08, 2007 at 06:15:51PM -0700, Tim Pepper wrote:
> > 
> > -   if (>lock_entry == all_lock_classes.next)
> > +   if (*pos == 0)
> > seq_printf(m, "all lock classes:\n");
> 
> Do not generate output outside of ->show() and you won't have these
> problems.  That's where your infinite output crap comes from.
> 
> IOW, NAK - fix the underlying problem.

Aaah...OK.  Can we add something like the following then:




Document that output must only come from _show() and SEQ_START_TOKEN is how
a _start() indicates a header is to be printed.

Signed-off-by: Tim Pepper <[EMAIL PROTECTED]>
Cc: Al Viro <[EMAIL PROTECTED]>

---

--- linux-2.6.orig/include/linux/seq_file.h
+++ linux-2.6.23-rc9/include/linux/seq_file.h
@@ -36,9 +36,10 @@ ssize_t seq_read(struct file *, char __u
 loff_t seq_lseek(struct file *, loff_t, int);
 int seq_release(struct inode *, struct file *);
 int seq_escape(struct seq_file *, const char *, const char *);
+
+/* these may only be called from a (*show) function */
 int seq_putc(struct seq_file *m, char c);
 int seq_puts(struct seq_file *m, const char *s);
-
 int seq_printf(struct seq_file *, const char *, ...)
__attribute__ ((format (printf,2,3)));
 
@@ -48,6 +49,11 @@ int single_open(struct file *, int (*)(s
 int single_release(struct inode *, struct file *);
 int seq_release_private(struct inode *, struct file *);
 
+/*
+ * return SEQ_START_TOKEN in your (*start) function and test for
+ * (v == SEQ_START_TOKEN) in * your (*show) funtion in order to
+ * print a header before your seq data
+ */
 #define SEQ_START_TOKEN ((void *)1)
 
 /*
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] lockdep: Avoid /proc/lockdep & lock_stat infinite output

2007-10-08 Thread Al Viro 
On Mon, Oct 08, 2007 at 06:15:51PM -0700, Tim Pepper wrote:
> 
> When a read() requests an amount of data smaller than the amount of data
> that the seq_file's foo_show() outputs, the output starts looping and
> outputs the "stuck" element's data infinitely.  There may be multiple
> sequential calls to foo_start(), foo_next()/foo_show(), and foo_stop()
> for a single open with sequential read of the file.  The _start() does not
> have to start with the 0th element and _show() might be called multiple
> times in a row for the same element for a given open/read of the seq_file.
>  
>  static void *l_start(struct seq_file *m, loff_t *pos)
>  {
> - struct lock_class *class = m->private;
> + struct lock_class *class;
> + loff_t i = 0;
>  
> - if (>lock_entry == all_lock_classes.next)
> + if (*pos == 0)
>   seq_printf(m, "all lock classes:\n");

Do not generate output outside of ->show() and you won't have these
problems.  That's where your infinite output crap comes from.

IOW, NAK - fix the underlying problem.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] lockdep: Avoid /proc/lockdep & lock_stat infinite output

2007-10-08 Thread Tim Pepper 

When a read() requests an amount of data smaller than the amount of data
that the seq_file's foo_show() outputs, the output starts looping and
outputs the "stuck" element's data infinitely.  There may be multiple
sequential calls to foo_start(), foo_next()/foo_show(), and foo_stop()
for a single open with sequential read of the file.  The _start() does not
have to start with the 0th element and _show() might be called multiple
times in a row for the same element for a given open/read of the seq_file.

Signed-off-by: Tim Pepper <[EMAIL PROTECTED]>
Cc: Peter Zijlstra <[EMAIL PROTECTED]>
Cc: Ingo Molnar <[EMAIL PROTECTED]>

---

Assuming people are fine with this, it should probably find its way
to stable.

If you haven't seen the infinite output: it's easy to trigger with a
simple 'cat /proc/lockdep' generally for me, a cat /proc/lock_stat piped
to a file or for either of them a dd with the default bs=512 (or smaller)
should do the job also.

With this change to the lock_stat handler the data->iter member no longer
attempts to hold state across calls, so it could be taken out of the
lock_stat_seq struct and replace by a local variable in each function
but that isn't a clear win to me so I just left it.

--- linux-2.6.23-rc9.orig/kernel/lockdep_proc.c
+++ linux-2.6.23-rc9/kernel/lockdep_proc.c
@@ -34,19 +34,23 @@ static void *l_next(struct seq_file *m, 
  lock_entry);
else
class = NULL;
-   m->private = class;
 
return class;
 }
 
 static void *l_start(struct seq_file *m, loff_t *pos)
 {
-   struct lock_class *class = m->private;
+   struct lock_class *class;
+   loff_t i = 0;
 
-   if (>lock_entry == all_lock_classes.next)
+   if (*pos == 0)
seq_printf(m, "all lock classes:\n");
 
+   list_for_each_entry(class, _lock_classes, lock_entry) {
+   if (i++ == *pos)
+   return class;
+   }
+   return NULL;
-   return class;
 }
 
 static void l_stop(struct seq_file *m, void *v)
@@ -101,7 +105,7 @@ static void print_name(struct seq_file *
 static int l_show(struct seq_file *m, void *v)
 {
unsigned long nr_forward_deps, nr_backward_deps;
-   struct lock_class *class = m->private;
+   struct lock_class *class = v;
struct lock_list *entry;
char c1, c2, c3, c4;
 
@@ -523,12 +527,15 @@ static void *ls_start(struct seq_file *m
 {
struct lock_stat_seq *data = m->private;
 
-   if (data->iter == data->stats)
-   seq_header(m);
+   data->iter = data->stats;
+   data->iter += *pos;
 
-   if (data->iter == data->iter_end)
+   if (data->iter >= data->iter_end)
data->iter = NULL;
 
+   if (data->iter == data->stats)
+   seq_header(m);
+
return data->iter;
 }
 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] lockdep: Avoid /proc/lockdep lock_stat infinite output

2007-10-08 Thread Tim Pepper 

When a read() requests an amount of data smaller than the amount of data
that the seq_file's foo_show() outputs, the output starts looping and
outputs the stuck element's data infinitely.  There may be multiple
sequential calls to foo_start(), foo_next()/foo_show(), and foo_stop()
for a single open with sequential read of the file.  The _start() does not
have to start with the 0th element and _show() might be called multiple
times in a row for the same element for a given open/read of the seq_file.

Signed-off-by: Tim Pepper [EMAIL PROTECTED]
Cc: Peter Zijlstra [EMAIL PROTECTED]
Cc: Ingo Molnar [EMAIL PROTECTED]

---

Assuming people are fine with this, it should probably find its way
to stable.

If you haven't seen the infinite output: it's easy to trigger with a
simple 'cat /proc/lockdep' generally for me, a cat /proc/lock_stat piped
to a file or for either of them a dd with the default bs=512 (or smaller)
should do the job also.

With this change to the lock_stat handler the data-iter member no longer
attempts to hold state across calls, so it could be taken out of the
lock_stat_seq struct and replace by a local variable in each function
but that isn't a clear win to me so I just left it.

--- linux-2.6.23-rc9.orig/kernel/lockdep_proc.c
+++ linux-2.6.23-rc9/kernel/lockdep_proc.c
@@ -34,19 +34,23 @@ static void *l_next(struct seq_file *m, 
  lock_entry);
else
class = NULL;
-   m-private = class;
 
return class;
 }
 
 static void *l_start(struct seq_file *m, loff_t *pos)
 {
-   struct lock_class *class = m-private;
+   struct lock_class *class;
+   loff_t i = 0;
 
-   if (class-lock_entry == all_lock_classes.next)
+   if (*pos == 0)
seq_printf(m, all lock classes:\n);
 
+   list_for_each_entry(class, all_lock_classes, lock_entry) {
+   if (i++ == *pos)
+   return class;
+   }
+   return NULL;
-   return class;
 }
 
 static void l_stop(struct seq_file *m, void *v)
@@ -101,7 +105,7 @@ static void print_name(struct seq_file *
 static int l_show(struct seq_file *m, void *v)
 {
unsigned long nr_forward_deps, nr_backward_deps;
-   struct lock_class *class = m-private;
+   struct lock_class *class = v;
struct lock_list *entry;
char c1, c2, c3, c4;
 
@@ -523,12 +527,15 @@ static void *ls_start(struct seq_file *m
 {
struct lock_stat_seq *data = m-private;
 
-   if (data-iter == data-stats)
-   seq_header(m);
+   data-iter = data-stats;
+   data-iter += *pos;
 
-   if (data-iter == data-iter_end)
+   if (data-iter = data-iter_end)
data-iter = NULL;
 
+   if (data-iter == data-stats)
+   seq_header(m);
+
return data-iter;
 }
 
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] lockdep: Avoid /proc/lockdep lock_stat infinite output

2007-10-08 Thread Al Viro 
On Mon, Oct 08, 2007 at 06:15:51PM -0700, Tim Pepper wrote:
 
 When a read() requests an amount of data smaller than the amount of data
 that the seq_file's foo_show() outputs, the output starts looping and
 outputs the stuck element's data infinitely.  There may be multiple
 sequential calls to foo_start(), foo_next()/foo_show(), and foo_stop()
 for a single open with sequential read of the file.  The _start() does not
 have to start with the 0th element and _show() might be called multiple
 times in a row for the same element for a given open/read of the seq_file.
  
  static void *l_start(struct seq_file *m, loff_t *pos)
  {
 - struct lock_class *class = m-private;
 + struct lock_class *class;
 + loff_t i = 0;
  
 - if (class-lock_entry == all_lock_classes.next)
 + if (*pos == 0)
   seq_printf(m, all lock classes:\n);

Do not generate output outside of -show() and you won't have these
problems.  That's where your infinite output crap comes from.

IOW, NAK - fix the underlying problem.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] lockdep: Avoid /proc/lockdep lock_stat infinite output

2007-10-08 Thread Tim Pepper 
On Tue 09 Oct at 02:30:11 +0100 [EMAIL PROTECTED] said:
 On Mon, Oct 08, 2007 at 06:15:51PM -0700, Tim Pepper wrote:
  
  -   if (class-lock_entry == all_lock_classes.next)
  +   if (*pos == 0)
  seq_printf(m, all lock classes:\n);
 
 Do not generate output outside of -show() and you won't have these
 problems.  That's where your infinite output crap comes from.
 
 IOW, NAK - fix the underlying problem.

Aaah...OK.  Can we add something like the following then:




Document that output must only come from _show() and SEQ_START_TOKEN is how
a _start() indicates a header is to be printed.

Signed-off-by: Tim Pepper [EMAIL PROTECTED]
Cc: Al Viro [EMAIL PROTECTED]

---

--- linux-2.6.orig/include/linux/seq_file.h
+++ linux-2.6.23-rc9/include/linux/seq_file.h
@@ -36,9 +36,10 @@ ssize_t seq_read(struct file *, char __u
 loff_t seq_lseek(struct file *, loff_t, int);
 int seq_release(struct inode *, struct file *);
 int seq_escape(struct seq_file *, const char *, const char *);
+
+/* these may only be called from a (*show) function */
 int seq_putc(struct seq_file *m, char c);
 int seq_puts(struct seq_file *m, const char *s);
-
 int seq_printf(struct seq_file *, const char *, ...)
__attribute__ ((format (printf,2,3)));
 
@@ -48,6 +49,11 @@ int single_open(struct file *, int (*)(s
 int single_release(struct inode *, struct file *);
 int seq_release_private(struct inode *, struct file *);
 
+/*
+ * return SEQ_START_TOKEN in your (*start) function and test for
+ * (v == SEQ_START_TOKEN) in * your (*show) funtion in order to
+ * print a header before your seq data
+ */
 #define SEQ_START_TOKEN ((void *)1)
 
 /*
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/