Re: [LTP] [PATCH 1/8] Hugetlb: Migrating libhugetlbfs fork-cow

Richard Palethorpe
2022-12-08T08:55:35Z

Hello,

Tarun Sahu <tsahu@linux.ibm.com> writes:

> Hi,
> Thanks for reviewing it.
> On Dec 05 2022, Richard Palethorpe wrote:
>> Hello,
>>
>> Tarun Sahu <tsahu@linux.ibm.com> writes:
>>
>> > +
>> > +#include <sys/wait.h>
>> > +#include <sys/mman.h>
>> > +#include <stdlib.h>
>> > +#include <unistd.h>
>> > +#include <sys/types.h>
>> > +
>> > +#include "hugetlb.h"
>> > +
>> > +#define RANDOM_CONSTANT 0x1234ABCD
>> > +#define OTHER_CONSTANT 0xfeef5678
>>
>> It seems their are actually 3 constants as "random" is inverted. I'd
>> prefer it if they had names like C1, C2, C3 with no connotations.
>>
> Yeah, Will be more cleaner, Will update it.
>
> --skip
>> > +
>> > + SAFE_WAITPID(pid, &status, 0);
>> > + if (WEXITSTATUS(status) != 0) {
>> > + tst_res(TFAIL, "Child failed: %d", WEXITSTATUS(status));
>> > + goto cleanup;
>> > + }
>>
>> This can be replaced with tst_reap_children();
> Ok. I will update it in v2.
>>
>> > +
>> > + tst_res(TINFO, "child_readback = 0x%x, parent_readback = 0x%x",
>> > + *child_readback, parent_readback);
>> > +
>> > + if (*child_readback != RANDOM_CONSTANT) {
>> > + tst_res(TFAIL, "Child read back 0x%x instead of 0x%x",
>> > + *child_readback, RANDOM_CONSTANT);
>>
>> I think this could be checked at the end of the child and the extra mmap
>> for child_readback removed. The LTP lib already creats some shared
>> memory with children to propagate results.
>>
>> Assuming that mmap is not needed for the original bug reproducer.
>>
> Looking at the fix: 86df86424939d316 ("Correct hash flushing from huge_ptep_set_wrprotect()")
> seems it can be done. I will look at it and will update it in v2.

Thanks and as you have the git commit hash, please add this to the tst_test.tags.

>> > + goto cleanup;
>>
>> I don't think this is necessary.
>>
>> > + }
>> > + if (parent_readback != ~RANDOM_CONSTANT) {
>>
>> These comparisons could be replaced with TST_EXP_EQ_LU or TST_EXP_EXPR.
>>
> Ok. Will update it.
>> --
>> Thank you,
>> Richard.
>>
>> --
>> Mailing list info: https://lists.linux.it/listinfo/ltp

--
Thank you,
Richard.