Re: [Qemu-devel] [PATCH 0/6] Refine exec

Re: [Qemu-devel] [PATCH 0/6] Refine exec

[Wei Yang]

Ping for this trivial series.

On Thu, Mar 21, 2019 at 04:25:49PM +0800, Wei Yang wrote:
>This serial refine exec a little.
>
>Wei Yang (6):
>  exec.c: replace hwaddr with uint64_t for better understanding
>  exec.c: remove an unnecessary assert on PHYS_MAP_NODE_NIL in
>    phys_map_node_alloc()
>  exec.c: get nodes_nb_alloc with one MAX calculation
>  exec.c: subpage->sub_section is already initialized to 0
>  exec.c: correct the maximum skip value during compact
>  exec.c: add a check between constants to see whether we could skip
>
> exec.c | 21 ++++++++++-----------
> 1 file changed, 10 insertions(+), 11 deletions(-)
>
>-- 
>2.19.1

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH 0/6] Refine exec

[Wei Yang]

Ping for this trivial series.

On Thu, Mar 21, 2019 at 04:25:49PM +0800, Wei Yang wrote:
>This serial refine exec a little.
>
>Wei Yang (6):
>  exec.c: replace hwaddr with uint64_t for better understanding
>  exec.c: remove an unnecessary assert on PHYS_MAP_NODE_NIL in
>    phys_map_node_alloc()
>  exec.c: get nodes_nb_alloc with one MAX calculation
>  exec.c: subpage->sub_section is already initialized to 0
>  exec.c: correct the maximum skip value during compact
>  exec.c: add a check between constants to see whether we could skip
>
> exec.c | 21 ++++++++++-----------
> 1 file changed, 10 insertions(+), 11 deletions(-)
>
>-- 
>2.19.1

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH 0/6] Refine exec

[Wei Yang]

On Thu, Mar 21, 2019 at 04:25:49PM +0800, Wei Yang wrote:
>This serial refine exec a little.
>

Ping again.

>Wei Yang (6):
>  exec.c: replace hwaddr with uint64_t for better understanding
>  exec.c: remove an unnecessary assert on PHYS_MAP_NODE_NIL in
>    phys_map_node_alloc()
>  exec.c: get nodes_nb_alloc with one MAX calculation
>  exec.c: subpage->sub_section is already initialized to 0
>  exec.c: correct the maximum skip value during compact
>  exec.c: add a check between constants to see whether we could skip
>
> exec.c | 21 ++++++++++-----------
> 1 file changed, 10 insertions(+), 11 deletions(-)
>
>-- 
>2.19.1

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH 0/6] Refine exec

[Paolo Bonzini]

On 19/08/19 05:06, Wei Yang wrote:
> On Thu, Mar 21, 2019 at 04:25:49PM +0800, Wei Yang wrote:
>> This serial refine exec a little.
>>
> 
> Ping again.

Queued all except 2, thanks!

Paolo

>> Wei Yang (6):
>>  exec.c: replace hwaddr with uint64_t for better understanding
>>  exec.c: remove an unnecessary assert on PHYS_MAP_NODE_NIL in
>>    phys_map_node_alloc()
>>  exec.c: get nodes_nb_alloc with one MAX calculation
>>  exec.c: subpage->sub_section is already initialized to 0
>>  exec.c: correct the maximum skip value during compact
>>  exec.c: add a check between constants to see whether we could skip
>>
>> exec.c | 21 ++++++++++-----------
>> 1 file changed, 10 insertions(+), 11 deletions(-)
>>
>> -- 
>> 2.19.1
> 

Re: [Qemu-devel] [PATCH 0/6] Refine exec

[Wei Yang]

On Thu, Aug 22, 2019 at 12:25:44PM +0200, Paolo Bonzini wrote:
>On 19/08/19 05:06, Wei Yang wrote:
>> On Thu, Mar 21, 2019 at 04:25:49PM +0800, Wei Yang wrote:
>>> This serial refine exec a little.
>>>
>> 
>> Ping again.
>
>Queued all except 2, thanks!
>

