[PATCH v1 0/5] candidate fixes for 5.1-rc1 (shippable, semihosting, OOM tcg)

[PATCH v1 0/5] candidate fixes for 5.1-rc1 (shippable, semihosting, OOM tcg)

[Alex Bennée]

Hi,

These are some candidate patches for rc1. 

The first is a quick fix that finally gets shippable up and running
again. We may want to consider a grand renaming of our docker scheme
but I don't think thats something worth dropping in while we are
stabilising - especially if we change the project name on gitlab. 

The semihosting fixes had just slipped off my radar but are pretty
minor bug fixes.

Finally I whipped up the last two because of a report of QEMU OOM'ing
on some CI systems with low memory. It doesn't restore the link to
guest ram size but instead introduces an advisory helper function to
probe guest physical memory.

Please review.

Alex Bennée (3):
  shippable: add one more qemu to registry url
  util: add qemu_get_host_physmem utility function
  accel/tcg: better handle memory constrained systems

KONRAD Frederic (2):
  semihosting: defer connect_chardevs a little more to use serialx
  semihosting: don't send the trailing '\0'

 include/qemu/osdep.h      | 10 ++++++++++
 accel/tcg/translate-all.c |  7 ++++++-
 hw/semihosting/console.c  |  4 +++-
 softmmu/vl.c              |  5 +++--
 util/oslib-posix.c        | 11 +++++++++++
 util/oslib-win32.c        |  6 ++++++
 .shippable.yml            |  2 +-
 7 files changed, 40 insertions(+), 5 deletions(-)

-- 
2.20.1

[PATCH v1 1/5] shippable: add one more qemu to registry url

[Alex Bennée]

The registry url is <project>/<repo>/qemu/<image>

Perhaps we should rationalise that some day but for now.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 .shippable.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.shippable.yml b/.shippable.yml
index f6b742432e5..89d8be4291b 100644
--- a/.shippable.yml
+++ b/.shippable.yml
@@ -27,7 +27,7 @@ env:
       TARGET_LIST=ppc64-softmmu,ppc64-linux-user,ppc64abi32-linux-user
 build:
   pre_ci_boot:
-    image_name: registry.gitlab.com/qemu-project/qemu/${IMAGE}
+    image_name: registry.gitlab.com/qemu-project/qemu/qemu/${IMAGE}
     image_tag: latest
     pull: true
     options: "-e HOME=/root"
-- 
2.20.1

Re: [PATCH v1 1/5] shippable: add one more qemu to registry url

[Philippe Mathieu-Daudé]

On 7/17/20 12:51 PM, Alex Bennée wrote:
> The registry url is <project>/<repo>/qemu/<image>
> 
> Perhaps we should rationalise that some day but for now.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  .shippable.yml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/.shippable.yml b/.shippable.yml
> index f6b742432e5..89d8be4291b 100644
> --- a/.shippable.yml
> +++ b/.shippable.yml
> @@ -27,7 +27,7 @@ env:
>        TARGET_LIST=ppc64-softmmu,ppc64-linux-user,ppc64abi32-linux-user
>  build:
>    pre_ci_boot:
> -    image_name: registry.gitlab.com/qemu-project/qemu/${IMAGE}
> +    image_name: registry.gitlab.com/qemu-project/qemu/qemu/${IMAGE}
>      image_tag: latest
>      pull: true
>      options: "-e HOME=/root"
> 

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH v1 2/5] semihosting: defer connect_chardevs a little more to use serialx

[Alex Bennée]

From: KONRAD Frederic <frederic.konrad@adacore.com>

With that we can just use -semihosting-config chardev=serial0.

Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
Message-Id: <1592215252-26742-1-git-send-email-frederic.konrad@adacore.com>
[AJB: tweak commit message]
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 softmmu/vl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/softmmu/vl.c b/softmmu/vl.c
index f476ef89edb..4fedbe60c39 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -4131,8 +4131,6 @@ void qemu_init(int argc, char **argv, char **envp)
 
     qemu_opts_foreach(qemu_find_opts("chardev"),
                       chardev_init_func, NULL, &error_fatal);
