Skip to contentSkip to conversation

Bug Report: diotest4 fails on mips64

LTP Hongzhi 2 participants 9 messages Jun 28, 2019 – Jul 4, 2019

lore
Share & tools
lore

Hongzhi

#0 lore raw

*Ltp:*

latest master

*Kernel:*

After v5.1 ARCH=mips64

*Error info:*

diotest4 10 TBROK : diotest4.c:368: can't mmap file: Invalid argument diotest4 11 TBROK : diotest4.c:368: Remaining cases broken

I think the first argument of mmap is invalid.

?? >shm_base = (char *)(((long)sbrk(0) + (shmsz - 1)) & ~(shmsz - 1));

?? >shm_base = mmap(shm_base, 0x100000, PROT_READ | PROT_WRITE,

Regards,

--Hongzhi

#1 lore raw

----- Original Message -----

Quoted 16 lines

*Ltp:*

latest master

*Kernel:*

After v5.1 ARCH=mips64

*Error info:*

diotest4 10 TBROK : diotest4.c:368: can't mmap file: Invalid argument diotest4 11 TBROK : diotest4.c:368: Remaining cases broken

Quoted 5 lines

I think the first argument of mmap is invalid.

?? >shm_base = (char *)(((long)sbrk(0) + (shmsz - 1)) & ~(shmsz - 1));

?? >shm_base = mmap(shm_base, 0x100000, PROT_READ | PROT_WRITE,

I don't see any note why that mmap needs to be MAP_FIXED. I'd drop it, let kernel pick an address:

testcases/kernel/io/direct_io/diotest4.c +2 -6
index e4616e400abd..bf200cd41a27 100644
@@ -352,18 +352,14 @@ int main(int argc, char *argv[])
        total++;
 
        /* Test-10: read, write to a mmaped file */
-       shm_base = (char *)(((long)sbrk(0) + (shmsz - 1)) & ~(shmsz - 1));
-       if (shm_base == NULL) {
-               tst_brkm(TBROK, cleanup, "sbrk failed: %s", strerror(errno));
-       }
        offset = 4096;
        count = bufsize;
        if ((fd = open(filename, O_DIRECT | O_RDWR)) < 0) {
                tst_brkm(TBROK, cleanup, "can't open %s: %s",
                         filename, strerror(errno));
        }
-       shm_base = mmap(shm_base, 0x100000, PROT_READ | PROT_WRITE,
-                       MAP_SHARED | MAP_FIXED, fd, 0);
+       shm_base = mmap(0, 0x100000, PROT_READ | PROT_WRITE,
+                       MAP_SHARED, fd, 0);
        if (shm_base == (caddr_t) - 1) {
                tst_brkm(TBROK, cleanup, "can't mmap file: %s",
                         strerror(errno));
#2 lore raw

Thanks for your work.

I will backport the patch to my project after merged.

--Hongzhi

Quoted 52 lines · Jan Stancek

----- Original Message -----

Quoted 16 lines

*Ltp:*

latest master

*Kernel:*

After v5.1 ARCH=mips64

*Error info:*

diotest4 10 TBROK : diotest4.c:368: can't mmap file: Invalid argument diotest4 11 TBROK : diotest4.c:368: Remaining cases broken

Quoted 5 lines

I think the first argument of mmap is invalid.

  ?? >shm_base = (char *)(((long)sbrk(0) + (shmsz - 1)) & ~(shmsz - 1));

?? >shm_base = mmap(shm_base, 0x100000, PROT_READ | PROT_WRITE,

I don't see any note why that mmap needs to be MAP_FIXED. I'd drop it, let kernel pick an address:

testcases/kernel/io/direct_io/diotest4.c +2 -6
index e4616e400abd..bf200cd41a27 100644
@@ -352,18 +352,14 @@ int main(int argc, char *argv[])
         total++;
 
         /* Test-10: read, write to a mmaped file */
-       shm_base = (char *)(((long)sbrk(0) + (shmsz - 1)) & ~(shmsz - 1));
-       if (shm_base == NULL) {
-               tst_brkm(TBROK, cleanup, "sbrk failed: %s", strerror(errno));
-       }
         offset = 4096;
         count = bufsize;
         if ((fd = open(filename, O_DIRECT | O_RDWR)) < 0) {
                 tst_brkm(TBROK, cleanup, "can't open %s: %s",
                          filename, strerror(errno));
         }
-       shm_base = mmap(shm_base, 0x100000, PROT_READ | PROT_WRITE,
-                       MAP_SHARED | MAP_FIXED, fd, 0);
+       shm_base = mmap(0, 0x100000, PROT_READ | PROT_WRITE,
+                       MAP_SHARED, fd, 0);
         if (shm_base == (caddr_t) - 1) {
                 tst_brkm(TBROK, cleanup, "can't mmap file: %s",
                          strerror(errno));
#3 lore raw

----- Original Message -----

Quoted 3 lines

Thanks for your work.

I will backport the patch to my project after merged.

Does it mean, that patch I posted fixes problem for you?

Do you have good/bad kernel version? I was looking at recent upstream changes, but didn't spot anything that could explain that EINVAL.

Quoted 62 lines

--Hongzhi

Quoted 54 lines · Jan Stancek

----- Original Message -----

Quoted 16 lines

*Ltp:*

latest master

*Kernel:*

After v5.1 ARCH=mips64

*Error info:*

diotest4 10 TBROK : diotest4.c:368: can't mmap file: Invalid argument diotest4 11 TBROK : diotest4.c:368: Remaining cases broken

Quoted 5 lines

I think the first argument of mmap is invalid.

  ?? >shm_base = (char *)(((long)sbrk(0) + (shmsz - 1)) & ~(shmsz - 1));

?? >shm_base = mmap(shm_base, 0x100000, PROT_READ | PROT_WRITE,

I don't see any note why that mmap needs to be MAP_FIXED. I'd drop it, let kernel pick an address:

testcases/kernel/io/direct_io/diotest4.c +2 -6
index e4616e400abd..bf200cd41a27 100644
@@ -352,18 +352,14 @@ int main(int argc, char *argv[])
         total++;
 
         /* Test-10: read, write to a mmaped file */
-       shm_base = (char *)(((long)sbrk(0) + (shmsz - 1)) & ~(shmsz - 1));
-       if (shm_base == NULL) {
-               tst_brkm(TBROK, cleanup, "sbrk failed: %s",
 strerror(errno));
-       }
         offset = 4096;
         count = bufsize;
         if ((fd = open(filename, O_DIRECT | O_RDWR)) < 0) {
                 tst_brkm(TBROK, cleanup, "can't open %s: %s",
                          filename, strerror(errno));
         }
-       shm_base = mmap(shm_base, 0x100000, PROT_READ | PROT_WRITE,
-                       MAP_SHARED | MAP_FIXED, fd, 0);
+       shm_base = mmap(0, 0x100000, PROT_READ | PROT_WRITE,
+                       MAP_SHARED, fd, 0);
         if (shm_base == (caddr_t) - 1) {
                 tst_brkm(TBROK, cleanup, "can't mmap file: %s",
                          strerror(errno));
#4 lore raw

----- Original Message -----

Quoted 11 lines

----- Original Message -----

Quoted 3 lines

Thanks for your work.

I will backport the patch to my project after merged.

Does it mean, that patch I posted fixes problem for you?

Do you have good/bad kernel version? I was looking at recent upstream changes, but didn't spot anything that could explain that EINVAL.

Problem appears to be that "mmap.h" doesn't include "config.h", so MMAP_GRANULARITY ends up being just single page.

Following fixes it for me on qemu emulated 5kc-malta, running 5.2.0-rc7:

include/lapi/mmap.h +2 -0
index 18547c0d4..98b6ade1e 100644
@@ -19,6 +19,8 @@
 #ifndef LAPI_MMAP_H__
 #define LAPI_MMAP_H__
 
+#include "config.h"
+
 #ifndef MAP_HUGETLB
 # define MAP_HUGETLB 0x40000
 #endif

but still, perhaps we should also drop that MAP_FIXED.

Quoted 68 lines
Quoted 55 lines

--Hongzhi

Quoted 47 lines · Jan Stancek

----- Original Message -----

Quoted 16 lines

*Ltp:*

latest master

*Kernel:*

After v5.1 ARCH=mips64

*Error info:*

diotest4 10 TBROK : diotest4.c:368: can't mmap file: Invalid argument diotest4 11 TBROK : diotest4.c:368: Remaining cases broken

Quoted 5 lines

I think the first argument of mmap is invalid.

  ?? >shm_base = (char *)(((long)sbrk(0) + (shmsz - 1)) & ~(shmsz - 1));

?? >shm_base = mmap(shm_base, 0x100000, PROT_READ | PROT_WRITE,

I don't see any note why that mmap needs to be MAP_FIXED. I'd drop it, let kernel pick an address:

testcases/kernel/io/direct_io/diotest4.c +0 -4
index e4616e400abd..bf200cd41a27 100644
@@ -352,18 +352,14 @@ int main(int argc, char *argv[])
         total++;
 
         /* Test-10: read, write to a mmaped file */
-       shm_base = (char *)(((long)sbrk(0) + (shmsz - 1)) & ~(shmsz -
 1));
-       if (shm_base == NULL) {
-               tst_brkm(TBROK, cleanup, "sbrk failed: %s",
 strerror(errno));
-       }
         offset = 4096;
         count = bufsize;
         if ((fd = open(filename, O_DIRECT | O_RDWR)) < 0) {
                 tst_brkm(TBROK, cleanup, "can't open %s: %s",
                          filename, strerror(errno));
         }
Quoted 9 lines
Quoted 8 lines
testcases/kernel/io/direct_io/diotest4.c +2 -2
-       shm_base = mmap(shm_base, 0x100000, PROT_READ | PROT_WRITE,
-                       MAP_SHARED | MAP_FIXED, fd, 0);
+       shm_base = mmap(0, 0x100000, PROT_READ | PROT_WRITE,
+                       MAP_SHARED, fd, 0);
         if (shm_base == (caddr_t) - 1) {

tst_brkm(TBROK, cleanup, "can't mmap file: %s",
strerror(errno));

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

#5 lore raw
Quoted 5 lines · Jan Stancek

----- Original Message -----

Quoted 3 lines

----- Original Message -----

Quoted 2 lines

Thanks for your work.

Quoted 8 lines
Quoted 6 lines
Quoted 1 line

I will backport the patch to my project after merged.

Does it mean, that patch I posted fixes problem for you?

Do you have good/bad kernel version? I was looking at recent upstream changes, but didn't spot anything that could explain that EINVAL.

Problem appears to be that "mmap.h" doesn't include "config.h", so MMAP_GRANULARITY ends up being just single page.

Sorry, can you tell me the relationship between config.h and MMAP_GRANULARITY ?

I tested several times that including "config.h" indeed fixed the bug.

Quoted 17 lines

Following fixes it for me on qemu emulated 5kc-malta, running 5.2.0-rc7:

include/lapi/mmap.h +2 -0
index 18547c0d4..98b6ade1e 100644
@@ -19,6 +19,8 @@
  #ifndef LAPI_MMAP_H__
  #define LAPI_MMAP_H__
 
+#include "config.h"
+
  #ifndef MAP_HUGETLB
  # define MAP_HUGETLB 0x40000
  #endif
Quoted 1 line

but still, perhaps we should also drop that MAP_FIXED.

Agree with it. Could I send the patch with your signed-off?

--Hongzhi

Quoted 64 lines
Quoted 64 lines
Quoted 53 lines

--Hongzhi

Quoted 46 lines · Jan Stancek

----- Original Message -----

Quoted 16 lines

*Ltp:*

latest master

*Kernel:*

After v5.1 ARCH=mips64

*Error info:*

diotest4 10 TBROK : diotest4.c:368: can't mmap file: Invalid argument diotest4 11 TBROK : diotest4.c:368: Remaining cases broken

Quoted 5 lines

I think the first argument of mmap is invalid.

   ?? >shm_base = (char *)(((long)sbrk(0) + (shmsz - 1)) & ~(shmsz - 1));
   ?? >shm_base = mmap(shm_base, 0x100000, PROT_READ | PROT_WRITE,

I don't see any note why that mmap needs to be MAP_FIXED. I'd drop it, let kernel pick an address:

testcases/kernel/io/direct_io/diotest4.c +0 -4
index e4616e400abd..bf200cd41a27 100644
@@ -352,18 +352,14 @@ int main(int argc, char *argv[])
          total++;
 
          /* Test-10: read, write to a mmaped file */
-       shm_base = (char *)(((long)sbrk(0) + (shmsz - 1)) & ~(shmsz -
 1));
-       if (shm_base == NULL) {
-               tst_brkm(TBROK, cleanup, "sbrk failed: %s",
 strerror(errno));
-       }
          offset = 4096;
          count = bufsize;
          if ((fd = open(filename, O_DIRECT | O_RDWR)) < 0) {
                  tst_brkm(TBROK, cleanup, "can't open %s: %s",
                           filename, strerror(errno));
          }
Quoted 8 lines
Quoted 8 lines
testcases/kernel/io/direct_io/diotest4.c +2 -2
-       shm_base = mmap(shm_base, 0x100000, PROT_READ | PROT_WRITE,
-                       MAP_SHARED | MAP_FIXED, fd, 0);
+       shm_base = mmap(0, 0x100000, PROT_READ | PROT_WRITE,
+                       MAP_SHARED, fd, 0);
          if (shm_base == (caddr_t) - 1) {

tst_brkm(TBROK, cleanup, "can't mmap file: %s",
strerror(errno));

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

#6 lore raw

----- Original Message -----

Quoted 10 lines · Jan Stancek
Quoted 8 lines

----- Original Message -----

Quoted 6 lines

----- Original Message -----

Quoted 3 lines

Thanks for your work.

I will backport the patch to my project after merged.

Does it mean, that patch I posted fixes problem for you?

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines

Do you have good/bad kernel version? I was looking at recent upstream changes, but didn't spot anything that could explain that EINVAL.

Problem appears to be that "mmap.h" doesn't include "config.h", so MMAP_GRANULARITY ends up being just single page.

Sorry, can you tell me the relationship between config.h and MMAP_GRANULARITY ?

configure detects that you have sys/smh.h, which is recorded in config.h:
/* Define to 1 if you have the <sys/shm.h> header file. */ #define HAVE_SYS_SHM_H 1

If your platform has shm.h, then MMAP_GRANULARITY is define as SHMLBA, otherwise as single page.

If lapi/mmap.h doesn't include config.h, then it's always the latter, because HAVE_SYS_SHM_H is undefined.

Quoted 21 lines

I tested several times that including "config.h" indeed fixed the bug.

Quoted 17 lines

Following fixes it for me on qemu emulated 5kc-malta, running 5.2.0-rc7:

include/lapi/mmap.h +2 -0
index 18547c0d4..98b6ade1e 100644
@@ -19,6 +19,8 @@
  #ifndef LAPI_MMAP_H__
  #define LAPI_MMAP_H__
 
+#include "config.h"
+
  #ifndef MAP_HUGETLB
  # define MAP_HUGETLB 0x40000
  #endif
Quoted 4 lines
Quoted 1 line

but still, perhaps we should also drop that MAP_FIXED.

Agree with it. Could I send the patch with your signed-off?

I sent one ~20 minutes ago, you should be on CC.

Quoted 70 lines

--Hongzhi

Quoted 65 lines
Quoted 65 lines
Quoted 54 lines

--Hongzhi

Quoted 47 lines · Jan Stancek

----- Original Message -----

Quoted 16 lines

*Ltp:*

latest master

*Kernel:*

After v5.1 ARCH=mips64

*Error info:*

diotest4 10 TBROK : diotest4.c:368: can't mmap file: Invalid argument diotest4 11 TBROK : diotest4.c:368: Remaining cases broken

Quoted 6 lines

I think the first argument of mmap is invalid.

   ?? >shm_base = (char *)(((long)sbrk(0) + (shmsz - 1)) & ~(shmsz -
   ?? >1));

   ?? >shm_base = mmap(shm_base, 0x100000, PROT_READ | PROT_WRITE,

I don't see any note why that mmap needs to be MAP_FIXED. I'd drop it, let kernel pick an address:

testcases/kernel/io/direct_io/diotest4.c +0 -4
index e4616e400abd..bf200cd41a27 100644
@@ -352,18 +352,14 @@ int main(int argc, char *argv[])
          total++;
 
          /* Test-10: read, write to a mmaped file */
-       shm_base = (char *)(((long)sbrk(0) + (shmsz - 1)) & ~(shmsz -
 1));
-       if (shm_base == NULL) {
-               tst_brkm(TBROK, cleanup, "sbrk failed: %s",
 strerror(errno));
-       }
          offset = 4096;
          count = bufsize;
          if ((fd = open(filename, O_DIRECT | O_RDWR)) < 0) {
                  tst_brkm(TBROK, cleanup, "can't open %s: %s",
                           filename, strerror(errno));
          }
Quoted 8 lines
Quoted 8 lines
testcases/kernel/io/direct_io/diotest4.c +2 -2
-       shm_base = mmap(shm_base, 0x100000, PROT_READ | PROT_WRITE,
-                       MAP_SHARED | MAP_FIXED, fd, 0);
+       shm_base = mmap(0, 0x100000, PROT_READ | PROT_WRITE,
+                       MAP_SHARED, fd, 0);
          if (shm_base == (caddr_t) - 1) {

tst_brkm(TBROK, cleanup, "can't mmap file: %s",
strerror(errno));

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

#7 lore raw
Quoted 46 lines · Jan Stancek

----- Original Message -----

Quoted 8 lines · Jan Stancek
Quoted 7 lines

----- Original Message -----

Quoted 6 lines

----- Original Message -----

Quoted 3 lines

Thanks for your work.

I will backport the patch to my project after merged.

Does it mean, that patch I posted fixes problem for you?

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

Do you have good/bad kernel version? I was looking at recent upstream changes, but didn't spot anything that could explain that EINVAL.

Problem appears to be that "mmap.h" doesn't include "config.h", so MMAP_GRANULARITY ends up being just single page.

Sorry, can you tell me the relationship between config.h and MMAP_GRANULARITY ?

configure detects that you have sys/smh.h, which is recorded in config.h:

   /* Define to 1 if you have the <sys/shm.h> header file. */
   #define HAVE_SYS_SHM_H 1

If your platform has shm.h, then MMAP_GRANULARITY is define as SHMLBA, otherwise as single page.

If lapi/mmap.h doesn't include config.h, then it's always the latter, because HAVE_SYS_SHM_H is undefined.

Quoted 19 lines

I tested several times that including "config.h" indeed fixed the bug.

Quoted 16 lines

Following fixes it for me on qemu emulated 5kc-malta, running 5.2.0-rc7:

include/lapi/mmap.h +2 -0
index 18547c0d4..98b6ade1e 100644
@@ -19,6 +19,8 @@
   #ifndef LAPI_MMAP_H__
   #define LAPI_MMAP_H__
 
+#include "config.h"
+
   #ifndef MAP_HUGETLB
   # define MAP_HUGETLB 0x40000
   #endif
Quoted 4 lines
Quoted 3 lines
Quoted 1 line

but still, perhaps we should also drop that MAP_FIXED.

Agree with it. Could I send the patch with your signed-off?

I sent one ~20 minutes ago, you should be on CC.

Yeah, I see it. Thank you very much.

--Hongzhi

Quoted 67 lines
Quoted 67 lines

--Hongzhi

Quoted 63 lines
Quoted 63 lines
Quoted 52 lines

--Hongzhi

Quoted 47 lines · Jan Stancek

----- Original Message -----

Quoted 16 lines

*Ltp:*

latest master

*Kernel:*

After v5.1 ARCH=mips64

*Error info:*

diotest4 10 TBROK : diotest4.c:368: can't mmap file: Invalid argument diotest4 11 TBROK : diotest4.c:368: Remaining cases broken

Quoted 6 lines

I think the first argument of mmap is invalid.

    ?? >shm_base = (char *)(((long)sbrk(0) + (shmsz - 1)) & ~(shmsz -
    ?? >1));

    ?? >shm_base = mmap(shm_base, 0x100000, PROT_READ | PROT_WRITE,

I don't see any note why that mmap needs to be MAP_FIXED. I'd drop it, let kernel pick an address:

testcases/kernel/io/direct_io/diotest4.c +0 -4
index e4616e400abd..bf200cd41a27 100644
@@ -352,18 +352,14 @@ int main(int argc, char *argv[])
           total++;
 
           /* Test-10: read, write to a mmaped file */
-       shm_base = (char *)(((long)sbrk(0) + (shmsz - 1)) & ~(shmsz -
 1));
-       if (shm_base == NULL) {
-               tst_brkm(TBROK, cleanup, "sbrk failed: %s",
 strerror(errno));
-       }
           offset = 4096;
           count = bufsize;
           if ((fd = open(filename, O_DIRECT | O_RDWR)) < 0) {
                   tst_brkm(TBROK, cleanup, "can't open %s: %s",
                            filename, strerror(errno));
           }
Quoted 8 lines
Quoted 8 lines
testcases/kernel/io/direct_io/diotest4.c +2 -2
-       shm_base = mmap(shm_base, 0x100000, PROT_READ | PROT_WRITE,
-                       MAP_SHARED | MAP_FIXED, fd, 0);
+       shm_base = mmap(0, 0x100000, PROT_READ | PROT_WRITE,
+                       MAP_SHARED, fd, 0);
           if (shm_base == (caddr_t) - 1) {

tst_brkm(TBROK, cleanup, "can't mmap file: %s",
strerror(errno));

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

#8 lore raw
In reply to Jan Stancek
Quoted 5 lines · Jan Stancek

----- Original Message -----

Quoted 3 lines

Thanks for your work.

I will backport the patch to my project after merged.

Does it mean, that patch I posted fixes problem for you?

Not for me, for LTP.

I just found the bug. Even if there is no me, the bug still exists.

Do you think the manually selecting addr for mmap is reasonable?

And the addr points to the top of heap.

| >shm_base = (char *)(((long)sbrk(0) + (shmsz - 1)) & ~(shmsz - 1));

|? >shm_base = mmap(shm_base, 0x100000, PROT_READ | PROT_WRITE,

Quoted 4 lines

Do you have good/bad kernel version? I was looking at recent upstream changes, but didn't spot anything that could explain that EINVAL.

I didn't test all versions of kernel. But after v5.1-mips can raise the problem.

Maybe the bug exists on mips since the testcase is created.

I guess that few people use mips now, so the bug is not found in time.

--Hongzhi

Quoted 59 lines
Quoted 59 lines

--Hongzhi

Quoted 53 lines · Jan Stancek

----- Original Message -----

Quoted 16 lines

*Ltp:*

latest master

*Kernel:*

After v5.1 ARCH=mips64

*Error info:*

diotest4 10 TBROK : diotest4.c:368: can't mmap file: Invalid argument diotest4 11 TBROK : diotest4.c:368: Remaining cases broken

Quoted 5 lines

I think the first argument of mmap is invalid.

   ?? >shm_base = (char *)(((long)sbrk(0) + (shmsz - 1)) & ~(shmsz - 1));
   ?? >shm_base = mmap(shm_base, 0x100000, PROT_READ | PROT_WRITE,

I don't see any note why that mmap needs to be MAP_FIXED. I'd drop it, let kernel pick an address:

testcases/kernel/io/direct_io/diotest4.c +2 -6
index e4616e400abd..bf200cd41a27 100644
@@ -352,18 +352,14 @@ int main(int argc, char *argv[])
          total++;
 
          /* Test-10: read, write to a mmaped file */
-       shm_base = (char *)(((long)sbrk(0) + (shmsz - 1)) & ~(shmsz - 1));
-       if (shm_base == NULL) {
-               tst_brkm(TBROK, cleanup, "sbrk failed: %s",
 strerror(errno));
-       }
          offset = 4096;
          count = bufsize;
          if ((fd = open(filename, O_DIRECT | O_RDWR)) < 0) {
                  tst_brkm(TBROK, cleanup, "can't open %s: %s",
                           filename, strerror(errno));
          }
-       shm_base = mmap(shm_base, 0x100000, PROT_READ | PROT_WRITE,
-                       MAP_SHARED | MAP_FIXED, fd, 0);
+       shm_base = mmap(0, 0x100000, PROT_READ | PROT_WRITE,
+                       MAP_SHARED, fd, 0);
          if (shm_base == (caddr_t) - 1) {
                  tst_brkm(TBROK, cleanup, "can't mmap file: %s",
                           strerror(errno));