Re: [Qemu-devel] [PATCH 05/16] translate-all: iterate over TBs in a page with PAGE_FOR_EACH_TB

[Richard Henderson]

On 02/28/2018 02:50 PM, Emilio G. Cota wrote:
> Is this any better?
> 
> #define TB_FOR_EACH_TAGGED(head, tb, n, field)  \
> for (n = (head) & 1, tb = (TranslationBlock *)((head) & ~1);\
>  tb; tb = (TranslationBlock *)tb->field[n], n = (uintptr_t)tb & 1, \
>  tb = (TranslationBlock *)((uintptr_t)tb & ~1))

Yes, thanks.


r~

Re: [Qemu-devel] [PATCH 05/16] translate-all: iterate over TBs in a page with PAGE_FOR_EACH_TB

[Emilio G. Cota]

On Wed, Feb 28, 2018 at 13:40:15 -0800, Richard Henderson wrote:
> On 02/26/2018 09:39 PM, Emilio G. Cota wrote:
> > +/* list iterators for lists of tagged pointers in TranslationBlock */
> > +#define TB_FOR_EACH_TAGGED(head, tb, n, field)  \
> > +for (n = (head) & 1,\
> > + tb = (TranslationBlock *)((head) & ~1);\
> > + tb;\
> > + tb = (TranslationBlock *)tb->field[n], \
> > + n = (uintptr_t)tb & 1, \
> > + tb = (TranslationBlock *)((uintptr_t)tb & ~1))
> > +
> > +#define PAGE_FOR_EACH_TB(pagedesc, tb, n)   \
> > +TB_FOR_EACH_TAGGED((pagedesc)->first_tb, tb, n, page_next)
> > +
> 
> I'm not sure I like the generalization of TB_FOR_EACH_TAGGED.  Do you use it
> for anything besides PAGE_FOR_EACH_TB?

Yes, see patch 13. I've added the following comment to the commit log:
 - Introduce the TB_FOR_EACH_TAGGED macro, and use it to define
   PAGE_FOR_EACH_TB, which improves readability. Note that
   TB_FOR_EACH_TAGGED will gain another user in a subsequent patch.

> Weird indentation in the clauses.

Is this any better?

#define TB_FOR_EACH_TAGGED(head, tb, n, field)  \
for (n = (head) & 1, tb = (TranslationBlock *)((head) & ~1);\
 tb; tb = (TranslationBlock *)tb->field[n], n = (uintptr_t)tb & 1, \
 tb = (TranslationBlock *)((uintptr_t)tb & ~1))

> Otherwise,
> Reviewed-by: Richard Henderson 

Thanks,

Emilio

Re: [Qemu-devel] [PATCH 05/16] translate-all: iterate over TBs in a page with PAGE_FOR_EACH_TB

[Richard Henderson]

On 02/26/2018 09:39 PM, Emilio G. Cota wrote:
> +/* list iterators for lists of tagged pointers in TranslationBlock */
> +#define TB_FOR_EACH_TAGGED(head, tb, n, field)  \
> +for (n = (head) & 1,\
> + tb = (TranslationBlock *)((head) & ~1);\
> + tb;\
> + tb = (TranslationBlock *)tb->field[n], \
> + n = (uintptr_t)tb & 1, \
> + tb = (TranslationBlock *)((uintptr_t)tb & ~1))
> +
> +#define PAGE_FOR_EACH_TB(pagedesc, tb, n)   \
> +TB_FOR_EACH_TAGGED((pagedesc)->first_tb, tb, n, page_next)
> +

I'm not sure I like the generalization of TB_FOR_EACH_TAGGED.  Do you use it
for anything besides PAGE_FOR_EACH_TB?

Weird indentation in the clauses.

Otherwise,
Reviewed-by: Richard Henderson 


r~

[Qemu-devel] [PATCH 05/16] translate-all: iterate over TBs in a page with PAGE_FOR_EACH_TB

[Emilio G. Cota]

This commit does several things, but to avoid churn I merged them all
into the same commit. To wit:

- Use uintptr_t instead of TranslationBlock * for the list of TBs in a page.
  Just like we did in (c37e6d7e "tcg: Use uintptr_t type for
  jmp_list_{next|first} fields of TB"), the rationale is the same: these
  are tagged pointers, not pointers. So use a more appropriate type.

- Only check the least significant bit of the tagged pointers. Masking
  with 3/~3 is unnecessary and confusing.

- Introduce the TB_FOR_EACH_TAGGED macro, and use it to define
  PAGE_FOR_EACH_TB, which improves readability.

- Update tb_page_remove to use PAGE_FOR_EACH_TB. In case there
  is a bug and we attempt to remove a TB that is not in the list, instead
  of segfaulting (since the list is NULL-terminated) we will reach
  g_assert_not_reached().

Signed-off-by: Emilio G. Cota 
---
 accel/tcg/translate-all.c | 65 +++
 include/exec/exec-all.h   |  2 +-
 2 files changed, 33 insertions(+), 34 deletions(-)

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 20ad3fc..06aa905 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -103,7 +103,7 @@
 
 typedef struct PageDesc {
 /* list of TBs intersecting this ram page */
-TranslationBlock *first_tb;
+uintptr_t first_tb;
 #ifdef CONFIG_SOFTMMU
 /* in order to optimize self modifying code, we count the number
of lookups we do to a given page to use a bitmap */
@@ -114,6 +114,18 @@ typedef struct PageDesc {
 #endif
 } PageDesc;
 
+/* list iterators for lists of tagged pointers in TranslationBlock */
+#define TB_FOR_EACH_TAGGED(head, tb, n, field)  \
+for (n = (head) & 1,\
+ tb = (TranslationBlock *)((head) & ~1);\
+ tb;\
+ tb = (TranslationBlock *)tb->field[n], \
+ n = (uintptr_t)tb & 1, \
+ tb = (TranslationBlock *)((uintptr_t)tb & ~1))
+
+#define PAGE_FOR_EACH_TB(pagedesc, tb, n)   \
+TB_FOR_EACH_TAGGED((pagedesc)->first_tb, tb, n, page_next)
+
 /* In system mode we want L1_MAP to be based on ram offsets,
while in user mode we want it to be based on virtual addresses.  */
 #if !defined(CONFIG_USER_ONLY)
@@ -818,7 +830,7 @@ static void page_flush_tb_1(int level, void **lp)
 PageDesc *pd = *lp;
 
 for (i = 0; i < V_L2_SIZE; ++i) {
-pd[i].first_tb = NULL;
+pd[i].first_tb = (uintptr_t)NULL;
 invalidate_page_bitmap(pd + i);
 }
 } else {
@@ -946,21 +958,21 @@ static void tb_page_check(void)
 
 #endif /* CONFIG_USER_ONLY */
 
-static inline void tb_page_remove(TranslationBlock **ptb, TranslationBlock *tb)
+static inline void tb_page_remove(PageDesc *pd, TranslationBlock *tb)
 {
 TranslationBlock *tb1;
+uintptr_t *pprev;
 unsigned int n1;
 
-for (;;) {
-tb1 = *ptb;
-n1 = (uintptr_t)tb1 & 3;
-tb1 = (TranslationBlock *)((uintptr_t)tb1 & ~3);
+pprev = >first_tb;
+PAGE_FOR_EACH_TB(pd, tb1, n1) {
 if (tb1 == tb) {
-*ptb = tb1->page_next[n1];
-break;
+*pprev = tb1->page_next[n1];
+return;
 }
-ptb = >page_next[n1];
+pprev = >page_next[n1];
 }
+g_assert_not_reached();
 }
 
 /* remove the TB from a list of TBs jumping to the n-th jump target of the TB 
*/
@@ -1048,12 +1060,12 @@ void tb_phys_invalidate(TranslationBlock *tb, 
tb_page_addr_t page_addr)
 /* remove the TB from the page list */
 if (tb->page_addr[0] != page_addr) {
 p = page_find(tb->page_addr[0] >> TARGET_PAGE_BITS);
-tb_page_remove(>first_tb, tb);
+tb_page_remove(p, tb);
 invalidate_page_bitmap(p);
 }
 if (tb->page_addr[1] != -1 && tb->page_addr[1] != page_addr) {
 p = page_find(tb->page_addr[1] >> TARGET_PAGE_BITS);
-tb_page_remove(>first_tb, tb);
+tb_page_remove(p, tb);
 invalidate_page_bitmap(p);
 }
 
@@ -1084,10 +1096,7 @@ static void build_page_bitmap(PageDesc *p)
 
 p->code_bitmap = bitmap_new(TARGET_PAGE_SIZE);
 
-tb = p->first_tb;
-while (tb != NULL) {
-n = (uintptr_t)tb & 3;
-tb = (TranslationBlock *)((uintptr_t)tb & ~3);
+PAGE_FOR_EACH_TB(p, tb, n) {
 /* NOTE: this is subtle as a TB may span two physical pages */
 if (n == 0) {
 /* NOTE: tb_end may be after the end of the page, but
@@ -1102,7 +,6 @@ static void build_page_bitmap(PageDesc *p)
 tb_end = ((tb->pc + tb->size) & ~TARGET_PAGE_MASK);
 }
 bitmap_set(p->code_bitmap, tb_start, tb_end - tb_start);
-tb = tb->page_next[n];
 }
 }
 #endif
@@ -1125,9 +1133,9 @@ static inline void tb_alloc_page(TranslationBlock *tb,
 p =