-    /* now chardevs have been created we may have semihosting to connect */
-    qemu_semihosting_connect_chardevs();
 
 #ifdef CONFIG_VIRTFS
     qemu_opts_foreach(qemu_find_opts("fsdev"),
@@ -4281,6 +4279,9 @@ void qemu_init(int argc, char **argv, char **envp)
     if (foreach_device_config(DEV_DEBUGCON, debugcon_parse) < 0)
         exit(1);
 
+    /* now chardevs have been created we may have semihosting to connect */
+    qemu_semihosting_connect_chardevs();
+
     /* If no default VGA is requested, the default is "none".  */
     if (default_vga) {
         vga_model = get_default_vga_model(machine_class);
-- 
2.20.1

Re: [PATCH v1 2/5] semihosting: defer connect_chardevs a little more to use serialx

[Richard Henderson]

On 7/17/20 3:51 AM, Alex Bennée wrote:
> From: KONRAD Frederic <frederic.konrad@adacore.com>
> 
> With that we can just use -semihosting-config chardev=serial0.
> 
> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
> Message-Id: <1592215252-26742-1-git-send-email-frederic.konrad@adacore.com>
> [AJB: tweak commit message]
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  softmmu/vl.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH v1 3/5] semihosting: don't send the trailing '\0'

[Alex Bennée]

From: KONRAD Frederic <frederic.konrad@adacore.com>

Don't send the trailing 0 from the string.

Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <1592215252-26742-2-git-send-email-frederic.konrad@adacore.com>
---
 hw/semihosting/console.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/semihosting/console.c b/hw/semihosting/console.c
index 22e7827824a..9b4fee92602 100644
--- a/hw/semihosting/console.c
+++ b/hw/semihosting/console.c
@@ -52,7 +52,9 @@ static GString *copy_user_string(CPUArchState *env, target_ulong addr)
 
     do {
         if (cpu_memory_rw_debug(cpu, addr++, &c, 1, 0) == 0) {
-            s = g_string_append_c(s, c);
+            if (c) {
+                s = g_string_append_c(s, c);
+            }
         } else {
             qemu_log_mask(LOG_GUEST_ERROR,
                           "%s: passed inaccessible address " TARGET_FMT_lx,
-- 
2.20.1

Re: [PATCH v1 3/5] semihosting: don't send the trailing '\0'

[Richard Henderson]

On 7/17/20 3:51 AM, Alex Bennée wrote:
> From: KONRAD Frederic <frederic.konrad@adacore.com>
> 
> Don't send the trailing 0 from the string.
> 
> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Message-Id: <1592215252-26742-2-git-send-email-frederic.konrad@adacore.com>
> ---
>  hw/semihosting/console.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)


Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

> diff --git a/hw/semihosting/console.c b/hw/semihosting/console.c
> index 22e7827824a..9b4fee92602 100644
> --- a/hw/semihosting/console.c
> +++ b/hw/semihosting/console.c
> @@ -52,7 +52,9 @@ static GString *copy_user_string(CPUArchState *env, target_ulong addr)
>  
>      do {
>          if (cpu_memory_rw_debug(cpu, addr++, &c, 1, 0) == 0) {
> -            s = g_string_append_c(s, c);
> +            if (c) {
> +                s = g_string_append_c(s, c);
> +            }
>          } else {
>              qemu_log_mask(LOG_GUEST_ERROR,
>                            "%s: passed inaccessible address " TARGET_FMT_lx,
> 

Next cycle, we could clean up this loop a bit, rather than testing c != 0
twice.  E.g.

    while (1) {
        if (cpu_memory_rw_debug(cpu, addr++, &c, 1, 0)) {
            error;
            break;
        }
        if (c == 0) {
            break;
        }
        s = g_string_append_c(s, c);
    }


r~

[PATCH v1 4/5] util: add qemu_get_host_physmem utility function

[Alex Bennée]

This will be used in a future patch. For POSIX systems _SC_PHYS_PAGES
isn't standardised but at least appears in the man pages for
Open/FreeBSD. The result is advisory so any users of it shouldn't just
fail if we can't work it out.

The win32 stub currently returns 0 until someone with a Windows system
can develop and test a patch.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: BALATON Zoltan <balaton@eik.bme.hu>
Cc: Christian Ehrhardt <christian.ehrhardt@canonical.com>
---
 include/qemu/osdep.h | 10 ++++++++++
 util/oslib-posix.c   | 11 +++++++++++
 util/oslib-win32.c   |  6 ++++++
 3 files changed, 27 insertions(+)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 4841b5c6b5f..7ff209983e2 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -665,4 +665,14 @@ static inline void qemu_reset_optind(void)
  */
 char *qemu_get_host_name(Error **errp);
 
+/**
+ * qemu_get_host_physmem:
+ *
+ * Operating system agnostiv way of querying host memory.
+ *
+ * Returns amount of physical memory on the system. This is purely
+ * advisery and may return 0 if we can't work it out.
+ */
+size_t qemu_get_host_physmem(void);
+
 #endif
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 36bf8593f8c..d9da782b896 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -839,3 +839,14 @@ char *qemu_get_host_name(Error **errp)
 
     return g_steal_pointer(&hostname);
 }
+
+size_t qemu_get_host_physmem(void)
+{
+#ifdef _SC_PHYS_PAGES
+    long pages = sysconf(_SC_PHYS_PAGES);
+    if (pages > 0) {
+        return pages * qemu_real_host_page_size;
+    }
+#endif
+    return 0;
+}
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 7eedbe5859a..31030463cc9 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -828,3 +828,9 @@ char *qemu_get_host_name(Error **errp)
 
     return g_utf16_to_utf8(tmp, size, NULL, NULL, NULL);
 }
+
+size_t qemu_get_host_physmem(void)
+{
+    /* currently unimplemented */
+    return 0;
+}
-- 
2.20.1

Re: [PATCH v1 4/5] util: add qemu_get_host_physmem utility function

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2417 bytes --]

On Fri, 17 Jul 2020, Alex Bennée wrote:
> This will be used in a future patch. For POSIX systems _SC_PHYS_PAGES
> isn't standardised but at least appears in the man pages for
> Open/FreeBSD. The result is advisory so any users of it shouldn't just
> fail if we can't work it out.
>
> The win32 stub currently returns 0 until someone with a Windows system
> can develop and test a patch.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: BALATON Zoltan <balaton@eik.bme.hu>
> Cc: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> ---
> include/qemu/osdep.h | 10 ++++++++++
> util/oslib-posix.c   | 11 +++++++++++
> util/oslib-win32.c   |  6 ++++++
> 3 files changed, 27 insertions(+)
>
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 4841b5c6b5f..7ff209983e2 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -665,4 +665,14 @@ static inline void qemu_reset_optind(void)
>  */
> char *qemu_get_host_name(Error **errp);
>
> +/**
> + * qemu_get_host_physmem:
> + *
> + * Operating system agnostiv way of querying host memory.

Typo: agnostiv -> agnostic

> + *
> + * Returns amount of physical memory on the system. This is purely
> + * advisery and may return 0 if we can't work it out.
> + */
> +size_t qemu_get_host_physmem(void);
> +
> #endif
> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index 36bf8593f8c..d9da782b896 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -839,3 +839,14 @@ char *qemu_get_host_name(Error **errp)
>
>     return g_steal_pointer(&hostname);
> }
> +
> +size_t qemu_get_host_physmem(void)
> +{
> +#ifdef _SC_PHYS_PAGES
> +    long pages = sysconf(_SC_PHYS_PAGES);
> +    if (pages > 0) {
> +        return pages * qemu_real_host_page_size;

The Linux man page warns that this product may overflow so maybe you could 
return pages here.

> +    }
> +#endif
> +    return 0;
> +}
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index 7eedbe5859a..31030463cc9 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -828,3 +828,9 @@ char *qemu_get_host_name(Error **errp)
>
>     return g_utf16_to_utf8(tmp, size, NULL, NULL, NULL);
> }
> +
> +size_t qemu_get_host_physmem(void)
> +{
> +    /* currently unimplemented */
> +    return 0;
> +}

For Windows this may help:

https://stackoverflow.com/questions/5553665/get-ram-system-size

not sure about other OSes.

Regards,
BALATON Zoltan

Re: [PATCH v1 4/5] util: add qemu_get_host_physmem utility function

[Christian Ehrhardt]

[-- Attachment #1: Type: text/plain, Size: 2997 bytes --]

On Fri, Jul 17, 2020 at 3:32 PM BALATON Zoltan <balaton@eik.bme.hu> wrote:

> On Fri, 17 Jul 2020, Alex Bennée wrote:
> > This will be used in a future patch. For POSIX systems _SC_PHYS_PAGES
> > isn't standardised but at least appears in the man pages for
> > Open/FreeBSD. The result is advisory so any users of it shouldn't just
> > fail if we can't work it out.
> >
> > The win32 stub currently returns 0 until someone with a Windows system
> > can develop and test a patch.
> >
> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> > Cc: BALATON Zoltan <balaton@eik.bme.hu>
> > Cc: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> > ---
> > include/qemu/osdep.h | 10 ++++++++++
> > util/oslib-posix.c   | 11 +++++++++++
> > util/oslib-win32.c   |  6 ++++++
> > 3 files changed, 27 insertions(+)
> >
> > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > index 4841b5c6b5f..7ff209983e2 100644
> > --- a/include/qemu/osdep.h
> > +++ b/include/qemu/osdep.h
> > @@ -665,4 +665,14 @@ static inline void qemu_reset_optind(void)
> >  */
> > char *qemu_get_host_name(Error **errp);
> >
> > +/**
> > + * qemu_get_host_physmem:
> > + *
> > + * Operating system agnostiv way of querying host memory.
>
> Typo: agnostiv -> agnostic
>
> > + *
> > + * Returns amount of physical memory on the system. This is purely
> > + * advisery and may return 0 if we can't work it out.
> > + */
> > +size_t qemu_get_host_physmem(void);
> > +
> > #endif
> > diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> > index 36bf8593f8c..d9da782b896 100644
> > --- a/util/oslib-posix.c
> > +++ b/util/oslib-posix.c
> > @@ -839,3 +839,14 @@ char *qemu_get_host_name(Error **errp)
> >
> >     return g_steal_pointer(&hostname);
> > }
> > +
> > +size_t qemu_get_host_physmem(void)
> > +{
> > +#ifdef _SC_PHYS_PAGES
> > +    long pages = sysconf(_SC_PHYS_PAGES);
> > +    if (pages > 0) {
> > +        return pages * qemu_real_host_page_size;
>
> The Linux man page warns that this product may overflow so maybe you could
> return pages here.
>

The caller might be even less aware of that than this function - so maybe
better handle it here.
How about handling overflows and cutting it to MiB before returning?


> > +    }
> > +#endif
> > +    return 0;
> > +}
> > diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> > index 7eedbe5859a..31030463cc9 100644
> > --- a/util/oslib-win32.c
> > +++ b/util/oslib-win32.c
> > @@ -828,3 +828,9 @@ char *qemu_get_host_name(Error **errp)
> >
> >     return g_utf16_to_utf8(tmp, size, NULL, NULL, NULL);
> > }
> > +
> > +size_t qemu_get_host_physmem(void)
> > +{
> > +    /* currently unimplemented */
> > +    return 0;
> > +}
>
> For Windows this may help:
>
> https://stackoverflow.com/questions/5553665/get-ram-system-size
>
> not sure about other OSes.
>
> Regards,
> BALATON Zoltan