Thanks~

>Paolo
>
>>> Wei Yang (6):
>>>  exec.c: replace hwaddr with uint64_t for better understanding
>>>  exec.c: remove an unnecessary assert on PHYS_MAP_NODE_NIL in
>>>    phys_map_node_alloc()
>>>  exec.c: get nodes_nb_alloc with one MAX calculation
>>>  exec.c: subpage->sub_section is already initialized to 0
>>>  exec.c: correct the maximum skip value during compact
>>>  exec.c: add a check between constants to see whether we could skip
>>>
>>> exec.c | 21 ++++++++++-----------
>>> 1 file changed, 10 insertions(+), 11 deletions(-)
>>>
>>> -- 
>>> 2.19.1
>> 
>

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH 2/6] exec.c: remove an unnecessary assert on PHYS_MAP_NODE_NIL in phys_map_node_alloc()

[Paolo Bonzini]

On 21/03/19 09:25, Wei Yang wrote:
> PHYS_MAP_NODE_NIL is assigned to PhysPageEntry.ptr in case this is not a
> leaf entry, while map->nodes_nb range in [0, nodes_nb_alloc).
> 
> Seems we are asserting on two different things, just remove it.

The assertion checks that this "if" is not entered incorrectly:

    if (lp->skip && lp->ptr == PHYS_MAP_NODE_NIL) {
        lp->ptr = phys_map_node_alloc(map, level == 0);
    }

Paolo

> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  exec.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/exec.c b/exec.c
> index 98ebd0dd1d..8e8b6bb1f9 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -242,7 +242,6 @@ static uint32_t phys_map_node_alloc(PhysPageMap *map, bool leaf)
>  
>      ret = map->nodes_nb++;
>      p = map->nodes[ret];
> -    assert(ret != PHYS_MAP_NODE_NIL);
>      assert(ret != map->nodes_nb_alloc);
>  
>      e.skip = leaf ? 0 : 1;
> 

Re: [Qemu-devel] [PATCH 2/6] exec.c: remove an unnecessary assert on PHYS_MAP_NODE_NIL in phys_map_node_alloc()

[Wei Yang]

On Thu, Aug 22, 2019 at 12:24:32PM +0200, Paolo Bonzini wrote:
>On 21/03/19 09:25, Wei Yang wrote:
>> PHYS_MAP_NODE_NIL is assigned to PhysPageEntry.ptr in case this is not a
>> leaf entry, while map->nodes_nb range in [0, nodes_nb_alloc).
>> 
>> Seems we are asserting on two different things, just remove it.
>
>The assertion checks that this "if" is not entered incorrectly:
>
>    if (lp->skip && lp->ptr == PHYS_MAP_NODE_NIL) {
>        lp->ptr = phys_map_node_alloc(map, level == 0);
>    }
>

Hmm... I may not get your point.

phys_map_node_alloc() will get an available PhysPageEntry and return its
index, which will be assigned to its parent's ptr.

The "if" checks on the parent's ptr, while the assertion asserts the index for
the new child. I may miss something?

>Paolo
>
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  exec.c | 1 -
>>  1 file changed, 1 deletion(-)
>> 
>> diff --git a/exec.c b/exec.c
>> index 98ebd0dd1d..8e8b6bb1f9 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -242,7 +242,6 @@ static uint32_t phys_map_node_alloc(PhysPageMap *map, bool leaf)
>>  
>>      ret = map->nodes_nb++;
>>      p = map->nodes[ret];
>> -    assert(ret != PHYS_MAP_NODE_NIL);
>>      assert(ret != map->nodes_nb_alloc);
>>  
>>      e.skip = leaf ? 0 : 1;
>> 

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH 2/6] exec.c: remove an unnecessary assert on PHYS_MAP_NODE_NIL in phys_map_node_alloc()

[Wei Yang]

