linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] mm, hugepages: fix size in hugetlb mremap() test
@ 2021-11-23 20:46 Yosry Ahmed
  2021-11-24  1:08 ` Mike Kravetz
  2021-11-24 20:38 ` [PATCH] mm, hugepages: make memory size variable in hugepage-mremap selftest Yosry Ahmed
  0 siblings, 2 replies; 6+ messages in thread
From: Yosry Ahmed @ 2021-11-23 20:46 UTC (permalink / raw)
  To: Andrew Morton, Shuah Khan
  Cc: linux-mm, linux-kselftest, linux-kernel, Mina Almasry, Yosry Ahmed

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(-)

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)
-- 
2.34.0.rc2.393.gf8c9666880-goog


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] mm, hugepages: fix size in hugetlb mremap() test
  2021-11-23 20:46 [PATCH] mm, hugepages: fix size in hugetlb mremap() test Yosry Ahmed
@ 2021-11-24  1:08 ` Mike Kravetz
  2021-11-24  2:19   ` Yosry Ahmed
  2021-11-24 20:38 ` [PATCH] mm, hugepages: make memory size variable in hugepage-mremap selftest Yosry Ahmed
  1 sibling, 1 reply; 6+ messages in thread
From: Mike Kravetz @ 2021-11-24  1:08 UTC (permalink / raw)
  To: Yosry Ahmed, Andrew Morton, Shuah Khan
  Cc: linux-mm, linux-kselftest, linux-kernel, Mina Almasry

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>
-- 
Mike Kravetz

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] mm, hugepages: fix size in hugetlb mremap() test
  2021-11-24  1:08 ` Mike Kravetz
@ 2021-11-24  2:19   ` Yosry Ahmed
  2021-11-24  3:42     ` Mike Kravetz
  0 siblings, 1 reply; 6+ messages in thread
From: Yosry Ahmed @ 2021-11-24  2:19 UTC (permalink / raw)
  To: Mike Kravetz
  Cc: Andrew Morton, Shuah Khan, linux-mm, linux-kselftest,
	linux-kernel, Mina Almasry

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] mm, hugepages: fix size in hugetlb mremap() test
  2021-11-24  2:19   ` Yosry Ahmed
@ 2021-11-24  3:42     ` Mike Kravetz
  2021-11-24 21:03       ` Mina Almasry
  0 siblings, 1 reply; 6+ messages in thread
From: Mike Kravetz @ 2021-11-24  3:42 UTC (permalink / raw)
  To: Yosry Ahmed
  Cc: Andrew Morton, Shuah Khan, linux-mm, linux-kselftest,
	linux-kernel, Mina Almasry

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!
-- 
Mike Kravetz

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] mm, hugepages: make memory size variable in hugepage-mremap selftest
  2021-11-23 20:46 [PATCH] mm, hugepages: fix size in hugetlb mremap() test Yosry Ahmed
  2021-11-24  1:08 ` Mike Kravetz
@ 2021-11-24 20:38 ` Yosry Ahmed
  1 sibling, 0 replies; 6+ messages in thread
From: Yosry Ahmed @ 2021-11-24 20:38 UTC (permalink / raw)
  To: Andrew Morton, Shuah Khan
  Cc: Mina Almasry, Mike Kravetz, linux-mm, linux-kselftest,
	linux-kernel, Yosry Ahmed

The hugetlb vma mremap() test currently maps 1GB of memory to trigger
pmd sharing and make sure that 'unshare' path in mremap code works.
The test originally only mapped 10MB of memory (as specified by the
header comment) but was later modified to 1GB to tackle this case.

However, not all machines will have 1GB of memory to spare for this
test. Adding a mapping size arg will allow run_vmtest.sh to pass an
adequate mapping size, while allowing users to run the test
independently with arbitrary size mappings.

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
---

This is a replacement for
mm-hugepages-fix-size-in-hugetlb-mremap-test.patch in -mm tree.

The original patch set the amount of memory used by the test to 10MB
instead of 1GB to avoid running out of memory. However, the test must
map 1GB or more to trigger pmd sharing. Hence, a variable mapping size
is the best solution here to allow for both options.

---
 tools/testing/selftests/vm/hugepage-mremap.c | 46 +++++++++++++-------
 tools/testing/selftests/vm/run_vmtests.sh    |  2 +-
 2 files changed, 31 insertions(+), 17 deletions(-)

diff --git a/tools/testing/selftests/vm/hugepage-mremap.c b/tools/testing/selftests/vm/hugepage-mremap.c
index 257df94697a5..2a7c33631a29 100644
--- a/tools/testing/selftests/vm/hugepage-mremap.c
+++ b/tools/testing/selftests/vm/hugepage-mremap.c
@@ -4,7 +4,11 @@
  *
  * Example of remapping huge page memory in a user application using the
  * mremap system call.  Code assumes a hugetlbfs filesystem is mounted
- * at './huge'.  The code will use 10MB worth of huge pages.
+ * at './huge'.  The amount of memory used by this test is decided by a command
+ * line argument in MBs. If missing, the default amount is 10MB.
+ *
+ * To make sure the test triggers pmd sharing and goes through the 'unshare'
+ * path in the mremap code use 1GB (1024) or more.
  */
 
 #define _GNU_SOURCE
@@ -18,8 +22,10 @@
 #include <linux/userfaultfd.h>
 #include <sys/ioctl.h>
 
-#define LENGTH (1UL * 1024 * 1024 * 1024)
+#define DEFAULT_LENGTH_MB 10UL
+#define MB_TO_BYTES(x) (x * 1024 * 1024)
 
+#define FILE_NAME "huge/hugepagefile"
 #define PROTECTION (PROT_READ | PROT_WRITE | PROT_EXEC)
 #define FLAGS (MAP_SHARED | MAP_ANONYMOUS)
 
@@ -28,20 +34,20 @@ static void check_bytes(char *addr)
 	printf("First hex is %x\n", *((unsigned int *)addr));
 }
 
-static void write_bytes(char *addr)
+static void write_bytes(char *addr, size_t len)
 {
 	unsigned long i;
 
-	for (i = 0; i < LENGTH; i++)
+	for (i = 0; i < len; i++)
 		*(addr + i) = (char)i;
 }
 
-static int read_bytes(char *addr)
+static int read_bytes(char *addr, size_t len)
 {
 	unsigned long i;
 
 	check_bytes(addr);
-	for (i = 0; i < LENGTH; i++)
+	for (i = 0; i < len; i++)
 		if (*(addr + i) != (char)i) {
 			printf("Mismatch at %lu\n", i);
 			return 1;
@@ -99,11 +105,19 @@ static void register_region_with_uffd(char *addr, size_t len)
 	}
 }
 
-int main(void)
+int main(int argc, char *argv[])
 {
+	/* Read memory length as the first arg if valid, otherwise fallback to
+	 * the default length. Any additional args are ignored.
+	 */
+	size_t length = argc > 1 ? (size_t)atoi(argv[1]) : 0UL;
+
+	length = length > 0 ? length : DEFAULT_LENGTH_MB;
+	length = MB_TO_BYTES(length);
+
 	int ret = 0;
 
-	int fd = open("/huge/test", O_CREAT | O_RDWR, 0755);
+	int fd = open(FILE_NAME, O_CREAT | O_RDWR, 0755);
 
 	if (fd < 0) {
 		perror("Open failed");
@@ -112,7 +126,7 @@ int main(void)
 
 	/* mmap to a PUD aligned address to hopefully trigger pmd sharing. */
 	unsigned long suggested_addr = 0x7eaa40000000;
-	void *haddr = mmap((void *)suggested_addr, LENGTH, PROTECTION,
+	void *haddr = mmap((void *)suggested_addr, length, PROTECTION,
 			   MAP_HUGETLB | MAP_SHARED | MAP_POPULATE, fd, 0);
 	printf("Map haddr: Returned address is %p\n", haddr);
 	if (haddr == MAP_FAILED) {
@@ -122,7 +136,7 @@ int main(void)
 
 	/* mmap again to a dummy address to hopefully trigger pmd sharing. */
 	suggested_addr = 0x7daa40000000;
-	void *daddr = mmap((void *)suggested_addr, LENGTH, PROTECTION,
+	void *daddr = mmap((void *)suggested_addr, length, PROTECTION,
 			   MAP_HUGETLB | MAP_SHARED | MAP_POPULATE, fd, 0);
 	printf("Map daddr: Returned address is %p\n", daddr);
 	if (daddr == MAP_FAILED) {
@@ -132,16 +146,16 @@ int main(void)
 
 	suggested_addr = 0x7faa40000000;
 	void *vaddr =
-		mmap((void *)suggested_addr, LENGTH, PROTECTION, FLAGS, -1, 0);
+		mmap((void *)suggested_addr, length, PROTECTION, FLAGS, -1, 0);
 	printf("Map vaddr: Returned address is %p\n", vaddr);
 	if (vaddr == MAP_FAILED) {
 		perror("mmap2");
 		exit(1);
 	}
 
-	register_region_with_uffd(haddr, LENGTH);
+	register_region_with_uffd(haddr, length);
 
-	void *addr = mremap(haddr, LENGTH, LENGTH,
+	void *addr = mremap(haddr, length, length,
 			    MREMAP_MAYMOVE | MREMAP_FIXED, vaddr);
 	if (addr == MAP_FAILED) {
 		perror("mremap");
@@ -150,10 +164,10 @@ int main(void)
 
 	printf("Mremap: Returned address is %p\n", addr);
 	check_bytes(addr);
-	write_bytes(addr);
-	ret = read_bytes(addr);
+	write_bytes(addr, length);
+	ret = read_bytes(addr, length);
 
-	munmap(addr, LENGTH);
+	munmap(addr, length);
 
 	return ret;
 }
diff --git a/tools/testing/selftests/vm/run_vmtests.sh b/tools/testing/selftests/vm/run_vmtests.sh
index a24d30af3094..75d401741394 100755
--- a/tools/testing/selftests/vm/run_vmtests.sh
+++ b/tools/testing/selftests/vm/run_vmtests.sh
@@ -111,7 +111,7 @@ fi
 echo "-----------------------"
 echo "running hugepage-mremap"
 echo "-----------------------"
-./hugepage-mremap
+./hugepage-mremap 256
 if [ $? -ne 0 ]; then
 	echo "[FAIL]"
 	exitcode=1
-- 
2.34.0.rc2.393.gf8c9666880-goog


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] mm, hugepages: fix size in hugetlb mremap() test
  2021-11-24  3:42     ` Mike Kravetz
@ 2021-11-24 21:03       ` Mina Almasry
  0 siblings, 0 replies; 6+ messages in thread
From: Mina Almasry @ 2021-11-24 21:03 UTC (permalink / raw)
  To: Mike Kravetz
  Cc: Yosry Ahmed, Andrew Morton, Shuah Khan, linux-mm,
	linux-kselftest, linux-kernel

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!

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-11-24 21:03 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-23 20:46 [PATCH] mm, hugepages: fix size in hugetlb mremap() test Yosry Ahmed
2021-11-24  1:08 ` Mike Kravetz
2021-11-24  2:19   ` Yosry Ahmed
2021-11-24  3:42     ` Mike Kravetz
2021-11-24 21:03       ` Mina Almasry
2021-11-24 20:38 ` [PATCH] mm, hugepages: make memory size variable in hugepage-mremap selftest Yosry Ahmed

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).