[PATCH 0/5] Simplify /proc/$pid/maps implementation

[PATCH 0/5] Simplify /proc/$pid/maps implementation

[Matthew Wilcox]

From: "Matthew Wilcox (Oracle)" <willy@infradead.org>

Back in 2005, we merged a patch from Akamai that sped up /proc/$pid/maps
by using f_version to stash the user virtual address that we'd just
displayed.  That wasn't necessary; we can just use the private *ppos for
the same purpose.  There have also been some other odd choices made over
the years that use the seq_file infrastructure in some non-idiomatic ways.

Tested by using 'dd' with various different 'bs=' parameters to check that
calling ->start, ->stop and ->next at various offsets work as expected.

Matthew Wilcox (Oracle) (5):
  proc: Inline vma_stop into m_stop
  proc: remove m_cache_vma
  proc: Use ppos instead of m->version
  seq_file: Remove m->version
  proc: Inline m_next_vma into m_next

 fs/proc/task_mmu.c       | 95 +++++++++++++---------------------------
 fs/seq_file.c            | 28 ------------
 include/linux/seq_file.h |  1 -
 3 files changed, 31 insertions(+), 93 deletions(-)

base-commit: d5226fa6dbae0569ee43ecfc08bdcd6770fc4755
-- 
2.25.0

[PATCH 1/5] proc: Inline vma_stop into m_stop

[Matthew Wilcox]

From: "Matthew Wilcox (Oracle)" <willy@infradead.org>

Instead of calling vma_stop() from m_start() and m_next(), do its work
in m_stop().

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/proc/task_mmu.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 9442631fd4af..41f63df4789d 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -123,15 +123,6 @@ static void release_task_mempolicy(struct proc_maps_private *priv)
 }
 #endif
 
-static void vma_stop(struct proc_maps_private *priv)
-{
-	struct mm_struct *mm = priv->mm;
-
-	release_task_mempolicy(priv);
-	up_read(&mm->mmap_sem);
-	mmput(mm);
-}
-
 static struct vm_area_struct *
 m_next_vma(struct proc_maps_private *priv, struct vm_area_struct *vma)
 {
@@ -163,11 +154,16 @@ static void *m_start(struct seq_file *m, loff_t *ppos)
 		return ERR_PTR(-ESRCH);
 
 	mm = priv->mm;
-	if (!mm || !mmget_not_zero(mm))
+	if (!mm || !mmget_not_zero(mm)) {
+		put_task_struct(priv->task);
+		priv->task = NULL;
 		return NULL;
+	}
 
 	if (down_read_killable(&mm->mmap_sem)) {
 		mmput(mm);
+		put_task_struct(priv->task);
+		priv->task = NULL;
 		return ERR_PTR(-EINTR);
 	}
 
@@ -195,7 +191,6 @@ static void *m_start(struct seq_file *m, loff_t *ppos)
 	if (pos == mm->map_count && priv->tail_vma)
 		return priv->tail_vma;
 
-	vma_stop(priv);
 	return NULL;
 }
 
@@ -206,21 +201,22 @@ static void *m_next(struct seq_file *m, void *v, loff_t *pos)
 
 	(*pos)++;
 	next = m_next_vma(priv, v);
-	if (!next)
-		vma_stop(priv);
 	return next;
 }
 
 static void m_stop(struct seq_file *m, void *v)
 {
 	struct proc_maps_private *priv = m->private;
+	struct mm_struct *mm = priv->mm;
 
-	if (!IS_ERR_OR_NULL(v))
-		vma_stop(priv);
-	if (priv->task) {
-		put_task_struct(priv->task);
-		priv->task = NULL;
-	}
+	if (!priv->task)
+		return;
+
+	release_task_mempolicy(priv);
+	up_read(&mm->mmap_sem);
+	mmput(mm);
+	put_task_struct(priv->task);
+	priv->task = NULL;
 }
 
 static int proc_maps_open(struct inode *inode, struct file *file,
-- 
2.25.0

[PATCH 2/5] proc: remove m_cache_vma

[Matthew Wilcox]

From: "Matthew Wilcox (Oracle)" <willy@infradead.org>

Instead of setting m->version in the show method, set it in m_next(),
where it should be.  Also remove the fallback code for failing to find
a vma, or version being zero.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/proc/task_mmu.c | 38 ++++++--------------------------------
 1 file changed, 6 insertions(+), 32 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 41f63df4789d..ff40d5d79f24 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -131,21 +131,14 @@ m_next_vma(struct proc_maps_private *priv, struct vm_area_struct *vma)
 	return vma->vm_next ?: priv->tail_vma;
 }
 
