Re: [Qemu-devel] [PATCH v4 2/5] qcow2: Assign the L2 cache relatively to image size

[Leonid Bloch <lbloch@janustech.com>]

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
>