Skip to contentSkip to conversation

Sorted seq_file

PATCH In review Mathieu Desnoyers 4 participants 14 messages Aug 12, 2007 – Aug 25, 2007 3 messages not in the archive

Subsystems (4)
VFS ext4 file system Proc filesystem OCFS2
lore
Share & tools
lore

Mathieu Desnoyers

author
#0 lore raw

cover Sorted seq_file

Hi Andrew,

I ran in a race regarding seq_file listing of modules. Here is a fix proposed to get a coherent output even if modules are being unloaded at the same time.

It is needed by the linux kernel markers.

It applies on top of 2.6.23-rc2-mm2 in this order:

seq_file_sorted.patch
module.c-sort-module-list.patch

Mathieu

Signature · 3 lines
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68
author
#1 lore raw

PATCH 1/2 Seq_file add support for sorted list

Add support for sorted list in seq_file. It aims at changing the way /proc/modules and kallsyms iterates on the module list to remove a race between module unload and module/symbol listing.

The list is sorted by ascending list_head pointer address.

Signed-off-by Mathieu Desnoyers <[email protected]>
---
 fs/seq_file.c            |   40 ++++++++++++++++++++++++++++++++++++++++
 include/linux/seq_file.h |   20 ++++++++++++++++++++
 2 files changed, 60 insertions(+)
fs/seq_file.c +40 -0
Index: linux-2.6-lttng/fs/seq_file.c
===================================================================
@@ -485,3 +485,43 @@ struct list_head *seq_list_next(void *v,
 }
 
 EXPORT_SYMBOL(seq_list_next);
+
+struct list_head *seq_sorted_list_start(struct list_head *head, void *pos)
+{
+	struct list_head *lh;
+
+	list_for_each(lh, head)
+		if ((void*)lh >= pos)
+			return lh;
+	return NULL;
+}
+
+EXPORT_SYMBOL(seq_sorted_list_start);
+
+struct list_head *seq_sorted_list_start_head(struct list_head *head, void *pos)
+{
+	struct list_head *lh;
+
+	if (!pos)
+		return head;
+	list_for_each(lh, head)
+		if ((void*)lh >= pos)
+			return lh->prev;
+	return NULL;
+}
+
+EXPORT_SYMBOL(seq_sorted_list_start_head);
+
+struct list_head *seq_sorted_list_next(void *p, struct list_head *head,
+		void **ppos)
+{
+	struct list_head *lh;
+	void *next;
+
+	lh = ((struct list_head *)p)->next;
+	next = (lh == head) ? NULL : lh;
+	*ppos = next ? next : (void*)-1UL;
+	return next;
+}
+
+EXPORT_SYMBOL(seq_sorted_list_next);
include/linux/seq_file.h +20 -0
Index: linux-2.6-lttng/include/linux/seq_file.h
===================================================================
@@ -61,5 +61,25 @@ extern struct list_head *seq_list_start_
 extern struct list_head *seq_list_next(void *v, struct list_head *head,
 		loff_t *ppos);
 
+/*
+ * Helpers for iteration over a list sorted by ascending head pointer address.
+ * To be used in contexts where preemption cannot be disabled to insure to
+ * continue iteration on a modified list starting at the same location where it
+ * stopped, or at a following location. It insures that the lost information
+ * will only be in elements added/removed from the list between iterations.
+ * void *pos is only used to get the next list element and may not be a valid
+ * list_head anymore when given to seq_sorted_list_start() or
+ * seq_sorted_list_start_head().
+ */
+extern struct list_head *seq_sorted_list_start(struct list_head *head,
+		void *pos);
+extern struct list_head *seq_sorted_list_start_head(struct list_head *head,
+		void *pos);
+/*
+ * next must be called with an existing p node
+ */
+extern struct list_head *seq_sorted_list_next(void *p, struct list_head *head,
+		void **ppos);
+
 #endif
 #endif
Signature · 3 lines
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68
author
#2 lore raw

PATCH 2/2 Sort module list by pointer address to get coherent sleepable seq_file iterators

In reply to Mathieu Desnoyers

