Re: [Qemu-devel] [PATCH v2 1/4] add and link a statistic struct to TBs

[Alex Bennée]

Alex Bennée <address@hidden> writes:

> Vanderson Martins do Rosario <address@hidden> writes:
>
>> When the tb_flush removes a block and it's recreated, this shouldn't
>> be creating a new block but using the one that is found by:
>>
>> lookup_result = g_list_find_custom(tb_ctx.tb_statistics, new_stats,
>> statistics_cmp);
>>
>> So the tb_statisticics will be reused and we could just add this
>> regen counter in the if statement: if (lookup_result)
>>
>> isn't that correct?
>
> Yes, although as I said earlier you want to use a QHT hash table rather
> than a g_list to avoid racing with multiple translations at once.
>
>>
>> Vanderson M. Rosario
>>
>>
>> On Tue, Jun 25, 2019 at 1:28 PM Alex Bennée <address@hidden> wrote:
>>
>>>
>>> Alex Bennée <address@hidden> writes:
>>>
>>> > vandersonmr <address@hidden> writes:
>>> >
>>> >> We want to store statistics for each TB even after flushes.
>>> >> We do not want to modify or grow the TB struct.
>>> >> So we create a new struct to contain this statistics and
>>> >> link it to each TB while they are created.
>>> >>
>>> >> Signed-off-by: Vanderson M. do Rosario <address@hidden>
>>> >> ---
>>> >>  accel/tcg/translate-all.c | 40 +++++++++++++++++++++++++++++++++++++++
>>> >>  include/exec/exec-all.h   | 21 ++++++++++++++++++++
>>> >>  include/exec/tb-context.h |  1 +
>>> >>  include/qemu/log.h        |  1 +
>>> >>  4 files changed, 63 insertions(+)
>>> >>
>>> >> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
>>> >> index 5d1e08b169..f7e99f90e0 100644
>>> >> --- a/accel/tcg/translate-all.c
>>> >> +++ b/accel/tcg/translate-all.c
>>> >> @@ -1118,6 +1118,18 @@ static inline void code_gen_alloc(size_t tb_size)
>>> >>      }
>>> >>  }
>>> >>
>>> >> +static gint statistics_cmp(gconstpointer p1, gconstpointer p2)
>>> >> +{
>>> >> +    const TBStatistics *a = (TBStatistics *) p1;
>>> >> +    const TBStatistics *b = (TBStatistics *) p2;
>>> >> +
>>> >> +    return (a->pc == b->pc &&
>>> >> +               a->cs_base == b->cs_base &&
>>> >> +               a->flags == b->flags &&
>>> >> +           a->page_addr[0] == b->page_addr[0] &&
>>> >> +               a->page_addr[1] == b->page_addr[1]) ? 0 : 1;
>>> >> +}
>>> >> +
>>> >
>>> > There are a bunch of white space errors that need fixing up as you
>>> > probably already know from patchew ;-)
>>> >
>>> >>  static bool tb_cmp(const void *ap, const void *bp)
>>> >>  {
>>> >>      const TranslationBlock *a = ap;
>>> >> @@ -1586,6 +1598,29 @@ static inline void tb_page_add(PageDesc *p,
>>> TranslationBlock *tb,
>>> >>  #endif
>>> >>  }
>>> >>
>>> >> +static void tb_insert_statistics_structure(TranslationBlock *tb) {
>>> >> +    TBStatistics *new_stats = g_new0(TBStatistics, 1);
>>> >> +    new_stats->pc = tb->pc;
>>> >> +    new_stats->cs_base = tb->cs_base;
>>> >> +    new_stats->flags = tb->flags;
>>> >> +    new_stats->page_addr[0] = tb->page_addr[0];
>>> >> +    new_stats->page_addr[1] = tb->page_addr[1];
>>> >> +
>>> >> +    GList *lookup_result = g_list_find_custom(tb_ctx.tb_statistics,
>>> new_stats, statistics_cmp);
>>> >> +
>>> >> +    if (lookup_result) {
>>> >> +            /* If there is already a TBStatistic for this TB from a
>>> previous flush
>>> >> +            * then just make the new TB point to the older TBStatistic
>>> >> +            */
>>> >> +            free(new_stats);
>>> >> +            tb->tb_stats = lookup_result->data;
>>> >> +    } else {
>>> >> +            /* If not, then points to the new tb_statistics and add it
>>> to the hash */
>>> >> +            tb->tb_stats = new_stats;
>>> >> +            tb_ctx.tb_statistics = g_list_prepend(tb_ctx.tb_statistics,
>>> >> new_stats);
>>> >
>>> > This is going to race as nothing prevents two threads attempting to
>>> > insert records at the same time.
>>> >
>>> >> +    }
>>> >> +}
>>> >> +
>>> >>  /* add a new TB and link it to the physical page tables. phys_page2 is
>>> >>   * (-1) to indicate that only one page contains the TB.
>>> >>   *
>>> >> @@ -1636,6 +1671,11 @@ tb_link_page(TranslationBlock *tb,
>>> tb_page_addr_t phys_pc,
>>> >>          void *existing_tb = NULL;
>>> >>          uint32_t h;
>>> >>
>>> >> +        if (qemu_loglevel_mask(CPU_LOG_HOT_TBS)) {
>>> >> +            /* create and link to its TB a structure to store
>>> statistics */
>>> >> +            tb_insert_statistics_structure(tb);
>>> >> +            }
>>> >> +
>>> >>          /* add in the hash table */
>>> >>          h = tb_hash_func(phys_pc, tb->pc, tb->flags, tb->cflags &
>>> CF_HASH_MASK,
>>> >>                           tb->trace_vcpu_dstate);
>>> >
>>> > Perhaps we could just have a second qht array which allows for
>>> > "unlocked" insertion of record. You could take advantage of the fact the
>>> > hash would be the same:
>>> >
>>> >         h = tb_hash_func(phys_pc, tb->pc, tb->flags, tb->cflags &
>>> CF_HASH_MASK,
>>> >                          tb->trace_vcpu_dstate);
>>> >         qht_insert(&tb_ctx.htable, tb, h, &existing_tb);
>>> >
>>> >         /* remove TB from the page(s) if we couldn't insert it */
>>> >         if (unlikely(existing_tb)) {
>>> >             ...
>>> >             tb = existing_tb;
>>> >         } else if (qemu_loglevel_mask(CPU_LOG_HOT_TBS)) {
>>> >             TBStatistics *new_stats = g_new0(TBStatistics, 1);
>>> >             bool ok;
>>> >             new_stats->pc = tb->pc;
>>> >             new_stats->cs_base = tb->cs_base;
>>> >             new_stats->flags = tb->flags;
>>> >             new_stats->page_addr[0] = tb->page_addr[0];
>>> >             new_stats->page_addr[1] = tb->page_addr[1];
>>> >             ok = qht_insert(&tb_ctx.tb_stats, new_stats, h, NULL);
>>> >             /*
>>> >              * This should never fail as we succeeded in inserting the
>>> >              * TB itself which means we haven't seen this combination
>>> yet.
>>> >              */
>>> >             g_assert(ok);
>>> >         }
>>> >
>>> > It would also avoid the clunkiness of having to allocate and then
>>> > freeing an unused structure.
>>>
>>>
>>> Actually thinking on this we still have to handle it. We may have
>>> tb_flushed away a translation and just be regenerating the same block.
>>> As TBStatistics should transcend tb_flush events we need to re-use it's
>>> structure.
>>>
>>> It would be worth counting the regens as well so we can see blocks that
>>> keep getting re-translated after each flush.

Another issue is lifetime. We really want to have the statistics
available before we generate the code (so we can pass a direct pointer
to the helper) and we can't wait until after we qht_insert the TB into
the buffer (as any thread can start executing that buffer from that
point). So we need to ensure tb->tb_stats is live and ready as
translation time.

--
Alex Bennée