* 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 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
[parent not found: <20190321082555.21118-3-richardw.yang@linux.intel.com>]
* 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 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, other threads:[~2019-09-16 7:50 UTC | newest] 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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).