QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* Re: [Qemu-devel] [PATCH 0/6] Refine exec
       [not found] <20190321082555.21118-1-richardw.yang@linux.intel.com>
@ 2019-04-22  0:59 ` Wei Yang
  2019-04-22  0:59   ` Wei Yang
  2019-08-19  3:06 ` Wei Yang
       [not found] ` <20190321082555.21118-3-richardw.yang@linux.intel.com>
  2 siblings, 1 reply; 13+ messages in thread
From: Wei Yang @ 2019-04-22  0:59 UTC (permalink / raw)
  To: Wei Yang; +Cc: qemu-devel, rth, pbonzini, mst

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH 0/6] Refine exec
  2019-04-22  0:59 ` [Qemu-devel] [PATCH 0/6] Refine exec Wei Yang
@ 2019-04-22  0:59   ` Wei Yang
  0 siblings, 0 replies; 13+ messages in thread
From: Wei Yang @ 2019-04-22  0:59 UTC (permalink / raw)
  To: Wei Yang; +Cc: pbonzini, mst, qemu-devel, rth

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH 0/6] Refine exec
       [not found] <20190321082555.21118-1-richardw.yang@linux.intel.com>
  2019-04-22  0:59 ` [Qemu-devel] [PATCH 0/6] Refine exec Wei Yang
@ 2019-08-19  3:06 ` Wei Yang
  2019-08-22 10:25   ` Paolo Bonzini
       [not found] ` <20190321082555.21118-3-richardw.yang@linux.intel.com>
  2 siblings, 1 reply; 13+ messages in thread
From: Wei Yang @ 2019-08-19  3:06 UTC (permalink / raw)
  To: Wei Yang; +Cc: pbonzini, mst, qemu-devel, rth

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH 2/6] exec.c: remove an unnecessary assert on PHYS_MAP_NODE_NIL in phys_map_node_alloc()
       [not found] ` <20190321082555.21118-3-richardw.yang@linux.intel.com>
@ 2019-08-22 10:24   ` Paolo Bonzini
  2019-08-23  1:07     ` Wei Yang
  0 siblings, 1 reply; 13+ messages in thread
From: Paolo Bonzini @ 2019-08-22 10:24 UTC (permalink / raw)
  To: Wei Yang, qemu-devel; +Cc: rth, mst

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;
> 



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH 0/6] Refine exec
  2019-08-19  3:06 ` Wei Yang
@ 2019-08-22 10:25   ` Paolo Bonzini
  2019-08-22 22:31     ` Wei Yang
  0 siblings, 1 reply; 13+ messages in thread
From: Paolo Bonzini @ 2019-08-22 10:25 UTC (permalink / raw)
  To: Wei Yang; +Cc: mst, qemu-devel, rth

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
> 



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH 0/6] Refine exec
  2019-08-22 10:25   ` Paolo Bonzini
@ 2019-08-22 22:31     ` Wei Yang
  0 siblings, 0 replies; 13+ messages in thread
From: Wei Yang @ 2019-08-22 22:31 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel, rth, Wei Yang, mst

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH 2/6] exec.c: remove an unnecessary assert on PHYS_MAP_NODE_NIL in phys_map_node_alloc()
  2019-08-22 10:24   ` [Qemu-devel] [PATCH 2/6] exec.c: remove an unnecessary assert on PHYS_MAP_NODE_NIL in phys_map_node_alloc() Paolo Bonzini
@ 2019-08-23  1:07     ` Wei Yang
  2019-09-12  2:51       ` Wei Yang
  0 siblings, 1 reply; 13+ messages in thread
From: Wei Yang @ 2019-08-23  1:07 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: rth, mst, Wei Yang, qemu-devel

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH 2/6] exec.c: remove an unnecessary assert on PHYS_MAP_NODE_NIL in phys_map_node_alloc()
  2019-08-23  1:07     ` Wei Yang
