[dpdk-dev] [PATCH v2] eal: make hugetlb initialization more robust

[Tan, Jianfeng]

Hi Sergio,


On 5/4/2016 7:07 PM, Sergio Gonzalez Monroy wrote:
> On 08/03/2016 01:42, Jianfeng Tan wrote:
>> This patch adds an option, --huge-trybest, to use a recover mechanism to
>> the case that there are not so many hugepages (declared in sysfs), which
>> can be used. It relys on a mem access to fault-in hugepages, and if 
>> fails
>> with SIGBUS, recover to previously saved stack environment with
>> siglongjmp().
>>
>> Test example:
>>    a. cgcreate -g hugetlb:/test-subgroup
>>    b. cgset -r hugetlb.1GB.limit_in_bytes=2147483648 test-subgroup
>>    c. cgexec -g hugetlb:test-subgroup \
>>       ./examples/helloworld/build/helloworld -c 0x2 -n 4 --huge-trybest
>
> I think you should mention in the commit message that this option also 
> covers the case
> of hugetlbfs mount with quota.

Yes, I should do that.

>
>>   +static sigjmp_buf jmpenv;
>> +
>> +static void sigbus_handler(int signo __rte_unused)
>> +{
>> +    siglongjmp(jmpenv, 1);
>> +}
>> +
>> +/* Put setjmp into a wrap method to avoid compiling error. Any 
>> non-volatile,
>> + * non-static local variable in the stack frame calling setjmp might be
>> + * clobbered by a call to longjmp.
>> + */
>> +static int wrap_setjmp(void)
>> +{
>> +    return setjmp(jmpenv);
>> +}
>
> Use sigsetjmp instead of setjmp and restore the signal masks.

The difference lies in whether signal mask will be saved for further 
restore. And you are right we should keep either sigsetjmp(xxx, 
1)/siglongjmp(xxx, 1) or setjmp()/longjmp. Nice catch! I'll go with the 
former.

>
>>   /*
>>    * Mmap all hugepages of hugepage table: it first open a file in
>>    * hugetlbfs, then mmap() hugepage_sz data in it. If orig is set, the
>> @@ -396,7 +413,7 @@ map_all_hugepages(struct hugepage_file *hugepg_tbl,
>>           if (fd < 0) {
>>               RTE_LOG(ERR, EAL, "%s(): open failed: %s\n", __func__,
>>                       strerror(errno));
>> -            return -1;
>> +            return i;
>
> When using --try-best, we could get an error and still work as expected.
> It can be confusing for users to see an error when it is expected 
> behavior.
>
> Any thoughts?

Shall we remove those RTE_LOG complaints, because the failure here does 
not mean we cannot satisfy the requirements of applications?

...
> There is another call to map_all_hugepages that you are not updating 
> the check of the return value.

You are right, the second map_all_hugepages's return value check should 
be changed to compare with the total hugepages owned by the hpi. I'll 
fix this.

Thanks,
Jianfeng

>
> Sergio
>