[PATCH] use seq_file for /proc/swaps

[PATCH] use seq_file for /proc/swaps

[Randy.Dunlap]

Hi,

This patch to 2.5.44 converts /proc/swaps to use seq_file.

It's basically the same patch that I posted a few days ago
with locking added [using swap_list_lock() and
swap_list_unlock(), as directed by Al].

Any comments on this version?

Thanks,
-- 
~Randy



--- ./fs/proc/proc_misc.c%swaps	Fri Oct 18 21:01:14 2002
+++ ./fs/proc/proc_misc.c	Mon Oct 21 20:11:48 2002
@@ -295,6 +295,18 @@
 	.release	= seq_release,
 };

+extern struct seq_operations swaps_op;
+static int swaps_open(struct inode *inode, struct file *file)
+{
+	return seq_open(file, &swaps_op);
+}
+static struct file_operations proc_swaps_operations = {
+	.open		= swaps_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= seq_release,
+};
+
 #ifdef CONFIG_MODULES
 extern struct seq_operations modules_op;
 static int modules_open(struct inode *inode, struct file *file)
@@ -503,13 +515,6 @@
 	return proc_calc_metrics(page, start, off, count, eof, len);
 }

-static int swaps_read_proc(char *page, char **start, off_t off,
-				 int count, int *eof, void *data)
-{
-	int len = get_swaparea_info(page);
-	return proc_calc_metrics(page, start, off, count, eof, len);
-}
-
 static int memory_read_proc(char *page, char **start, off_t off,
 				 int count, int *eof, void *data)
 {
@@ -616,7 +621,6 @@
 		{"rtc",		ds1286_read_proc},
 #endif
 		{"locks",	locks_read_proc},
-		{"swaps",	swaps_read_proc},
 		{"iomem",	memory_read_proc},
 		{"execdomains",	execdomains_read_proc},
 		{NULL,}
@@ -632,6 +636,7 @@
 		entry->proc_fops = &proc_kmsg_operations;
 	create_seq_entry("cpuinfo", 0, &proc_cpuinfo_operations);
 	create_seq_entry("partitions", 0, &proc_partitions_operations);
+	create_seq_entry("swaps", 0, &proc_swaps_operations);
 #if !defined(CONFIG_ARCH_S390)
 	create_seq_entry("interrupts", 0, &proc_interrupts_operations);
 #endif
--- ./mm/swapfile.c%swaps	Fri Oct 18 21:01:17 2002
+++ ./mm/swapfile.c	Mon Oct 28 19:59:11 2002
@@ -15,6 +15,7 @@
 #include <linux/shm.h>
 #include <linux/blkdev.h>
 #include <linux/buffer_head.h>
+#include <linux/seq_file.h>

 #include <asm/pgtable.h>
 #include <linux/swapops.h>
@@ -1041,45 +1042,91 @@
 	return err;
 }

