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.
No strong feelings, though.
--
Peter Xu
