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

[Richard Palethorpe <rpalethorpe@suse.de>]

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.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp