| Message ID | 1641578854-14232-1-git-send-email-quic_pintu@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] sysinfo: include availram field in sysinfo struct | expand |
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? 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. > 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.
From: Pintu Kumar > Sent: 07 January 2022 18:08 > > 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. > > Thus, this patch introduce a new field as availram in sysinfo > so that all the info total/free/available can be retrieved from > one place itself. > ... > diff --git a/include/uapi/linux/sysinfo.h b/include/uapi/linux/sysinfo.h > index 435d5c2..fe84c6a 100644 > --- a/include/uapi/linux/sysinfo.h > +++ b/include/uapi/linux/sysinfo.h > @@ -19,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.. */ There are 4 pad bytes here on most 64bit architectures. > + __kernel_ulong_t availram; /* Memory available for allocation */ > + char _f[20-3*sizeof(__kernel_ulong_t)-sizeof(__u32)]; /* Padding: libc5 uses this.. */ > }; You've not compile-time tested the size of the structure. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
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.
On Sat, 8 Jan 2022 at 03:52, David Laight <David.Laight@aculab.com> wrote: > > From: Pintu Kumar > > Sent: 07 January 2022 18:08 > > > > 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. > > > > Thus, this patch introduce a new field as availram in sysinfo > > so that all the info total/free/available can be retrieved from > > one place itself. > > > ... > > diff --git a/include/uapi/linux/sysinfo.h b/include/uapi/linux/sysinfo.h > > index 435d5c2..fe84c6a 100644 > > --- a/include/uapi/linux/sysinfo.h > > +++ b/include/uapi/linux/sysinfo.h > > @@ -19,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.. */ > > There are 4 pad bytes here on most 64bit architectures. > > > + __kernel_ulong_t availram; /* Memory available for allocation */ > > + char _f[20-3*sizeof(__kernel_ulong_t)-sizeof(__u32)]; /* Padding: libc5 uses this.. */ > > }; > > You've not compile-time tested the size of the structure. > With "32" instead of "20" in padding I get these size of sysinfo: In x86-64 kernel, with app 64-bit: Size of sysinfo = 128 In x86-64 kernel, with app 32-bit:: Size of sysinfo = 76 In arm-64 kernel, with app 32-bit: Size of sysinfo = 76 Okay the sys robot reported some issue in 64-bit build. {{{ >> include/uapi/linux/sysinfo.h:23:14: error: size of array '_f' is too large >> 23 | char _f[20-3*sizeof(__kernel_ulong_t)-sizeof(__u32)]; /* Padding: libc5 uses this.. */ >> | ^~ }}} Also, I got the same issue while building for arm64, so I tried to adjust like this: char _f[32-3*sizeof(__kernel_ulong_t)-sizeof(__u32)]; With this the build works on both 32/64 but output fails when running 32-bit program on 64-bit kernel. Also, the free command on 64-bit reports "stack smashing error".. How do we resolve this issue to make it work on both arch ? Also, I don't really understand the significance of that number "20" in padding ? Thanks, Pintu
From: Pintu Agarwal > Sent: 08 January 2022 16:53 > > On Sat, 8 Jan 2022 at 03:52, David Laight <David.Laight@aculab.com> wrote: > > > > From: Pintu Kumar > > > Sent: 07 January 2022 18:08 > > > > > > 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. > > > > > > Thus, this patch introduce a new field as availram in sysinfo > > > so that all the info total/free/available can be retrieved from > > > one place itself. > > > > > ... > > > diff --git a/include/uapi/linux/sysinfo.h b/include/uapi/linux/sysinfo.h > > > index 435d5c2..fe84c6a 100644 > > > --- a/include/uapi/linux/sysinfo.h > > > +++ b/include/uapi/linux/sysinfo.h > > > @@ -19,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.. */ > > > > There are 4 pad bytes here on most 64bit architectures. > > > > > + __kernel_ulong_t availram; /* Memory available for allocation */ > > > + char _f[20-3*sizeof(__kernel_ulong_t)-sizeof(__u32)]; /* Padding: libc5 uses this.. */ > > > }; > > > > You've not compile-time tested the size of the structure. > > > With "32" instead of "20" in padding I get these size of sysinfo: > In x86-64 kernel, with app 64-bit: Size of sysinfo = 128 > In x86-64 kernel, with app 32-bit:: Size of sysinfo = 76 > In arm-64 kernel, with app 32-bit: Size of sysinfo = 76 You need to compare the sizes before and after your patch to ensure it doesn't change on any architecture. > Okay the sys robot reported some issue in 64-bit build. > {{{ > >> include/uapi/linux/sysinfo.h:23:14: error: size of array '_f' is too large > >> 23 | char _f[20-3*sizeof(__kernel_ulong_t)-sizeof(__u32)]; /* Padding: libc5 uses > this.. */ > >> | ^~ > }}} > > Also, I got the same issue while building for arm64, so I tried to > adjust like this: > char _f[32-3*sizeof(__kernel_ulong_t)-sizeof(__u32)]; > > With this the build works on both 32/64 but output fails when running > 32-bit program on 64-bit kernel. > Also, the free command on 64-bit reports "stack smashing error".. > > How do we resolve this issue to make it work on both arch ? > Also, I don't really understand the significance of that number "20" > in padding ? My guess is that someone added a char _f[20] pad to allow for expansion. Then two __kernel_ulong_t and one __u32 field were added, so the size of the pad was reduced. When __kernel_ulong_t is 64bit then it seems to be char _f[0] - which might generate a compile warning since you are supposed to use char _f[] to indicate an extensible structure. There is, however, 4 bytes of pad after the _f[] on most 64bit architectures. So actually there isn't enough space to anything useful at all. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Sat, Jan 08, 2022 at 09:54:37PM +0530, Pintu Agarwal wrote: > 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. Thanks for explanation. > > > 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 ? If there is no space left in uapi structures then we simply can't extend the syscall, since we're not allowed to break api. an option is to deprecate old interface and introduce a new one but this is a painfull procedure that's why i'm not convinced that we should extend sysinfo right now. but final decision is up to mantainers of course.
On Sun, 9 Jan 2022 at 04:05, David Laight <David.Laight@aculab.com> wrote: > > From: Pintu Agarwal > > Sent: 08 January 2022 16:53 > > > > On Sat, 8 Jan 2022 at 03:52, David Laight <David.Laight@aculab.com> wrote: > > > > > > From: Pintu Kumar > > > > Sent: 07 January 2022 18:08 > > > > > > > > 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. > > > > > > > > Thus, this patch introduce a new field as availram in sysinfo > > > > so that all the info total/free/available can be retrieved from > > > > one place itself. > > > > > > > ... > > > > diff --git a/include/uapi/linux/sysinfo.h b/include/uapi/linux/sysinfo.h > > > > index 435d5c2..fe84c6a 100644 > > > > --- a/include/uapi/linux/sysinfo.h > > > > +++ b/include/uapi/linux/sysinfo.h > > > > @@ -19,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.. */ > > > > > > There are 4 pad bytes here on most 64bit architectures. > > > > > > > + __kernel_ulong_t availram; /* Memory available for allocation */ > > > > + char _f[20-3*sizeof(__kernel_ulong_t)-sizeof(__u32)]; /* Padding: libc5 uses this.. */ > > > > }; > > > > > > You've not compile-time tested the size of the structure. > > > > > With "32" instead of "20" in padding I get these size of sysinfo: > > In x86-64 kernel, with app 64-bit: Size of sysinfo = 128 > > In x86-64 kernel, with app 32-bit:: Size of sysinfo = 76 > > In arm-64 kernel, with app 32-bit: Size of sysinfo = 76 > > You need to compare the sizes before and after your patch > to ensure it doesn't change on any architecture. Without the changes: On 32-bit, the size of structure = 64 On 64-bit, the size of structure = 112 With the addition of my new field (availram) if I try to fix the size issue on one arch, the other arch gets disturbed. I could fix the same size issue on 64-bit with below changes: __kernel_ulong_t freeswap; /* swap space still available */ __u16 procs; /* Number of current processes */ __u16 pad; /* Explicit padding for m68k */ + __u32 mem_unit; /* Memory unit size in bytes */ ============> Move the mem_unit up to adjust the padding __kernel_ulong_t totalhigh; /* Total high memory size */ __kernel_ulong_t freehigh; /* Available high memory size */ - __u32 mem_unit; /* Memory unit size in bytes */ + __kernel_ulong_t availram; /* Memory available for allocation */ ========> Add the new field here - char _f[20-2*sizeof(__kernel_ulong_t)-sizeof(__u32)]; /* Padding: libc5 uses this.. */ + char _f[28-3*sizeof(__kernel_ulong_t)-sizeof(__u32)]; /* Padding: libc5 uses this.. */ ====> Increase the size to 28 (thus _f becomes 0 like original) + //char _f[4]; }; Output with 64-bit build: $ gcc test-sysinfo.c ; ./a.out Total RAM: 32715804 kB Free RAM: 1111296 kB Size of sysinfo = 112 Size of sysinfo uptime = 8 Size of sysinfo loads = 24 Size of sysinfo totalram = 8 Size of sysinfo pad = 2 Size of sysinfo memunit = 4 Size of sysinfo _f = 0 Output with 32-bit build: $ gcc test-sysinfo.c -m32 ; ./a.out Total RAM: 7987 kB Free RAM: 271 kB Size of sysinfo = 72 Size of sysinfo uptime = 4 Size of sysinfo loads = 12 Size of sysinfo totalram = 4 Size of sysinfo pad = 2 Size of sysinfo memunit = 4 Size of sysinfo _f = 12 Are there any more suggestions/ideas to experiment with padding changes before we give-up ? Can we handle it using : __arch64__ check ? Or, the only option is to add one more, say: sysinfo64 ? > > Okay the sys robot reported some issue in 64-bit build. > > {{{ > > >> include/uapi/linux/sysinfo.h:23:14: error: size of array '_f' is too large > > >> 23 | char _f[20-3*sizeof(__kernel_ulong_t)-sizeof(__u32)]; /* Padding: libc5 uses > > this.. */ > > >> | ^~ > > }}} > > > > Also, I got the same issue while building for arm64, so I tried to > > adjust like this: > > char _f[32-3*sizeof(__kernel_ulong_t)-sizeof(__u32)]; > > > > With this the build works on both 32/64 but output fails when running > > 32-bit program on 64-bit kernel. > > Also, the free command on 64-bit reports "stack smashing error".. > > > > How do we resolve this issue to make it work on both arch ? > > Also, I don't really understand the significance of that number "20" > > in padding ? > > My guess is that someone added a char _f[20] pad to allow for expansion. > Then two __kernel_ulong_t and one __u32 field were added, so the > size of the pad was reduced. > When __kernel_ulong_t is 64bit then it seems to be char _f[0] > - which might generate a compile warning since you are supposed > to use char _f[] to indicate an extensible structure. > There is, however, 4 bytes of pad after the _f[] on most 64bit > architectures. > Thanks, yes even I guessed the same. > So actually there isn't enough space to anything useful at all. > Is this problem does not exist in other UAPI structures ? Seems like nothing can be done to allow future expansion without breaking existing things and without developing the new one. Thanks, Pintu
diff --git a/include/uapi/linux/sysinfo.h b/include/uapi/linux/sysinfo.h index 435d5c2..fe84c6a 100644 --- a/include/uapi/linux/sysinfo.h +++ b/include/uapi/linux/sysinfo.h @@ -19,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.. */ }; #endif /* _LINUX_SYSINFO_H */ 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; u32 totalswap; u32 freeswap; u16 procs; @@ -2732,6 +2734,7 @@ COMPAT_SYSCALL_DEFINE1(sysinfo, struct compat_sysinfo __user *, info) s.freeram >>= bitcount; s.sharedram >>= bitcount; s.bufferram >>= bitcount; + s.availram >>= bitcount; s.totalswap >>= bitcount; s.freeswap >>= bitcount; s.totalhigh >>= bitcount; @@ -2747,6 +2750,7 @@ COMPAT_SYSCALL_DEFINE1(sysinfo, struct compat_sysinfo __user *, info) s_32.freeram = s.freeram; s_32.sharedram = s.sharedram; s_32.bufferram = s.bufferram; + s_32.availram = s.availram; s_32.totalswap = s.totalswap; s_32.freeswap = s.freeswap; s_32.procs = s.procs; diff --git a/mm/page_alloc.c b/mm/page_alloc.c index b5d62e1..5013c75 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -5786,6 +5786,7 @@ void si_meminfo(struct sysinfo *val) val->sharedram = global_node_page_state(NR_SHMEM); val->freeram = global_zone_page_state(NR_FREE_PAGES); val->bufferram = nr_blockdev_pages(); + val->availram = si_mem_available(); val->totalhigh = totalhigh_pages(); val->freehigh = nr_free_highpages(); val->mem_unit = PAGE_SIZE; @@ -5807,6 +5808,7 @@ void si_meminfo_node(struct sysinfo *val, int nid) val->totalram = managed_pages; val->sharedram = node_page_state(pgdat, NR_SHMEM); val->freeram = sum_zone_node_page_state(nid, NR_FREE_PAGES); + val->availram = si_mem_available(); #ifdef CONFIG_HIGHMEM for (zone_type = 0; zone_type < MAX_NR_ZONES; zone_type++) { struct zone *zone = &pgdat->node_zones[zone_type];