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

Tarun Sahu
2022-12-07T12:48:29Z

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