Re: patch: qemu + hugetlbfs..

[john cooper <john.cooper@third-harmonic.com>]

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