Message-ID: <91d662f1-baf6-1114-f237-a66ebc164009@suse.cz>
Date:   Fri, 7 Jan 2022 17:58:24 +0100
From:   Vlastimil Babka <vbabka@...e.cz>
To:     Pintu Agarwal <pintu.ping@...il.com>,
        Christian Brauner <christian.brauner@...ntu.com>
Cc:     Cyrill Gorcunov <gorcunov@...il.com>,
        Pintu Kumar <quic_pintu@...cinc.com>,
        open list <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-mm <linux-mm@...ck.org>, ebiederm@...ssion.com,
        sfr@...b.auug.org.au, legion@...nel.org, sashal@...nel.org,
        chris.hyser@...cle.com, ccross@...gle.com, pcc@...gle.com,
        dave@...olabs.net, caoxiaofeng@...ong.com, david@...hat.com,
        Linux API <linux-api@...r.kernel.org>
Subject: Re: [PATCH] sysinfo: include availram field in sysinfo struct

CC linux-api

On 1/7/22 14:44, Pintu Agarwal wrote:
> On Fri, 7 Jan 2022 at 17:35, Christian Brauner
> <christian.brauner@...ntu.com> wrote:
>>
>> On Thu, Jan 06, 2022 at 08:27:47PM +0300, Cyrill Gorcunov wrote:
>> > On Thu, Jan 06, 2022 at 10:19:55PM +0530, Pintu Agarwal wrote:
>> > > > > diff --git a/include/uapi/linux/sysinfo.h b/include/uapi/linux/sysinfo.h
>> > > > > index 435d5c2..6e77e90 100644
>> > > > > --- a/include/uapi/linux/sysinfo.h
>> > > > > +++ b/include/uapi/linux/sysinfo.h
>> > > > > @@ -12,6 +12,7 @@ struct sysinfo {
>> > > > >       __kernel_ulong_t freeram;       /* Available memory size */
>> > > > >       __kernel_ulong_t sharedram;     /* Amount of shared memory */
>> > > > >       __kernel_ulong_t bufferram;     /* Memory used by buffers */
>> > > > > +     __kernel_ulong_t availram;      /* Memory available for allocation */
>> > > > >       __kernel_ulong_t totalswap;     /* Total swap space size */
>> > > > >       __kernel_ulong_t freeswap;      /* swap space still available */
>> > > > >       __u16 procs;                    /* Number of current processes */
>> > > >
>> > > > Hi! Sorry, but I don't understand -- the sysinfo structure seems to
>> > > > be part of user API, no? Don't we break it up here?
>> > >
>> > > Yes, the corresponding user space header /usr/include/linux/sysinfo.h
>> > > also needs to be updated.
>> > > When we generate the kernel header it will be updated automatically.
>> >
>> > Wait. The userspace may pass old structure here, and in result we
>> > return incorrect layout which won't match old one, no? Old binary
>> > code has no clue about this header update.
>>
>> Yes, that won't work as done.
>>
>> If we want to do this it needs to be done at the end of the struct right
>> before the padding field and the newly added field substracted from the
>> padding. (Not the preferred way to do it these days for new structs.)
>>
>> A new kernel can then pass in the struct with the newly added field and
>> an old kernel can just fill the struct in as usual. New kernel will
>> update the field with the correct value.
>>
>> But there's a catch depending on the type of value.
>> The problem with these types of extensions is that you'll often need
>> indicators to and from the kernel whether the extension is supported.
>>
>> Consider an extension where 0 is a valid value meaning "this resource is
>> completely used". Since the kernel and userspace always agree on the
>> size of the struct the kernel will zero the whole struct. So if in your
>> newly added field 0 is a valid value you can't differentiate between 0
>> as a valid value indicating that your resource isn't available and 0 as
>> the kernel not supporting your extension.
>>
>> Other APIs solve this and similar problems by having a request mask and
>> a return mask.  Userspace fills in what values it wants reported in the
>> request mask and kernel sets the supported flags in the return mask.
>> This way you can differentiate between the two (see statx).
>>
>> If the 0 example is not a concern or acceptable for userspace it's
>> probably fine. But you need to document that having 0 returned can mean
>> both things.
>>
>> Or, you select a value different from 0 (-1?) that you can use to
>> indicate to userspace that the resource is used up so 0 can just mean
>> "kernel doesn't support this extension".
> 
> Thanks all for your inputs.
> As Eric suggested in other thread (pasting here for reference):
> {
>> Before the padding and you should reduce the size of the padding by the
>> size of your new field.
> 
>>> Also, I could not understand what this is for ?
>>> Do we need to update this since sture is changed ?
> 
>> In general padding like that is so new fields can be added.  The
>> comment about libc5 makes me a wonder a bit, but I expect libc5 just
>> added the padding in it's copy of the structure that it exported to
>> userspace many many years ago so that new fields could be added.
> 
>> Eric
> }
> 
> I made the changes like below and this seems to work even with older user space.
> I mean earlier, when I ran "free" command it was giving "stack
> smashing..." error,
> but now the "free" command (which comes as part of busybox) works fine
> even without recompiling with the updated header.
> 
> These are the header changes for quick look:
> {{{
> diff --git a/include/uapi/linux/sysinfo.h b/include/uapi/linux/sysinfo.h
> index 6e77e90..fe84c6a 100644
> --- a/include/uapi/linux/sysinfo.h
> +++ b/include/uapi/linux/sysinfo.h
> @@ -12,7 +12,6 @@ struct sysinfo {
>         __kernel_ulong_t freeram;       /* Available memory size */
>         __kernel_ulong_t sharedram;     /* Amount of shared memory */
>         __kernel_ulong_t bufferram;     /* Memory used by buffers */
> -       __kernel_ulong_t availram;      /* Memory available for allocation */
>         __kernel_ulong_t totalswap;     /* Total swap space size */
>         __kernel_ulong_t freeswap;      /* swap space still available */
>         __u16 procs;                    /* Number of current processes */
> @@ -20,7 +19,8 @@ struct sysinfo {
>         __kernel_ulong_t totalhigh;     /* Total high memory size */
>         __kernel_ulong_t freehigh;      /* Available high memory size */
>         __u32 mem_unit;                 /* Memory unit size in bytes */
> -       char _f[20-2*sizeof(__kernel_ulong_t)-sizeof(__u32)];   /*
> Padding: libc5 uses this.. */
> +       __kernel_ulong_t availram;      /* Memory available for allocation */
> +       char _f[20-3*sizeof(__kernel_ulong_t)-sizeof(__u32)];   /*
> Padding: libc5 uses this.. */
>  };
> }}}
> 
> If this is fine, I will push the new patch set.

Please CC linux-api@...r.kernel.org on the new posting.

> Thanks,
> Pintu
> 

Powered by blists - more mailing lists