[Qemu-devel] [PATCH] exec.c: Use tb1->phys_hash_next directly in tb_remove

[Qemu-devel] [PATCH] exec.c: Use tb1->phys_hash_next directly in tb_remove

[陳韋任 (Wei-Ren Chen)]

  When tb_remove was first commited at fd6ce8f6, there were three different
calls pass different names to offsetof. In current codebase, the other two
calls are replaced with tb_page_remove. There is no need to have a general
tb_remove. Omit passing the third parameter and using tb1->phys_hash_next
directly.

Signed-off-by: Chen Wei-Ren <chenwj@iis.sinica.edu.tw>
---
 exec.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/exec.c b/exec.c
index 8435de0..e54fce2 100644
--- a/exec.c
+++ b/exec.c
@@ -863,17 +863,16 @@ static void tb_page_check(void)
 #endif
 
 /* invalidate one TB */
-static inline void tb_remove(TranslationBlock **ptb, TranslationBlock *tb,
-                             int next_offset)
+static inline void tb_hash_remove(TranslationBlock **ptb, TranslationBlock *tb)
 {
     TranslationBlock *tb1;
     for(;;) {
         tb1 = *ptb;
         if (tb1 == tb) {
-            *ptb = *(TranslationBlock **)((char *)tb1 + next_offset);
+            *ptb = tb1->phys_hash_next;
             break;
         }
-        ptb = (TranslationBlock **)((char *)tb1 + next_offset);
+        ptb = &(tb1->phys_hash_next);
     }
 }
 
@@ -940,8 +939,7 @@ void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr)
     /* remove the TB from the hash list */
     phys_pc = tb->page_addr[0] + (tb->pc & ~TARGET_PAGE_MASK);
     h = tb_phys_hash_func(phys_pc);
-    tb_remove(&tb_phys_hash[h], tb,
-              offsetof(TranslationBlock, phys_hash_next));
+    tb_hash_remove(&tb_phys_hash[h], tb);
 
     /* remove the TB from the page list */
     if (tb->page_addr[0] != page_addr) {
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH] exec.c: Use tb1->phys_hash_next directly in tb_remove

[Peter Maydell]

On 20 November 2012 12:30, 陳韋任 (Wei-Ren Chen) <chenwj@iis.sinica.edu.tw> wrote:
>   When tb_remove was first commited at fd6ce8f6, there were three different
> calls pass different names to offsetof. In current codebase, the other two
> calls are replaced with tb_page_remove. There is no need to have a general
> tb_remove. Omit passing the third parameter and using tb1->phys_hash_next
> directly.

I like this, it makes the function less confusing to remove this
now-unneeded generality.

> Signed-off-by: Chen Wei-Ren <chenwj@iis.sinica.edu.tw>
> ---
>  exec.c |   10 ++++------
>  1 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 8435de0..e54fce2 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -863,17 +863,16 @@ static void tb_page_check(void)
>  #endif
>
>  /* invalidate one TB */

This comment is now a bit out of date (and in fact it has been for
some time) and should probably be deleted. (The function that really
needs a comment is the top level tb_phys_invalidate(), rather than
the helpers tb_hash_remove/tb_page_remove/tb_jmp_remove.)

