Re: [PATCH v2] sysinfo: include availram field in sysinfo struct

From: Pintu Agarwal <pintu.ping@gmail.com>
To: Cyrill Gorcunov <gorcunov@gmail.com>
Cc: Pintu Kumar <quic_pintu@quicinc.com>,
	open list <linux-kernel@vger.kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	linux-mm <linux-mm@kvack.org>,
	ebiederm@xmission.com,
	Christian Brauner <christian.brauner@ubuntu.com>,
	sfr@canb.auug.org.au, legion@kernel.org, sashal@kernel.org,
	chris.hyser@oracle.com, Colin Cross <ccross@google.com>,
	Peter Collingbourne <pcc@google.com>,
	dave@stgolabs.net, caoxiaofeng@yulong.com, david@redhat.com,
	Vlastimil Babka <vbabka@suse.cz>,
	linux-api@vger.kernel.org
Subject: Re: [PATCH v2] sysinfo: include availram field in sysinfo struct
Date: Sat, 8 Jan 2022 21:54:37 +0530	[thread overview]
Message-ID: <CAOuPNLhcvbk3-rTPqmJj5LBOh4VaZ+Bc=-_j6xKOLM-kH6jkfw@mail.gmail.com> (raw)
In-Reply-To: <YdiqLKS5Sv9eWwu2@grain>

On Sat, 8 Jan 2022 at 02:31, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>
> On Fri, Jan 07, 2022 at 11:37:34PM +0530, Pintu Kumar wrote:
> > The sysinfo member does not have any "available ram" field and
> > the bufferram field is not much helpful either, to get a rough
> > estimate of available ram needed for allocation.
> >
> > One needs to parse MemAvailable field separately from /proc/meminfo
> > to get this info instead of directly getting if from sysinfo itself.
>
> Who exactly needs this change? Do you have some application for which
> parsing /proc/meminfo is a hot path so it needs this information via
> sysinfo interface?
>
Thank you so much for your feedback...
I had a need to get total/free/available memory in my application (on
a memory constraint system).
First I tried to parse these from /proc/meminfo but then I saw sysinfo
already provides some information,
however available field was missing. Just to get available field I
need to again do all the file operations.

Then I saw, even the "free" command doing this redundant work.
Use sysinfo system call to get "total" and "free" memory then again
get "available" memory from /proc/meminfo.
Yet again, I see, even in kernel its reading from two different places
while populating the /proc/meminfo.
Also, I am sure there are plenty of other applications where this can
be improved with this.
Moreover, I think with this field there is not much use of other ram
fields in sysinfo.
Thus I felt a need to introduce this field to ease some operations.

> Don't get me wrong please but such extension really need a strong
> justification because they are part of UAPI and there is not that much
> space left in sysinfo structure. We will _have_ to live with this new
> field forever so I propose to not introduce anything new here until
> we have no other choise or parsing meminfo become a really bottleneck.
>
My guess is that this situation might exist in other places as well ?
How do we handle new field addition to existing system calls ?

> > diff --git a/kernel/sys.c b/kernel/sys.c
> > index ecc4cf0..7059515 100644
> > --- a/kernel/sys.c
> > +++ b/kernel/sys.c
> > @@ -2671,6 +2671,7 @@ static int do_sysinfo(struct sysinfo *info)
> >       info->freeram <<= bitcount;
> >       info->sharedram <<= bitcount;
> >       info->bufferram <<= bitcount;
> > +     info->availram <<= bitcount;
> >       info->totalswap <<= bitcount;
> >       info->freeswap <<= bitcount;
> >       info->totalhigh <<= bitcount;
> > @@ -2700,6 +2701,7 @@ struct compat_sysinfo {
> >       u32 freeram;
> >       u32 sharedram;
> >       u32 bufferram;
> > +     u32 availram;
>
> If only I'm not missing something ovious, this is part of UAPI as well.
Yes, this structure is part of the common UAPI header.