diff mbox series

[v1,2/4] accel/tcg: remove link between guest ram and TCG cache size

Message ID 20200226181020.19592-3-alex.bennee@linaro.org
State Superseded
Headers show
Series Fix codegen translation cache size | expand

Commit Message

Alex Bennée Feb. 26, 2020, 6:10 p.m. UTC 
Basing the TB cache size on the ram_size was always a little heuristic
and was broken by a1b18df9a4 which caused ram_size not to be fully
realised at the time we initialise the TCG translation cache.

The current DEFAULT_CODE_GEN_BUFFER_SIZE may still be a little small
but follow-up patches will address that.

Fixes: a1b18df9a4
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Cc: Niek Linnenbank <nieklinnenbank@gmail.com>
Cc: Igor Mammedov <imammedo@redhat.com>
---
 accel/tcg/translate-all.c | 8 --------
 1 file changed, 8 deletions(-)

-- 
2.20.1

Comments

Niek Linnenbank Feb. 26, 2020, 10:26 p.m. UTC | #1 
Hi Alex,

On Wed, Feb 26, 2020 at 7:10 PM Alex Bennée <alex.bennee@linaro.org> wrote:

> Basing the TB cache size on the ram_size was always a little heuristic

> and was broken by a1b18df9a4 which caused ram_size not to be fully

> realised at the time we initialise the TCG translation cache.

>


Now I'm beginning to understand the issue better. So without this patch,
the TCG translation
cache effectively was disabled, causing the slowdown, correct?


>

> The current DEFAULT_CODE_GEN_BUFFER_SIZE may still be a little small

> but follow-up patches will address that.

>

> Fixes: a1b18df9a4

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

> Cc: Niek Linnenbank <nieklinnenbank@gmail.com>

> Cc: Igor Mammedov <imammedo@redhat.com>

>

---
>  accel/tcg/translate-all.c | 8 --------

>  1 file changed, 8 deletions(-)

>

> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c

> index 238b0e575bf..5b66af783b5 100644

> --- a/accel/tcg/translate-all.c

> +++ b/accel/tcg/translate-all.c

> @@ -938,15 +938,7 @@ static inline size_t size_code_gen_buffer(size_t

> tb_size)

