| Message ID | 20240731144532.5997-1-iii@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | bsd-user/main: Allow setting tb-size | expand |
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>
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
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(+)
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(+) > >
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. [...]
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 --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); }
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(+)