On Fri, Aug 23, 2019 at 09:07:50AM +0800, Wei Yang wrote:
>On Thu, Aug 22, 2019 at 12:24:32PM +0200, Paolo Bonzini wrote:
>>On 21/03/19 09:25, Wei Yang wrote:
>>> PHYS_MAP_NODE_NIL is assigned to PhysPageEntry.ptr in case this is not a
>>> leaf entry, while map->nodes_nb range in [0, nodes_nb_alloc).
>>> 
>>> Seems we are asserting on two different things, just remove it.
>>
>>The assertion checks that this "if" is not entered incorrectly:
>>
>>    if (lp->skip && lp->ptr == PHYS_MAP_NODE_NIL) {
>>        lp->ptr = phys_map_node_alloc(map, level == 0);
>>    }
>>
>
>Hmm... I may not get your point.
>
>phys_map_node_alloc() will get an available PhysPageEntry and return its
>index, which will be assigned to its parent's ptr.
>
>The "if" checks on the parent's ptr, while the assertion asserts the index for
>the new child. I may miss something?
>

Hi, Paolo,

Do I miss something?


-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH 2/6] exec.c: remove an unnecessary assert on PHYS_MAP_NODE_NIL in phys_map_node_alloc()

[Paolo Bonzini]

On 12/09/19 04:51, Wei Yang wrote:
> On Fri, Aug 23, 2019 at 09:07:50AM +0800, Wei Yang wrote:
>> On Thu, Aug 22, 2019 at 12:24:32PM +0200, Paolo Bonzini wrote:
>>> On 21/03/19 09:25, Wei Yang wrote:
>>>> PHYS_MAP_NODE_NIL is assigned to PhysPageEntry.ptr in case this is not a
>>>> leaf entry, while map->nodes_nb range in [0, nodes_nb_alloc).
>>>>
>>>> Seems we are asserting on two different things, just remove it.
>>>
>>> The assertion checks that this "if" is not entered incorrectly:
>>>
>>>    if (lp->skip && lp->ptr == PHYS_MAP_NODE_NIL) {
>>>        lp->ptr = phys_map_node_alloc(map, level == 0);
>>>    }
>>>
>>
>> Hmm... I may not get your point.
>>
>> phys_map_node_alloc() will get an available PhysPageEntry and return its
>> index, which will be assigned to its parent's ptr.
>>
>> The "if" checks on the parent's ptr, while the assertion asserts the index for
>> the new child. I may miss something?
>>
> 
> Hi, Paolo,
> 
> Do I miss something?

Sorry, I was on vacation.  phys_page_set_level can be called multiple
times, with the same lp.  The assertion checks that only the first call
will reach phys_map_node_alloc.

Paolo

Re: [Qemu-devel] [PATCH 2/6] exec.c: remove an unnecessary assert on PHYS_MAP_NODE_NIL in phys_map_node_alloc()

[Wei Yang]

On Thu, Sep 12, 2019 at 02:42:26PM +0200, Paolo Bonzini wrote:
>On 12/09/19 04:51, Wei Yang wrote:
>> On Fri, Aug 23, 2019 at 09:07:50AM +0800, Wei Yang wrote:
>>> On Thu, Aug 22, 2019 at 12:24:32PM +0200, Paolo Bonzini wrote:
>>>> On 21/03/19 09:25, Wei Yang wrote:
>>>>> PHYS_MAP_NODE_NIL is assigned to PhysPageEntry.ptr in case this is not a
>>>>> leaf entry, while map->nodes_nb range in [0, nodes_nb_alloc).
>>>>>
>>>>> Seems we are asserting on two different things, just remove it.
>>>>
>>>> The assertion checks that this "if" is not entered incorrectly:
>>>>
>>>>    if (lp->skip && lp->ptr == PHYS_MAP_NODE_NIL) {
>>>>        lp->ptr = phys_map_node_alloc(map, level == 0);
>>>>    }
>>>>
>>>
>>> Hmm... I may not get your point.
>>>
>>> phys_map_node_alloc() will get an available PhysPageEntry and return its
>>> index, which will be assigned to its parent's ptr.
>>>
>>> The "if" checks on the parent's ptr, while the assertion asserts the index for
>>> the new child. I may miss something?
>>>
>> 
>> Hi, Paolo,
>> 
>> Do I miss something?
>
>Sorry, I was on vacation.  phys_page_set_level can be called multiple
>times, with the same lp.  The assertion checks that only the first call
>will reach phys_map_node_alloc.
>