>  {

>      /* Size the buffer.  */

>      if (tb_size == 0) {

> -#ifdef USE_STATIC_CODE_GEN_BUFFER

>          tb_size = DEFAULT_CODE_GEN_BUFFER_SIZE;

>

-#else
> -        /* ??? Needs adjustments.  */

> -        /* ??? If we relax the requirement that CONFIG_USER_ONLY use the

> -           static buffer, we could size this on RESERVED_VA, on the text

> -           segment size of the executable, or continue to use the

> default.  */

> -        tb_size = (unsigned long)(ram_size / 4);

>


As you wrote in the commit message, I think we are indeed reducing the
cache size here to 32MiB
versus a larger size without this patch. In the next patch #4 in this
series you are increasing it for 64-bit hosts,
but what about the 32-bit hosts? Or will that be addressed in a later
series?

For now, this fix works and resolves the slowdown, so:

Tested-by: Niek Linnenbank <nieklinnenbank@gmail.com>


Regards,
Niek


> -#endif

>      }

>      if (tb_size < MIN_CODE_GEN_BUFFER_SIZE) {

>          tb_size = MIN_CODE_GEN_BUFFER_SIZE;

> --

> 2.20.1

>

>


-- 
Niek Linnenbank
<div dir="ltr"><div>Hi Alex,<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Feb 26, 2020 at 7:10 PM Alex Bennée &lt;<a href="mailto:alex.bennee@linaro.org">alex.bennee@linaro.org</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Basing the TB cache size on the ram_size was always a little heuristic<br>
and was broken by a1b18df9a4 which caused ram_size not to be fully<br>
realised at the time we initialise the TCG translation cache.<br></blockquote><div><br></div><div>Now I&#39;m beginning to understand the issue better. So without this patch, the TCG translation</div><div>cache effectively was disabled, causing the slowdown, correct?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
The current DEFAULT_CODE_GEN_BUFFER_SIZE may still be a little small<br>
but follow-up patches will address that.<br>
<br>
Fixes: a1b18df9a4<br>
Signed-off-by: Alex Bennée &lt;<a href="mailto:alex.bennee@linaro.org" target="_blank">alex.bennee@linaro.org</a>&gt;<br>

Cc: Niek Linnenbank &lt;<a href="mailto:nieklinnenbank@gmail.com" target="_blank">nieklinnenbank@gmail.com</a>&gt;<br>
Cc: Igor Mammedov &lt;<a href="mailto:imammedo@redhat.com" target="_blank">imammedo@redhat.com</a>&gt; <br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
---<br>
 accel/tcg/translate-all.c | 8 --------<br>
 1 file changed, 8 deletions(-)<br>
<br>
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c<br>
index 238b0e575bf..5b66af783b5 100644<br>
--- a/accel/tcg/translate-all.c<br>
+++ b/accel/tcg/translate-all.c<br>
@@ -938,15 +938,7 @@ static inline size_t size_code_gen_buffer(size_t tb_size)<br>
 {<br>
     /* Size the buffer.  */<br>
     if (tb_size == 0) {<br>
-#ifdef USE_STATIC_CODE_GEN_BUFFER<br>
         tb_size = DEFAULT_CODE_GEN_BUFFER_SIZE; <br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
-#else<br>
-        /* ??? Needs adjustments.  */<br>
-        /* ??? If we relax the requirement that CONFIG_USER_ONLY use the<br>
-           static buffer, we could size this on RESERVED_VA, on the text<br>
-           segment size of the executable, or continue to use the default.  */<br>
-        tb_size = (unsigned long)(ram_size / 4);<br></blockquote><div><br></div><div>As you wrote in the commit message, I think we are indeed reducing the cache size here to 32MiB</div><div>versus a larger size without this patch. In the next patch #4 in this series you are increasing it for 64-bit hosts,</div><div>but what about the 32-bit hosts? Or will that be addressed in a later series?<br></div><div><br></div><div>For now, this fix works and resolves the slowdown, so:</div><div><br></div><div>Tested-by: Niek Linnenbank &lt;<a href="mailto:nieklinnenbank@gmail.com">nieklinnenbank@gmail.com</a>&gt;<br></div><div><br></div><div>Regards,</div><div>Niek<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
-#endif<br>
     }<br>
     if (tb_size &lt; MIN_CODE_GEN_BUFFER_SIZE) {<br>
         tb_size = MIN_CODE_GEN_BUFFER_SIZE;<br>
-- <br>
2.20.1<br>
<br>
</blockquote></div><br clear="all"><br>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div>Niek Linnenbank<br><br></div></div></div></div>
Richard Henderson Feb. 26, 2020, 10:49 p.m. UTC | #2 
On 2/26/20 10:10 AM, Alex Bennée wrote:
> Basing the TB cache size on the ram_size was always a little heuristic

> and was broken by a1b18df9a4 which caused ram_size not to be fully

> realised at the time we initialise the TCG translation cache.

> 

> The current DEFAULT_CODE_GEN_BUFFER_SIZE may still be a little small

> but follow-up patches will address that.

> 

> Fixes: a1b18df9a4

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

> Cc: Niek Linnenbank <nieklinnenbank@gmail.com>

> Cc: Igor Mammedov <imammedo@redhat.com>

> ---

>  accel/tcg/translate-all.c | 8 --------

>  1 file changed, 8 deletions(-)


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



r~
Richard Henderson Feb. 26, 2020, 10:50 p.m. UTC | #3 
On 2/26/20 2:26 PM, Niek Linnenbank wrote:
> Hi Alex,

> 

> On Wed, Feb 26, 2020 at 7:10 PM Alex Bennée <alex.bennee@linaro.org

> <mailto:alex.bennee@linaro.org>> wrote:

> 

>     Basing the TB cache size on the ram_size was always a little heuristic

>     and was broken by a1b18df9a4 which caused ram_size not to be fully

>     realised at the time we initialise the TCG translation cache.

> 

> 

> Now I'm beginning to understand the issue better. So without this patch, the

> TCG translation

> cache effectively was disabled, causing the slowdown, correct?


Yes.


r~
Philippe Mathieu-Daudé Feb. 27, 2020, 10:58 a.m. UTC | #4 
On 2/26/20 7:10 PM, Alex Bennée wrote:
> Basing the TB cache size on the ram_size was always a little heuristic

> and was broken by a1b18df9a4 which caused ram_size not to be fully

> realised at the time we initialise the TCG translation cache.

> 

> The current DEFAULT_CODE_GEN_BUFFER_SIZE may still be a little small

> but follow-up patches will address that.

> 

> Fixes: a1b18df9a4

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

> Cc: Niek Linnenbank <nieklinnenbank@gmail.com>

> Cc: Igor Mammedov <imammedo@redhat.com>

> ---

>   accel/tcg/translate-all.c | 8 --------

>   1 file changed, 8 deletions(-)

> 

> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c

> index 238b0e575bf..5b66af783b5 100644

> --- a/accel/tcg/translate-all.c

> +++ b/accel/tcg/translate-all.c

> @@ -938,15 +938,7 @@ static inline size_t size_code_gen_buffer(size_t tb_size)

>   {

>       /* Size the buffer.  */

>       if (tb_size == 0) {

> -#ifdef USE_STATIC_CODE_GEN_BUFFER

>           tb_size = DEFAULT_CODE_GEN_BUFFER_SIZE;

> -#else

> -        /* ??? Needs adjustments.  */

> -        /* ??? If we relax the requirement that CONFIG_USER_ONLY use the

> -           static buffer, we could size this on RESERVED_VA, on the text

> -           segment size of the executable, or continue to use the default.  */

> -        tb_size = (unsigned long)(ram_size / 4);

> -#endif

>       }

>       if (tb_size < MIN_CODE_GEN_BUFFER_SIZE) {

>           tb_size = MIN_CODE_GEN_BUFFER_SIZE;

> 


Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
diff mbox series

Patch

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 238b0e575bf..5b66af783b5 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -938,15 +938,7 @@  static inline size_t size_code_gen_buffer(size_t tb_size)
 {
     /* Size the buffer.  */
     if (tb_size == 0) {
-#ifdef USE_STATIC_CODE_GEN_BUFFER
         tb_size = DEFAULT_CODE_GEN_BUFFER_SIZE;
-#else
-        /* ??? Needs adjustments.  */
-        /* ??? If we relax the requirement that CONFIG_USER_ONLY use the
-           static buffer, we could size this on RESERVED_VA, on the text
-           segment size of the executable, or continue to use the default.  */
-        tb_size = (unsigned long)(ram_size / 4);
-#endif
     }
     if (tb_size < MIN_CODE_GEN_BUFFER_SIZE) {
         tb_size = MIN_CODE_GEN_BUFFER_SIZE;