Re: [PATCH] libdpkg: Use the amount of available memory instead phys_mem/2
Hi!
Thanks for the patch!
[ You might want to take a peek at doc/coding-style.txt, although
I've covered these in the review. :) ]
On Fri, 2021-01-08 at 23:03:30 +0100, Sebastian Andrzej Siewior wrote:
> On Linux there is the field `MemAvailable' in `/proc/meminfo' which
Please no unbalanced `' quotes. :)
> holds the amount of memory which is available as of now. It considers
> the fact that the page cache can purge (if not all) some memory can be
> reclaimed (for instance by writting back dirty inodes) and some memory
> should remain free just in case. This amount of memory can be used
> without the need to swap-out some memory. The complete definition can
> be located at [0].
>
> The advantage over PHYS_MEM/2 is that it considers the current
> status/usage of the system with assumung that half of what is physically
^assuming?
> avilable is usable.
Right, it's probably a better metric to use, but I'm not sure we
should take the entire value as is, instead of scaling it down,
otherwise we might still try to use too much memory and leaving the
rest of the system w/o.
> [0] https://www.kernel.org/doc/html/latest/filesystems/proc.html
> Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
> ---
>
> This is a bit of my multithreaded XZ decompressor for dpkg. For
> compression the PhysMem/2 estimation is probably good enough since
> mostly used the buildd and owns the system..
> For decompression the amount of memory should be close to reality so it
> does not start threads and allocate a lot of memory if the system is
> quite busy at the amout if package upgrade/installation.
Once ready I'll queue this for 1.21.x, as it indeed didn't seem urgent
to rush into 1.20.x at the point of the freeze.
> diff --git a/lib/dpkg/compress.c b/lib/dpkg/compress.c
> index 41991317afe53..7ec9144a56290 100644
> --- a/lib/dpkg/compress.c
> +++ b/lib/dpkg/compress.c
> @@ -523,6 +523,88 @@ filter_lzma_error(struct io_lzma *io, lzma_ret ret)
> dpkg_lzma_strerror(ret, io->status));
> }
>
> +#ifdef HAVE_LZMA_MT
> +# ifdef __linux__
> +
> +#include <sys/stat.h>
> +#include <fcntl.h>
Please move this with the other header inclusions. The <sys/*> on its
own paragraph after <compat.h>, and <fcntl.h> with the system ones,
before <unistd.h>. And I'd probably just include these unconditionally.
> +/*
> + * An estimate of how much memory is available. Swap will not be used, the page
> + * cache may be purged, not everything will be reclaimed what might be
> + * reclaimed, watermarks are considers.
> + */
> +static char str_MemAvailable[] = "MemAvailable";
> +
> +static int
> +get_avail_mem(uint64_t *val)
> +{
> + char buf[4096];
> + char *str;
> + ssize_t bytes;
> + int fd;
> +
> + *val = 0;
> +
> + fd = open("/proc/meminfo", O_RDONLY);
> + if (fd < 0)
> + return -1;
> +
> + bytes = read(fd, buf, sizeof(buf));
> + close(fd);
We could use file_slurp() here.
> + if (bytes <= 0)
> + return -1;
> +
> + buf[bytes] = '\0';
> +
> + str = buf;
> + while (1) {
> + char *end;
> +
> + end = strchr(str, ':');
> + if (!end)
> + break;
> + if ((end - str) == sizeof(str_MemAvailable) - 1) {
> + if (!strncmp(str, str_MemAvailable,
> + sizeof(str_MemAvailable) - 1)) {
Use == instead of !, which makes it obvious what the comparison is
about.
> + uint64_t num;
> +
> + str = end + 1;
> + num = strtoull(str, &end, 10);
I'm not very fond of strtou*() functions, as they still accept
negative numbers, just use strtoimax(). We also need to reset
errno and check it afterwards to be sure there's a range error.
> + if (!num)
> + return -1;
> + if (num == ULLONG_MAX)
> + return -1;
> + /* it should end with ' kb\n' */
While I see the Linux kernel does not currently support any other
unit, I'd be more comfortable checking for it explicitly so that in
the unlikely case this ever changes, we do not compute bogus numbers.
> + if (*end != ' ')
> + return -1;
> +
> + num *= 1024;
> + *val = num;
> + return 0;
> + }
> + }
> +
> + end = strchr(end + 1, '\n');
> + if (!end)
> + break;
> + str = end + 1;
> + }
> + return -1;
> +}
> +
Thanks,
Guillem
Reply to: