Re: [Qemu-devel] [PATCH v3 42/43] tcg: introduce regions to split code_gen_buffer

["Emilio G. Cota" <cota@braap.org>]

On Wed, Jul 19, 2017 at 22:04:50 -1000, Richard Henderson wrote:
> On 07/19/2017 05:09 PM, Emilio G. Cota wrote:
> >+    /* We do not yet support multiple TCG contexts, so use one region for now */
> >+    n_regions = 1;
> >+
> >+    /* start on a page-aligned address */
> >+    buf = QEMU_ALIGN_PTR_UP(buf, qemu_real_host_page_size);
> >+    g_assert(buf < tcg_init_ctx.code_gen_buffer + size);
> >+
> >+    /* discard that initial portion */
> >+    size -= buf - tcg_init_ctx.code_gen_buffer;
> 
> It seems pointless wasting most of a page after the prologue when n_regions
> == 1.  We don't really need to start on a page boundary in that case.
> 
> >+    /* make region_size a multiple of page_size */
> >+    region_size = size / n_regions;
> >+    region_size = QEMU_ALIGN_DOWN(region_size, qemu_real_host_page_size);
> 
> This division can result in a number of pages at the end of the region being
> unused.  Is it worthwhile freeing them?  Or marking them mprotect_none along
> with the last guard page?

Perhaps we should then enlarge both the first and last regions so that we
fully use the buffer.

What do you think of the below? It's a delta over the v3 patch.

		Emilio

---8<---

diff --git a/tcg/tcg.c b/tcg/tcg.c
index a5c01be..8bf8bca 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -129,7 +129,8 @@ static unsigned int n_tcg_ctxs;
  */
 struct tcg_region_state {
     QemuMutex lock;
-    void *buf;      /* set at init time */
+    void *start;    /* set at init time */
+    void *end;      /* set at init time */
     size_t n;       /* set at init time */
     size_t size;    /* size of one region; set at init time */
     size_t current; /* protected by the lock */
@@ -277,13 +278,27 @@ TCGLabel *gen_new_label(void)
 
 static void tcg_region_assign(TCGContext *s, size_t curr_region)
 {
+    void *aligned = QEMU_ALIGN_PTR_UP(region.start, qemu_real_host_page_size);
     void *buf;
+    size_t size = region.size;
 
-    buf = region.buf + curr_region * (region.size + qemu_real_host_page_size);
+    /* The beginning of the first region might not be page-aligned */
+    if (curr_region == 0) {
+        buf = region.start;
+        size += aligned - buf;
+    } else {
+        buf = aligned + curr_region * (region.size +  qemu_real_host_page_size);
+    }
+    /* the last region might be larger than region.size */
+    if (curr_region == region.n - 1) {
+        void *aligned_end = buf + size;
+
+        size += region.end - qemu_real_host_page_size - aligned_end;
+    }
     s->code_gen_buffer = buf;
     s->code_gen_ptr = buf;
-    s->code_gen_buffer_size = region.size;
-    s->code_gen_highwater = buf + region.size - TCG_HIGHWATER;
+    s->code_gen_buffer_size = size;
+    s->code_gen_highwater = buf + size - TCG_HIGHWATER;
 }
 
 static bool tcg_region_alloc__locked(TCGContext *s)
@@ -409,44 +424,50 @@ static size_t tcg_n_regions(void)
 void tcg_region_init(void)
 {
     void *buf = tcg_init_ctx.code_gen_buffer;
+    void *aligned;
     size_t size = tcg_init_ctx.code_gen_buffer_size;
+    size_t page_size = qemu_real_host_page_size;
     size_t region_size;
     size_t n_regions;
     size_t i;
+    int rc;
 
     n_regions = tcg_n_regions();
 
-    /* start on a page-aligned address */
-    buf = QEMU_ALIGN_PTR_UP(buf, qemu_real_host_page_size);
-    g_assert(buf < tcg_init_ctx.code_gen_buffer + size);
-
-    /* discard that initial portion */
-    size -= buf - tcg_init_ctx.code_gen_buffer;
-
-    /* make region_size a multiple of page_size */
-    region_size = size / n_regions;
-    region_size = QEMU_ALIGN_DOWN(region_size, qemu_real_host_page_size);
+    /* The first region will be 'aligned - buf' bytes larger than the others */
+    aligned = QEMU_ALIGN_PTR_UP(buf, page_size);
+    g_assert(aligned < tcg_init_ctx.code_gen_buffer + size);
+    /*
+     * Make region_size a multiple of page_size, using aligned as the start.
+     * As a result of this we might end up with a few extra pages at the end of
+     * the buffer; we will assign those to the last region.
+     */
+    region_size = (size - (aligned - buf)) / n_regions;
+    region_size = QEMU_ALIGN_DOWN(region_size, page_size);
 
     /* A region must have at least 2 pages; one code, one guard */
-    g_assert(region_size >= 2 * qemu_real_host_page_size);
+    g_assert(region_size >= 2 * page_size);
 
     /* init the region struct */
     qemu_mutex_init(&region.lock);
     region.n = n_regions;
-    region.buf = buf;
+    region.start = buf;
+    /* page-align the end, since its last page will be a guard page */
+    region.end = QEMU_ALIGN_PTR_DOWN(buf + size, page_size);
     /* do not count the guard page in region.size */
-    region.size = region_size - qemu_real_host_page_size;
+    region.size = region_size - page_size;
 
-    /* set guard pages */
-    for (i = 0; i < region.n; i++) {
+    /* set guard pages for the first n-1 regions */
+    for (i = 0; i < region.n - 1; i++) {
         void *guard;
-        int rc;
 
-        guard = region.buf + region.size;
-        guard += i * (region.size + qemu_real_host_page_size);
-        rc = qemu_mprotect_none(guard, qemu_real_host_page_size);
+        guard = aligned + region.size + i * (region.size + page_size);
+        rc = qemu_mprotect_none(guard, page_size);
         g_assert(!rc);
     }
+    /* the last region gets the rest of the buffer */
+    rc = qemu_mprotect_none(region.end - page_size, page_size);
+    g_assert(!rc);
 
     /* In user-mode we support only one ctx, so do the initial allocation now */
 #ifdef CONFIG_USER_ONLY