On 8/9/18 6:31 PM, Alberto Garcia wrote:
On Thu 09 Aug 2018 04:04:23 PM CEST, Leonid Bloch wrote:
- int min_refcount_cache = MIN_REFCOUNT_CACHE_SIZE * s->cluster_size;
+ uint64_t min_refcount_cache = MIN_REFCOUNT_CACHE_SIZE * s->cluster_size;
Why do you need to change this data type here? min_refcount_cache is
guaranteed to fit in an int.
No necessity here, just it participates in arithmetics with other
uint64_t's afterwards, so it might as well be uint64_t from the
get-go.
The compiler already does that when needed, so it's not so important
(and it adds noise to the patch).
I didn't say it's important or increases the performance in any way. It
just looks nicer, more logical, and too small of a change to require a
separate patch, even a trivial one. Since it's just next to the lines
I'm modifying anyway, I made this change because it looks nicer and more
consistent.
- *refcount_cache_size =
- MIN(combined_cache_size, min_refcount_cache);
+ *refcount_cache_size = MIN(combined_cache_size,
+ min_refcount_cache);
There are no functional changes, why do you need to change the
indentation here?
It's in the "immediate area (few lines) of the lines [I'm] changing".
But there's no need to change those lines unless there's an obvious
mistake. In this case it's just a matter of style so they just add noise
to the patch.
Again, it just looks nicer, more readable, compliant to the generally
accepted style, and right next to the functional changes. It's a style
improvement which is in the immediate vicinity of the functional
improvements. I made another one, you must have seen it already, in v5.
Look, it just looks better. It's possible to make another patch for
these cosmetic changes, but is it worth it when they are right next to
the functional changes? It's a bit of noise in the patch, versus noise
in the Git history.
+QEMU will use a default L2 cache sufficient to cover the entire virtual
+size of an image, which with the default cluster size will result in 1 MB
+of cache for every 8 GB of virtual image size:
This is not true. QEMU will use a default size of 32MB, which may or
may not cover the entire image.
But no, QEMU will not use a default size of 32MB. It will use a
default size which is just enough to cover the image, unless the
needed size is larger than 32 MB.
Now: QEMU will use a default L2 cache of up to 32MB, which may or may
not be enough to cover the entire image.
Previously: QEMU will use a default L2 cache of 1MB, which may or may
not be enough to cover the entire image.
Now: QEMU will use just enough to fit the image, unless it's more than
32 MB.
Then: QEMU will use MAX(1 MB, 8 * cluster_size).
Anyway, this place should not mention the default cache sizes, and I
have reworded it in v5 (maybe let's fix this in patch 1/5?). This
section is all about explaining the numbers needed for the cache. The
defaults are mentioned below.
I would prefer if you would say "we increased the default cache size
so now we cover larger images" instead of "the default cache size
will now cover the entire image", because the latter is not true.
But it's not correct: we did not increase the default size, we made
the default size fit the image size, and set a maximum. It's not the
same, do you agree?
I don't think we made the default size fit the image size, because if
you override the default size QEMU will still adjust it if it's too
large.
Now there is no such thing as the default size, there is the "default
maximum size". It fits the image by default now, unless it needs to be
larger than the max.
What we did is guarantee that QEMU will use *up to* l2-cache-size
bytes, regardless of whether l2-cache-size is set by the user or is the
default value. Plus, we increased that default value to 32MB.
Yes! :)
But the meaning of "default" now and before is different. Before it was
the "default size", and now - "default maximum size".
From the end user's point of view, who had a VM with images of 8GB,
200GB and 2TB, the most visible result is that the L2 cache is now
larger, enough for the first two images but still not enough for the
third. That's the big change, both in terms of performance and memory
usage, and it's easy to measure.
The other change (the cache size fits the image size) is not immediately
visible, and certainly not with a 32MB cache.
Isn't it the same change? :)
Let's make an experiment:
- Take QEMU stable or master (without these patches)
- Create a 16GB qcow2 image and fill it completely with data
- Run QEMU and attach that image with l2-cache-size=2M (2 MB L2 cache)
- Read the complete image to make sure that all L2 tables are cached
- Measure the amount of memory that QEMU is using, e.g. with smem
(you can do that before and after caching the L2 tables)
Repeat the same experiment with l2-cache-size=2G (2 GB L2 cache). Do you
see any difference?
Actually, I did this experiment before, after Kevin's suggestions. I
know what you want to say: it's not actually used, but allocated in the
virtual memory. But still, with this patch it will be just enough
allocation.
Look, we agree on the functional changes, right? It's just the cosmetic
changes and the documentation that remain unsettled. :)
Leonid.
Berto