@ 2019-09-12  2:51       ` Wei Yang
  2019-09-12 12:42         ` Paolo Bonzini
  0 siblings, 1 reply; 13+ messages in thread
From: Wei Yang @ 2019-09-12  2:51 UTC (permalink / raw)
  To: Wei Yang; +Cc: Paolo Bonzini, rth, qemu-devel, mst

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH 2/6] exec.c: remove an unnecessary assert on PHYS_MAP_NODE_NIL in phys_map_node_alloc()
  2019-09-12  2:51       ` Wei Yang
@ 2019-09-12 12:42         ` Paolo Bonzini
  2019-09-12 23:02           ` Wei Yang
  0 siblings, 1 reply; 13+ messages in thread
From: Paolo Bonzini @ 2019-09-12 12:42 UTC (permalink / raw)
  To: Wei Yang; +Cc: rth, qemu-devel, mst

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH 2/6] exec.c: remove an unnecessary assert on PHYS_MAP_NODE_NIL in phys_map_node_alloc()
  2019-09-12 12:42         ` Paolo Bonzini
@ 2019-09-12 23:02           ` Wei Yang
  2019-09-13  9:12             ` Paolo Bonzini
  0 siblings, 1 reply; 13+ messages in thread
From: Wei Yang @ 2019-09-12 23:02 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel, mst, Wei Yang, rth

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH 2/6] exec.c: remove an unnecessary assert on PHYS_MAP_NODE_NIL in phys_map_node_alloc()
  2019-09-12 23:02           ` Wei Yang
@ 2019-09-13  9:12             ` Paolo Bonzini
  2019-09-16  2:02               ` Wei Yang
  0 siblings, 1 reply; 13+ messages in thread
From: Paolo Bonzini @ 2019-09-13  9:12 UTC (permalink / raw)
  To: Wei Yang; +Cc: qemu-devel, mst, Wei Yang, rth

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH 2/6] exec.c: remove an unnecessary assert on PHYS_MAP_NODE_NIL in phys_map_node_alloc()
  2019-09-13  9:12             ` Paolo Bonzini
@ 2019-09-16  2:02               ` Wei Yang
  2019-09-16  7:50                 ` Paolo Bonzini
  0 siblings, 1 reply; 13+ messages in thread
From: Wei Yang @ 2019-09-16  2:02 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel, rth, mst, Wei Yang, 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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH 2/6] exec.c: remove an unnecessary assert on PHYS_MAP_NODE_NIL in phys_map_node_alloc()
  2019-09-16  2:02               ` Wei Yang
@ 2019-09-16  7:50                 ` Paolo Bonzini
  0 siblings, 0 replies; 13+ messages in thread
From: Paolo Bonzini @ 2019-09-16  7:50 UTC (permalink / raw)
  To: Wei Yang; +Cc: rth, mst, qemu-devel, Wei Yang

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
> 



^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, back to index

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20190321082555.21118-1-richardw.yang@linux.intel.com>
2019-04-22  0:59 ` [Qemu-devel] [PATCH 0/6] Refine exec Wei Yang
2019-04-22  0:59   ` Wei Yang
2019-08-19  3:06 ` Wei Yang
2019-08-22 10:25   ` Paolo Bonzini
2019-08-22 22:31     ` Wei Yang
     [not found] ` <20190321082555.21118-3-richardw.yang@linux.intel.com>
2019-08-22 10:24   ` [Qemu-devel] [PATCH 2/6] exec.c: remove an unnecessary assert on PHYS_MAP_NODE_NIL in phys_map_node_alloc() Paolo Bonzini
2019-08-23  1:07     ` Wei Yang
2019-09-12  2:51       ` Wei Yang
2019-09-12 12:42         ` Paolo Bonzini
2019-09-12 23:02           ` Wei Yang
2019-09-13  9:12             ` Paolo Bonzini
2019-09-16  2:02               ` Wei Yang
2019-09-16  7:50                 ` Paolo Bonzini

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org qemu-devel@archiver.kernel.org
	public-inbox-index qemu-devel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox