Re: [Qemu-devel] [PATCH 20/22] tcg: dynamically allocate from code_gen_buffer using equally-sized regions

[Richard Henderson]

On 07/08/2017 09:50 PM, Emilio G. Cota wrote:
> +static void code_gen_set_region_size(TCGContext *s)
> +{
> +    size_t per_cpu = s->code_gen_buffer_size / smp_cpus;
> +    size_t div;
> +
> +    assert(per_cpu);
> +    /*
> +     * Use a single region if all we have is one vCPU.
> +     * We could also use a single region with !mttcg, but at this time we have
> +     * not yet processed the thread=single|multi flag.
> +     */
> +    if (smp_cpus == 1) {
> +        tcg_region_set_size(0);
> +        return;
> +    }
> +
> +    for (div = 8; div > 0; div--) {
> +        size_t region_size = per_cpu / div;
> +
> +        if (region_size >= 2 * MIN_CODE_GEN_BUFFER_SIZE) {
> +            tcg_region_set_size(region_size);
> +            return;
> +        }
> +    }
> +    tcg_region_set_size(per_cpu);
> +}

Is it worth setting a guard page after each of these regions?  The guard page 
on the main buffer has caught bugs before (although not in a while now).  If 
not, then we might drop the guard page on the main buffer too.

> +static void tcg_region_set_size__locked(size_t size)
> +{
> +    if (!size) {
> +        region.size = tcg_init_ctx->code_gen_buffer_size;
> +        region.n = 1;
> +    } else {
> +        region.size = size;
> +        region.n = tcg_init_ctx->code_gen_buffer_size / size;
> +    }
> +    if (unlikely(region.size < TCG_HIGHWATER)) {
> +        tcg_abort();
> +    }
> +}
> +
> +/*
> + * Call this function at init time (i.e. only once). Calling this function is
> + * optional: if no region size is set, a single region will be used.
> + *
> + * Note: calling this function *after* calling tcg_region_init() is a bug.
> + */
> +void tcg_region_set_size(size_t size)
> +{
> +    tcg_debug_assert(!region.size);
> +
> +    qemu_mutex_lock(&tcg_lock);
> +    tcg_region_set_size__locked(size);
> +    qemu_mutex_unlock(&tcg_lock);
> +}

If this is called during init, then why does it need a lock?  Surely this is 
before we spawn threads.

>      if (unlikely(next > s->code_gen_highwater)) {
> -        return NULL;
> +        if (!tcg_region_alloc(s)) {
> +            return NULL;
> +        }
> +        goto retry;
>      }

Nit: positive logic is almost always clearer: if (region_alloc) goto retry;


r~