From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34291) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZeqDX-0007lj-Lf for qemu-devel@nongnu.org; Wed, 23 Sep 2015 16:00:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZeqDU-0000qU-7x for qemu-devel@nongnu.org; Wed, 23 Sep 2015 16:00:39 -0400 Received: from mail-io0-x232.google.com ([2607:f8b0:4001:c06::232]:35872) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZeqDT-0000qC-Mf for qemu-devel@nongnu.org; Wed, 23 Sep 2015 16:00:36 -0400 Received: by ioii196 with SMTP id i196so54853781ioi.3 for ; Wed, 23 Sep 2015 13:00:35 -0700 (PDT) Sender: Richard Henderson References: <1442953507-4074-1-git-send-email-rth@twiddle.net> <1442953507-4074-25-git-send-email-rth@twiddle.net> From: Richard Henderson Message-ID: <560304E0.3020907@twiddle.net> Date: Wed, 23 Sep 2015 13:00:32 -0700 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 24/25] tcg: Allocate a guard page after code_gen_buffer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: =?UTF-8?Q?Alex_Benn=c3=a9e?= , QEMU Developers , Aurelien Jarno On 09/23/2015 12:39 PM, Peter Maydell wrote: >> +# ifdef _WIN32 > > Why the space before ifdef here ? #ifdef USE_STATIC_CODE_GEN_BUFFER # ifdef _WIN32 # else # endif /* WIN32 */ #elif defined(_WIN32) #else #endif It's something that glibc requires for its coding style, and I find myself using it most of the time. >> +static inline void do_protect(void *addr, long size, int prot) >> +{ >> + DWORD old_protect; >> + VirtualProtect(addr, size, PAGE_EXECUTE_READWRITE, &old_protect); > > The 'prot' argument isn't used -- did you mean to pass it > in as VirtualProtect argument 3 ? Oops, yes. >> static inline void *alloc_code_gen_buffer(void) >> { >> void *buf = static_code_gen_buffer; >> + size_t full_size, size; >> + >> + /* The size of the buffer, rounded down to end on a page boundary. */ >> + full_size = (((uintptr_t)buf + sizeof(static_code_gen_buffer)) >> + & qemu_real_host_page_mask) - (uintptr_t)buf; >> + >> + /* Reserve a guard page. */ >> + size = full_size - qemu_real_host_page_size; >> + >> + /* Honor a command-line option limiting the size of the buffer. */ >> + if (size > tcg_ctx.code_gen_buffer_size) { >> + size = (((uintptr_t)buf + tcg_ctx.code_gen_buffer_size) >> + & qemu_real_host_page_mask) - (uintptr_t)buf; >> + } >> + tcg_ctx.code_gen_buffer_size = size; >> + >> #ifdef __mips__ >> - if (cross_256mb(buf, tcg_ctx.code_gen_buffer_size)) { >> - buf = split_cross_256mb(buf, tcg_ctx.code_gen_buffer_size); >> + if (cross_256mb(buf, size)) { >> + buf = split_cross_256mb(buf, size); >> + size = tcg_ctx.code_gen_buffer_size; >> } >> #endif >> - map_exec(buf, tcg_ctx.code_gen_buffer_size); >> + >> + map_exec(buf, size); >> + map_none(buf + size, qemu_real_host_page_size); >> + qemu_madvise(buf, size, QEMU_MADV_HUGEPAGE); > > I think we're now doing the MADV_HUGEPAGE over "buffer size > minus a page" rather than "buffer size". Does that mean > we've gone from doing the madvise on a whole number of > hugepages to doing it on something that's not a whole number > of hugepages, and if so does the kernel decide not to use > hugepages here? On the whole I don't think it matters. The static buffer isn't page aligned to begin with, much less hugepage aligned, so the fact that we're allocating a round number like 32mb here doesn't really mean much. The beginning and/or end pages of the buffer definitely aren't going to be hugepage. Worse, the same is true for the mmap path, since I've never seen the kernel select a hugepage aligned address. You'd think that adding MAP_HUGEPAGE would be akin to MADV_HUGEPAGE, with the additional hint that the address should be appropriately aligned for the hugepage, but no. It implies forced use of something from the hugepage pool and that requires extra suid capabilities. I've wondered about over-allocating on the mmap path, so that we can choose the hugepage aligned subregion. But as far as I can tell, my kernel doesn't allocate hugepages at all, no matter what we do. So it seems a little silly to go so far out of the way to get an aligned buffer. r~