[PATCH 0/4] Tidy up of modules using seq_open()

[PATCH 0/4] Tidy up of modules using seq_open()

[Rob Jones]

Many modules use seq_open() when seq_open_private() or
__seq_open_private() would be more appropriate and result in
simpler, cleaner code.

This patch sequence changes those instances in IPC, MM and LIB.

Rob Jones (4):
  ipc: Use __seq_open_private() instead of seq_open()
  mm: Use seq_open_private() instead of seq_open()
  mm: Use __seq_open_private() instead of seq_open()
  lib: Use seq_open_private() instead of seq_open()

 ipc/util.c          |   20 ++++----------------
 lib/dynamic_debug.c |   17 ++---------------
 mm/slab.c           |   22 +++++++++-------------
 mm/vmalloc.c        |   20 +++++---------------
 4 files changed, 20 insertions(+), 59 deletions(-)

-- 
1.7.10.4

[PATCH 1/4] ipc: Use __seq_open_private() instead of seq_open()

[Rob Jones]

Using __seq_open_private() removes boilerplate code from
sysvipc_proc_open().

The resultant code is shorter and easier to follow.

However, please note that  __seq_open_private() call kzalloc() rather than
kmalloc() which may affect timing due to the memory initialisation overhead.

Signed-off-by: Rob Jones <rob.jones@codethink.co.uk>
---
 ipc/util.c |   20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/ipc/util.c b/ipc/util.c