Ah, I am just back from vacation too. The mountain makes me painful. :-) 

So I guess you are talking about the situation of wrap around.
PHYS_MAP_NODE_NIL is an indicator that this lp is not used yet. And we are not
expecting any valid lp has its ptr with value equal or bigger than
PHYS_MAP_NODE_NIL.

If this is the case, I am thinking this won't happen in current
implementation. Because if my understanding is correct, the total number of
PhysPageEntry would be less than PHYS_MAP_NODE_NIL.

First let's look at the PHYS_MAP_NODE_NIL's value

    PHYS_MAP_NODE_NIL = 2^26 = 6710 8864.
    
So we could represent 6710 8864 PhysPageEntry at most.

Then let's look how many PhysPageEntry would be. The PhysPageEntry structure
forms a tree with 9 nodes on each level with P_L2_LEVELS levels. This means
the tree could have 9^P_L2_LEVELS nodes at most. 

Since

#define ADDR_SPACE_BITS 64
#define P_L2_BITS 9
#define P_L2_LEVELS (((ADDR_SPACE_BITS - TARGET_PAGE_BITS - 1) / P_L2_BITS) + 1)

And TARGET_PAGE_BITS >= 12, so P_L2_LEVELS is smaller than 

    7 = (64 - 12 - 1) / 9 + 1.

This leads to 

    9^P_L2_LEVELS <= 9^7 = 478 2969

It shows PHYS_MAP_NODE_NIL may represents more node the tree could hold.

The assert here is not harmful, while maybe we can have a better way to handle
it?

>Paolo

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH 2/6] exec.c: remove an unnecessary assert on PHYS_MAP_NODE_NIL in phys_map_node_alloc()

[Paolo Bonzini]

On 13/09/19 01:02, Wei Yang wrote:
> It shows PHYS_MAP_NODE_NIL may represents more node the tree could hold.

Which is good, it means the assert can trigger.

> The assert here is not harmful, while maybe we can have a better way to handle
> it?

I don't know... The assert just says "careful, someone treats
PHYS_MAP_NODE_NIL specially!".  It's documentation too.

Paolo

Re: [Qemu-devel] [PATCH 2/6] exec.c: remove an unnecessary assert on PHYS_MAP_NODE_NIL in phys_map_node_alloc()

[Wei Yang]

On Fri, Sep 13, 2019 at 11:12:05AM +0200, Paolo Bonzini wrote:
>On 13/09/19 01:02, Wei Yang wrote:
>> It shows PHYS_MAP_NODE_NIL may represents more node the tree could hold.
>
>Which is good, it means the assert can trigger.
>

Per my understanding, it means the assert can't trigger.

>> The assert here is not harmful, while maybe we can have a better way to handle
>> it?
>
>I don't know... The assert just says "careful, someone treats
>PHYS_MAP_NODE_NIL specially!".  It's documentation too.
>
>Paolo

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH 2/6] exec.c: remove an unnecessary assert on PHYS_MAP_NODE_NIL in phys_map_node_alloc()

[Paolo Bonzini]

On 16/09/19 04:02, Wei Yang wrote:
> On Fri, Sep 13, 2019 at 11:12:05AM +0200, Paolo Bonzini wrote:
>> On 13/09/19 01:02, Wei Yang wrote:
>>> It shows PHYS_MAP_NODE_NIL may represents more node the tree could hold.
>>
>> Which is good, it means the assert can trigger.
>>
> 
> Per my understanding, it means the assert can't trigger.

You're right, sorry.  That's what I meant.

Paolo

>>> The assert here is not harmful, while maybe we can have a better way to handle
>>> it?
>>
>> I don't know... The assert just says "careful, someone treats
>> PHYS_MAP_NODE_NIL specially!".  It's documentation too.
>>
>> Paolo
>