Christian Ehrhardt <christian.ehrha...@canonical.com> writes:
> On Thu, Jul 16, 2020 at 6:27 PM Alex Bennée <alex.ben...@linaro.org> wrote: > >> >> Christian Ehrhardt <christian.ehrha...@canonical.com> writes: >> >> > On Wed, Jul 15, 2020 at 5:58 PM BALATON Zoltan <bala...@eik.bme.hu> >> wrote: >> > >> >> See commit 47a2def4533a2807e48954abd50b32ecb1aaf29a and the next two >> >> following it. >> >> >> > >> > Thank you Zoltan for pointing out this commit, I agree that this seems >> to be >> > the trigger for the issues I'm seeing. Unfortunately the common CI host >> size >> > is 1-2G. For example on Ubuntu Autopkgtests 1.5G. >> > Those of them running guests do so in 0.5-1G size in TCG mode >> > (as they often can't rely on having KVM available). >> > >> > The 1G TB buffer + 0.5G actual guest size + lack of dynamic downsizing >> > on memory pressure (never existed) makes these systems go OOM-Killing >> > the qemu process. >> >> Ooops. I admit the assumption was that most people running system >> emulation would be doing it on beefier machines. >> >> > The patches indicated that the TB flushes on a full guest boot are a >> > good indicator of the TB size efficiency. From my old checks I had: >> > >> > - Qemu 4.2 512M guest with 32M default overwritten by ram-size/4 >> > TB flush count 14, 14, 16 >> > - Qemu 5.0 512M guest with 1G default >> > TB flush count 1, 1, 1 >> > >> > I agree that ram/4 seems odd, especially on huge guests that is a lot >> > potentially wasted. And most environments have a bit of breathing >> > room 1G is too big in small host systems and the common CI system falls >> > into this category. So I tuned it down to 256M for a test. >> > >> > - Qemu 4.2 512M guest with tb-size 256M >> > TB flush count 5, 5, 5 >> > - Qemu 5.0 512M guest with tb-size 256M >> > TB flush count 5, 5, 5 >> > - Qemu 5.0 512M guest with 256M default in code >> > TB flush count 5, 5, 5 >> > >> > So performance wise the results are as much in-between as you'd think >> from a >> > TB size in between. And the memory consumption which (for me) is the >> actual >> > current issue to fix would be back in line again as expected. >> >> So I'm glad you have the workaround. >> >> > >> > So on one hand I'm suggesting something like: >> > --- a/accel/tcg/translate-all.c >> > +++ b/accel/tcg/translate-all.c >> > @@ -944,7 +944,7 @@ static void page_lock_pair(PageDesc **re >> > * Users running large scale system emulation may want to tweak their >> > * runtime setup via the tb-size control on the command line. >> > */ >> > -#define DEFAULT_CODE_GEN_BUFFER_SIZE_1 (1 * GiB) >> > +#define DEFAULT_CODE_GEN_BUFFER_SIZE_1 (256 * MiB) >> >> The problem we have is any number we pick here is arbitrary. And while >> it did regress your use-case changing it again just pushes a performance >> regression onto someone else. > > > Thanks for your feedback Alex! > > That is true "for you" since 5.0 is released from upstreams POV. > But from the downstreams POV no 5.0 exists for Ubuntu yet and I'd break > many places releasing it like that. > Sadly the performance gain to the other cases will most likely go unnoticed. > > >> The most (*) 64 bit desktop PCs have 16Gb >> of RAM, almost all have more than 8gb. And there is a workaround. >> > > Due to our work around virtualization the values representing > "most 64 bit desktop PCs" aren't the only thing that matters :-) > > ... > > >> > This is a bit more tricky than it seems as ram_sizes is no more >> > present in that context but it is enough to discuss it. >> > That should serve all cases - small and large - better as a pure >> > static default of 1G or always ram/4? >> >> I'm definitely against re-introducing ram_size into the mix. The >> original commit (a1b18df9a4) that broke this introduced an ordering >> dependency which we don't want to bring back. >> > > I agree with that reasoning, but currently without any size dependency > the "arbitrary value" we picked to be 1G is even more fixed than it was > before. > Compared to pre v5.0 for now I can only decide to > a) tune it down -> performance impact for huge guests > b) keep it at 1G -> functional breakage with small hosts > > I'd be more amenable to something that took into account host memory and >> limited the default if it was smaller than a threshold. Is there a way >> to probe that that doesn't involve slurping /proc/meminfo? >> > > I agree that a host-size dependency might be the better way to go, > yet I have no great cross-platform resilient way to get that. > Maybe we can make it like "if I can get some value consider it, > otherwise use the current default". > That would improve many places already, while keeping the rest at the > current behavior. > > >> > >> > P.S. I added Alex being the Author of the offending patch and >> Richard/Paolo >> > for being listed in the Maintainers file for TCG. >> >> >> > From Zoltan (unifying the thread a bit): > >> I agree that this should be dependent on host memory size not guest >> ram_size but it might be tricky to get that value because different host >> OSes would need different ways. > > Well - where it isn't available we will continue to take the default > qemu 5.0 already had. If required on other platforms as well they can add > their way of host memory detection into this as needed. > >> Maybe a new qemu_host_mem_size portability >> function will be needed that implements this for different host OSes. >> POSIX may or may not have sysconf _SC_PHYS_PAGES and _SC_AVPHYS_PAGES > > We should not try to get into the business of _SC_AVPHYS_PAGES and > try to understand/assume what might be cache or otherwise (re)usable. > Since we only look for some alignment to hosts size _SC_PHYS_PAGES should > be good enough and available in more places than the other options. > >> and linux has sysinfo but don't know how reliable these are. > > sysconf is slightly more widely available than sysinfo and has enough for > what we need. > > > I have combined the thoughts above into a patch and it works well in > my tests. > > 32G Host: > pages 8187304.000000 > pagesize 4096.000000 > max_default 4191899648 > final tb_size 1073741824 > > (qemu) info jit > Translation buffer state: > gen code size 210425059/1073736659 > TB count 368273 > TB avg target size 20 max=1992 bytes > TB avg host size 330 bytes (expansion ratio: 16.1) > cross page TB count 1656 (0%) > direct jump count 249813 (67%) (2 jumps=182112 49%) > TB hash buckets 197613/262144 (75.38% head buckets used) > TB hash occupancy 34.15% avg chain occ. Histogram: [0,10)%|▆ █ > ▅▁▃▁▁|[90,100]% > TB hash avg chain 1.020 buckets. Histogram: 1|█▁▁|3 > > Statistics: > TB flush count 1 > TB invalidate count 451673 > TLB full flushes 0 > TLB partial flushes 154819 > TLB elided flushes 191627 > [TCG profiler not compiled] > > => 1G TB size not changed compared to v5.0 - as intended > > > But on a small 1.5G Host it now works without OOM: > > pages 379667.000000 > pagesize 4096.000000 > max_default 194389504 > final tb_size 194389504 > > (qemu) info jit > Translation buffer state: > gen code size 86179731/194382803 > TB count 149995 > TB avg target size 20 max=1992 bytes > TB avg host size 333 bytes (expansion ratio: 16.5) > cross page TB count 716 (0%) > direct jump count 98854 (65%) (2 jumps=74962 49%) > TB hash buckets 58842/65536 (89.79% head buckets used) > TB hash occupancy 51.46% avg chain occ. Histogram: [0,10)%|▃ ▇ > █▂▆▁▄|[90,100]% > TB hash avg chain 1.091 buckets. Histogram: 1|█▁▁|3 > > Statistics: > TB flush count 10 > TB invalidate count 31733 > TLB full flushes 0 > TLB partial flushes 180891 > TLB elided flushes 244107 > [TCG profiler not compiled] > > => ~185M which is way more reasonable given the host size > > The patch will have a rather large comment in it, I'm not sure if the full > comment is needed, but I wanted to leave a trace what&why is going > on in the code for the next one who comes by. > > Submitting as a proper patch to the list in a bit ... Ahh did you not get CC'ed by my patch this morning? -- Alex Bennée
