qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 09/15] memory: prepare flatview and radix-tree f


From: liu ping fan
Subject: Re: [Qemu-devel] [PATCH 09/15] memory: prepare flatview and radix-tree for rcu style access
Date: Sat, 11 Aug 2012 09:58:44 +0800

On Wed, Aug 8, 2012 at 5:41 PM, Avi Kivity <address@hidden> wrote:
> On 08/08/2012 09:25 AM, Liu Ping Fan wrote:
>> From: Liu Ping Fan <address@hidden>
>>
>> Flatview and radix view are all under the protection of pointer.
>> And this make sure the change of them seem to be atomic!
>>
>> The mr accessed by radix-tree leaf or flatview will be reclaimed
>> after the prev PhysMap not in use any longer
>>
>
> IMO this cleverness should come much later.  Let's first take care of
> dropping the big qemu lock, then make swithcing memory maps more efficient.
>
> The initial paths could look like:
>
>   lookup:
>      take mem_map_lock
>      lookup
>      take ref
>      drop mem_map_lock
>
>   update:
>      take mem_map_lock (in core_begin)
>      do updates
>      drop memo_map_lock
>
> Later we can replace mem_map_lock with either a rwlock or (real) rcu.
>
>
>>
>>  #if !defined(CONFIG_USER_ONLY)
>>
>> -static void phys_map_node_reserve(unsigned nodes)
>> +static void phys_map_node_reserve(PhysMap *map, unsigned nodes)
>>  {
>> -    if (phys_map_nodes_nb + nodes > phys_map_nodes_nb_alloc) {
>> +    if (map->phys_map_nodes_nb + nodes > map->phys_map_nodes_nb_alloc) {
>>          typedef PhysPageEntry Node[L2_SIZE];
>> -        phys_map_nodes_nb_alloc = MAX(phys_map_nodes_nb_alloc * 2, 16);
>> -        phys_map_nodes_nb_alloc = MAX(phys_map_nodes_nb_alloc,
>> -                                      phys_map_nodes_nb + nodes);
>> -        phys_map_nodes = g_renew(Node, phys_map_nodes,
>> -                                 phys_map_nodes_nb_alloc);
>> +        map->phys_map_nodes_nb_alloc = MAX(map->phys_map_nodes_nb_alloc * 2,
>> +                                                                        16);
>> +        map->phys_map_nodes_nb_alloc = MAX(map->phys_map_nodes_nb_alloc,
>> +                                      map->phys_map_nodes_nb + nodes);
>> +        map->phys_map_nodes = g_renew(Node, map->phys_map_nodes,
>> +                                 map->phys_map_nodes_nb_alloc);
>>      }
>>  }
>
> Please have a patch that just adds the map parameter to all these
> functions.  This makes the later patch, that adds the copy, easier to read.
>
>> +
>> +void cur_map_update(PhysMap *next)
>> +{
>> +    qemu_mutex_lock(&cur_map_lock);
>> +    physmap_put(cur_map);
>> +    cur_map = next;
>> +    smp_mb();
>> +    qemu_mutex_unlock(&cur_map_lock);
>> +}
>
> IMO this can be mem_map_lock.
>
> If we take my previous suggestion:
>
>   lookup:
>      take mem_map_lock
>      lookup
>      take ref
>      drop mem_map_lock
>
>   update:
>      take mem_map_lock (in core_begin)
>      do updates
>      drop memo_map_lock
>
> And update it to
>
>
>   update:
>      prepare next_map (in core_begin)
>      do updates
>      take mem_map_lock (in core_commit)
>      switch maps
>      drop mem_map_lock
>      free old map
>
>
> Note the lookup path copies the MemoryRegionSection instead of
> referencing it.  Thus we can destroy the old map without worrying; the
> only pointers will point to MemoryRegions, which will be protected by
> the refcounts on their Objects.
>
Just find there may be a leak here. If mrs points to subpage, then the
subpage_t  could be crashed by destroy.
To avoid such situation, we can walk down the chain to pin us on the
Object based mr, but then we must expose the address convert in
subpage_read() right here. Right?

Regards,
pingfan

> This can be easily switched to rcu:
>
>   update:
>      prepare next_map (in core_begin)
>      do updates
>      switch maps - rcu_assign_pointer
>      call_rcu(free old map) (or synchronize_rcu; free old maps)
>
> Again, this should be done after the simplictic patch that enables
> parallel lookup but keeps just one map.
>
>
>
> --
> error compiling committee.c: too many arguments to function



reply via email to

[Prev in Thread] Current Thread [Next in Thread]