-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

[-- Attachment #2: Type: text/html, Size: 4292 bytes --]

Re: [PATCH v1 4/5] util: add qemu_get_host_physmem utility function

[Richard Henderson]

On 7/17/20 7:24 AM, Christian Ehrhardt wrote:
>     > +size_t qemu_get_host_physmem(void)
>     > +{
>     > +#ifdef _SC_PHYS_PAGES
>     > +    long pages = sysconf(_SC_PHYS_PAGES);
>     > +    if (pages > 0) {
>     > +        return pages * qemu_real_host_page_size;
> 
>     The Linux man page warns that this product may overflow so maybe you could
>     return pages here.
> 
> 
> The caller might be even less aware of that than this function - so maybe
> better handle it here.
> How about handling overflows and cutting it to MiB before returning?

Indeed, the caller may be less aware, so we should handle it here.  But I don't
think truncating to MiB helps at all, because again, the caller has to handle
overflow.

Better, I think, to saturate the result to ~(size_t)0 and leave it at that.


r~

Re: [PATCH v1 4/5] util: add qemu_get_host_physmem utility function

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 7/17/20 7:24 AM, Christian Ehrhardt wrote:
>>     > +size_t qemu_get_host_physmem(void)
>>     > +{
>>     > +#ifdef _SC_PHYS_PAGES
>>     > +    long pages = sysconf(_SC_PHYS_PAGES);
>>     > +    if (pages > 0) {
>>     > +        return pages * qemu_real_host_page_size;
>> 
>>     The Linux man page warns that this product may overflow so maybe you could
>>     return pages here.
>> 
>> 
>> The caller might be even less aware of that than this function - so maybe
>> better handle it here.
>> How about handling overflows and cutting it to MiB before returning?
>
> Indeed, the caller may be less aware, so we should handle it here.  But I don't
> think truncating to MiB helps at all, because again, the caller has to handle
> overflow.
>
> Better, I think, to saturate the result to ~(size_t)0 and leave it at
> that.

So I went for:

  size_t qemu_get_host_physmem(void)
  {
  #ifdef _SC_PHYS_PAGES
      long pages = sysconf(_SC_PHYS_PAGES);
      if (pages > 0) {
          if (pages > SIZE_MAX / qemu_real_host_page_size) {
              return SIZE_MAX;
          } else {
              return pages * qemu_real_host_page_size;
          }
      }
  #endif
      return 0;
  }

apparently the first case of saturating integer arithmetic outside of
the instruction emulation in QEMU :-/

-- 
Alex Bennée

Re: [PATCH v1 4/5] util: add qemu_get_host_physmem utility function

[Richard Henderson]

On 7/17/20 3:51 AM, Alex Bennée wrote:
> +size_t qemu_get_host_physmem(void)
> +{
> +#ifdef _SC_PHYS_PAGES
> +    long pages = sysconf(_SC_PHYS_PAGES);
> +    if (pages > 0) {
> +        return pages * qemu_real_host_page_size;
> +    }
> +#endif
> +    return 0;
> +}

Is it worth examining our own RLIMIT_{AS,DATA} as well?

I suppose that's not usually what is tweaked in the example of a ram-limited
container...


r~

Re: [PATCH v1 4/5] util: add qemu_get_host_physmem utility function

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 7/17/20 3:51 AM, Alex Bennée wrote:
>> +size_t qemu_get_host_physmem(void)
>> +{
>> +#ifdef _SC_PHYS_PAGES
>> +    long pages = sysconf(_SC_PHYS_PAGES);
>> +    if (pages > 0) {
>> +        return pages * qemu_real_host_page_size;
>> +    }
>> +#endif
>> +    return 0;
>> +}
>
> Is it worth examining our own RLIMIT_{AS,DATA} as well?

We don't anywhere else in the code so for now I'd say not.

>
> I suppose that's not usually what is tweaked in the example of a ram-limited
> container...
>
>
> r~


-- 
Alex Bennée

[PATCH v1 5/5] accel/tcg: better handle memory constrained systems

[Alex Bennée]

It turns out there are some 64 bit systems that have relatively low
amounts of physical memory available to them (typically CI system).
Even with swapping available a 1GB translation buffer that fills up
can put the machine under increased memory pressure. Detect these low
memory situations and reduce tb_size appropriately.

Fixes: 600e17b261
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: BALATON Zoltan <balaton@eik.bme.hu>
Cc: Christian Ehrhardt <christian.ehrhardt@canonical.com>
---
 accel/tcg/translate-all.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 2afa46bd2b1..2ff0ba6d19b 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -976,7 +976,12 @@ static inline size_t size_code_gen_buffer(size_t tb_size)
 {
     /* Size the buffer.  */
     if (tb_size == 0) {
-        tb_size = DEFAULT_CODE_GEN_BUFFER_SIZE;
+        size_t phys_mem = qemu_get_host_physmem();
+        if (phys_mem > 0 && phys_mem < (2 * DEFAULT_CODE_GEN_BUFFER_SIZE)) {
+            tb_size = phys_mem / 4;
+        } else {
+            tb_size = DEFAULT_CODE_GEN_BUFFER_SIZE;
+        }
     }
     if (tb_size < MIN_CODE_GEN_BUFFER_SIZE) {
         tb_size = MIN_CODE_GEN_BUFFER_SIZE;
-- 
2.20.1

Re: [PATCH v1 5/5] accel/tcg: better handle memory constrained systems

[Christian Ehrhardt]

[-- Attachment #1: Type: text/plain, Size: 1728 bytes --]

On Fri, Jul 17, 2020 at 12:51 PM Alex Bennée <alex.bennee@linaro.org> wrote:

> It turns out there are some 64 bit systems that have relatively low
> amounts of physical memory available to them (typically CI system).
> Even with swapping available a 1GB translation buffer that fills up
> can put the machine under increased memory pressure. Detect these low
> memory situations and reduce tb_size appropriately.
>
> Fixes: 600e17b261
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: BALATON Zoltan <balaton@eik.bme.hu>
> Cc: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> ---
>  accel/tcg/translate-all.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index 2afa46bd2b1..2ff0ba6d19b 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -976,7 +976,12 @@ static inline size_t size_code_gen_buffer(size_t
> tb_size)
>  {
>      /* Size the buffer.  */
>      if (tb_size == 0) {
> -        tb_size = DEFAULT_CODE_GEN_BUFFER_SIZE;
> +        size_t phys_mem = qemu_get_host_physmem();
> +        if (phys_mem > 0 && phys_mem < (2 *
> DEFAULT_CODE_GEN_BUFFER_SIZE)) {
> +            tb_size = phys_mem / 4;
>

In my experiments I've found that /8 more closely matches the former
behavior
on small hosts while at the same time not affecting common large hosts.


> +        } else {
> +            tb_size = DEFAULT_CODE_GEN_BUFFER_SIZE;
> +        }
>      }
>      if (tb_size < MIN_CODE_GEN_BUFFER_SIZE) {
>          tb_size = MIN_CODE_GEN_BUFFER_SIZE;
> --
> 2.20.1
>
>

-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

[-- Attachment #2: Type: text/html, Size: 2626 bytes --]

Re: [PATCH v1 5/5] accel/tcg: better handle memory constrained systems

[Daniel P. Berrangé]

On Fri, Jul 17, 2020 at 11:51:39AM +0100, Alex Bennée wrote:
> It turns out there are some 64 bit systems that have relatively low
> amounts of physical memory available to them (typically CI system).
> Even with swapping available a 1GB translation buffer that fills up
> can put the machine under increased memory pressure. Detect these low
> memory situations and reduce tb_size appropriately.
> 
> Fixes: 600e17b261
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: BALATON Zoltan <balaton@eik.bme.hu>
> Cc: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> ---
>  accel/tcg/translate-all.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index 2afa46bd2b1..2ff0ba6d19b 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -976,7 +976,12 @@ static inline size_t size_code_gen_buffer(size_t tb_size)
>  {
>      /* Size the buffer.  */
>      if (tb_size == 0) {
> -        tb_size = DEFAULT_CODE_GEN_BUFFER_SIZE;
> +        size_t phys_mem = qemu_get_host_physmem();
> +        if (phys_mem > 0 && phys_mem < (2 * DEFAULT_CODE_GEN_BUFFER_SIZE)) {
> +            tb_size = phys_mem / 4;
> +        } else {
> +            tb_size = DEFAULT_CODE_GEN_BUFFER_SIZE;
> +        }

I'm not convinced this is going to work when running QEMU inside a
container environment with RAM cap, because the physmem level is
completely unrelated to the RAM the container is permitted to actually
use in practice. ie host has 32 GB of RAM, but the container QEMU is
in only has 1 GB permitted.

I don't have much of a better suggestion, as I don't think we want
to get into reading the cgroups memory limits. It does feel like the
assumption we can blindly use a 1GB cache though is invalid even
with this patch applied.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v1 5/5] accel/tcg: better handle memory constrained systems

[Alex Bennée]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Fri, Jul 17, 2020 at 11:51:39AM +0100, Alex Bennée wrote:
>> It turns out there are some 64 bit systems that have relatively low
>> amounts of physical memory available to them (typically CI system).
>> Even with swapping available a 1GB translation buffer that fills up
>> can put the machine under increased memory pressure. Detect these low
>> memory situations and reduce tb_size appropriately.
>> 
>> Fixes: 600e17b261
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Cc: BALATON Zoltan <balaton@eik.bme.hu>
>> Cc: Christian Ehrhardt <christian.ehrhardt@canonical.com>
>> ---
>>  accel/tcg/translate-all.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>> 
>> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
>> index 2afa46bd2b1..2ff0ba6d19b 100644
>> --- a/accel/tcg/translate-all.c
>> +++ b/accel/tcg/translate-all.c
>> @@ -976,7 +976,12 @@ static inline size_t size_code_gen_buffer(size_t tb_size)
>>  {
>>      /* Size the buffer.  */
>>      if (tb_size == 0) {
>> -        tb_size = DEFAULT_CODE_GEN_BUFFER_SIZE;
>> +        size_t phys_mem = qemu_get_host_physmem();
>> +        if (phys_mem > 0 && phys_mem < (2 * DEFAULT_CODE_GEN_BUFFER_SIZE)) {
>> +            tb_size = phys_mem / 4;
>> +        } else {
>> +            tb_size = DEFAULT_CODE_GEN_BUFFER_SIZE;
>> +        }
>
> I'm not convinced this is going to work when running QEMU inside a
> container environment with RAM cap, because the physmem level is
> completely unrelated to the RAM the container is permitted to actually
> use in practice. ie host has 32 GB of RAM, but the container QEMU is
> in only has 1 GB permitted.

What will happen when the mmap happens? Will a capped container limit
the attempted mmap? I would hope the container case at least gave
different feedback than a "silent" OOM.

> I don't have much of a better suggestion, as I don't think we want
> to get into reading the cgroups memory limits. It does feel like the
> assumption we can blindly use a 1GB cache though is invalid even
> with this patch applied.
>
> Regards,
> Daniel


-- 
Alex Bennée

Re: [PATCH v1 5/5] accel/tcg: better handle memory constrained systems

[Daniel P. Berrangé]

On Fri, Jul 17, 2020 at 03:55:15PM +0100, Alex Bennée wrote:
> 
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Fri, Jul 17, 2020 at 11:51:39AM +0100, Alex Bennée wrote:
> >> It turns out there are some 64 bit systems that have relatively low
> >> amounts of physical memory available to them (typically CI system).
> >> Even with swapping available a 1GB translation buffer that fills up
> >> can put the machine under increased memory pressure. Detect these low
> >> memory situations and reduce tb_size appropriately.
> >> 
> >> Fixes: 600e17b261
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >> Cc: BALATON Zoltan <balaton@eik.bme.hu>
> >> Cc: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> >> ---
> >>  accel/tcg/translate-all.c | 7 ++++++-
> >>  1 file changed, 6 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> >> index 2afa46bd2b1..2ff0ba6d19b 100644
> >> --- a/accel/tcg/translate-all.c
> >> +++ b/accel/tcg/translate-all.c
> >> @@ -976,7 +976,12 @@ static inline size_t size_code_gen_buffer(size_t tb_size)
> >>  {
> >>      /* Size the buffer.  */
> >>      if (tb_size == 0) {
> >> -        tb_size = DEFAULT_CODE_GEN_BUFFER_SIZE;
> >> +        size_t phys_mem = qemu_get_host_physmem();
> >> +        if (phys_mem > 0 && phys_mem < (2 * DEFAULT_CODE_GEN_BUFFER_SIZE)) {
> >> +            tb_size = phys_mem / 4;
> >> +        } else {
> >> +            tb_size = DEFAULT_CODE_GEN_BUFFER_SIZE;
> >> +        }
> >
> > I'm not convinced this is going to work when running QEMU inside a
> > container environment with RAM cap, because the physmem level is
> > completely unrelated to the RAM the container is permitted to actually
> > use in practice. ie host has 32 GB of RAM, but the container QEMU is
> > in only has 1 GB permitted.
> 
> What will happen when the mmap happens? Will a capped container limit
> the attempted mmap? I would hope the container case at least gave
> different feedback than a "silent" OOM.

IIRC it should trigger the OOM killer on process(s) within the container.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|