mem/hugetlb: shift an empty region to use in hugemmap02.c

testcases/kernel/mem/include/mem.h +2 -0

+/* MMAP */
+int range_is_mapped(unsigned long low, unsigned long high)
 #endif

testcases/kernel/mem/lib/mem.c +1 -0

index 118b6c0..e0e1bdf 100644
@@ -1113,3 +1113,46 @@ void update_shm_size(size_t * shm_size)
 		*shm_size = shmmax;
 	}
 }
+

testcases/kernel/mem/lib/mem.c +5 -0

+		return -1;
+	}
+
+	while (1) {
+		unsigned long start, end;

testcases/kernel/mem/lib/mem.c +13 -0

+		if ((start >= low) && (start < high)) {
+			fclose(f);
+			return 1;
+		}
+		if ((end >= low) && (end < high)) {
+			fclose(f);
+			return 1;
+		}
+	}
+
+	fclose(f);
+	return 0;
+}

Hi!
Generall idea looks good, a few comments below.

Review

Quoted 4 lines

testcases/kernel/mem/include/mem.h +2 -0

+/* MMAP */
+int range_is_mapped(unsigned long low, unsigned long high)
 #endif

(quoted diff) +0 -0

b/testcases/kernel/mem/lib/mem.c

Quoted 12 lines

index 118b6c0..e0e1bdf 100644

testcases/kernel/mem/lib/mem.c +5 -0

@@ -1113,3 +1113,46 @@ void update_shm_size(size_t * shm_size)
              *shm_size = shmmax;
      }
 }
+
+int range_is_mapped(unsigned long low, unsigned long high)
+{
+     FILE *f;
+     int MAPS_BUF_SZ = 4096;

testcases/kernel/mem/lib/mem.c void update_shm_size(size_t * shm_size) · line 1126

Quoted 6 lines

testcases/kernel/mem/lib/mem.c +6 -0

+     char line[MAPS_BUF_SZ];
+     char *tmp;
+
+     f = fopen("/proc/self/maps", "r");
+     if (!f) {
+             printf("Failed to open /proc/self/maps.\n");

                ^
                should rather be tst_resm(TWARN, );

Quoted 9 lines

testcases/kernel/mem/lib/mem.c +9 -0

+             return -1;
+     }
+
+     while (1) {
+             unsigned long start, end;
+             int ret;
+
+             tmp = fgets(line, MAPS_BUF_SZ, f);
+             if (!tmp)

testcases/kernel/mem/lib/mem.c void update_shm_size(size_t * shm_size) · line 1141

Quoted 6 lines

testcases/kernel/mem/lib/mem.c +6 -0

+                     break;
+
+             ret = sscanf(line, "%lx-%lx", &start, &end);
+             if (ret != 2) {
+                     printf("Couldn't parse /proc/self/maps line: %s\n",
+                                     line);

^
here as well

testcases/kernel/mem/lib/mem.c void update_shm_size(size_t * shm_size) · line 1144

Quoted 3 lines

testcases/kernel/mem/lib/mem.c +3 -0

+                     fclose(f);
+                     return -1;
+             }

Why don't you just use fscanf()? It doesn't make sense to read the line into the buffer and then sscanf() the values.

testcases/kernel/mem/lib/mem.c void update_shm_size(size_t * shm_size) · line 1158

Quoted 13 lines

testcases/kernel/mem/lib/mem.c +13 -0

+             if ((start >= low) && (start < high)) {
+                     fclose(f);
+                     return 1;
+             }
+             if ((end >= low) && (end < high)) {
+                     fclose(f);
+                     return 1;
+             }
+     }
+
+     fclose(f);
+     return 0;
+}

--
Cyril Hrubis
[email protected]

Hi,

Quoted 20 lines · Cyril Hrubis

Hi!
Generall idea looks good, a few comments below.

Review

Quoted 4 lines

testcases/kernel/mem/include/mem.h +2 -0

+/* MMAP */
+int range_is_mapped(unsigned long low, unsigned long high)
 #endif

(quoted diff) +0 -0

b/testcases/kernel/mem/lib/mem.c

Quoted 12 lines

index 118b6c0..e0e1bdf 100644

testcases/kernel/mem/lib/mem.c +5 -0

@@ -1113,3 +1113,46 @@ void update_shm_size(size_t * shm_size)
              *shm_size = shmmax;
      }
 }
