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

[Tarun Sahu <tsahu@linux.ibm.com>]

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.
> > +		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

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