On 27.08.24 20:41, Peter Xu wrote:
On Tue, Aug 27, 2024 at 08:00:07PM +0200, David Hildenbrand wrote:
On 27.08.24 19:57, Peter Xu wrote:
On Tue, Aug 27, 2024 at 10:37:15AM +0200, David Hildenbrand wrote:
/* Called with ram_list.mutex held */
-static void dirty_memory_extend(ram_addr_t old_ram_size,
- ram_addr_t new_ram_size)
+static void dirty_memory_extend(ram_addr_t new_ram_size)
{
- ram_addr_t old_num_blocks = DIV_ROUND_UP(old_ram_size,
- DIRTY_MEMORY_BLOCK_SIZE);
ram_addr_t new_num_blocks = DIV_ROUND_UP(new_ram_size,
DIRTY_MEMORY_BLOCK_SIZE);
int i;
- /* Only need to extend if block count increased */
- if (new_num_blocks <= old_num_blocks) {
- return;
- }
One nitpick here: IMHO we could move the n_blocks cache in ram_list
instead, then we keep the check here and avoid caching it three times with
the same value.
yes, as written in the patch description: "We'll store the number of blocks
along with the actual pointer to keep it simple."
It's cleaner to me to store it along the RCU-freed data structure that has
this size.
Yep, I can get that.
I think one reason I had my current preference is to avoid things like:
for (...) {
if (...)
return;
}
I'd at least want to sanity check before "return" to make sure all three
bitmap chunks are having the same size. It gave me the feeling that we
could process "blocks[]" differently but we actually couldn't - In our case
it has the ram_list mutex when update, so it must be guaranteed. However
due to the same reason, I see it cleaner to just keep the counter there
too.
I'll move it to the higher level because I have more important stuff to
work on and want to get this off my plate.
"num_blocks" does not quite make sense in RAMList (where we have a
different "blocks" variable) so I'll call it "num_dirty_blocks" or sth
like that.
--
Cheers,
David / dhildenb