+
+int range_is_mapped(unsigned long low, unsigned long high)
+{
+     FILE *f;
+     int MAPS_BUF_SZ = 4096;

Quoted 9 lines

testcases/kernel/mem/lib/mem.c void update_shm_size(size_t * shm_size) · line 1126

Quoted 6 lines

testcases/kernel/mem/lib/mem.c +6 -0

+     char line[MAPS_BUF_SZ];
+     char *tmp;
+
+     f = fopen("/proc/self/maps", "r");
+     if (!f) {
+             printf("Failed to open /proc/self/maps.\n");

                ^
                should rather be tst_resm(TWARN, );

ok. looks like using

    FILE * f = SAFE_FOPEN(NULL, "/proc/self/maps", "r");

is better than above.

Quoted 15 lines

Quoted 4 lines

testcases/kernel/mem/lib/mem.c +4 -0

+             return -1;
+     }
+
+     while (1) {

testcases/kernel/mem/lib/mem.c void update_shm_size(size_t * shm_size)

Quoted 10 lines

testcases/kernel/mem/lib/mem.c +10 -0

+             unsigned long start, end;
+             int ret;
+
+             tmp = fgets(line, MAPS_BUF_SZ, f);
+             if (!tmp)
+                     break;
+
+             ret = sscanf(line, "%lx-%lx", &start, &end);
+             if (ret != 2) {
+                     printf("Couldn't parse /proc/self/maps line:

%s\n",

Quoted 4 lines

testcases/kernel/mem/lib/mem.c void update_shm_size(size_t * shm_size) · line 1141

Quoted 1 line

testcases/kernel/mem/lib/mem.c +1 -0

+                                     line);

^
here as well

ok.

Quoted 7 lines

testcases/kernel/mem/lib/mem.c void update_shm_size(size_t * shm_size) · line 1144

Quoted 3 lines

testcases/kernel/mem/lib/mem.c +3 -0

+                     fclose(f);
+                     return -1;
+             }

Why don't you just use fscanf()? It doesn't make sense to read the line into the buffer and then sscanf() the values.

hmm, we need to read the '/proc/self/maps' line by line, It easy to use fgets() to get each line in and then sscanf() that.

fscanf() will never read the newline at the end of the first line. So when it is called the second time, it will fail (returning 0, not EOF) and read nothing, leaving the [start, end] unchanged.
maybe I don't know if fscanf() could achieve that also?

so what about something like:
--------------------------------------
int range_is_mapped(unsigned long low, unsigned long high)

{
    FILE *f = SAFE_FOPEN(NULL, "/proc/self/maps", "r");

    int MAPS_BUF_SZ = 4096;
    char line[MAPS_BUF_SZ];

    while (fgets(line, MAPS_BUF_SZ, f)) {
        unsigned long start, end;
        int ret;

        ret = sscanf(line, "%lx-%lx", &start, &end);
        if (ret != 2) {
            tst_resm(TWARN | TERRNO, "Couldn't parse /proc/self/maps
line.");
            SAFE_FCLOSE(NULL, f);
        }

        if ((start >= low) && (start < high)) {
            SAFE_FCLOSE(NULL, f);
            return 1;
        }
        if ((end >= low) && (end < high)) {
            SAFE_FCLOSE(NULL, f);
            return 1;
        }
    }

    SAFE_FCLOSE(NULL, f);
    return 0;
}

Quoted 9 lines

Quoted 9 lines

testcases/kernel/mem/lib/mem.c +9 -0

+             if ((start >= low) && (start < high)) {
+                     fclose(f);
+                     return 1;
+             }
+             if ((end >= low) && (end < high)) {
+                     fclose(f);
+                     return 1;
+             }
+     }

Quoted 9 lines

testcases/kernel/mem/lib/mem.c void update_shm_size(size_t * shm_size) · line 1158

Quoted 4 lines

testcases/kernel/mem/lib/mem.c +4 -0

+
+     fclose(f);
+     return 0;
+}

--
Cyril Hrubis
[email protected]

--
Regards,
Li Wang
Email: [email protected]

okay, seems you are right. I tried and achieved another one, following code test pass on my s390x system.
-----------------------------------------
int range_is_mapped(unsigned long low, unsigned long high)

{
    FILE *f = SAFE_FOPEN(NULL, "/proc/self/maps", "r");

    char buf[512];

    while (!feof(f)) {
        unsigned long start, end;
        int ret;

        ret = fscanf(f, "%lx-%lx %[^\n]%*c]", &start, &end, buf);

Hi!

Quoted 6 lines

okay, seems you are right. I tried and achieved another one, following code test pass on my s390x system.
-----------------------------------------
int range_is_mapped(unsigned long low, unsigned long high)

{
    FILE *f = SAFE_FOPEN(NULL, "/proc/self/maps", "r");

If we start to use SAFE_MACROS() here we have to pass the cleanup callback to the range_is_mapped() as a first parameter so that we can pass correct cleanup callback to the SAFE_MACROS().

Quoted 7 lines

    char buf[512];

    while (!feof(f)) {
        unsigned long start, end;
        int ret;

Quoted 1 line

        ret = fscanf(f, "%lx-%lx %[^\n]%*c]", &start, &end, buf);

You can do even better with:

                ret = fscanf(f, "%lx-%lx %*[^\n]\n", &start, &end);

Which says read two long ints and then ignore everything until the end of line.

You could have ommited the last '\n' in the format string, but then you would need to exit the loop when the fscanf() returns -1 since the feof() would not exit the loop since there would still be the last newline in the buffer after we read the last line...

From: "Li Wang" <[email protected]>
To: "Cyril Hrubis" <[email protected]>, "Jan Stancek" <[email protected]>
Cc: [email protected]
Sent: Tuesday, 13 October, 2015 12:15:23 PM
Subject: Re: [LTP] [PATCH v2] mem/hugetlb: shift an empty region to use in hugemmap02.c

Hi,

Quoted 54 lines · Li Wang

Hi,

Quoted 20 lines · Cyril Hrubis

Hi!
Generall idea looks good, a few comments below.

Review

Quoted 4 lines

testcases/kernel/mem/include/mem.h +2 -0

+/* MMAP */
+int range_is_mapped(unsigned long low, unsigned long high)
 #endif

(quoted diff) +0 -0

b/testcases/kernel/mem/lib/mem.c

Quoted 12 lines

index 118b6c0..e0e1bdf 100644

testcases/kernel/mem/lib/mem.c +5 -0

@@ -1113,3 +1113,46 @@ void update_shm_size(size_t * shm_size)
              *shm_size = shmmax;
      }
 }
+
+int range_is_mapped(unsigned long low, unsigned long high)
+{
+     FILE *f;
+     int MAPS_BUF_SZ = 4096;

Quoted 9 lines

testcases/kernel/mem/lib/mem.c void update_shm_size(size_t * shm_size) · line 1126

Quoted 6 lines

testcases/kernel/mem/lib/mem.c +6 -0

+     char line[MAPS_BUF_SZ];
+     char *tmp;
+
+     f = fopen("/proc/self/maps", "r");
+     if (!f) {
+             printf("Failed to open /proc/self/maps.\n");

                ^
                should rather be tst_resm(TWARN, );

ok. looks like using

    FILE * f = SAFE_FOPEN(NULL, "/proc/self/maps", "r");

is better than above.

Quoted 15 lines

Quoted 4 lines

testcases/kernel/mem/lib/mem.c +4 -0

+             return -1;
+     }
+
+     while (1) {

testcases/kernel/mem/lib/mem.c void update_shm_size(size_t * shm_size)

Quoted 10 lines

testcases/kernel/mem/lib/mem.c +10 -0

+             unsigned long start, end;
+             int ret;
+
+             tmp = fgets(line, MAPS_BUF_SZ, f);
+             if (!tmp)
+                     break;
+
+             ret = sscanf(line, "%lx-%lx", &start, &end);
+             if (ret != 2) {
+                     printf("Couldn't parse /proc/self/maps line:

%s\n",

Quoted 16 lines

Quoted 4 lines

testcases/kernel/mem/lib/mem.c void update_shm_size(size_t * shm_size) · line 1141

Quoted 1 line

testcases/kernel/mem/lib/mem.c +1 -0

+                                     line);

^
here as well

ok.

Quoted 7 lines

testcases/kernel/mem/lib/mem.c void update_shm_size(size_t * shm_size) · line 1144

Quoted 3 lines

testcases/kernel/mem/lib/mem.c +3 -0

+                     fclose(f);
+                     return -1;
+             }

Why don't you just use fscanf()? It doesn't make sense to read the line into the buffer and then sscanf() the values.

okay, seems you are right. I tried and achieved another one, following code test pass on my s390x system.
-----------------------------------------
int range_is_mapped(unsigned long low, unsigned long high)

{
    FILE *f = SAFE_FOPEN(NULL, "/proc/self/maps", "r");

    char buf[512];

    while (!feof(f)) {
        unsigned long start, end;
        int ret;

        ret = fscanf(f, "%lx-%lx %[^\n]%*c]", &start, &end, buf);
        if (ret != 3) {
            tst_resm(TWARN | TERRNO, "Couldn't parse /proc/self/maps
line.");
            SAFE_FCLOSE(NULL, f);
        }

        if ((start >= low) && (start < high)) {
            SAFE_FCLOSE(NULL, f);
            return 1;
        }
        if ((end >= low) && (end < high)) {
            SAFE_FCLOSE(NULL, f);
            return 1;
        }
    }

    SAFE_FCLOSE(NULL, f);
    return 0;
}

so, which one is better? I will format patch V3 when getting some proposals.

cc' jstancek@ if has some good idea.

From: "Jan Stancek" <[email protected]>
To: "Li Wang" <[email protected]>, "Cyril Hrubis" <[email protected]>
Cc: [email protected]
Sent: Tuesday, 13 October, 2015 12:54:46 PM
Subject: Re: [LTP] [PATCH v2] mem/hugetlb: shift an empty region to use in hugemmap02.c

The approach above should work, but it also stores data to buf, that we don't care about. You could do fscanf + "while (fgetc(f) != '\n');" to forward to next line, if you want to avoid using buf.

                ret = fscanf(f, "%lx-%lx %*[^\n]\n", &start, &end);