[Qemu-devel] Re: [PATCH 2/6] qemu-kvm: Modify and introduce wrapper functions to access phys_ram_dirty.

[Anthony Liguori]

On 03/16/2010 07:45 AM, Avi Kivity wrote:
> On 03/16/2010 12:53 PM, Yoshiaki Tamura wrote:
> > Modifies wrapper functions for byte-based phys_ram_dirty bitmap to
> > bit-based phys_ram_dirty bitmap, and adds more wrapper functions to 
> > prevent
> > direct access to the phys_ram_dirty bitmap.
>
> > +
> > +static inline int cpu_physical_memory_get_dirty_flags(ram_addr_t addr)
> > +{
> > +    unsigned long mask;
> > +    int index = (addr>>  TARGET_PAGE_BITS) / HOST_LONG_BITS;
> > +    int offset = (addr>>  TARGET_PAGE_BITS)&  (HOST_LONG_BITS - 1);
> > +    int ret = 0;
> > +
> > +    mask = 1UL<<  offset;
> > +    if (phys_ram_vga_dirty[index]&  mask)
> > +        ret |= VGA_DIRTY_FLAG;
> > +    if (phys_ram_code_dirty[index]&  mask)
> > +        ret |=  CODE_DIRTY_FLAG;
> > +    if (phys_ram_migration_dirty[index]&  mask)
> > +        ret |= MIGRATION_DIRTY_FLAG;
> > +
> > +    return ret;
> >   }
> >
> >   static inline int cpu_physical_memory_get_dirty(ram_addr_t addr,
> >                                                   int dirty_flags)
> >   {
> > -    return phys_ram_dirty[addr>>  TARGET_PAGE_BITS]&  dirty_flags;
> > +    return cpu_physical_memory_get_dirty_flags(addr)&  dirty_flags;
> >   }
>
> This turns one cacheline access into three.  If the dirty bitmaps were 
> in an array, you could do
>
>   return dirty_bitmaps[dirty_index][addr >> (TARGET_PAGE_BITS + 
> BITS_IN_LONG)] & mask;
>
> with one cacheline access.

As far as I can tell, we only ever call with a single flag so your 
suggestion makes sense.

I'd suggest introducing these functions before splitting the bitmap up.  
It makes review a bit easier.

> >   static inline void cpu_physical_memory_set_dirty(ram_addr_t addr)
> >   {
> > -    phys_ram_dirty[addr>>  TARGET_PAGE_BITS] = 0xff;
> > +    unsigned long mask;
> > +    int index = (addr>>  TARGET_PAGE_BITS) / HOST_LONG_BITS;
> > +    int offset = (addr>>  TARGET_PAGE_BITS)&  (HOST_LONG_BITS - 1);
> > +
> > +    mask = 1UL<<  offset;
> > +    phys_ram_vga_dirty[index] |= mask;
> > +    phys_ram_code_dirty[index] |= mask;
> > +    phys_ram_migration_dirty[index] |= mask;
> > +}
>
> This is also three cacheline accesses.  I think we should have a 
> master bitmap which is updated by set_dirty(), and which is or'ed into 
> the other bitmaps when they are accessed.  At least the vga and 
> migration bitmaps are only read periodically, not randomly, so this 
> would be very fast.  In a way, this is similar to how the qemu bitmap 
> is updated from the kvm bitmap today.
>
> I am not sure about the code bitmap though.

I think your suggestion makes sense and would also work for the code bitmap.

Regards,

Anthony Liguori