-int get_swaparea_info(char *buf)
+#ifdef CONFIG_PROC_FS
+/* iterator */
+static void *swap_start(struct seq_file *swap, loff_t *pos)
 {
-	char * page = (char *) __get_free_page(GFP_KERNEL);
 	struct swap_info_struct *ptr = swap_info;
-	int i, len;
+	int i;
+	loff_t l = *pos;
+	char * page = (char *) __get_free_page(GFP_KERNEL);

-	if (!page)
-		return -ENOMEM;
+	swap->private = page;	/* save for swap_show */
+	swap_list_lock();

-	len = sprintf(buf, "Filename\t\t\t\tType\t\tSize\tUsed\tPriority\n");
-	for (i = 0 ; i < nr_swapfiles ; i++, ptr++) {
-		int j, usedswap;
-		struct file *file;
-		char *path;
+	if (!page)
+		return ERR_PTR(-ENOMEM);

+	for (i = 0; i < nr_swapfiles; i++, ptr++) {
 		if (!(ptr->flags & SWP_USED) || !ptr->swap_map)
 			continue;
+		if (!l--)
+			return ptr;
+	}

-		file = ptr->swap_file;
-		path = d_path(file->f_dentry, file->f_vfsmnt, page, PAGE_SIZE);
-		for (j = 0,usedswap = 0; j < ptr->max; ++j)
-			switch (ptr->swap_map[j]) {
-				case SWAP_MAP_BAD:
-				case 0:
-					continue;
-				default:
-					usedswap++;
-			}
-		len += sprintf(buf + len, "%-39s %s\t%d\t%d\t%d\n",
-			       path,
-			       S_ISBLK(file->f_dentry->d_inode->i_mode) ?
-					"partition" : "file\t",
-			       ptr->pages << (PAGE_SHIFT - 10),
-			       usedswap << (PAGE_SHIFT - 10),
-			       ptr->prio);
+	return NULL;
+}
+
+static void *swap_next(struct seq_file *swap, void *v, loff_t *pos)
+{
+	struct swap_info_struct *ptr = v;
+	void *endptr = (void *) swap_info + nr_swapfiles * sizeof(struct swap_info_struct);
+
+	for (++ptr; ptr < (struct swap_info_struct *) endptr; ptr++) {
+		if (!(ptr->flags & SWP_USED) || !ptr->swap_map)
+			continue;
+		++*pos;
+		return ptr;
 	}
-	free_page((unsigned long) page);
-	return len;
+
+	return NULL;
+}
+
+static void swap_stop(struct seq_file *swap, void *v)
+{
+	swap_list_unlock();
+	free_page((unsigned long) swap->private);
+	swap->private = NULL;
+}
+
+static int swap_show(struct seq_file *swap, void *v)
+{
+	struct swap_info_struct *ptr = v;
+	int j, usedswap;
+	struct file *file;
+	char *path;
+
+	if (v == swap_info)
+		seq_puts(swap, "Filename\t\t\t\tType\t\tSize\tUsed\tPriority\n");
+
+	file = ptr->swap_file;
+	path = d_path(file->f_dentry, file->f_vfsmnt, swap->private, PAGE_SIZE);
+
+	for (j = 0, usedswap = 0; j < ptr->max; ++j)
+		switch (ptr->swap_map[j]) {
+			case SWAP_MAP_BAD:
+			case 0:
+				continue;
+			default:
+				usedswap++;
+		}
+	seq_printf(swap, "%-39s %s\t%d\t%d\t%d\n",
+		       path,
+		       S_ISBLK(file->f_dentry->d_inode->i_mode) ?
+				"partition" : "file\t",
+		       ptr->pages << (PAGE_SHIFT - 10),
+		       usedswap << (PAGE_SHIFT - 10),
+		       ptr->prio);
+	return 0;
 }
+
+struct seq_operations swaps_op = {
+	.start =	swap_start,
+	.next =		swap_next,
+	.stop =		swap_stop,
+	.show =		swap_show
+};
+#endif

 /*
  * Written 01/25/92 by Simmule Turner, heavily changed by Linus.

Re: [PATCH] use seq_file for /proc/swaps

[Christoph Hellwig]

On Mon, Oct 28, 2002 at 09:36:44PM -0800, Randy.Dunlap wrote:
> 
> Hi,
> 
> This patch to 2.5.44 converts /proc/swaps to use seq_file.
> 
> It's basically the same patch that I posted a few days ago
> with locking added [using swap_list_lock() and
> swap_list_unlock(), as directed by Al].
> 
> Any comments on this version?

Looks fine.  Any chance you could move proc_swaps_operations and
the entry creating to swapfile.c so when uclinux makes this file
conditional on CONFIG_SWAP we don't need ifdefs in proc_misc.c?

Re: [PATCH] use seq_file for /proc/swaps

[Randy.Dunlap]

On Tue, 29 Oct 2002, Christoph Hellwig wrote:

| On Mon, Oct 28, 2002 at 09:36:44PM -0800, Randy.Dunlap wrote:
| >
| > Hi,
| >
| > This patch to 2.5.44 converts /proc/swaps to use seq_file.
| >
| > It's basically the same patch that I posted a few days ago
| > with locking added [using swap_list_lock() and
| > swap_list_unlock(), as directed by Al].
| >
| > Any comments on this version?
|
| Looks fine.  Any chance you could move proc_swaps_operations and
| the entry creating to swapfile.c so when uclinux makes this file
| conditional on CONFIG_SWAP we don't need ifdefs in proc_misc.c?

Are you and/or Greg U. going to do a CONFIG_SWAP option?

Sure, some (small) chance of moving it.  I just tried that exercise,
and the results aren't very interesting, at least not to me, and
here's why.

I moved the proc_swaps_operations to swapfile.c like you asked.
I like that part of this patch.
But it makes more sense to me to move the create_seq_entry() call
to mm/swap.c:: in its __init swap_setup() function
(or add another __init function in swapfile.c to do this ?).
That meant that I had to duplicate create_seq_entry() or
export it.  For now I have duplicated it, and that would make
3 copies of it in the kernel -- and that's bad IMO, so it would
need to be exported (but I didn't do that for now).

And then there's the issue of do we want to keep the
/proc misc entries closely located (in source code) or not...
I kinda like having them in one place.

And there's the question of __init ordering: when will the __init
function mm/swap.c::swap_setup(), calling create_seq_entry(),
happen in relation to create_proc_entry() being ready to work?
I.e., is there a chance that create_seq_entry() could fail
even though it shouldn't?  This should be solvable by using
initcall levels (subsystem levels), but I didn't look closely
at that yet.

So yeah, it's do-able.  I just don't see that it's worth it.

Patch below (to previous patch, which is already in BK) compiles.
Not tested.

-- 
~Randy




--- ./fs/proc/proc_misc.c%swap2	Mon Oct 21 20:11:48 2002
+++ ./fs/proc/proc_misc.c	Tue Oct 29 20:17:11 2002
@@ -295,18 +295,6 @@
 	.release	= seq_release,
 };

-extern struct seq_operations swaps_op;
-static int swaps_open(struct inode *inode, struct file *file)
-{
-	return seq_open(file, &swaps_op);
-}
-static struct file_operations proc_swaps_operations = {
-	.open		= swaps_open,
-	.read		= seq_read,
-	.llseek		= seq_lseek,
-	.release	= seq_release,
-};
-
 #ifdef CONFIG_MODULES
 extern struct seq_operations modules_op;
 static int modules_open(struct inode *inode, struct file *file)
@@ -636,7 +624,6 @@
 		entry->proc_fops = &proc_kmsg_operations;
 	create_seq_entry("cpuinfo", 0, &proc_cpuinfo_operations);
 	create_seq_entry("partitions", 0, &proc_partitions_operations);
-	create_seq_entry("swaps", 0, &proc_swaps_operations);
 #if !defined(CONFIG_ARCH_S390)
 	create_seq_entry("interrupts", 0, &proc_interrupts_operations);
 #endif
--- ./mm/swapfile.c%swap2	Mon Oct 28 20:56:06 2002
+++ ./mm/swapfile.c	Tue Oct 29 20:38:18 2002
@@ -6,6 +6,7 @@
  */

 #include <linux/config.h>
+#include <linux/module.h>
 #include <linux/mm.h>
 #include <linux/slab.h>
 #include <linux/kernel_stat.h>
@@ -1127,6 +1128,18 @@
 	.stop =		swap_stop,
 	.show =		swap_show
 };