-static void m_cache_vma(struct seq_file *m, struct vm_area_struct *vma)
-{
-	if (m->count < m->size)	/* vma is copied successfully */
-		m->version = m_next_vma(m->private, vma) ? vma->vm_end : -1UL;
-}
-
 static void *m_start(struct seq_file *m, loff_t *ppos)
 {
 	struct proc_maps_private *priv = m->private;
 	unsigned long last_addr = m->version;
 	struct mm_struct *mm;
 	struct vm_area_struct *vma;
-	unsigned int pos = *ppos;
 
-	/* See m_cache_vma(). Zero at the start or after lseek. */
+	/* See m_next(). Zero at the start or after lseek. */
 	if (last_addr == -1UL)
 		return NULL;
 
@@ -170,28 +163,11 @@ static void *m_start(struct seq_file *m, loff_t *ppos)
 	hold_task_mempolicy(priv);
 	priv->tail_vma = get_gate_vma(mm);
 
-	if (last_addr) {
-		vma = find_vma(mm, last_addr - 1);
-		if (vma && vma->vm_start <= last_addr)
-			vma = m_next_vma(priv, vma);
-		if (vma)
-			return vma;
-	}
-
-	m->version = 0;
-	if (pos < mm->map_count) {
-		for (vma = mm->mmap; pos; pos--) {
-			m->version = vma->vm_start;
-			vma = vma->vm_next;
-		}
+	vma = find_vma(mm, last_addr);
+	if (vma)
 		return vma;
-	}
-
-	/* we do not bother to update m->version in this case */
-	if (pos == mm->map_count && priv->tail_vma)
-		return priv->tail_vma;
 
-	return NULL;
+	return priv->tail_vma;
 }
 
 static void *m_next(struct seq_file *m, void *v, loff_t *pos)
@@ -201,6 +177,8 @@ static void *m_next(struct seq_file *m, void *v, loff_t *pos)
 
 	(*pos)++;
 	next = m_next_vma(priv, v);
+	m->version = next ? next->vm_start : -1UL;
+
 	return next;
 }
 
@@ -359,7 +337,6 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma)
 static int show_map(struct seq_file *m, void *v)
 {
 	show_map_vma(m, v);
-	m_cache_vma(m, v);
 	return 0;
 }
 
@@ -843,8 +820,6 @@ static int show_smap(struct seq_file *m, void *v)
 		seq_printf(m, "ProtectionKey:  %8u\n", vma_pkey(vma));
 	show_smap_vma_flags(m, vma);
 
-	m_cache_vma(m, vma);
-
 	return 0;
 }
 
@@ -1883,7 +1858,6 @@ static int show_numa_map(struct seq_file *m, void *v)
 	seq_printf(m, " kernelpagesize_kB=%lu", vma_kernel_pagesize(vma) >> 10);
 out:
 	seq_putc(m, '\n');
-	m_cache_vma(m, vma);
 	return 0;
 }
 
-- 
2.25.0

[PATCH 3/5] proc: Use ppos instead of m->version

[Matthew Wilcox]

From: "Matthew Wilcox (Oracle)" <willy@infradead.org>

The ppos is a private cursor, just like m->version.  Use the canonical
cursor, not a special one.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/proc/task_mmu.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index ff40d5d79f24..9f10ef72b839 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -134,7 +134,7 @@ m_next_vma(struct proc_maps_private *priv, struct vm_area_struct *vma)
 static void *m_start(struct seq_file *m, loff_t *ppos)
 {
 	struct proc_maps_private *priv = m->private;
-	unsigned long last_addr = m->version;
+	unsigned long last_addr = *ppos;
 	struct mm_struct *mm;
 	struct vm_area_struct *vma;
 
@@ -170,14 +170,13 @@ static void *m_start(struct seq_file *m, loff_t *ppos)
 	return priv->tail_vma;
 }
 
-static void *m_next(struct seq_file *m, void *v, loff_t *pos)
+static void *m_next(struct seq_file *m, void *v, loff_t *ppos)
 {
 	struct proc_maps_private *priv = m->private;
 	struct vm_area_struct *next;
 
-	(*pos)++;
 	next = m_next_vma(priv, v);
-	m->version = next ? next->vm_start : -1UL;
+	*ppos = next ? next->vm_start : -1UL;
 
 	return next;
 }
