• [PATCH] libdpkg: Use the amount of available memory instead phys_mem/2

    From Sebastian Andrzej Siewior@21:1/5 to All on Fri Jan 8 23:10:02 2021
    On Linux there is the field `MemAvailable' in `/proc/meminfo' which
    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 avilable is usable.

    [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.

    lib/dpkg/compress.c | 95 ++++++++++++++++++++++++++++++++++++++++++---
    1 file changed, 90 insertions(+), 5 deletions(-)

    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>
    +
    +/*
    + * An estimate of how much memory is available. Swap will not be used, the page
    + * cache may be purged, not everything will be
  • From Guillem Jover@21:1/5 to Sebastian Andrzej Siewior on Sun Jan 24 01:50:02 2021
    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

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)
  • From Sebastian Andrzej Siewior@21:1/5 to Guillem Jover on Sun Jan 24 23:40:01 2021
    On 2021-01-24 01:47:18 [+0100], Guillem Jover wrote:
    Hi!
    Hi,

    [ You might want to take a peek at doc/coding-style.txt, although
    I've covered these in the review. :) ]

    sadly, I've been lookig…

    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. :)

    okay.

    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?

    correct.

    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.

    If you do have an idea how to scale, sure.
    So that value holds the amount of memory that should be available as of
    now. That should fine unless two+ programs are currently doing something according to that value. Otherwise we should be good. Knock knock wood.

    I was afraid of the parallel decompressor eating too much memory. The compressor is more or less limited as there (in my imagination) one
    dpkg-build instance on a system/buildd. The decompressor on the other
    hand runs on a user system which may already use more than physmem/2.
    So if you have an idea how to limit it further…

    [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.

    Yes. The parallel decompression has been posted upstream for review and
    I don't see this comming for Bullseye. So yes.

    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.

    Okay.

    +/*
    + * 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.

    from looking at file_slurp_fd() it uses fstat() to determine the amount
    of memory to allocate. This does not work with the proc file. It always
    reports 0 and you have to read until EOF to get the whole file.
    In theorhy, we could read less since the string we look for is at the beginning.

    + 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.

    okay.

    + 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.

    Not sure what you are getting at. The kernel does not put negative
    numbers. strtoimax() would have a limit at 2TiB which is not something I
    expect on 32bit. But then you may run 32bit on a 64bit kernel ;)

    I update as asked…

    + 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.

    Okay.

    + 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

    Sebastian

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)
  • From Sebastian Andrzej Siewior@21:1/5 to All on Sat Jan 30 23:30:02 2021
    On Linux there is the field 'MemAvailable' in '/proc/meminfo' which
    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 writing 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 assuming that half of what is physically available is usable.

    [0] https://www.kernel.org/doc/html/latest/filesystems/proc.html
    Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
    ---
    v1…v2:
    - replaced `' with ''
    - spelling
    - replaced !val with val == 0
    - using strtoimax() for string to value, setting errno to 0 and
    checking it against ERANGE if the max values was returned.
    - moved includes to the top
    - keeping open+read+close stat() does not work on proc files

    lib/dpkg/compress.c | 99 ++++++++++++++++++++++++++++++++++++++++++---
    1 file changed, 94 insertions(+), 5 deletions(-)

    diff --git a/lib/dpkg/compress.c b/lib/dpkg/compress.c
    index 41991317afe53..84adb8b06c1a2 100644
    --- a/lib/dpkg/compress.c
    +++ b/lib/dpkg/compress.c
    @@ -23,8 +23,11 @@
    #include <config.h>
    #include <compat.h>

    +#include <sys/stat.h>
    +
    #include <errno.h>
    #include <string.h>
    +#include <fcntl.h>
    #include <unistd.h>
    #include <stdbool.h>
    #include <stdlib.h>
    @@ -523,6 +526,89 @@ filter_lzma_error(struct io_lzma *io, lzma_ret ret)
    dpkg_lzma_strerror(ret, io->status));
    }

    +#ifdef HAVE_LZMA_MT
    +# ifdef __linux__
    +
    +/*
    + * An estimate of how much memor