+
+static int swaps_open(struct inode *inode, struct file *file)
+{
+	return seq_open(file, &swaps_op);
+}
+struct file_operations proc_swaps_operations = {
+	.open		= swaps_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= seq_release,
+};
+EXPORT_SYMBOL(proc_swaps_operations);
 #endif

 /*
--- ./mm/swap.c%swap2	Fri Oct 18 21:01:49 2002
+++ ./mm/swap.c	Tue Oct 29 20:36:53 2002
@@ -13,6 +13,7 @@
  * Buffermem limits added 12.3.98, Rik van Riel.
  */

+#include <linux/config.h>
 #include <linux/mm.h>
 #include <linux/kernel_stat.h>
 #include <linux/swap.h>
@@ -22,6 +23,7 @@
 #include <linux/mm_inline.h>
 #include <linux/buffer_head.h>
 #include <linux/prefetch.h>
+#include <linux/proc_fs.h>

 /* How many pages do we try to swap or page in/out together? */
 int page_cluster;
@@ -269,6 +271,18 @@
 	return pagevec_count(pvec);
 }

+#ifdef CONFIG_PROC_FS
+static void __init create_seq_entry(char *name, mode_t mode, struct file_operations *f)
+{
+	struct proc_dir_entry *entry;
+	entry = create_proc_entry(name, mode, NULL);
+	if (entry)
+		entry->proc_fops = f;
+}
+
+extern struct file_operations proc_swaps_operations;
+#endif
+
 /*
  * Perform any setup for the swap system
  */