index 2eb0d1e..98cb51d 100644
--- a/ipc/util.c
+++ b/ipc/util.c
@@ -892,28 +892,16 @@ static const struct seq_operations sysvipc_proc_seqops = {
 
 static int sysvipc_proc_open(struct inode *inode, struct file *file)
 {
-	int ret;
-	struct seq_file *seq;
 	struct ipc_proc_iter *iter;
 
-	ret = -ENOMEM;
-	iter = kmalloc(sizeof(*iter), GFP_KERNEL);
+	iter = __seq_open_private(file, &sysvipc_proc_seqops, sizeof(*iter));
 	if (!iter)
-		goto out;
-
-	ret = seq_open(file, &sysvipc_proc_seqops);
-	if (ret) {
-		kfree(iter);
-		goto out;
-	}
-
-	seq = file->private_data;
-	seq->private = iter;
+		return -ENOMEM;
 
 	iter->iface = PDE_DATA(inode);
 	iter->ns    = get_ipc_ns(current->nsproxy->ipc_ns);
-out:
-	return ret;
+
+	return 0;
 }
 
 static int sysvipc_proc_release(struct inode *inode, struct file *file)
-- 
1.7.10.4

[PATCH 2/4] mm: Use seq_open_private() instead of seq_open()

[Rob Jones]

Using seq_open_private() removes boilerplate code from vmalloc_open().

The resultant code is shorter and easier to follow.

However, please note that seq_open_private() call kzalloc() rather than
kmalloc() which may affect timing due to the memory initialisation overhead.

Signed-off-by: Rob Jones <rob.jones@codethink.co.uk>
---
 mm/vmalloc.c |   20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index bf233b2..0d6caee 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -2647,21 +2647,11 @@ static const struct seq_operations vmalloc_op = {
 
 static int vmalloc_open(struct inode *inode, struct file *file)
 {
-	unsigned int *ptr = NULL;
-	int ret;
-
-	if (IS_ENABLED(CONFIG_NUMA)) {
-		ptr = kmalloc(nr_node_ids * sizeof(unsigned int), GFP_KERNEL);
-		if (ptr == NULL)
-			return -ENOMEM;
-	}
-	ret = seq_open(file, &vmalloc_op);
-	if (!ret) {
-		struct seq_file *m = file->private_data;
-		m->private = ptr;
-	} else
-		kfree(ptr);
-	return ret;
+	if (IS_ENABLED(CONFIG_NUMA))
+		return seq_open_private(file, &vmalloc_op,
+					nr_node_ids * sizeof(unsigned int));
+	else
+		return seq_open(file, &vmalloc_op);
 }
 
 static const struct file_operations proc_vmalloc_operations = {
-- 
1.7.10.4

[PATCH 3/4] mm: Use __seq_open_private() instead of seq_open()

[Rob Jones]

Using __seq_open_private() removes boilerplate code from slabstats_open()

The resultant code is shorter and easier to follow.

This patch does not change any functionality.

Signed-off-by: Rob Jones <rob.jones@codethink.co.uk>
---
 mm/slab.c |   22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index 19d9218..d67f319 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -4339,19 +4339,15 @@ static const struct seq_operations slabstats_op = {
 
 static int slabstats_open(struct inode *inode, struct file *file)
 {
-	unsigned long *n = kzalloc(PAGE_SIZE, GFP_KERNEL);
-	int ret = -ENOMEM;
-	if (n) {
-		ret = seq_open(file, &slabstats_op);
-		if (!ret) {
-			struct seq_file *m = file->private_data;
-			*n = PAGE_SIZE / (2 * sizeof(unsigned long));
-			m->private = n;
-			n = NULL;
-		}
-		kfree(n);
-	}
-	return ret;
+	unsigned long *n;
+
+	n = __seq_open_private(file, &slabstats_op, PAGE_SIZE);
+	if (!n)
+		return -ENOMEM;
+
+	*n = PAGE_SIZE / (2 * sizeof(unsigned long));
+
+	return 0;
 }
 
 static const struct file_operations proc_slabstats_operations = {
-- 
1.7.10.4

Re: [PATCH 3/4] mm: Use __seq_open_private() instead of seq_open()

[Christoph Lameter]

On Fri, 29 Aug 2014, Rob Jones wrote:

> Using __seq_open_private() removes boilerplate code from slabstats_open()

Acked-by: Christoph Lameter <cl@linux.com>

[PATCH 4/4] lib: Use seq_open_private() instead of seq_open()

[Rob Jones]

Using seq_open_private() removes boilerplate code from ddebug_proc_open()

The resultant code is shorter and easier to follow.

This patch does not change any functionality.

Signed-off-by: Rob Jones <rob.jones@codethink.co.uk>
---
 lib/dynamic_debug.c |   17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 7288e38..e067fb5 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -827,22 +827,9 @@ static const struct seq_operations ddebug_proc_seqops = {
  */
 static int ddebug_proc_open(struct inode *inode, struct file *file)
 {
-	struct ddebug_iter *iter;
-	int err;
-
 	vpr_info("called\n");
-
-	iter = kzalloc(sizeof(*iter), GFP_KERNEL);
-	if (iter == NULL)
-		return -ENOMEM;
-
-	err = seq_open(file, &ddebug_proc_seqops);
-	if (err) {
-		kfree(iter);
-		return err;
-	}
-	((struct seq_file *)file->private_data)->private = iter;
-	return 0;
+	return seq_open_private(file, &ddebug_proc_seqops,
+				sizeof(struct ddebug_iter));
 }
 
 static const struct file_operations ddebug_proc_fops = {
-- 
1.7.10.4

Re: [PATCH 4/4] lib: Use seq_open_private() instead of seq_open()

[Jason Baron]

On 08/29/2014 12:06 PM, Rob Jones wrote:
> Using seq_open_private() removes boilerplate code from ddebug_proc_open()
>

Looks good.

Acked-by: Jason Baron <jbaron@akamai.com>

Re: [PATCH 0/4] Tidy up of modules using seq_open()

[Andrew Morton]

On Fri, 29 Aug 2014 17:06:36 +0100 Rob Jones <rob.jones@codethink.co.uk> wrote:

> Many modules use seq_open() when seq_open_private() or
> __seq_open_private() would be more appropriate and result in
> simpler, cleaner code.
> 
> This patch sequence changes those instances in IPC, MM and LIB.

Looks good to me.

I can't begin to imagine why we added the global, exported-to-modules
seq_open_private() without bothering to document it, so any time you
feel like adding the missing kerneldoc...

And psize should have been size_t, ho hum.

Re: [PATCH 0/4] Tidy up of modules using seq_open()

[Rob Jones]

On 29/08/14 20:14, Andrew Morton wrote:
> On Fri, 29 Aug 2014 17:06:36 +0100 Rob Jones <rob.jones@codethink.co.uk> wrote:
>
>> Many modules use seq_open() when seq_open_private() or
>> __seq_open_private() would be more appropriate and result in
>> simpler, cleaner code.
>>
>> This patch sequence changes those instances in IPC, MM and LIB.
>
> Looks good to me.
>
> I can't begin to imagine why we added the global, exported-to-modules
> seq_open_private() without bothering to document it, so any time you
> feel like adding the missing kerneldoc...

Already done, I waited for that to be accepted before I submitted this
patch :-)

>
> And psize should have been size_t, ho hum.

I'll fix that while I'm in the vicinity.

>
>

-- 
Rob Jones
Codethink Ltd
mailto:rob.jones@codethink.co.uk
tel:+44 161 236 5575