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)" <[email protected]> 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.
On Wed, 13 Apr 2022 18:25:53 -0400 "Alex Xu (Hello71)" <[email protected]> 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)" <[email protected]> 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?
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)" <[email protected]> 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.