On Wed 08 Aug 2018 04:35:19 PM CEST, Leonid Bloch wrote:
The way I see it: there are two simple changes from the user's point of
view (they can even be two separate patches).
1) The default l2-cache-size is now 32MB. DEFAULT_L2_CACHE_CLUSTERS is
useless now and disappears.
I don't think that it can be a separate patch, because unless the other
logic is changed, the cache will occupy 32 MB *always*, regardless of
the image size, and that's quite a big and unneeded overhead.
Change the order of both patches then :-)
Do you really think it's necessary? The increase of the default max
size is directly tied to the functionality change: it will be harmful
to increase the maximum before the new functionality is implemented,
and there is no need to change the functionality if the default max is
not increased.
I think that we're looking at this from two different perspectives.
a) If I understand you correctly, you see this as a way to make the user
forget about the L2 cache: we guarantee that it's going to be big
enough for the entire image, so simply forget about it. Exception: if
you're using very large images you will have to set its size
manually, but for the vast majority of cases you'll be alright with
the default (32MB).
b) The way I see it: setting the right L2 cache size is not trivial, it
depends on the image and cluster sizes, and it involves a trade-off
between how much memory you want to use and how much performance
you're willing to sacrifice. QEMU has many use cases and there's no
good default, you need to make the numbers yourself if you want to
fine-tune it. Don't blindly trust the new default size (32MB) because
it won't be enough for many cases. But we can promise you this: make
l2-cache-size the maximum amount of memory you're willing to spend on
this disk image's cache, and we guarantee that we'll only use the
amount that we need to give you the best performance.
I hope (a) was a fair description of what you're trying to achieve with
these patches. But I also hope that you can see why making l2_cache_size
= MIN(l2_cache_size, virtual_disk_size / (s->cluster_size / 8)) is a
worthwhile change on its own, even if we didn't increase the default
cache size to 32MB.
Berto