Home » Mailing list archives » linux-kernel » 2007 » October » 9

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

Previous thread: [PATCH 2/2] aic94xx: Use sas_request_addr() to provide SAS addr if the adapter lacks one by Darrick J. Wong on Monday, October 8, 2007 - 8:43 pm. (1 message)

Next thread: Re: [PATCH] param_sysfs_builtin memchr argument fix by Dave Young on Monday, October 8, 2007 - 9:21 pm. (1 message)
[view original message]
From: Tim Pepper <lnxninja@...>
To: Peter Zijlstra <a.p.zijlstra@...>
Cc: Ingo Molnar <mingo@...>, <linux-kernel@...>
Subject: [PATCH] lockdep: Avoid /proc/lockdep &amp; lock_stat infinite output
Date: Monday, October 8, 2007 - 9:15 pm

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 <lnxninja@linux.vnet.ibm.com>

Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>

Cc: Ingo Molnar <mingo@elte.hu>


---


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 p...
[ message continues ]
[view original message]
From: Al Viro <viro@...>
To: Tim Pepper <lnxninja@...>
Cc: Peter Zijlstra <a.p.zijlstra@...>, Ingo Molnar <mingo@...>, <linux-kernel@...>
Subject: Re: [PATCH] lockdep: Avoid /proc/lockdep &amp; lock_stat infinite output
Date: Monday, October 8, 2007 - 9:30 pm

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.

-
[view original message]
From: Peter Zijlstra <a.p.zijlstra@...>
To: Al Viro <viro@...>
Cc: Tim Pepper <lnxninja@...>, Ingo Molnar <mingo@...>, <linux-kernel@...>
Subject: Re: [PATCH] lockdep: Avoid /proc/lockdep &amp; lock_stat infinite output
Date: Tuesday, October 9, 2007 - 7:14 am

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 <lnxninja@linux.vnet.ibm.com>

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>

---

 kernel/lockdep_proc.c |   69 +++++++++++++++++++++++----------------------=

-----

 1 file changed, 33 insertions(+), 36 deletions(-)


Index: linux-2.6/kernel/lockdep_proc.c

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

--- linux-2.6.orig/kernel/lockdep_proc.c

+++ linux-2.6/kernel/lockdep_proc.c

@@ -23,32 +23,25 @@

=20

 #include "lockdep_internals.h"

=20

-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 =3D v;

-

-	(*pos)++;

-

-	if (class->lock_entry.next !=3D &all_lock_classes)

-		class =3D list_entry(class->lock_entry.next, struct lock_class,

-				  lock_entry);

-	else

-		class =3D NULL;

-	m->private =3D class;

+	struct lock_class *class;

+	int i =3D 0;

=20

-	return class;

+	list_for_each_entry(class, &all_lock_classes, lock_entry) {

+		if (i++ =3D=3D *pos)

+			return class;

+	}

+	return NULL;

 }

=20

-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 =3D m->private;

-

-	if (&class->lock_entry =3D=3D all_lock_classes.next)

-		seq_printf(m, "all lock classes:\n");

-

-	return class;

+	(*pos)++;

+	return l_start(m, pos);

 }

=20

+

 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 =3D m->private;

+	struct lock_class *class =3D v;

 	struct lock_list...
[ message continues ]
[view original message]
From: Tim Pepper <lnxninja@...>
To: Peter Zijlstra <a.p.zijlstra@...>
Cc: Al Viro <viro@...>, Tim Pepper <lnxninja@...>, Ingo Molnar <mingo@...>, <linux-kernel@...>
Subject: Re: [PATCH] lockdep: Avoid /proc/lockdep &amp; lock_stat infinite output
Date: Tuesday, October 9, 2007 - 6:10 pm

Yes.  The way the original lockdep_proc.c code was doing its pointers

around its seq data was definitely wrong, regardless of the output


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 <lnxninja@linux.vnet.ibm.com>

Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>

Cc: Ingo Molnar <mingo@elte.hu>

Cc: Al Viro <viro@ftp.linux.org.uk>


---


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


...
[ message continues ]
[view original message]
From: Tim Pepper <lnxninja@...>
To: Al Viro <viro@...>
Cc: Tim Pepper <lnxninja@...>, Peter Zijlstra <a.p.zijlstra@...>, Ingo Molnar <mingo@...>, <linux-kernel@...>
Subject: Re: [PATCH] lockdep: Avoid /proc/lockdep &amp; lock_stat infinite output
Date: Tuesday, October 9, 2007 - 12:04 am

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 <lnxninja@linux.vnet.ibm.com>

Cc: Al Viro <viro@ftp.linux.org.uk>


---


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


 /*

-
Previous thread: [PATCH 2/2] aic94xx: Use sas_request_addr() to provide SAS addr if the adapter lacks one by Darrick J. Wong on Monday, October 8, 2007 - 8:43 pm. (1 message)

Next thread: Re: [PATCH] param_sysfs_builtin memchr argument fix by Dave Young on Monday, October 8, 2007 - 9:21 pm. (1 message)