diff mbox

[13/15] exec: use mmap for PhysPageMap->nodes

Message ID 57836783.4070100@kamp.de
State New
Headers show

Commit Message

Peter Lieven July 11, 2016, 9:31 a.m. UTC 
Am 28.06.2016 um 12:43 schrieb Paolo Bonzini:
>
> On 28/06/2016 11:01, Peter Lieven wrote:
>> this was causing serious framentation in conjunction with the
>> subpages since RCU was introduced. The node space was allocated
>> at approx 32kB then reallocted to approx 75kB and this a few hundred
>> times at startup. And thanks to RCU the freeing was delayed.
>>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
> The size of the node from the previous as->dispatch could be used as a
> hint for the new one perhaps, avoiding the reallocation?

This here seems also to work:


Question is still, mmap for this?

Peter

Comments

Peter Lieven July 11, 2016, 9:44 a.m. UTC | #1 
Am 11.07.2016 um 11:31 schrieb Peter Lieven:
> Am 28.06.2016 um 12:43 schrieb Paolo Bonzini:
>>
>> On 28/06/2016 11:01, Peter Lieven wrote:
>>> this was causing serious framentation in conjunction with the
>>> subpages since RCU was introduced. The node space was allocated
>>> at approx 32kB then reallocted to approx 75kB and this a few hundred
>>> times at startup. And thanks to RCU the freeing was delayed.
>>>
>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> The size of the node from the previous as->dispatch could be used as a
>> hint for the new one perhaps, avoiding the reallocation?
>
> This here seems also to work:
>
> diff --git a/exec.c b/exec.c
> index 0122ef7..2691c0a 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -187,10 +187,12 @@ struct CPUAddressSpace {
>
>  static void phys_map_node_reserve(PhysPageMap *map, unsigned nodes)
>  {
> +    static unsigned alloc_hint = 16;
>      if (map->nodes_nb + nodes > map->nodes_nb_alloc) {
> -        map->nodes_nb_alloc = MAX(map->nodes_nb_alloc * 2, 16);
> +        map->nodes_nb_alloc = MAX(map->nodes_nb_alloc, alloc_hint);
>          map->nodes_nb_alloc = MAX(map->nodes_nb_alloc, map->nodes_nb + nodes);
>          map->nodes = g_renew(Node, map->nodes, map->nodes_nb_alloc);
> +        alloc_hint = map->nodes_nb_alloc;
>      }
>  }
>
>
> Question is still, mmap for this?

Side note: I added some counters and found that on my test system at maximum 453 allocations of 28 * sizeof(Nodes) bytes where active at
the same time. During runtime its only 9. So this might explain why the alloc + realloc causes fragmentation of the brk heap.

Peter
Paolo Bonzini July 11, 2016, 10:37 a.m. UTC | #2 
On 11/07/2016 11:31, Peter Lieven wrote:
> Am 28.06.2016 um 12:43 schrieb Paolo Bonzini:
>>
>> On 28/06/2016 11:01, Peter Lieven wrote:
>>> this was causing serious framentation in conjunction with the
>>> subpages since RCU was introduced. The node space was allocated
>>> at approx 32kB then reallocted to approx 75kB and this a few hundred
>>> times at startup. And thanks to RCU the freeing was delayed.
>>>
>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> The size of the node from the previous as->dispatch could be used as a
>> hint for the new one perhaps, avoiding the reallocation?
> 
> This here seems also to work:
> 
> diff --git a/exec.c b/exec.c
> index 0122ef7..2691c0a 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -187,10 +187,12 @@ struct CPUAddressSpace {
> 
>  static void phys_map_node_reserve(PhysPageMap *map, unsigned nodes)
>  {
> +    static unsigned alloc_hint = 16;
>      if (map->nodes_nb + nodes > map->nodes_nb_alloc) {
> -        map->nodes_nb_alloc = MAX(map->nodes_nb_alloc * 2, 16);
> +        map->nodes_nb_alloc = MAX(map->nodes_nb_alloc, alloc_hint);
>          map->nodes_nb_alloc = MAX(map->nodes_nb_alloc, map->nodes_nb +
> nodes);
>          map->nodes = g_renew(Node, map->nodes, map->nodes_nb_alloc);
> +        alloc_hint = map->nodes_nb_alloc;
>      }
>  }
> 
> 
> Question is still, mmap for this?

Nice!  Can you submit a patch for this?

Paolo
Peter Lieven July 12, 2016, 2:34 p.m. UTC | #3 
Am 11.07.2016 um 12:37 schrieb Paolo Bonzini:
>
> On 11/07/2016 11:31, Peter Lieven wrote:
>> Am 28.06.2016 um 12:43 schrieb Paolo Bonzini:
>>> On 28/06/2016 11:01, Peter Lieven wrote:
>>>> this was causing serious framentation in conjunction with the
>>>> subpages since RCU was introduced. The node space was allocated
>>>> at approx 32kB then reallocted to approx 75kB and this a few hundred
>>>> times at startup. And thanks to RCU the freeing was delayed.
>>>>
>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>> The size of the node from the previous as->dispatch could be used as a
>>> hint for the new one perhaps, avoiding the reallocation?
>> This here seems also to work:
>>
>> diff --git a/exec.c b/exec.c
>> index 0122ef7..2691c0a 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -187,10 +187,12 @@ struct CPUAddressSpace {
>>
>>  static void phys_map_node_reserve(PhysPageMap *map, unsigned nodes)
>>  {
>> +    static unsigned alloc_hint = 16;
>>      if (map->nodes_nb + nodes > map->nodes_nb_alloc) {
>> -        map->nodes_nb_alloc = MAX(map->nodes_nb_alloc * 2, 16);
>> +        map->nodes_nb_alloc = MAX(map->nodes_nb_alloc, alloc_hint);
>>          map->nodes_nb_alloc = MAX(map->nodes_nb_alloc, map->nodes_nb +
>> nodes);
>>          map->nodes = g_renew(Node, map->nodes, map->nodes_nb_alloc);
>> +        alloc_hint = map->nodes_nb_alloc;
>>      }
>>  }
>>
>>
>> Question is still, mmap for this?
> Nice!  Can you submit a patch for this?

Of course, but please see my other comment. We still should consider mmap for this cause we have close to 500 Physmaps about 70KB each which
are all allocted at the same time - I think due to RCU.

Peter
Paolo Bonzini July 13, 2016, 10:27 a.m. UTC | #4 
On 12/07/2016 16:34, Peter Lieven wrote:
> Am 11.07.2016 um 12:37 schrieb Paolo Bonzini:
>>
>> On 11/07/2016 11:31, Peter Lieven wrote:
>>> Am 28.06.2016 um 12:43 schrieb Paolo Bonzini:
>>>> On 28/06/2016 11:01, Peter Lieven wrote:
>>>>> this was causing serious framentation in conjunction with the
>>>>> subpages since RCU was introduced. The node space was allocated
>>>>> at approx 32kB then reallocted to approx 75kB and this a few hundred
>>>>> times at startup. And thanks to RCU the freeing was delayed.
>>>>>
>>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>> The size of the node from the previous as->dispatch could be used as a
>>>> hint for the new one perhaps, avoiding the reallocation?
>>> This here seems also to work:
>>>
>>> diff --git a/exec.c b/exec.c
>>> index 0122ef7..2691c0a 100644
>>> --- a/exec.c
>>> +++ b/exec.c
>>> @@ -187,10 +187,12 @@ struct CPUAddressSpace {
>>>
>>>  static void phys_map_node_reserve(PhysPageMap *map, unsigned nodes)
>>>  {
>>> +    static unsigned alloc_hint = 16;
>>>      if (map->nodes_nb + nodes > map->nodes_nb_alloc) {
>>> -        map->nodes_nb_alloc = MAX(map->nodes_nb_alloc * 2, 16);
>>> +        map->nodes_nb_alloc = MAX(map->nodes_nb_alloc, alloc_hint);
>>>          map->nodes_nb_alloc = MAX(map->nodes_nb_alloc, map->nodes_nb +
>>> nodes);
>>>          map->nodes = g_renew(Node, map->nodes, map->nodes_nb_alloc);
>>> +        alloc_hint = map->nodes_nb_alloc;
>>>      }
>>>  }
>>>
>>>
>>> Question is still, mmap for this?
>> Nice!  Can you submit a patch for this?
> 
> Of course, but please see my other comment. We still should consider mmap for this cause we have close to 500 Physmaps about 70KB each which
> are all allocted at the same time - I think due to RCU.

That I think is not material for 2.7, and also I want to take a look at
creating nodes lazily.

Paolo
Peter Lieven July 14, 2016, 2:47 p.m. UTC | #5 
Am 13.07.2016 um 12:27 schrieb Paolo Bonzini:
>
> On 12/07/2016 16:34, Peter Lieven wrote:
>> Am 11.07.2016 um 12:37 schrieb Paolo Bonzini:
>>> On 11/07/2016 11:31, Peter Lieven wrote:
>>>> Am 28.06.2016 um 12:43 schrieb Paolo Bonzini:
>>>>> On 28/06/2016 11:01, Peter Lieven wrote:
>>>>>> this was causing serious framentation in conjunction with the
>>>>>> subpages since RCU was introduced. The node space was allocated
>>>>>> at approx 32kB then reallocted to approx 75kB and this a few hundred
>>>>>> times at startup. And thanks to RCU the freeing was delayed.
>>>>>>
>>>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>>> The size of the node from the previous as->dispatch could be used as a
>>>>> hint for the new one perhaps, avoiding the reallocation?
>>>> This here seems also to work:
>>>>
>>>> diff --git a/exec.c b/exec.c
>>>> index 0122ef7..2691c0a 100644
>>>> --- a/exec.c
>>>> +++ b/exec.c
>>>> @@ -187,10 +187,12 @@ struct CPUAddressSpace {
>>>>
>>>>   static void phys_map_node_reserve(PhysPageMap *map, unsigned nodes)
>>>>   {
>>>> +    static unsigned alloc_hint = 16;
>>>>       if (map->nodes_nb + nodes > map->nodes_nb_alloc) {
>>>> -        map->nodes_nb_alloc = MAX(map->nodes_nb_alloc * 2, 16);
>>>> +        map->nodes_nb_alloc = MAX(map->nodes_nb_alloc, alloc_hint);
>>>>           map->nodes_nb_alloc = MAX(map->nodes_nb_alloc, map->nodes_nb +
>>>> nodes);
>>>>           map->nodes = g_renew(Node, map->nodes, map->nodes_nb_alloc);
>>>> +        alloc_hint = map->nodes_nb_alloc;
>>>>       }
>>>>   }
>>>>
>>>>
>>>> Question is still, mmap for this?
>>> Nice!  Can you submit a patch for this?
>> Of course, but please see my other comment. We still should consider mmap for this cause we have close to 500 Physmaps about 70KB each which
>> are all allocted at the same time - I think due to RCU.
> That I think is not material for 2.7, and also I want to take a look at
> creating nodes lazily.

Okay, then I will send a patch to avoid the realloc and we look at this later.

Peter


>
> Paolo
diff mbox

Patch

diff --git a/exec.c b/exec.c
index 0122ef7..2691c0a 100644
--- a/exec.c
+++ b/exec.c
@@ -187,10 +187,12 @@  struct CPUAddressSpace {

  static void phys_map_node_reserve(PhysPageMap *map, unsigned nodes)
  {
+    static unsigned alloc_hint = 16;
      if (map->nodes_nb + nodes > map->nodes_nb_alloc) {
-        map->nodes_nb_alloc = MAX(map->nodes_nb_alloc * 2, 16);
+        map->nodes_nb_alloc = MAX(map->nodes_nb_alloc, alloc_hint);
          map->nodes_nb_alloc = MAX(map->nodes_nb_alloc, map->nodes_nb + nodes);
          map->nodes = g_renew(Node, map->nodes, map->nodes_nb_alloc);
+        alloc_hint = map->nodes_nb_alloc;
      }
  }