From mboxrd@z Thu Jan 1 00:00:00 1970 From: john cooper Subject: Re: patch: qemu + hugetlbfs.. Date: Tue, 08 Jul 2008 20:23:54 -0400 Message-ID: <4874051A.8000802@third-harmonic.com> References: <4873E400.4000409@third-harmonic.com> <4873F395.6030209@codemonkey.ws> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, mtosatti@redhat.com, john.cooper@redhat.com, john cooper To: Anthony Liguori Return-path: Received: from dpc691978010.direcpc.com ([69.19.78.10]:39887 "EHLO anvil.third-harmonic.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751580AbYGIAfh (ORCPT ); Tue, 8 Jul 2008 20:35:37 -0400 In-Reply-To: <4873F395.6030209@codemonkey.ws> Sender: kvm-owner@vger.kernel.org List-ID: Anthony Liguori wrote: >> +/* assertion checking >> + */ >> +#define ASSERT(c) if (!(c)) _assert(#c, __FILE__, __LINE__) >> + >> +static inline void _assert(char *text, char *file, int line) { >> + fprintf(stderr, "ASSERTION FAILED: [%s] %s:%d\n", text, file, line); >> + kill(getpid(), 12); /* dump core */ >> +} >> + >> > > Is it really necessary to add this as part of this patch? No, certainly not strictly necessary. >> +int mem_fallback = {0}; /* allow alloc from alternate page size */ >> > This is a bizarre way to initialize an integer. I had no idea you could > even do this for a scalar. Just initialize to 1 and 0. That's historically been valid syntax since pre-K&R. But certainly can be adapted to the context style if it raises concern. >> -void *alloc_mem_area(unsigned long memory, const char *path) >> +/* fault in associated page, fail syscall when free page is unavailable >> + */ +static inline int fault_in(void *a) >> +{ >> + return (gettimeofday(a, NULL) < 0 ? errno != EFAULT : 1); >> +} >> > > I don't like this very much. Why not just MAP_POPULATE? I like it even less. MAP_POPULATE does not fault in physical hpages to the map. Again this was a qemu-side interim bandaid. >> +/* we failed to fault in hpage *a, fall back to conventional page >> mapping >> + */ >> +int remap_hpage(void *a, int sz) >> +{ >> + ASSERT(!(sz & (EXEC_PAGESIZE - 1))); >> + if (munmap(a, sz) < 0) >> + perror("remap_hpage: munmap"); >> + else if (mmap(a, sz, PROT_READ|PROT_WRITE, >> + MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED, -1, 0) == MAP_FAILED) >> + perror("remap_hpage: mmap"); >> + else >> + return (1); >> + return (0); >> +} >> > > I think this would be simplified with MAP_POPULATE since you can fail in > large chunks of memory instead of potentially having a highly fragmented > set of VMAs. Here for 4K pages we only need to setup the map. If we later fault on a physically absent 4K page we'll wait if a page isn't immediately available. Rather in the case of a hpage being unavailable, we'll terminate. Note at this point we've effectively locked onto whatever hpages we've been able to map as they can't be reclaimed from us until we exit. Also it appears tab formatting in this patch may need to be scrubbed some as the mailers seem to be jostling the whitespace. >> +int bool_arg(const char *optarg, const char *flag_name) >> +{ >> + if (!optarg) >> + ; >> + else if (altmatch("y|yes|1", optarg)) >> + return (1); >> + else if (altmatch("n|no|0", optarg)) >> + return (0); >> + fprintf(stderr, "usage: %s [0|1]\n", flag_name); >> + exit(1); >> } >> + > > This is introducing too much infrastructure. Just follow the > conventions in the rest of the code. No need to make the options take a > boolean argument. The options themselves are boolean arguments. That's how it originally existed. However with the defaults different for the two flags it seemed a bit clunky to have one recall what the initial disposition of the option was and to toggle its behavior if that wasn't the intention. But admittedly I don't have a strong affinity either way. I was only concerned with usability and clarity of the flags. -john -- john.cooper@third-harmonic.com