| Message ID | 20211123204644.3458700-1-yosryahmed@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm, hugepages: fix size in hugetlb mremap() test | expand |
On 11/23/21 12:46, Yosry Ahmed wrote: > The hugetlb vma mremap() test mentions in the header comment that it > uses 10MB worth of huge pages, when it actually uses 1GB. This causes > the test to fail on devices with smaller memories. > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > --- > tools/testing/selftests/vm/hugepage-mremap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) I'll let Mina comment, but I think I know what happened. The original version of the test did indeed use 10MB. However, the mremap code must 'unshare' and shared pmd mappings before remapping. Since sharing requires mappings of at least 1GB, the size was changed to make sure unsharing worked. In the end, I believe I suggested adding hugepage-mremap to run_vmtests.sh. The script does not try to configure a GB worth of huge pages. And, I think it is somewhat unreasonable to suggest users gave a spare GB to run the test. I'm OK with restoring the original value. Acked-by: Mike Kravetz <mike.kravetz@oracle.com>
On Tue, Nov 23, 2021 at 5:08 PM Mike Kravetz <mike.kravetz@oracle.com> wrote: > > On 11/23/21 12:46, Yosry Ahmed wrote: > > The hugetlb vma mremap() test mentions in the header comment that it > > uses 10MB worth of huge pages, when it actually uses 1GB. This causes > > the test to fail on devices with smaller memories. > > > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > > --- > > tools/testing/selftests/vm/hugepage-mremap.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > I'll let Mina comment, but I think I know what happened. Thanks for taking the time to review this and explain what happened. > > > The original version of the test did indeed use 10MB. However, the mremap > code must 'unshare' and shared pmd mappings before remapping. Since sharing > requires mappings of at least 1GB, the size was changed to make sure unsharing > worked. > > In the end, I believe I suggested adding hugepage-mremap to run_vmtests.sh. > The script does not try to configure a GB worth of huge pages. And, I think > it is somewhat unreasonable to suggest users gave a spare GB to run the test. Alternatively, we can pass an optional argument to the test that makes it use 1GB instead of 10MB. This way, if the test is run with run_vmtests.sh the default behavior would be to use 10MB, making sure users do not run out of memory. Otherwise, an interested user could run the test without run_vmtest.sh and provide the extra argument to make the test use 1GB and make sure that unsharing works correctly. Thoughts? > > I'm OK with restoring the original value. > > Acked-by: Mike Kravetz <mike.kravetz@oracle.com> > -- > Mike Kravetz
On 11/23/21 18:19, Yosry Ahmed wrote: > On Tue, Nov 23, 2021 at 5:08 PM Mike Kravetz <mike.kravetz@oracle.com> wrote: >> >> On 11/23/21 12:46, Yosry Ahmed wrote: >>> The hugetlb vma mremap() test mentions in the header comment that it >>> uses 10MB worth of huge pages, when it actually uses 1GB. This causes >>> the test to fail on devices with smaller memories. >>> >>> Signed-off-by: Yosry Ahmed <yosryahmed@google.com> >>> --- >>> tools/testing/selftests/vm/hugepage-mremap.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> I'll let Mina comment, but I think I know what happened. > > Thanks for taking the time to review this and explain what happened. > >> >> >> The original version of the test did indeed use 10MB. However, the mremap >> code must 'unshare' and shared pmd mappings before remapping. Since sharing >> requires mappings of at least 1GB, the size was changed to make sure unsharing >> worked. >> >> In the end, I believe I suggested adding hugepage-mremap to run_vmtests.sh. >> The script does not try to configure a GB worth of huge pages. And, I think >> it is somewhat unreasonable to suggest users gave a spare GB to run the test. > > Alternatively, we can pass an optional argument to the test that makes it use > 1GB instead of 10MB. This way, if the test is run with run_vmtests.sh the > default behavior would be to use 10MB, making sure users do not run out of > memory. Otherwise, an interested user could run the test without run_vmtest.sh > and provide the extra argument to make the test use 1GB and make sure that > unsharing works correctly. Thoughts? > Passing a 'mapping size' argument as you suggest would be best. That way run_vmtest.sh can pass in a size such as 10MB, but the test could be used independently with arbitrary size mappings. If you have the time to do this, go for it!
On Tue, Nov 23, 2021 at 7:42 PM Mike Kravetz <mike.kravetz@oracle.com> wrote: > > On 11/23/21 18:19, Yosry Ahmed wrote: > > On Tue, Nov 23, 2021 at 5:08 PM Mike Kravetz <mike.kravetz@oracle.com> wrote: > >> > >> On 11/23/21 12:46, Yosry Ahmed wrote: > >>> The hugetlb vma mremap() test mentions in the header comment that it > >>> uses 10MB worth of huge pages, when it actually uses 1GB. This causes > >>> the test to fail on devices with smaller memories. > >>> > >>> Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > >>> --- > >>> tools/testing/selftests/vm/hugepage-mremap.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> I'll let Mina comment, but I think I know what happened. > > > > Thanks for taking the time to review this and explain what happened. > > > >> > >> > >> The original version of the test did indeed use 10MB. However, the mremap > >> code must 'unshare' and shared pmd mappings before remapping. Since sharing > >> requires mappings of at least 1GB, the size was changed to make sure unsharing > >> worked. > >> > >> In the end, I believe I suggested adding hugepage-mremap to run_vmtests.sh. > >> The script does not try to configure a GB worth of huge pages. And, I think > >> it is somewhat unreasonable to suggest users gave a spare GB to run the test. > > > > Alternatively, we can pass an optional argument to the test that makes it use > > 1GB instead of 10MB. This way, if the test is run with run_vmtests.sh the > > default behavior would be to use 10MB, making sure users do not run out of > > memory. Otherwise, an interested user could run the test without run_vmtest.sh > > and provide the extra argument to make the test use 1GB and make sure that > > unsharing works correctly. Thoughts? > > > > Passing a 'mapping size' argument as you suggest would be best. That way > run_vmtest.sh can pass in a size such as 10MB, but the test could be used > independently with arbitrary size mappings. > > If you have the time to do this, go for it! Yes, Mike's recollection of events makes sense to me. I think that was the mistake that happened (sorry!) Making it configurable makes sense to me. I'm out for the rest of the week but I'll try to review sooner if possible. Thanks for looking into this Yosry!
diff --git a/tools/testing/selftests/vm/hugepage-mremap.c b/tools/testing/selftests/vm/hugepage-mremap.c index 257df94697a5..551e68f97926 100644 --- a/tools/testing/selftests/vm/hugepage-mremap.c +++ b/tools/testing/selftests/vm/hugepage-mremap.c @@ -18,7 +18,7 @@ #include <linux/userfaultfd.h> #include <sys/ioctl.h> -#define LENGTH (1UL * 1024 * 1024 * 1024) +#define LENGTH (10UL * 1024 * 1024) #define PROTECTION (PROT_READ | PROT_WRITE | PROT_EXEC) #define FLAGS (MAP_SHARED | MAP_ANONYMOUS)
The hugetlb vma mremap() test mentions in the header comment that it uses 10MB worth of huge pages, when it actually uses 1GB. This causes the test to fail on devices with smaller memories. Signed-off-by: Yosry Ahmed <yosryahmed@google.com> --- tools/testing/selftests/vm/hugepage-mremap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)