@@ -285,4 +299,7 @@
 	 * Right now other parts of the system means that we
 	 * _really_ don't want to cluster much more
 	 */
+#ifdef CONFIG_PROC_FS
+	create_seq_entry("swaps", 0, &proc_swaps_operations);
+#endif
 }
--- ./mm/Makefile%swap2	Fri Oct 18 21:02:00 2002
+++ ./mm/Makefile	Tue Oct 29 20:39:20 2002
@@ -2,7 +2,8 @@
 # Makefile for the linux memory manager.
 #

-export-objs := shmem.o filemap.o mempool.o page_alloc.o page-writeback.o
+export-objs := shmem.o filemap.o mempool.o page_alloc.o page-writeback.o \
+		swapfile.o

 obj-y	 := memory.o mmap.o filemap.o mprotect.o mlock.o mremap.o \
 	    vmalloc.o slab.o bootmem.o swap.o vmscan.o page_io.o \

Re: [PATCH] use seq_file for /proc/swaps

[Christoph Hellwig]

On Tue, Oct 29, 2002 at 10:12:59PM -0800, Randy.Dunlap wrote:
> Are you and/or Greg U. going to do a CONFIG_SWAP option?

Yes. It's already in -ac.

> I moved the proc_swaps_operations to swapfile.c like you asked.
> I like that part of this patch.
> But it makes more sense to me to move the create_seq_entry() call
> to mm/swap.c:: in its __init swap_setup() function
> (or add another __init function in swapfile.c to do this ?).

Despite it's name swap.c has absolutely nothing to do with
swapping :)  So please add another initfunc to swapfile.c.

> That meant that I had to duplicate create_seq_entry() or
> export it.  For now I have duplicated it, and that would make
> 3 copies of it in the kernel -- and that's bad IMO, so it would
> need to be exported (but I didn't do that for now).

Not sure whether that three lines of code are really worth exporting :)

> And there's the question of __init ordering: when will the __init
> function mm/swap.c::swap_setup(), calling create_seq_entry(),
> happen in relation to create_proc_entry() being ready to work?

create_proc_entry() works as soon as kmalloc() works, i.e. it's fine
for all initcalls.`

Re: [PATCH] use seq_file for /proc/swaps

[Randy.Dunlap]

Hi Christoph,

I see you've already done this.  I was about to work on it
tonight (Monday night my time).  Oh well...thanks.


On Wed, 30 Oct 2002, Christoph Hellwig wrote:

| On Tue, Oct 29, 2002 at 10:12:59PM -0800, Randy.Dunlap wrote:
|
| > That meant that I had to duplicate create_seq_entry() or
| > export it.  For now I have duplicated it, and that would make
| > 3 copies of it in the kernel -- and that's bad IMO, so it would
| > need to be exported (but I didn't do that for now).
|
| Not sure whether that three lines of code are really worth exporting :)

Yeah, but I don't like duplicating code either.
More to find (or miss) whenever global changes are needed.

| > And there's the question of __init ordering: when will the __init
| > function mm/swap.c::swap_setup(), calling create_seq_entry(),
| > happen in relation to create_proc_entry() being ready to work?
|
| create_proc_entry() works as soon as kmalloc() works, i.e. it's fine
| for all initcalls.`

I was concerned about the ordering of procswaps_init() and
proc_root_init().  What happens if procswaps_init() is
called before proc_root_init() is called?
Or is there initcall ordering that prevents that?
If so, where is it?

-- 
~Randy