-- 
2.25.0

[PATCH 4/5] seq_file: Remove m->version

[Matthew Wilcox]

From: "Matthew Wilcox (Oracle)" <willy@infradead.org>

The process maps file was the only user of version (introduced back
in 2005).  Now that it uses ppos instead, we can remove it.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/seq_file.c            | 28 ----------------------------
 include/linux/seq_file.h |  1 -
 2 files changed, 29 deletions(-)

diff --git a/fs/seq_file.c b/fs/seq_file.c
index 1600034a929b..79781ebd2145 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -67,13 +67,6 @@ int seq_open(struct file *file, const struct seq_operations *op)
 	// to the lifetime of the file.
 	p->file = file;
 
-	/*
-	 * Wrappers around seq_open(e.g. swaps_open) need to be
-	 * aware of this. If they set f_version themselves, they
-	 * should call seq_open first and then set f_version.
-	 */
-	file->f_version = 0;
-
 	/*
 	 * seq_files support lseek() and pread().  They do not implement
 	 * write() at all, but we clear FMODE_PWRITE here for historical
@@ -94,7 +87,6 @@ static int traverse(struct seq_file *m, loff_t offset)
 	int error = 0;
 	void *p;
 
-	m->version = 0;
 	m->index = 0;
 	m->count = m->from = 0;
 	if (!offset)
@@ -160,26 +152,12 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
 
 	mutex_lock(&m->lock);
 
-	/*
-	 * seq_file->op->..m_start/m_stop/m_next may do special actions
-	 * or optimisations based on the file->f_version, so we want to
-	 * pass the file->f_version to those methods.
-	 *
-	 * seq_file->version is just copy of f_version, and seq_file
-	 * methods can treat it simply as file version.
-	 * It is copied in first and copied out after all operations.
-	 * It is convenient to have it as  part of structure to avoid the
-	 * need of passing another argument to all the seq_file methods.
-	 */
-	m->version = file->f_version;
-
 	/*
 	 * if request is to read from zero offset, reset iterator to first
 	 * record as it might have been already advanced by previous requests
 	 */
 	if (*ppos == 0) {
 		m->index = 0;
-		m->version = 0;
 		m->count = 0;
 	}
 
@@ -190,7 +168,6 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
 		if (err) {
 			/* With prejudice... */
 			m->read_pos = 0;
-			m->version = 0;
 			m->index = 0;
 			m->count = 0;
 			goto Done;
@@ -243,7 +220,6 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
 		m->buf = seq_buf_alloc(m->size <<= 1);
 		if (!m->buf)
 			goto Enomem;
-		m->version = 0;
 		p = m->op->start(m, &m->index);
 	}
 	m->op->stop(m, p);
@@ -287,7 +263,6 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
 		*ppos += copied;
 		m->read_pos += copied;
 	}
-	file->f_version = m->version;
 	mutex_unlock(&m->lock);
 	return copied;
 Enomem:
@@ -313,7 +288,6 @@ loff_t seq_lseek(struct file *file, loff_t offset, int whence)
 	loff_t retval = -EINVAL;
 
 	mutex_lock(&m->lock);
-	m->version = file->f_version;
 	switch (whence) {
 	case SEEK_CUR:
 		offset += file->f_pos;
@@ -329,7 +303,6 @@ loff_t seq_lseek(struct file *file, loff_t offset, int whence)
 				/* with extreme prejudice... */
 				file->f_pos = 0;
 				m->read_pos = 0;
-				m->version = 0;
 				m->index = 0;
 				m->count = 0;
 			} else {
@@ -340,7 +313,6 @@ loff_t seq_lseek(struct file *file, loff_t offset, int whence)
 			file->f_pos = offset;
 		}
 	}
-	file->f_version = m->version;
 	mutex_unlock(&m->lock);
 	return retval;
 }
diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index 5998e1f4ff06..0d434bc4a779 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -21,7 +21,6 @@ struct seq_file {
 	size_t pad_until;
 	loff_t index;
 	loff_t read_pos;
-	u64 version;
 	struct mutex lock;
 	const struct seq_operations *op;
 	int poll_event;
-- 
2.25.0

[PATCH 5/5] proc: Inline m_next_vma into m_next

[Matthew Wilcox]

From: "Matthew Wilcox (Oracle)" <willy@infradead.org>

It's clearer to just put this inline.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/proc/task_mmu.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 9f10ef72b839..87641df8c0d0 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -123,14 +123,6 @@ static void release_task_mempolicy(struct proc_maps_private *priv)
 }
 #endif
 