> -static inline void tb_remove(TranslationBlock **ptb, TranslationBlock *tb,
> -                             int next_offset)
> +static inline void tb_hash_remove(TranslationBlock **ptb, TranslationBlock *tb)
>  {
>      TranslationBlock *tb1;
>      for(;;) {
>          tb1 = *ptb;
>          if (tb1 == tb) {
> -            *ptb = *(TranslationBlock **)((char *)tb1 + next_offset);
> +            *ptb = tb1->phys_hash_next;
>              break;
>          }
> -        ptb = (TranslationBlock **)((char *)tb1 + next_offset);
> +        ptb = &(tb1->phys_hash_next);

You don't need these brackets.

>      }
>  }
>
> @@ -940,8 +939,7 @@ void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr)
>      /* remove the TB from the hash list */
>      phys_pc = tb->page_addr[0] + (tb->pc & ~TARGET_PAGE_MASK);
>      h = tb_phys_hash_func(phys_pc);
> -    tb_remove(&tb_phys_hash[h], tb,
> -              offsetof(TranslationBlock, phys_hash_next));
> +    tb_hash_remove(&tb_phys_hash[h], tb);
>
>      /* remove the TB from the page list */
>      if (tb->page_addr[0] != page_addr) {
> --
> 1.7.3.4


-- PMM

Re: [Qemu-devel] [PATCH] exec.c: Use tb1->phys_hash_next directly in tb_remove

[陳韋任 (Wei-Ren Chen)]

  ping?

On Tue, Nov 20, 2012 at 12:41:03PM +0000, Peter Maydell wrote:
> On 20 November 2012 12:30, 陳韋任 (Wei-Ren Chen) <chenwj@iis.sinica.edu.tw> wrote:
> >   When tb_remove was first commited at fd6ce8f6, there were three different
> > calls pass different names to offsetof. In current codebase, the other two
> > calls are replaced with tb_page_remove. There is no need to have a general
> > tb_remove. Omit passing the third parameter and using tb1->phys_hash_next
> > directly.
> 
> I like this, it makes the function less confusing to remove this
> now-unneeded generality.
> 
> > Signed-off-by: Chen Wei-Ren <chenwj@iis.sinica.edu.tw>
> > ---
> >  exec.c |   10 ++++------
> >  1 files changed, 4 insertions(+), 6 deletions(-)
> >
> > diff --git a/exec.c b/exec.c
> > index 8435de0..e54fce2 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -863,17 +863,16 @@ static void tb_page_check(void)
> >  #endif
> >
> >  /* invalidate one TB */
> 
> This comment is now a bit out of date (and in fact it has been for
> some time) and should probably be deleted. (The function that really
> needs a comment is the top level tb_phys_invalidate(), rather than
> the helpers tb_hash_remove/tb_page_remove/tb_jmp_remove.)
> 
> > -static inline void tb_remove(TranslationBlock **ptb, TranslationBlock *tb,
> > -                             int next_offset)
> > +static inline void tb_hash_remove(TranslationBlock **ptb, TranslationBlock *tb)
> >  {
> >      TranslationBlock *tb1;
> >      for(;;) {
> >          tb1 = *ptb;
> >          if (tb1 == tb) {
> > -            *ptb = *(TranslationBlock **)((char *)tb1 + next_offset);
> > +            *ptb = tb1->phys_hash_next;
> >              break;
> >          }
> > -        ptb = (TranslationBlock **)((char *)tb1 + next_offset);
> > +        ptb = &(tb1->phys_hash_next);
> 
> You don't need these brackets.
> 
> >      }
> >  }
> >
> > @@ -940,8 +939,7 @@ void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr)
> >      /* remove the TB from the hash list */
> >      phys_pc = tb->page_addr[0] + (tb->pc & ~TARGET_PAGE_MASK);
> >      h = tb_phys_hash_func(phys_pc);
> > -    tb_remove(&tb_phys_hash[h], tb,
> > -              offsetof(TranslationBlock, phys_hash_next));
> > +    tb_hash_remove(&tb_phys_hash[h], tb);
> >
> >      /* remove the TB from the page list */
> >      if (tb->page_addr[0] != page_addr) {
> > --
> > 1.7.3.4
> 
> 
> -- PMM

-- 
Wei-Ren Chen (陳韋任)
Computer Systems Lab, Institute of Information Science,
Academia Sinica, Taiwan (R.O.C.)
Tel:886-2-2788-3799 #1667
Homepage: http://people.cs.nctu.edu.tw/~chenwj

Re: [Qemu-devel] [PATCH] exec.c: Use tb1->phys_hash_next directly in tb_remove

[Peter Maydell]

On 25 November 2012 07:50, 陳韋任 (Wei-Ren Chen) <chenwj@iis.sinica.edu.tw> wrote:
>   ping?

This is v1 of the patch, you've sent a v2 and should be pinging that
instead... Also (a) it won't go in before 1.3 release now so not
much point in being too eager with the pings (b) you could cc
qemu-trivial.

-- PMM

Re: [Qemu-devel] [PATCH] exec.c: Use tb1->phys_hash_next directly in tb_remove

[陳韋任 (Wei-Ren Chen)]

On Sun, Nov 25, 2012 at 10:47:00AM +0000, Peter Maydell wrote:
> On 25 November 2012 07:50, 陳韋任 (Wei-Ren Chen) <chenwj@iis.sinica.edu.tw> wrote:
> >   ping?
> 
> This is v1 of the patch, you've sent a v2 and should be pinging that
> instead... Also (a) it won't go in before 1.3 release now so not
> much point in being too eager with the pings (b) you could cc
> qemu-trivial.

  Got it. ;)

-- 
Wei-Ren Chen (陳韋任)
Computer Systems Lab, Institute of Information Science,
Academia Sinica, Taiwan (R.O.C.)
Tel:886-2-2788-3799 #1667
Homepage: http://people.cs.nctu.edu.tw/~chenwj