[PATCH] seq_file: remove redundant assignment of index to m->index

[PATCH] seq_file: remove redundant assignment of index to m->index

[Donglin Peng]

There are two redundant assignments in the traverse() function, because
the while loop will break after these two assignments, and after that
the variable index will be assigned to m->index again.

Signed-off-by: Peng Donglin <dolinux.peng@gmail.com>
---
 fs/seq_file.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/seq_file.c b/fs/seq_file.c
index eea09f6..2298656 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -120,14 +120,12 @@ static int traverse(struct seq_file *m, loff_t offset)
         if (pos + m->count > offset) {
             m->from = offset - pos;
             m->count -= m->from;
-            m->index = index;
             break;
         }
         pos += m->count;
         m->count = 0;
         if (pos == offset) {
             index++;
-            m->index = index;
             break;
         }
         p = m->op->next(m, p, &index);
-- 
1.9.1

Re: [PATCH] seq_file: remove redundant assignment of index to m->index

[Joe Perches]

On Sat, 2018-02-10 at 23:59 +0800, Donglin Peng wrote:
> There are two redundant assignments in the traverse() function, because
> the while loop will break after these two assignments, and after that
> the variable index will be assigned to m->index again.
[]
> diff --git a/fs/seq_file.c b/fs/seq_file.c
[]
> @@ -120,14 +120,12 @@ static int traverse(struct seq_file *m, loff_t offset)
>          if (pos + m->count > offset) {
>              m->from = offset - pos;
>              m->count -= m->from;
> -            m->index = index;
>              break;
>          }
>          pos += m->count;
>          m->count = 0;
>          if (pos == offset) {
>              index++;
> -            m->index = index;
>              break;
>          }
>          p = m->op->next(m, p, &index);

Of course this looks correct, but how
are you _absolutely sure_ about this?

Perhaps the m->op->stop(m, p) call below
the break, which takes m as an argument,
needs an updated m->index.

Unless you can verify _all_ the indirect
paths that stop() can take, this patch may
not be correct.

Re: [PATCH] seq_file: remove redundant assignment of index to m->index

[Matthew Wilcox]

On Sat, Feb 10, 2018 at 10:04:23AM -0800, Joe Perches wrote:
> > @@ -120,14 +120,12 @@ static int traverse(struct seq_file *m, loff_t offset)
> >          if (pos + m->count > offset) {
> >              m->from = offset - pos;
> >              m->count -= m->from;
> > -            m->index = index;
> >              break;
> >          }
> >          pos += m->count;
> >          m->count = 0;
> >          if (pos == offset) {
> >              index++;
> > -            m->index = index;
> >              break;
> >          }
> >          p = m->op->next(m, p, &index);
> 
> Of course this looks correct, but how
> are you _absolutely sure_ about this?
> 
> Perhaps the m->op->stop(m, p) call below
> the break, which takes m as an argument,
> needs an updated m->index.

Not only that, but ->next might also look at m->index.
This is not performance critical; don't try to optimise it.

  Programmers waste enormous amounts of time thinking about, or worrying
  about, the speed of noncritical parts of their programs, and these
  attempts at efficiency actually have a strong negative impact when
  debugging and maintenance are considered. We should forget about small
  efficiencies, say about 97% of the time: premature optimization is the
  root of all evil. Yet we should not pass up our opportunities in that
  critical 3%.  -- Donald Knuth

Re: [PATCH] seq_file: remove redundant assignment of index to m->index

[Donglin Peng]

On Sun, Feb 11, 2018 at 9:02 AM, Matthew Wilcox <willy@infradead.org> wrote:
> On Sat, Feb 10, 2018 at 10:04:23AM -0800, Joe Perches wrote:
>> > @@ -120,14 +120,12 @@ static int traverse(struct seq_file *m, loff_t offset)
>> >          if (pos + m->count > offset) {
>> >              m->from = offset - pos;
>> >              m->count -= m->from;
>> > -            m->index = index;
>> >              break;
>> >          }
>> >          pos += m->count;
>> >          m->count = 0;
>> >          if (pos == offset) {
>> >              index++;
>> > -            m->index = index;
>> >              break;
>> >          }
>> >          p = m->op->next(m, p, &index);
>>
>> Of course this looks correct, but how
>> are you _absolutely sure_ about this?
>>
>> Perhaps the m->op->stop(m, p) call below
>> the break, which takes m as an argument,
>> needs an updated m->index.
>
> Not only that, but ->next might also look at m->index.
I think there is no chance to call op->next, because the loop will
break immediately
after the assignment.