diff mbox series

[2/6] exec.c: remove an unnecessary assert on PHYS_MAP_NODE_NIL in phys_map_node_alloc()

Message ID 20190321082555.21118-3-richardw.yang@linux.intel.com
State New
Headers show
Series Refine exec | expand

Commit Message

Wei Yang March 21, 2019, 8:25 a.m. UTC 
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.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 exec.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Paolo Bonzini Aug. 22, 2019, 10:24 a.m. UTC | #1 
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;
>
Wei Yang Aug. 23, 2019, 1:07 a.m. UTC | #2 
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 Sept. 12, 2019, 2:51 a.m. UTC | #3 
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?
Paolo Bonzini Sept. 12, 2019, 12:42 p.m. UTC | #4 
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
Wei Yang Sept. 12, 2019, 11:02 p.m. UTC | #5 
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
Paolo Bonzini Sept. 13, 2019, 9:12 a.m. UTC | #6 
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
Wei Yang Sept. 16, 2019, 2:02 a.m. UTC | #7 
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
Paolo Bonzini Sept. 16, 2019, 7:50 a.m. UTC | #8 
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
>
diff mbox series

Patch

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;