Re: [PATCH v5 2/2] mm: hugetlb: proc: add HugetlbPages field to /proc/PID/status

Michal Hocko
2015-08-31T09:12:41Z

On Thu 27-08-15 10:23:51, Jörn Engel wrote:
> On Thu, Aug 27, 2015 at 08:48:18AM +0200, Michal Hocko wrote:
> >
> > > On x86, HUGE_MAX_HSTATE == 2. I don't consider that to be expensive.
> > >
> > > If you are concerned about the memory allocation of struct hugetlb_usage,
> > > it could easily be embedded directly in struct mm_struct.
> >
> > Yes I am concerned about that and
> > 9 files changed, 112 insertions(+), 1 deletion(-)
> > for something that is even not clear to be really required. And I still
> > haven't heard any strong usecase to justify it.
> >
> > Can we go with the single and much simpler cumulative number first and
> > only add the break down list if it is _really_ required? We can even
> > document that the future version of /proc/<pid>/status might add an
> > additional information to prepare all the parsers to be more careful.
>
> I don't care much which way we decide. But I find your reasoning a bit
> worrying. If someone asks for a by-size breakup of hugepages in a few
> years, you might have existing binaries that depend on the _absence_ of
> those extra characters on the line.
>
> Compare:
> HugetlbPages: 18432 kB
> HugetlbPages: 1069056 kB (1*1048576kB 10*2048kB)
>
> Once someone has written a script that greps for 'HugetlbPages:.*kB$',
> you have lost the option of adding anything else to the line.

If you think that an explicit note in the documentation is
not sufficient then I believe we can still handle it backward
compatible. Like separate entries for each existing hugetlb page:
HugetlbPages: 1069056 kB
Hugetlb2MPages: 20480 kB
Hugetlb1GPages: 1048576 kB

or something similar. I would even argue this would be slightly easier
to parse. So it is not like we would be locked into anything.

> You have
> created yet another ABI compatibility headache today in order to save
> 112 lines of code.
>
> That may be a worthwhile tradeoff, I don't know. But at least I realize
> there is a cost, while you seem to ignore that component. There is
> value in not painting yourself into a corner.

My primary point was that we are adding a code for a feature nobody
actually asked for just because somebody might ask for it in future.
--
Michal Hocko
SUSE Labs