linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] mm/smaps_rollup: return empty file for kthreads instead of ESRCH
       [not found] <20220413211357.26938-1-alex_y_xu.ref@yahoo.ca>
@ 2022-04-13 21:13 ` Alex Xu (Hello71)
  2022-04-13 21:27   ` Andrew Morton
  0 siblings, 1 reply; 9+ messages in thread
From: Alex Xu (Hello71) @ 2022-04-13 21:13 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Vlastimil Babka, Daniel Colascione, linux-mm, linux-kernel,
	linux-fsdevel, Alexey Dobriyan, linux-api, Alex Xu (Hello71)

This restores the behavior prior to 258f669e7e88 ("mm:
/proc/pid/smaps_rollup: convert to single value seq_file"), making it
once again consistent with maps and smaps, and allowing patterns like
awk '$1=="Anonymous:"{x+=$2}END{print x}' /proc/*/smaps_rollup to work.
Searching all Debian packages for "smaps_rollup" did not find any
programs which would be affected by this change.

Fixes: 258f669e7e88 ("mm: /proc/pid/smaps_rollup: convert to single value seq_file")

Signed-off-by: Alex Xu (Hello71) <alex_y_xu@yahoo.ca>
---
 fs/proc/task_mmu.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index f46060eb91b5..d7de4584a271 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -883,10 +883,8 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
 		return -ESRCH;
 
 	mm = priv->mm;
-	if (!mm || !mmget_not_zero(mm)) {
-		ret = -ESRCH;
+	if (!mm || !mmget_not_zero(mm))
 		goto out_put_task;
-	}
 
 	memset(&mss, 0, sizeof(mss));
 
-- 
2.35.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] mm/smaps_rollup: return empty file for kthreads instead of ESRCH
  2022-04-13 21:13 ` [PATCH] mm/smaps_rollup: return empty file for kthreads instead of ESRCH Alex Xu (Hello71)
@ 2022-04-13 21:27   ` Andrew Morton
  2022-04-13 22:25     ` Alex Xu (Hello71)
  0 siblings, 1 reply; 9+ messages in thread
From: Andrew Morton @ 2022-04-13 21:27 UTC (permalink / raw)
  To: Alex Xu (Hello71)
  Cc: Vlastimil Babka, Daniel Colascione, linux-mm, linux-kernel,
	linux-fsdevel, Alexey Dobriyan, linux-api

On Wed, 13 Apr 2022 17:13:57 -0400 "Alex Xu (Hello71)" <alex_y_xu@yahoo.ca> wrote:

> This restores the behavior prior to 258f669e7e88 ("mm:
> /proc/pid/smaps_rollup: convert to single value seq_file"), making it
> once again consistent with maps and smaps, and allowing patterns like
> awk '$1=="Anonymous:"{x+=$2}END{print x}' /proc/*/smaps_rollup to work.
> Searching all Debian packages for "smaps_rollup" did not find any
> programs which would be affected by this change.

Thanks.

258f669e7e88 was 4 years ago, so I guess a -stable backport isn't
really needed.

However, we need to be concerned about causing new regressions, and I
don't think you've presented enough information for this to be determined.

So please provide us with a full description of how the smaps_rollup
output will be altered by this patch.  Quoting example output would be
helpful.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mm/smaps_rollup: return empty file for kthreads instead of ESRCH
  2022-04-13 21:27   ` Andrew Morton
@ 2022-04-13 22:25     ` Alex Xu (Hello71)
  2022-04-13 23:06       ` Andrew Morton
  0 siblings, 1 reply; 9+ messages in thread
From: Alex Xu (Hello71) @ 2022-04-13 22:25 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Alexey Dobriyan, Daniel Colascione, linux-api, linux-fsdevel,
	linux-kernel, linux-mm, Vlastimil Babka

Excerpts from Andrew Morton's message of April 13, 2022 5:27 pm:
> On Wed, 13 Apr 2022 17:13:57 -0400 "Alex Xu (Hello71)" <alex_y_xu@yahoo.ca> wrote:
> 
>> This restores the behavior prior to 258f669e7e88 ("mm:
>> /proc/pid/smaps_rollup: convert to single value seq_file"), making it
>> once again consistent with maps and smaps, and allowing patterns like
>> awk '$1=="Anonymous:"{x+=$2}END{print x}' /proc/*/smaps_rollup to work.
>> Searching all Debian packages for "smaps_rollup" did not find any
>> programs which would be affected by this change.
> 
> Thanks.
> 
> 258f669e7e88 was 4 years ago, so I guess a -stable backport isn't
> really needed.
> 
> However, we need to be concerned about causing new regressions, and I
> don't think you've presented enough information for this to be determined.
> 
> So please provide us with a full description of how the smaps_rollup
> output will be altered by this patch.  Quoting example output would be
> helpful.
> 
> 

Current behavior (4.19+):

$ cat /proc/2/smaps; echo $?
0
$ cat /proc/2/smaps_rollup; echo $?
cat: /proc/2/smaps_rollup: No such process
1
$ strace -yP /proc/2/smaps_rollup cat /proc/2/smaps_rollup
openat(AT_FDCWD</>, "/proc/2/smaps_rollup", O_RDONLY) = 3</proc/2/smaps_rollup>
newfstatat(3</proc/2/smaps_rollup>, "", {st_mode=S_IFREG|0444, st_size=0, ...}, AT_EMPTY_PATH) = 0
fadvise64(3</proc/2/smaps_rollup>, 0, 0, POSIX_FADV_SEQUENTIAL) = 0
read(3</proc/2/smaps_rollup>, 0x7fa475f5d000, 131072) = -1 ESRCH (No such process)
cat: /proc/2/smaps_rollup: No such process
close(3</proc/2/smaps_rollup>)          = 0
+++ exited with 1 +++

Pre-4.19 and post-patch behavior:

$ cat /proc/2/smaps; echo $?
0
$ cat /proc/2/smaps_rollup; echo $?
0
$ strace -yP /proc/2/smaps_rollup cat /proc/2/smaps_rollup
openat(AT_FDCWD</>, "/proc/2/smaps_rollup", O_RDONLY) = 3</proc/2/smaps_rollup>
newfstatat(3</proc/2/smaps_rollup>, "", {st_mode=S_IFREG|0444, st_size=0, ...}, AT_EMPTY_PATH) = 0
fadvise64(3</proc/2/smaps_rollup>, 0, 0, POSIX_FADV_SEQUENTIAL) = 0
read(3</proc/2/smaps_rollup>, "", 131072) = 0
close(3</proc/2/smaps_rollup>)          = 0
+++ exited with 0 +++

I agree that this type of change must be done carefully to avoid 
introducing inadvertent regressions. However, I think this particular 
change is highly unlikely to introduce regressions for the following 
reasons:

1. I cannot think of a plausible case which would be affected. The only 
   case I can possibly imagine is a program checking whether a process 
   is a kernel thread, but this seems like a particularly silly method. 
   Moreover, the method is already broken on kernels before 4.14 
   (because smaps_rollup does not exist) and before 4.19 (because 
   smaps_rollup worked like smaps). A plausible method would be opening 
   /proc/x/(s)maps and checking that it is empty, which some programs 
   actually do.

2. Research on Debian Code Search did not find any apparent cases. I also 
   searched GitHub Code Search but found too many irrelevant results with 
   no useful way to filter them out.

3. As mentioned previously, this was already the behavior between 4.14 
   and 4.18 (inclusive).

Cheers,
Alex.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mm/smaps_rollup: return empty file for kthreads instead of ESRCH
  2022-04-13 22:25     ` Alex Xu (Hello71)
@ 2022-04-13 23:06       ` Andrew Morton
  2022-04-14  3:23         ` Matthew Wilcox
  0 siblings, 1 reply; 9+ messages in thread
From: Andrew Morton @ 2022-04-13 23:06 UTC (permalink / raw)
  To: Alex Xu (Hello71)
  Cc: Alexey Dobriyan, Daniel Colascione, linux-api, linux-fsdevel,
	linux-kernel, linux-mm, Vlastimil Babka

On Wed, 13 Apr 2022 18:25:53 -0400 "Alex Xu (Hello71)" <alex_y_xu@yahoo.ca> wrote:

> Excerpts from Andrew Morton's message of April 13, 2022 5:27 pm:
> > On Wed, 13 Apr 2022 17:13:57 -0400 "Alex Xu (Hello71)" <alex_y_xu@yahoo.ca> wrote:
> > 
> >> This restores the behavior prior to 258f669e7e88 ("mm:
> >> /proc/pid/smaps_rollup: convert to single value seq_file"), making it
> >> once again consistent with maps and smaps, and allowing patterns like
> >> awk '$1=="Anonymous:"{x+=$2}END{print x}' /proc/*/smaps_rollup to work.
> >> Searching all Debian packages for "smaps_rollup" did not find any
> >> programs which would be affected by this change.
> > 
> > Thanks.
> > 
> > 258f669e7e88 was 4 years ago, so I guess a -stable backport isn't
> > really needed.
> > 
> > However, we need to be concerned about causing new regressions, and I
> > don't think you've presented enough information for this to be determined.
> > 
> > So please provide us with a full description of how the smaps_rollup
> > output will be altered by this patch.  Quoting example output would be
> > helpful.
> > 
> > 
> 
> Current behavior (4.19+):
> 
> $ cat /proc/2/smaps; echo $?
> 0
> $ cat /proc/2/smaps_rollup; echo $?
> cat: /proc/2/smaps_rollup: No such process
> 1
> $ strace -yP /proc/2/smaps_rollup cat /proc/2/smaps_rollup
> openat(AT_FDCWD</>, "/proc/2/smaps_rollup", O_RDONLY) = 3</proc/2/smaps_rollup>
> newfstatat(3</proc/2/smaps_rollup>, "", {st_mode=S_IFREG|0444, st_size=0, ...}, AT_EMPTY_PATH) = 0
> fadvise64(3</proc/2/smaps_rollup>, 0, 0, POSIX_FADV_SEQUENTIAL) = 0
> read(3</proc/2/smaps_rollup>, 0x7fa475f5d000, 131072) = -1 ESRCH (No such process)
> cat: /proc/2/smaps_rollup: No such process
> close(3</proc/2/smaps_rollup>)          = 0
> +++ exited with 1 +++
> 
> Pre-4.19 and post-patch behavior:
> 
> $ cat /proc/2/smaps; echo $?
> 0
> $ cat /proc/2/smaps_rollup; echo $?
> 0
> $ strace -yP /proc/2/smaps_rollup cat /proc/2/smaps_rollup
> openat(AT_FDCWD</>, "/proc/2/smaps_rollup", O_RDONLY) = 3</proc/2/smaps_rollup>
> newfstatat(3</proc/2/smaps_rollup>, "", {st_mode=S_IFREG|0444, st_size=0, ...}, AT_EMPTY_PATH) = 0
> fadvise64(3</proc/2/smaps_rollup>, 0, 0, POSIX_FADV_SEQUENTIAL) = 0
> read(3</proc/2/smaps_rollup>, "", 131072) = 0
> close(3</proc/2/smaps_rollup>)          = 0
> +++ exited with 0 +++

OK, thanks.

But the current behaviour is appropriate, isn't it?  An attempt to read
the maps of a process which has no maps returns -ESRCH.  Seems sensible
enough.

On the other hand, returning a zero-length read() is also appropriate.

> I agree that this type of change must be done carefully to avoid 
> introducing inadvertent regressions. However, I think this particular 
> change is highly unlikely to introduce regressions for the following 
> reasons:
> 
> 1. I cannot think of a plausible case which would be affected. The only 
>    case I can possibly imagine is a program checking whether a process 
>    is a kernel thread, but this seems like a particularly silly method. 
>    Moreover, the method is already broken on kernels before 4.14 
>    (because smaps_rollup does not exist) and before 4.19 (because 
>    smaps_rollup worked like smaps). A plausible method would be opening 
>    /proc/x/(s)maps and checking that it is empty, which some programs 
>    actually do.

Well, I suppose a poorly coded application could do something like

	if (read(fd, buf, 1000) >= 0)
		assume_buf_now_contains_data()

> 2. Research on Debian Code Search did not find any apparent cases. I also 
>    searched GitHub Code Search but found too many irrelevant results with 
>    no useful way to filter them out.

I don't think this will work very well.  smaps_rollup is the sort of
system tuning thing for which organizations will develop in-house
tooling which never get relesaed externally.

> 3. As mentioned previously, this was already the behavior between 4.14 
>    and 4.18 (inclusive).
> 

Yup.  Hm, tricky.  I'd prefer to leave it alone if possible.  How
serious a problem is this, really?  

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mm/smaps_rollup: return empty file for kthreads instead of ESRCH
  2022-04-13 23:06       ` Andrew Morton
@ 2022-04-14  3:23         ` Matthew Wilcox
  2022-04-14  6:55           ` Alexey Dobriyan
  0 siblings, 1 reply; 9+ messages in thread
From: Matthew Wilcox @ 2022-04-14  3:23 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Alex Xu (Hello71),
	Alexey Dobriyan, Daniel Colascione, linux-api, linux-fsdevel,
	linux-kernel, linux-mm, Vlastimil Babka

On Wed, Apr 13, 2022 at 04:06:13PM -0700, Andrew Morton wrote:
> On Wed, 13 Apr 2022 18:25:53 -0400 "Alex Xu (Hello71)" <alex_y_xu@yahoo.ca> wrote:
> > > 258f669e7e88 was 4 years ago, so I guess a -stable backport isn't
> > > really needed.
> > 
> > Current behavior (4.19+):
[...]
> > Pre-4.19 and post-patch behavior:
> 
> I don't think this will work very well.  smaps_rollup is the sort of
> system tuning thing for which organizations will develop in-house
> tooling which never get relesaed externally.
> 
> > 3. As mentioned previously, this was already the behavior between 4.14 
> >    and 4.18 (inclusive).
> > 
> 
> Yup.  Hm, tricky.  I'd prefer to leave it alone if possible.  How
> serious a problem is this, really?  

I don't think "It's been like this for four years" is as solid an argument
as you might like.  Certain distributions (of the coloured millinery
variety, for example) haven't updated their kernel since then and so
there may well be many organisations who have not been exposed to the
current behaviour.  Even my employers distribution, while it offers a
5.4 based kernel, still has many customers who have not moved from the
4.14 kernel.  Inertia is a real thing, and restoring this older behaviour
might well be an improvement.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mm/smaps_rollup: return empty file for kthreads instead of ESRCH
  2022-04-14  3:23         ` Matthew Wilcox
@ 2022-04-14  6:55           ` Alexey Dobriyan
  2022-04-14  7:38             ` Vlastimil Babka
  2022-04-14 15:24             ` Alex Xu (Hello71)
  0 siblings, 2 replies; 9+ messages in thread
From: Alexey Dobriyan @ 2022-04-14  6:55 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Andrew Morton, Alex Xu (Hello71),
	Daniel Colascione, linux-api, linux-fsdevel, linux-kernel,
	linux-mm, Vlastimil Babka

On Thu, Apr 14, 2022 at 04:23:13AM +0100, Matthew Wilcox wrote:
> On Wed, Apr 13, 2022 at 04:06:13PM -0700, Andrew Morton wrote:
> > On Wed, 13 Apr 2022 18:25:53 -0400 "Alex Xu (Hello71)" <alex_y_xu@yahoo.ca> wrote:
> > > > 258f669e7e88 was 4 years ago, so I guess a -stable backport isn't
> > > > really needed.
> > > 
> > > Current behavior (4.19+):
> [...]
> > > Pre-4.19 and post-patch behavior:
> > 
> > I don't think this will work very well.  smaps_rollup is the sort of
> > system tuning thing for which organizations will develop in-house
> > tooling which never get relesaed externally.
> > 
> > > 3. As mentioned previously, this was already the behavior between 4.14 
> > >    and 4.18 (inclusive).
> > > 
> > 
> > Yup.  Hm, tricky.  I'd prefer to leave it alone if possible.  How
> > serious a problem is this, really?  
> 
> I don't think "It's been like this for four years" is as solid an argument
> as you might like.  Certain distributions (of the coloured millinery
> variety, for example) haven't updated their kernel since then and so
> there may well be many organisations who have not been exposed to the
> current behaviour.  Even my employers distribution, while it offers a
> 5.4 based kernel, still has many customers who have not moved from the
> 4.14 kernel.  Inertia is a real thing, and restoring this older behaviour
> might well be an improvement.

Returning ESRCH is better so that programs don't waste time reading and
closing empty files and instantiating useless inodes.

Of course it is different if this patch was sent as response to a regression.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mm/smaps_rollup: return empty file for kthreads instead of ESRCH
  2022-04-14  6:55           ` Alexey Dobriyan
@ 2022-04-14  7:38             ` Vlastimil Babka
  2022-04-27 20:34               ` Andrew Morton
  2022-04-14 15:24             ` Alex Xu (Hello71)
  1 sibling, 1 reply; 9+ messages in thread
From: Vlastimil Babka @ 2022-04-14  7:38 UTC (permalink / raw)
  To: Alexey Dobriyan, Matthew Wilcox
  Cc: Andrew Morton, Alex Xu (Hello71),
	Daniel Colascione, linux-api, linux-fsdevel, linux-kernel,
	linux-mm

On 4/14/22 08:55, Alexey Dobriyan wrote:
> On Thu, Apr 14, 2022 at 04:23:13AM +0100, Matthew Wilcox wrote:
>> On Wed, Apr 13, 2022 at 04:06:13PM -0700, Andrew Morton wrote:
>> > On Wed, 13 Apr 2022 18:25:53 -0400 "Alex Xu (Hello71)" <alex_y_xu@yahoo.ca> wrote:
>> > > > 258f669e7e88 was 4 years ago, so I guess a -stable backport isn't
>> > > > really needed.
>> > > 
>> > > Current behavior (4.19+):
>> [...]
>> > > Pre-4.19 and post-patch behavior:
>> > 
>> > I don't think this will work very well.  smaps_rollup is the sort of
>> > system tuning thing for which organizations will develop in-house
>> > tooling which never get relesaed externally.
>> > 
>> > > 3. As mentioned previously, this was already the behavior between 4.14 
>> > >    and 4.18 (inclusive).
>> > > 
>> > 
>> > Yup.  Hm, tricky.  I'd prefer to leave it alone if possible.  How
>> > serious a problem is this, really?  
>> 
>> I don't think "It's been like this for four years" is as solid an argument
>> as you might like.  Certain distributions (of the coloured millinery
>> variety, for example) haven't updated their kernel since then and so
>> there may well be many organisations who have not been exposed to the
>> current behaviour.  Even my employers distribution, while it offers a
>> 5.4 based kernel, still has many customers who have not moved from the
>> 4.14 kernel.  Inertia is a real thing, and restoring this older behaviour
>> might well be an improvement.
> 
> Returning ESRCH is better so that programs don't waste time reading and
> closing empty files and instantiating useless inodes.

Hm, unfortunately I don't remember why I put return -ESRCH for this case in
addition to get_proc_task() failing. I doubt it was a conscious decision to
treat kthreads differently - I think I would have preferred consistency with
maps/smaps.

Can the awk use case be fixed with some flag to make it ignore the errors?

> Of course it is different if this patch was sent as response to a regression.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mm/smaps_rollup: return empty file for kthreads instead of ESRCH
  2022-04-14  6:55           ` Alexey Dobriyan
  2022-04-14  7:38             ` Vlastimil Babka
@ 2022-04-14 15:24             ` Alex Xu (Hello71)
  1 sibling, 0 replies; 9+ messages in thread
From: Alex Xu (Hello71) @ 2022-04-14 15:24 UTC (permalink / raw)
  To: Alexey Dobriyan
  Cc: Matthew Wilcox, Andrew Morton, Daniel Colascione, linux-api,
	linux-fsdevel, linux-kernel, linux-mm, Vlastimil Babka

Excerpts from Alexey Dobriyan's message of April 14, 2022 2:55 am:
> Returning ESRCH is better so that programs don't waste time reading and
> closing empty files and instantiating useless inodes.

Yes, except ESRCH is not returned for open, it is returned for read.

> Of course it is different if this patch was sent as response to a regression.

I'm not sure I would classify it as a regression; I don't have an 
existing program which broke, it is a new program which happens to use 
some functionality which worked with a previous kernel. It is 
theoretically possible that some program exists that currently uses 
4.14, and will break if upgraded to 4.19+, but it is also possible that 
some program exists that currently uses 4.19+ and will break if this 
patch is applied.

Cheers,
Alex.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mm/smaps_rollup: return empty file for kthreads instead of ESRCH
  2022-04-14  7:38             ` Vlastimil Babka
@ 2022-04-27 20:34               ` Andrew Morton
  0 siblings, 0 replies; 9+ messages in thread
From: Andrew Morton @ 2022-04-27 20:34 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Alexey Dobriyan, Matthew Wilcox, Alex Xu (Hello71),
	Daniel Colascione, linux-api, linux-fsdevel, linux-kernel,
	linux-mm

On Thu, 14 Apr 2022 09:38:14 +0200 Vlastimil Babka <vbabka@suse.cz> wrote:

> > Returning ESRCH is better so that programs don't waste time reading and
> > closing empty files and instantiating useless inodes.
> 
> Hm, unfortunately I don't remember why I put return -ESRCH for this case in
> addition to get_proc_task() failing. I doubt it was a conscious decision to
> treat kthreads differently - I think I would have preferred consistency with
> maps/smaps.
> 
> Can the awk use case be fixed with some flag to make it ignore the errors?

This is all too hard.  I think I'll drop the patch for now.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-04-27 20:34 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20220413211357.26938-1-alex_y_xu.ref@yahoo.ca>
2022-04-13 21:13 ` [PATCH] mm/smaps_rollup: return empty file for kthreads instead of ESRCH Alex Xu (Hello71)
2022-04-13 21:27   ` Andrew Morton
2022-04-13 22:25     ` Alex Xu (Hello71)
2022-04-13 23:06       ` Andrew Morton
2022-04-14  3:23         ` Matthew Wilcox
2022-04-14  6:55           ` Alexey Dobriyan
2022-04-14  7:38             ` Vlastimil Babka
2022-04-27 20:34               ` Andrew Morton
2022-04-14 15:24             ` Alex Xu (Hello71)

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).