-static struct vm_area_struct *
-m_next_vma(struct proc_maps_private *priv, struct vm_area_struct *vma)
-{
-	if (vma == priv->tail_vma)
-		return NULL;
-	return vma->vm_next ?: priv->tail_vma;
-}
-
 static void *m_start(struct seq_file *m, loff_t *ppos)
 {
 	struct proc_maps_private *priv = m->private;
@@ -173,9 +165,15 @@ static void *m_start(struct seq_file *m, loff_t *ppos)
 static void *m_next(struct seq_file *m, void *v, loff_t *ppos)
 {
 	struct proc_maps_private *priv = m->private;
-	struct vm_area_struct *next;
+	struct vm_area_struct *next, *vma = v;
+
+	if (vma == priv->tail_vma)
+		next = NULL;
+	else if (vma->vm_next)
+		next = vma->vm_next;
+	else
+		next = priv->tail_vma;
 
-	next = m_next_vma(priv, v);
 	*ppos = next ? next->vm_start : -1UL;
 
 	return next;
-- 
2.25.0

Re: [PATCH 0/5] Simplify /proc/$pid/maps implementation

[Vlastimil Babka]

On 2/29/20 5:59 PM, Matthew Wilcox wrote:
> From: "Matthew Wilcox (Oracle)" <willy@infradead.org>

Re: subject, it's not only maps, but also smaps, smaps_rollu, numa_maps

> Back in 2005, we merged a patch from Akamai that sped up /proc/$pid/maps
> by using f_version to stash the user virtual address that we'd just
> displayed.  That wasn't necessary; we can just use the private *ppos for
> the same purpose.  There have also been some other odd choices made over
> the years that use the seq_file infrastructure in some non-idiomatic ways.
> 
> Tested by using 'dd' with various different 'bs=' parameters to check that
> calling ->start, ->stop and ->next at various offsets work as expected.
> 
> Matthew Wilcox (Oracle) (5):
>   proc: Inline vma_stop into m_stop
>   proc: remove m_cache_vma
>   proc: Use ppos instead of m->version
>   seq_file: Remove m->version
>   proc: Inline m_next_vma into m_next

For all of them:

Acked-by: Vlastimil Babka <vbabka@suse.cz>
Thanks!

>  fs/proc/task_mmu.c       | 95 +++++++++++++---------------------------
>  fs/seq_file.c            | 28 ------------
>  include/linux/seq_file.h |  1 -
>  3 files changed, 31 insertions(+), 93 deletions(-)
> 
> base-commit: d5226fa6dbae0569ee43ecfc08bdcd6770fc4755
> 

Re: [PATCH 3/5] proc: Use ppos instead of m->version

[Alexey Dobriyan]

On Sat, Feb 29, 2020 at 08:59:08AM -0800, Matthew Wilcox wrote:
> -static void *m_next(struct seq_file *m, void *v, loff_t *pos)
> +static void *m_next(struct seq_file *m, void *v, loff_t *ppos)

This looks like hungarian notation.

Re: [PATCH 0/5] Simplify /proc/$pid/maps implementation

[Alexey Dobriyan]

On Sat, Feb 29, 2020 at 08:59:05AM -0800, Matthew Wilcox wrote:
> From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
> 
> Back in 2005, we merged a patch from Akamai that sped up /proc/$pid/maps
> by using f_version to stash the user virtual address that we'd just
> displayed.  That wasn't necessary; we can just use the private *ppos for
> the same purpose.  There have also been some other odd choices made over
> the years that use the seq_file infrastructure in some non-idiomatic ways.
> 
> Tested by using 'dd' with various different 'bs=' parameters to check that
> calling ->start, ->stop and ->next at various offsets work as expected.

/proc part looks OK, I only ask to include this description into first
patch, so it doesn't get lost. Often 0/N patch is the most interesting
part of a series.

Re: [PATCH 3/5] proc: Use ppos instead of m->version

[Matthew Wilcox]

On Tue, Mar 03, 2020 at 10:55:29PM +0300, Alexey Dobriyan wrote:
> On Sat, Feb 29, 2020 at 08:59:08AM -0800, Matthew Wilcox wrote:
> > -static void *m_next(struct seq_file *m, void *v, loff_t *pos)
> > +static void *m_next(struct seq_file *m, void *v, loff_t *ppos)
> 
> This looks like hungarian notation.

It's the standard naming convention used throughout the VFS.  loff_t is
pos, loff_t * is ppos.

$ git grep 'loff_t \*' fs/*.c |wc
     77     556    5233
$ git grep 'loff_t \*ppos' fs/*.c |wc
     43     309    2974
$ git grep 'loff_t \*pos' fs/*.c |wc
     22     168    1524

Re: [PATCH 0/5] Simplify /proc/$pid/maps implementation

[Matthew Wilcox]

On Tue, Mar 03, 2020 at 10:56:50PM +0300, Alexey Dobriyan wrote:
> On Sat, Feb 29, 2020 at 08:59:05AM -0800, Matthew Wilcox wrote:
> > From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
> > 
> > Back in 2005, we merged a patch from Akamai that sped up /proc/$pid/maps
> > by using f_version to stash the user virtual address that we'd just
> > displayed.  That wasn't necessary; we can just use the private *ppos for
> > the same purpose.  There have also been some other odd choices made over
> > the years that use the seq_file infrastructure in some non-idiomatic ways.
> > 
> > Tested by using 'dd' with various different 'bs=' parameters to check that
> > calling ->start, ->stop and ->next at various offsets work as expected.
> 
> /proc part looks OK, I only ask to include this description into first
> patch, so it doesn't get lost. Often 0/N patch is the most interesting
> part of a series.

I'm perfectly fine with this justification for the patch series being
lost.  I think this is the least interesting part of what I wrote.  And
will be the least interesting part for future researchers ... "Why did
this code get converted to behave exactly the same way as all the other
code" isn't really an interesting question.

Re: [PATCH 3/5] proc: Use ppos instead of m->version

[Alexey Dobriyan]

On Tue, Mar 03, 2020 at 12:29:23PM -0800, Matthew Wilcox wrote:
> On Tue, Mar 03, 2020 at 10:55:29PM +0300, Alexey Dobriyan wrote:
> > On Sat, Feb 29, 2020 at 08:59:08AM -0800, Matthew Wilcox wrote:
> > > -static void *m_next(struct seq_file *m, void *v, loff_t *pos)
> > > +static void *m_next(struct seq_file *m, void *v, loff_t *ppos)
> > 
> > This looks like hungarian notation.
> 
> It's the standard naming convention used throughout the VFS.  loff_t is
> pos, loff_t * is ppos.
> 
> $ git grep 'loff_t \*' fs/*.c |wc
>      77     556    5233
> $ git grep 'loff_t \*ppos' fs/*.c |wc
>      43     309    2974
> $ git grep 'loff_t \*pos' fs/*.c |wc
>      22     168    1524

Yes, people copy-pasted terrible thing for years!
Oh well, whatever...

Re: [PATCH 3/5] proc: Use ppos instead of m->version

[Matthew Wilcox]

On Tue, Mar 03, 2020 at 11:53:03PM +0300, Alexey Dobriyan wrote:
> On Tue, Mar 03, 2020 at 12:29:23PM -0800, Matthew Wilcox wrote:
> > On Tue, Mar 03, 2020 at 10:55:29PM +0300, Alexey Dobriyan wrote:
> > > On Sat, Feb 29, 2020 at 08:59:08AM -0800, Matthew Wilcox wrote:
> > > > -static void *m_next(struct seq_file *m, void *v, loff_t *pos)
> > > > +static void *m_next(struct seq_file *m, void *v, loff_t *ppos)
> > > 
> > > This looks like hungarian notation.
> > 
> > It's the standard naming convention used throughout the VFS.  loff_t is
> > pos, loff_t * is ppos.
> > 
> > $ git grep 'loff_t \*' fs/*.c |wc
> >      77     556    5233
> > $ git grep 'loff_t \*ppos' fs/*.c |wc
> >      43     309    2974
> > $ git grep 'loff_t \*pos' fs/*.c |wc
> >      22     168    1524
> 
> Yes, people copy-pasted terrible thing for years!
> Oh well, whatever...

In an environment where we sometimes pass loff_t and sometimes pass
loff_t *, this convention is a great way to catch copy-and-paste mistakes.
If I have 'pos += done' in a function which takes a loff_t pos, and I
copy-and-paste it to a function which takes a 'loff_t *pos', it's going
to create a bug that hits at runtime.  If that function takes an
loff_t *ppos instead, it'll be a compile-time error, and I'll know to
transform it to *ppos += done;