A race that appears both in /proc/modules and in kallsyms: if, between the seq file reads, the process is put to sleep and at this moment a module is or removed from the module list, the listing will skip an amount of modules/symbols corresponding to the amount of elements present in the unloaded module, but at the current position in the list if the iteration is located after the removed module.

The cleanest way I found to deal with this problem is to sort the module list. We can then keep the old struct module * as the old iterator, knowing the it may be removed between the seq file reads, but we only use it as "get next". If it is not present in the module list, the next pointer will be used.

By doing this, removing a given module will now only fuzz the output related to this specific module, not any random module anymore. Since modprobe uses /proc/modules, it might be important to make sure multiple concurrent running modprobes won't interfere with each other.

Signed-off-by Mathieu Desnoyers <[email protected]>
---
 kernel/module.c |   28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)
kernel/module.c +23 -5
Index: linux-2.6-lttng/kernel/module.c
===================================================================
@@ -63,8 +63,10 @@ extern int module_sysfs_initialized;
 /* If this is set, the section belongs in the init part of the module */
 #define INIT_OFFSET_MASK (1UL << (BITS_PER_LONG-1))
 
-/* List of modules, protected by module_mutex or preempt_disable
- * (add/delete uses stop_machine). */
+/*
+ * List of modules, protected by module_mutex or preempt_disable
+ * (add/delete uses stop_machine). Sorted by ascending list node address.
+ */
 DEFINE_MUTEX(module_mutex);
 LIST_HEAD(modules);
 static DECLARE_MUTEX(notify_mutex);

@@ -2134,10 +2136,24 @@ nomodsectinfo:
 /*
  * link the module with the whole machine is stopped with interrupts off
  * - this defends against kallsyms not taking locks
+ * We sort the modules by struct module pointer address to permit correct
+ * iteration over modules of, at least, kallsyms for preemptible operations,
+ * such as read(). Sorting by struct module pointer address is equivalent to
+ * sort by list node address.
  */
 static int __link_module(void *_mod)
 {
-	struct module *mod = _mod;
+	struct module *mod = _mod, *iter;
+
+	list_for_each_entry_reverse(iter, &modules, list) {
+		BUG_ON(iter == mod);	/* Should never be in the list twice */
+		if (iter < mod) {
+			/* We belong to the location right after iter. */
+			list_add(&mod->list, &iter->list);
+			return 0;
+		}
+	}
+	/* We should be added at the head of the list */
 	list_add(&mod->list, &modules);
 	return 0;
 }

@@ -2402,12 +2418,14 @@ unsigned long module_kallsyms_lookup_nam
 static void *m_start(struct seq_file *m, loff_t *pos)
 {
 	mutex_lock(&module_mutex);
-	return seq_list_start(&modules, *pos);
+	if (!*pos)
+		m->private = NULL;
+	return seq_sorted_list_start(&modules, m->private);
 }
 
 static void *m_next(struct seq_file *m, void *p, loff_t *pos)
 {
-	return seq_list_next(p, &modules, pos);
+	return seq_sorted_list_next(p, &modules, &m->private);
 }
 
 static void m_stop(struct seq_file *m, void *p)
Signature · 3 lines
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68
#3 lore raw

Re: [patch 2/2] Sort module list by pointer address to get coherent sleepable seq_file iterators

Review

Quoted 6 lines · Mathieu Desnoyers

static void *m_next(struct seq_file *m, void *p, loff_t *pos) {

(quoted diff) +1 -1
-	return seq_list_next(p, &modules, pos);
+	return seq_sorted_list_next(p, &modules, &m->private);
 }

In theory it is not safe to use something other than the passed in *pos as an position indicator. Because seq_file do not always call ->next() to advance to the next item. Look at seq_file.c, it sometimes increase the pos/index directly! Which also prevents pos to skip forward, which is preferred in your case.

The attached patch tries to fix it.

The seq_file.c is so twisted!

Fengguang
===

seqfile: remove seq_file's assumption about iterators

The seq_file implementation has some hardcoded index++/pos++ lines, which assumes iterators to be *continuous* integers.

This patch replaces the index++ lines with calls to m->next(), so that seq_file users can freely use discrete forms of iterators, such as ascending addresses.

