diff mbox series

bsd-user/main: Allow setting tb-size

Message ID 20240731144532.5997-1-iii@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series bsd-user/main: Allow setting tb-size | expand

Commit Message

Ilya Leoshkevich July 31, 2024, 2:45 p.m. UTC
While qemu-system can set tb-size using -accel tcg,tb-size=n, there
is no similar knob for qemu-bsd-user. Add one in a way similar to how
one-insn-per-tb is already handled.

Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 bsd-user/main.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Philippe Mathieu-Daudé July 31, 2024, 8:53 p.m. UTC | #1
On 31/7/24 16:45, Ilya Leoshkevich wrote:
> While qemu-system can set tb-size using -accel tcg,tb-size=n, there
> is no similar knob for qemu-bsd-user. Add one in a way similar to how
> one-insn-per-tb is already handled.
> 
> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   bsd-user/main.c | 9 +++++++++
>   1 file changed, 9 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Warner Losh July 31, 2024, 9:21 p.m. UTC | #2
On Wed, Jul 31, 2024 at 8:45 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:

> While qemu-system can set tb-size using -accel tcg,tb-size=n, there
> is no similar knob for qemu-bsd-user. Add one in a way similar to how
> one-insn-per-tb is already handled.
>

Cool! Are you using bsd-user and need this for some reason? Or is this
purely theoretical? Is there a larger context I can read about somewhere?

I'll merge it either way (so none of the above is a criticism, I'm genuinely
curious) , but I don't get too many bsd-user fixes and this one is unusual.


> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  bsd-user/main.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/bsd-user/main.c b/bsd-user/main.c
> index cc980e6f401..7c230b0c7a5 100644
> --- a/bsd-user/main.c
> +++ b/bsd-user/main.c
> @@ -60,6 +60,7 @@ uintptr_t qemu_host_page_size;
>  intptr_t qemu_host_page_mask;
>
>  static bool opt_one_insn_per_tb;
> +static unsigned long opt_tb_size;
>  uintptr_t guest_base;
>  bool have_guest_base;
>  /*
> @@ -169,6 +170,7 @@ static void usage(void)
>             "                  (use '-d help' for a list of log items)\n"
>             "-D logfile        write logs to 'logfile' (default stderr)\n"
>             "-one-insn-per-tb  run with one guest instruction per emulated
> TB\n"
> +           "-tb-size size     TCG translation block cache size\n"
>             "-strace           log system calls\n"
>             "-trace
> [[enable=]<pattern>][,events=<file>][,file=<file>]\n"
>             "                  specify tracing options\n"
> @@ -387,6 +389,11 @@ int main(int argc, char **argv)
>              seed_optarg = optarg;
>          } else if (!strcmp(r, "one-insn-per-tb")) {
>              opt_one_insn_per_tb = true;
> +        } else if (!strcmp(r, "tb-size")) {
> +            r = argv[optind++];
> +            if (qemu_strtoul(r, NULL, 0, &opt_tb_size)) {
> +                usage();
> +            }
>          } else if (!strcmp(r, "strace")) {
>              do_strace = 1;
>          } else if (!strcmp(r, "trace")) {
> @@ -452,6 +459,8 @@ int main(int argc, char **argv)
>          accel_init_interfaces(ac);
>          object_property_set_bool(OBJECT(accel), "one-insn-per-tb",
>                                   opt_one_insn_per_tb, &error_abort);
> +        object_property_set_int(OBJECT(accel), "tb-size",
> +                                opt_tb_size, &error_abort);
>          ac->init_machine(NULL);
>      }
>

Reviewed-by: Warner Losh <imp@bsdimp.com>

I'll queue this to my bsd-user-2024-q3-2 branch. I hope to land it, just
after 9.1.0 release.

Warner
Philippe Mathieu-Daudé July 31, 2024, 9:42 p.m. UTC | #3
On 31/7/24 23:21, Warner Losh wrote:
> On Wed, Jul 31, 2024 at 8:45 AM Ilya Leoshkevich <iii@linux.ibm.com 
> <mailto:iii@linux.ibm.com>> wrote:
> 
>     While qemu-system can set tb-size using -accel tcg,tb-size=n, there
>     is no similar knob for qemu-bsd-user. Add one in a way similar to how
>     one-insn-per-tb is already handled.
> 
> 
> Cool! Are you using bsd-user and need this for some reason? Or is this
> purely theoretical? Is there a larger context I can read about somewhere?

Trying to keep user interface parity between linux/bsd.

Ideally this duplication should be unified in common-user/.

> I'll merge it either way (so none of the above is a criticism, I'm genuinely
> curious) , but I don't get too many bsd-user fixes and this one is unusual.
> 
>     Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org
>     <mailto:philmd@linaro.org>>
>     Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com
>     <mailto:iii@linux.ibm.com>>
>     ---
>       bsd-user/main.c | 9 +++++++++
>       1 file changed, 9 insertions(+)
Warner Losh July 31, 2024, 11:18 p.m. UTC | #4
On Wed, Jul 31, 2024 at 3:42 PM Philippe Mathieu-Daudé <philmd@linaro.org>
wrote:

> On 31/7/24 23:21, Warner Losh wrote:
> > On Wed, Jul 31, 2024 at 8:45 AM Ilya Leoshkevich <iii@linux.ibm.com
> > <mailto:iii@linux.ibm.com>> wrote:
> >
> >     While qemu-system can set tb-size using -accel tcg,tb-size=n, there
> >     is no similar knob for qemu-bsd-user. Add one in a way similar to how
> >     one-insn-per-tb is already handled.
> >
> >
> > Cool! Are you using bsd-user and need this for some reason? Or is this
> > purely theoretical? Is there a larger context I can read about somewhere?
>
> Trying to keep user interface parity between linux/bsd.
>
> Ideally this duplication should be unified in common-user/.
>

I'd love that.

Anyway, both of these patches queued to my branch


> > I'll merge it either way (so none of the above is a criticism, I'm
> genuinely
> > curious) , but I don't get too many bsd-user fixes and this one is
> unusual.
> >
> >     Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org
> >     <mailto:philmd@linaro.org>>
> >     Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com
> >     <mailto:iii@linux.ibm.com>>
> >     ---
> >       bsd-user/main.c | 9 +++++++++
> >       1 file changed, 9 insertions(+)
>
>
Ilya Leoshkevich Aug. 1, 2024, 8:23 a.m. UTC | #5
On Wed, 2024-07-31 at 15:21 -0600, Warner Losh wrote:
> On Wed, Jul 31, 2024 at 8: 45 AM Ilya Leoshkevich
> <iii@ linux. ibm. com> wrote: While qemu-system can set tb-size using
> -accel tcg,tb-size=n, there is no similar knob for qemu-bsd-user. Add
> one in a way similar to how one-insn-per-tb is already
> 
> 
> 
> On Wed, Jul 31, 2024 at 8:45 AM Ilya Leoshkevich <iii@linux.ibm.com>
> wrote:
> > While qemu-system can set tb-size using -accel tcg,tb-size=n, there
> > is no similar knob for qemu-bsd-user. Add one in a way similar to
> > how
> > one-insn-per-tb is already handled.
> > 
> 
> 
> Cool! Are you using bsd-user and need this for some reason? Or is
> this
> purely theoretical? Is there a larger context I can read about
> somewhere?

I needed this on Linux in order to debug an issue where I suspected
full TB invalidation may be an issue.
It turned out to be something completely different, but I found it
useful: setting it to, e.g., 4096 makes full TB invalidation very rare,
so if a problem is still reproducible, then the root causes is
something else.
Philippe suggested to implement this for BSD as well in order to keep
the interfaces in sync.

[...]
Warner Losh Aug. 1, 2024, 2:43 p.m. UTC | #6
On Thu, Aug 1, 2024 at 2:25 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:

> On Wed, 2024-07-31 at 15:21 -0600, Warner Losh wrote:
>
> On Wed, Jul 31, 2024 at 8: 45 AM Ilya Leoshkevich <iii@ linux. ibm. com>
> wrote: While qemu-system can set tb-size using -accel tcg,tb-size=n, there
> is no similar knob for qemu-bsd-user. Add one in a way similar to how
> one-insn-per-tb is already
> On Wed, Jul 31, 2024 at 8:45 AM Ilya Leoshkevich <iii@linux.ibm.com>
> wrote:
>
> While qemu-system can set tb-size using -accel tcg,tb-size=n, there
> is no similar knob for qemu-bsd-user. Add one in a way similar to how
> one-insn-per-tb is already handled.
>
>
> Cool! Are you using bsd-user and need this for some reason? Or is this
> purely theoretical? Is there a larger context I can read about somewhere?
>
>
> I needed this on Linux in order to debug an issue where I suspected full
> TB invalidation may be an issue.
> It turned out to be something completely different, but I found it useful:
> setting it to, e.g., 4096 makes full TB invalidation very rare, so if a
> problem is still reproducible, then the root causes is something else.
> Philippe suggested to implement this for BSD as well in order to keep the
> interfaces in sync.
>

Excellent! Thank you for taking the time to do this! And the other bug fix.
Both have been queued to my first post 9.1 pull branch.

Warner
diff mbox series

Patch

diff --git a/bsd-user/main.c b/bsd-user/main.c
index cc980e6f401..7c230b0c7a5 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -60,6 +60,7 @@  uintptr_t qemu_host_page_size;
 intptr_t qemu_host_page_mask;
 
 static bool opt_one_insn_per_tb;
+static unsigned long opt_tb_size;
 uintptr_t guest_base;
 bool have_guest_base;
 /*
@@ -169,6 +170,7 @@  static void usage(void)
            "                  (use '-d help' for a list of log items)\n"
            "-D logfile        write logs to 'logfile' (default stderr)\n"
            "-one-insn-per-tb  run with one guest instruction per emulated TB\n"
+           "-tb-size size     TCG translation block cache size\n"
            "-strace           log system calls\n"
            "-trace            [[enable=]<pattern>][,events=<file>][,file=<file>]\n"
            "                  specify tracing options\n"
@@ -387,6 +389,11 @@  int main(int argc, char **argv)
             seed_optarg = optarg;
         } else if (!strcmp(r, "one-insn-per-tb")) {
             opt_one_insn_per_tb = true;
+        } else if (!strcmp(r, "tb-size")) {
+            r = argv[optind++];
+            if (qemu_strtoul(r, NULL, 0, &opt_tb_size)) {
+                usage();
+            }
         } else if (!strcmp(r, "strace")) {
             do_strace = 1;
         } else if (!strcmp(r, "trace")) {
@@ -452,6 +459,8 @@  int main(int argc, char **argv)
         accel_init_interfaces(ac);
         object_property_set_bool(OBJECT(accel), "one-insn-per-tb",
                                  opt_one_insn_per_tb, &error_abort);
+        object_property_set_int(OBJECT(accel), "tb-size",
+                                opt_tb_size, &error_abort);
         ac->init_machine(NULL);
     }