Cc Randy Dunlap <[email protected]>
Cc Martin Bligh <[email protected]>
Signed-off-by Wu Fengguang <[email protected]>
---
 fs/seq_file.c |   22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)
fs/seq_file.c +12 -10
@@ -13,6 +13,8 @@
 #include <asm/uaccess.h>
 #include <asm/page.h>
 
+#define SEQFILE_SHOW_NEXT	LONG_MAX
+
 /**
  *	seq_open -	initialize sequential file
  *	@file: file we initialize

@@ -93,6 +95,7 @@ ssize_t seq_read(struct file *file, char
 	/* if not empty - flush it first */
 	if (m->count) {
 		n = min(m->count, size);
+		BUG_ON(m->from == SEQFILE_SHOW_NEXT);
 		err = copy_to_user(buf, m->buf + m->from, n);
 		if (err)
 			goto Efault;

@@ -102,7 +105,7 @@ ssize_t seq_read(struct file *file, char
 		buf += n;
 		copied += n;
 		if (!m->count)
-			m->index++;
+			m->from = SEQFILE_SHOW_NEXT;
 		if (!size)
 			goto Done;
 	}

@@ -113,9 +116,11 @@ ssize_t seq_read(struct file *file, char
 		err = PTR_ERR(p);
 		if (!p || IS_ERR(p))
 			break;
-		err = m->op->show(m, p);
-		if (err)
-			break;
+		if (m->from != SEQFILE_SHOW_NEXT) {
+			err = m->op->show(m, p);
+			if (err)
+				break;
+		}
 		if (m->count < m->size)
 			goto Fill;
 		m->op->stop(m, p);

@@ -156,7 +161,7 @@ Fill:
 	if (m->count)
 		m->from = n;
 	else
-		pos++;
+		m->from = SEQFILE_SHOW_NEXT;
 	m->index = pos;
 Done:
 	if (!copied)

@@ -211,12 +216,9 @@ static int traverse(struct seq_file *m, 
 		}
 		pos += m->count;
 		m->count = 0;
-		if (pos == offset) {
-			index++;
-			m->index = index;
-			break;
-		}
 		p = m->op->next(m, p, &index);
+		if (pos == offset)
+			break;
 	}
 	m->op->stop(m, p);
 	return error;

A message that is not in the archive (sent off-list or not yet received): [email protected]

#5 lore raw

Re: [patch 2/2] Sort module list by pointer address to get coherent sleepable seq_file iterators

Quoted 18 lines · Fengguang Wu

kernel/module.c unsigned long module_kallsyms_lookup_nam · line 2429

Quoted 6 lines · Mathieu Desnoyers

static void *m_next(struct seq_file *m, void *p, loff_t *pos) {

kernel/module.c +1 -1
-	return seq_list_next(p, &modules, pos);
+	return seq_sorted_list_next(p, &modules, &m->private);
 }

In theory it is not safe to use something other than the passed in *pos as an position indicator. Because seq_file do not always call ->next() to advance to the next item. Look at seq_file.c, it sometimes increase the pos/index directly! Which also prevents pos to skip forward, which is preferred in your case.

The attached patch tries to fix it.

The seq_file.c is so twisted!

Quoted 7 lines

Fengguang
===

seqfile: remove seq_file's assumption about iterators

The seq_file implementation has some hardcoded index++/pos++ lines, which assumes iterators to be *continuous* integers.

What the fuck? It assumes no such thing and a lot of iterators are nothing like integers. What are you talking about?

#6 lore raw

Re: [patch 2/2] Sort module list by pointer address to get coherent sleepable seq_file iterators

Quoted 8 lines · Al Viro
Quoted 5 lines · Fengguang Wu

seqfile: remove seq_file's assumption about iterators

The seq_file implementation has some hardcoded index++/pos++ lines, which assumes iterators to be *continuous* integers.

What the fuck? It assumes no such thing and a lot of iterators are nothing like integers. What are you talking about?

Oh I used the wrong term...

Take for example this function from lwn.net:

static void *ct_seq_next(struct seq_file *s, void *v, loff_t *pos)

{
        loff_t *spos = (loff_t *) v;
        *pos = ++(*spos);
        return spos;
}

I mean 'pos' is sometimes increased in ct_seq_next(), and sometimes from seq_file.c/seq_read(), too. Thus we cannot reliably do this:

        *pos = (*spos) + some_variable_offset;

You are referring to spos as the iterator, are you?

Maybe I'm wrong. I'll dip more into it.

A message that is not in the archive (sent off-list or not yet received): [email protected]

#8 lore raw

Re: [patch 2/2] Sort module list by pointer address to get coherent sleepable seq_file iterators

Quoted 11 lines · Fengguang Wu

static void *ct_seq_next(struct seq_file *s, void *v, loff_t *pos)

{
        loff_t *spos = (loff_t *) v;
        *pos = ++(*spos);
        return spos;
}

I mean 'pos' is sometimes increased in ct_seq_next(), and sometimes from seq_file.c/seq_read(), too. Thus we cannot reliably do this:

        *pos = (*spos) + some_variable_offset;

Of course we can. These guys can be sparse - note that ->start() takes a pointer, and for a good reason. ->start(m, p, pos) should get the first entry with offset >= *pos (or NULL if we are done) and set *pos accordingly.

That m->index++ is "we are done with the partial, step just past it, so that ->start() will pick the first real entry after it the next time it's called".

For dense case we don't need to update *pos in ->start() - either we already have one with offset == *pos (and no update is needed), or we are finished and should return NULL.

However, we have every right to live with sparse offsets; prototype of ->start() had been done the way it's done exactly to allow that kind of use.

#9 lore raw

Re: [patch 2/2] Sort module list by pointer address to get coherent sleepable seq_file iterators

Quoted 22 lines · Al Viro
Quoted 11 lines · Fengguang Wu

static void *ct_seq_next(struct seq_file *s, void *v, loff_t *pos)

{
        loff_t *spos = (loff_t *) v;
        *pos = ++(*spos);
        return spos;
}

I mean 'pos' is sometimes increased in ct_seq_next(), and sometimes from seq_file.c/seq_read(), too. Thus we cannot reliably do this:

        *pos = (*spos) + some_variable_offset;

Of course we can. These guys can be sparse - note that ->start() takes a pointer, and for a good reason. ->start(m, p, pos) should get the first entry with offset >= *pos (or NULL if we are done) and set *pos accordingly.

That m->index++ is "we are done with the partial, step just past it, so that ->start() will pick the first real entry after it the next time it's called".

Quoted 7 lines

For dense case we don't need to update *pos in ->start() - either we already have one with offset == *pos (and no update is needed), or we are finished and should return NULL.

However, we have every right to live with sparse offsets; prototype of ->start() had been done the way it's done exactly to allow that kind of use.

So sparse offsets are supported, with some special cares on ->start.

My case is to scan the address space in ranges. The "object" is the start offset of a range:

__________________#######______________________#############__________
^start ^start

Now the solution can be:
- ->show shows the current range
- ->next seeks to next range
- ->start must *also* do the seek

The last requirement is made clear by you, a fact I refused to accept :) My old concept was that a ->next should be called to move pages forward after a new start.

#10 lore raw

Re: [patch 2/2] Sort module list by pointer address to get coherent sleepable seq_file iterators

In reply to Al Viro

Al Viro,

Does this sounds like a good fix?
===

seq_file version fixes

- f_version is 'unsigned long', it's pointless to do more than that.
- m->version should not be reset when we are bumping up the buf size.

Signed-off-by Fengguang Wu <[email protected]>
---
 fs/seq_file.c            |    1 -
 include/linux/seq_file.h |    2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)
include/linux/seq_file.h +1 -1
@@ -18,7 +18,7 @@ struct seq_file {
 	size_t from;
 	size_t count;
 	loff_t index;
-	loff_t version;
+	unsigned long version;
 	struct mutex lock;
 	const struct seq_operations *op;
 	void *private;
fs/seq_file.c +0 -1
@@ -134,7 +134,6 @@ ssize_t seq_read(struct file *file, char
 		if (!m->buf)
 			goto Enomem;
 		m->count = 0;
-		m->version = 0;
 	}
 	m->op->stop(m, p);
 	m->count = 0;

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#12 lore raw

Re: [patch 2/2] Sort module list by pointer address to get coherent sleepable seq_file iterators

Quoted 8 lines · * Fengguang Wu ([email protected]) wrote:

Al Viro,

Does this sounds like a good fix?
===

seq_file version fixes

- f_version is 'unsigned long', it's pointless to do more than that.

Hrm, this is weird...

fs.h:

struct inode
u64 i_version;

and

struct file
unsigned long f_version;

Users do:

fs/ext3/dir.c:

if (filp->f_version != inode->i_version) {

So why isn't f_version a u64 ? It becomes a problem if versions gets higher than 2^32 and we are on an architecture where longs are 32 bits. I think the problem is the f_version field type, not in seq_file at all. I'll prepare a patch for this.

Quoted 2 lines

- m->version should not be reset when we are bumping up the buf size.

Hrmmmm, what is this twisted use of versions anyway ?!?

If I look at other version users elsewhere in the kernel, they mostly do:

repeat:
f_version = i_version
do something
if (f_version != i_version)
repeat;

So they can see if the underlying inode has changed during the operation. seq_file does it completely the other way around:

m->version = f_version;
do something

and, well, versions are never really used at all.

If we want to use versioning there, we should keep a version counter associated with the ressource pointed used by seq_files that would be incremented each time the data structures are modified.

Then, in the read side, we could sanely do:

seq open():
f_version = current version

seq read():
repeat:
m->version = f_version;
do something
if (m->version != current version)
repeat;

This would only make sure that the given read operation has consistent data. It would not certify data consistency across reads.

I have looked at fs/proc.c/task_mmu.c use of m->version, and I think it is just really weird. I think the proper way to do it would be to put the last_addr in a field of a structure to which m->private would point to.

Mathieu

Quoted 34 lines
Signed-off-by Fengguang Wu <[email protected]>
---
 fs/seq_file.c            |    1 -
 include/linux/seq_file.h |    2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)
include/linux/seq_file.h +1 -1
@@ -18,7 +18,7 @@ struct seq_file {
 	size_t from;
 	size_t count;
 	loff_t index;
-	loff_t version;
+	unsigned long version;
 	struct mutex lock;
 	const struct seq_operations *op;
 	void *private;
fs/seq_file.c +0 -2
@@ -134,7 +134,6 @@ ssize_t seq_read(struct file *file, char
 		if (!m->buf)
 			goto Enomem;
 		m->count = 0;
-		m->version = 0;
 	}
 	m->op->stop(m, p);
 	m->count = 0;
 
-

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/

Signature · 3 lines
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68
author
#13 lore raw

PATCH 1/1 Fix f_version type: should be u64 instead of unsigned long

In reply to a message not in the archive

Fix f_version type: should be u64 instead of long

There is a type inconsistency between struct inode i_version and struct file f_version.

fs.h:

struct inode
u64 i_version;

and

struct file
unsigned long f_version;

Users do:

fs/ext3/dir.c:

if (filp->f_version != inode->i_version) {

So why isn't f_version a u64 ? It becomes a problem if versions gets higher than 2^32 and we are on an architecture where longs are 32 bits.

This patch changes the f_version type to u64, and updates the users accordingly.

It applies to 2.6.23-rc2-mm2.

Signed-off-by Mathieu Desnoyers <[email protected]>
CC Martin Bligh <[email protected]>
CC Randy Dunlap <[email protected]>
CC Al Viro <[email protected]>
---
 fs/ext3/dir.c            |    2 +-
 fs/ext4/dir.c            |    2 +-
 fs/ocfs2/dir.c           |    2 +-
 fs/proc/base.c           |    4 ++--
 include/linux/fs.h       |    2 +-
 include/linux/seq_file.h |    2 +-
 6 files changed, 7 insertions(+), 7 deletions(-)
include/linux/fs.h +1 -1
Index: linux-2.6-lttng/include/linux/fs.h
===================================================================
@@ -799,7 +799,7 @@ struct file {
 	unsigned int		f_uid, f_gid;
 	struct file_ra_state	f_ra;
 
-	unsigned long		f_version;
+	u64			f_version;
 #ifdef CONFIG_SECURITY
 	void			*f_security;
 #endif
fs/ext3/dir.c +1 -1
Index: linux-2.6-lttng/fs/ext3/dir.c
===================================================================
@@ -210,7 +210,7 @@ revalidate:
 				 * not the directory has been modified
 				 * during the copy operation.
 				 */
-				unsigned long version = filp->f_version;
+				u64 version = filp->f_version;
 
 				error = filldir(dirent, de->name,
 						de->name_len,
fs/ext4/dir.c +1 -1
Index: linux-2.6-lttng/fs/ext4/dir.c
===================================================================
@@ -210,7 +210,7 @@ revalidate:
 				 * not the directory has been modified
 				 * during the copy operation.
 				 */
-				unsigned long version = filp->f_version;
+				u64 version = filp->f_version;
 
 				error = filldir(dirent, de->name,
 						de->name_len,
fs/ocfs2/dir.c +1 -1
Index: linux-2.6-lttng/fs/ocfs2/dir.c
===================================================================
@@ -183,7 +183,7 @@ revalidate:
 				 * not the directory has been modified
 				 * during the copy operation.
 				 */
-				unsigned long version = filp->f_version;
+				u64 version = filp->f_version;
 				unsigned char d_type = DT_UNKNOWN;
 
 				if (de->file_type < OCFS2_FT_MAX)
fs/proc/base.c +2 -2
Index: linux-2.6-lttng/fs/proc/base.c
===================================================================
@@ -2570,7 +2570,7 @@ static int proc_task_readdir(struct file
 	/* f_version caches the tgid value that the last readdir call couldn't
 	 * return. lseek aka telldir automagically resets f_version to 0.
 	 */
-	tid = filp->f_version;
+	tid = (int)filp->f_version;
 	filp->f_version = 0;
 	for (task = first_tid(leader, tid, pos - 2);
 	     task;

@@ -2579,7 +2579,7 @@ static int proc_task_readdir(struct file
 		if (proc_task_fill_cache(filp, dirent, filldir, task, tid) < 0) {
 			/* returning this tgid failed, save it as the first
 			 * pid for the next readir call */
-			filp->f_version = tid;
+			filp->f_version = (u64)tid;
 			put_task_struct(task);
 			break;
 		}
include/linux/seq_file.h +1 -1
Index: linux-2.6-lttng/include/linux/seq_file.h
===================================================================
@@ -18,7 +18,7 @@ struct seq_file {
 	size_t from;
 	size_t count;
 	loff_t index;
-	loff_t version;
+	u64 version;
 	struct mutex lock;
 	const struct seq_operations *op;
 	void *private;
Signature · 3 lines
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68
author
#14 lore raw

PATCH 1/1 Sort module list - use ppos instead of m->private

In reply to Al Viro

Sort modules list - use ppos instead of m->private

When reading the data by small chunks (i.e. byte by byte), the index (ppos) is incremented by seq_read() directly and no "next" callback is called when going to the next module.

Therefore, use ppos instead of m->private to deal with the fact that this index is incremented directly to pass to the next module in seq_read() after the buffer has been emptied.

Signed-off-by Mathieu Desnoyers <[email protected]>
---
 fs/seq_file.c            |   17 +++++++++--------
 include/linux/seq_file.h |    4 ++--
 kernel/module.c          |    6 ++----
 3 files changed, 13 insertions(+), 14 deletions(-)
kernel/module.c +2 -4
Index: linux-2.6-lttng/kernel/module.c
===================================================================
@@ -2418,14 +2418,12 @@ unsigned long module_kallsyms_lookup_nam
 static void *m_start(struct seq_file *m, loff_t *pos)
 {
 	mutex_lock(&module_mutex);
-	if (!*pos)
-		m->private = NULL;
-	return seq_sorted_list_start(&modules, m->private);
+	return seq_sorted_list_start(&modules, (void*)(long)pos);
 }
 
 static void *m_next(struct seq_file *m, void *p, loff_t *pos)
 {
-	return seq_sorted_list_next(p, &modules, &m->private);
+	return seq_sorted_list_next(p, &modules, (void**)pos);
 }
 
 static void m_stop(struct seq_file *m, void *p)
include/linux/seq_file.h +2 -2
Index: linux-2.6-lttng/include/linux/seq_file.h
===================================================================
@@ -73,9 +73,9 @@ extern struct list_head *seq_list_next(v
  * seq_sorted_list_start_head().
  */
 extern struct list_head *seq_sorted_list_start(struct list_head *head,
-		void *pos);
+		void **ppos);
 extern struct list_head *seq_sorted_list_start_head(struct list_head *head,
-		void *pos);
+		void **ppos);
 /*
  * next must be called with an existing p node
  */
fs/seq_file.c +9 -8
Index: linux-2.6-lttng/fs/seq_file.c
===================================================================
@@ -501,27 +501,28 @@ struct list_head *seq_list_next(void *v,
 
 EXPORT_SYMBOL(seq_list_next);
 
-struct list_head *seq_sorted_list_start(struct list_head *head, void *pos)
+struct list_head *seq_sorted_list_start(struct list_head *head, void **ppos)
 {
 	struct list_head *lh;
 
 	list_for_each(lh, head)
-		if ((void*)lh >= pos)
-			return lh;
+		if ((void*)lh >= *ppos)
+			return *ppos = lh;
 	return NULL;
 }
 
 EXPORT_SYMBOL(seq_sorted_list_start);
 
-struct list_head *seq_sorted_list_start_head(struct list_head *head, void *pos)
+struct list_head *seq_sorted_list_start_head(struct list_head *head,
+		void **ppos)
 {
 	struct list_head *lh;
 
-	if (!pos)
-		return head;
+	if (!ppos)
+		return *ppos = head;
 	list_for_each(lh, head)
-		if ((void*)lh >= pos)
-			return lh->prev;
+		if ((void*)lh >= *ppos)
+			return *ppos = lh->prev;
 	return NULL;
 }
Signature · 3 lines
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68
#15 lore raw

Re: [PATCH] Sort module list - use ppos instead of m->private

Quoted 9 lines · On Fri, 24 Aug 2007 11:39:33 -0400 Mathieu Desnoyers…

Sort modules list - use ppos instead of m->private

When reading the data by small chunks (i.e. byte by byte), the index (ppos) is incremented by seq_read() directly and no "next" callback is called when going to the next module.

Therefore, use ppos instead of m->private to deal with the fact that this index is incremented directly to pass to the next module in seq_read() after the buffer has been emptied.

Confused. What problem is this patch fixing? I'm guessing that something is going wrong when /proc/modules is read one-byte-at-a-time?

<tests that>

<nope>

Better changelogs, please.

author
#16 lore raw

Re: [PATCH] Sort module list - use ppos instead of m->private

Quoted 13 lines · * Andrew Morton ([email protected]) wrote:
Quoted 9 lines · On Fri, 24 Aug 2007 11:39:33 -0400 Mathieu Desnoyers…

Sort modules list - use ppos instead of m->private

When reading the data by small chunks (i.e. byte by byte), the index (ppos) is incremented by seq_read() directly and no "next" callback is called when going to the next module.

Therefore, use ppos instead of m->private to deal with the fact that this index is incremented directly to pass to the next module in seq_read() after the buffer has been emptied.

Quoted 9 lines

Confused. What problem is this patch fixing? I'm guessing that something is going wrong when /proc/modules is read one-byte-at-a-time?

<tests that>

<nope>

Better changelogs, please.

Ok, will append this in the changelog (I sent this to Rusty earlier today):

Small test program for this:

#include <stdio.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>

#define BUFSIZE 1024

int main()

{
        int fd = open("/proc/modules", O_RDONLY);
        char buf[BUFSIZE];
        ssize_t size;

        do {
                size = read(fd, buf, 1);
                printf("%c", buf[0]);
                usleep(100000);
        } while(size > 0);

        close(fd);
        return 0;
}

Before fix, it prints the first module indefinitely. The patch fixes this.

I will also append more detail to "Sort module list by pointer address to get coherent sleepable seq_file iterators" changelog before the 2.6.23-rc3-mm1 repost.

Mathieu

Signature · 3 lines
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68