| Message ID | efa47acd652c93a54fb66e17183524b54e561533.1683274510.git.pengfei.xu@intel.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v1,1/1] libs: libltpnuma: Fix one fake failure when CXL(Compute eXpress Link) node memory is not used | expand |
Hi! > > - if (!mem_total || !mem_used) { > - tst_res(TWARN, "Failed to parse '%s'", path); > + tst_res(TINFO,"Node%i: mem_total:%ld kB, mem_used:%ld kB", node, > + mem_total, mem_used); > + > + if (!mem_total) { > + tst_res(TWARN, "Failed to parse '%s', MemTotal is 0", path); > return 0; Why don't we use the return value from the sscanf instead? What about: diff --git a/libs/libltpnuma/tst_numa.c b/libs/libltpnuma/tst_numa.c index ef4c8e879..7e7504cac 100644 --- a/libs/libltpnuma/tst_numa.c +++ b/libs/libltpnuma/tst_numa.c @@ -149,11 +149,19 @@ static int node_has_enough_memory(int node, size_t min_kb) while (fgets(buf, sizeof(buf), fp)) { long val; - if (sscanf(buf, "%*s %*i MemTotal: %li", &val) == 1) + if (sscanf(buf, "%*s %*i MemTotal: %li", &val) == 1) { mem_total = val; + } else { + tst_res(TWARN, "Faield to parse '%s' MemTotal", path); + return 0; + } - if (sscanf(buf, "%*s %*i MemUsed: %li", &val) == 1) + if (sscanf(buf, "%*s %*i MemUsed: %li", &val) == 1) { mem_used = val; + } else { + tst_res(TWARN, "Faield to parse '%s' MemUsed", path); + return 0; + } if (sscanf(buf, "%*s %*i FilePages: %li", &val) == 1) file_pages = val; @@ -161,10 +169,6 @@ static int node_has_enough_memory(int node, size_t min_kb) fclose(fp); - if (!mem_total || !mem_used) { - tst_res(TWARN, "Failed to parse '%s'", path); - return 0; - } mem_avail = mem_total - mem_used + (9 * file_pages)/10;
Hi Cyril, On 2023-05-09 at 13:59:01 +0200, Cyril Hrubis wrote: > Hi! > > > > - if (!mem_total || !mem_used) { > > - tst_res(TWARN, "Failed to parse '%s'", path); > > + tst_res(TINFO,"Node%i: mem_total:%ld kB, mem_used:%ld kB", node, > > + mem_total, mem_used); > > + > > + if (!mem_total) { > > + tst_res(TWARN, "Failed to parse '%s', MemTotal is 0", path); > > return 0; > > Why don't we use the return value from the sscanf instead? > > What about: I tried as follow: # ./get_mempolicy01 tst_test.c:1560: TINFO: Timeout per run is 0h 00m 30s tst_numa.c:208: TINFO: Found 3 NUMA memory nodes tst_numa.c:162: TWARN: Faield to parse '/sys/devices/system/node/node0/meminfo' MemUsed:65220292 tst_numa.c:162: TWARN: Faield to parse '/sys/devices/system/node/node1/meminfo' MemUsed:65167784 tst_numa.c:162: TWARN: Faield to parse '/sys/devices/system/node/node2/meminfo' MemUsed:4194304 get_mempolicy01.c:149: TCONF: test requires at least one NUMA memory node Node 0 and 1 could not be tested with follow patch installed, seems when node2/meminfo "MemUsed: 0"，sscanf("...%li", &val) will return 0 if only 0 value will be scaned, and the while loop will scan the char one by one and impact all other node meminfo scan result, it's strange. If used my patch, I didn't meet this issue. Thanks! BR. > > diff --git a/libs/libltpnuma/tst_numa.c b/libs/libltpnuma/tst_numa.c > index ef4c8e879..7e7504cac 100644 > --- a/libs/libltpnuma/tst_numa.c > +++ b/libs/libltpnuma/tst_numa.c > @@ -149,11 +149,19 @@ static int node_has_enough_memory(int node, size_t min_kb) > while (fgets(buf, sizeof(buf), fp)) { > long val; > > - if (sscanf(buf, "%*s %*i MemTotal: %li", &val) == 1) > + if (sscanf(buf, "%*s %*i MemTotal: %li", &val) == 1) { > mem_total = val; > + } else { > + tst_res(TWARN, "Faield to parse '%s' MemTotal", path); > + return 0; > + } > > - if (sscanf(buf, "%*s %*i MemUsed: %li", &val) == 1) > + if (sscanf(buf, "%*s %*i MemUsed: %li", &val) == 1) { > mem_used = val; > + } else { > + tst_res(TWARN, "Faield to parse '%s' MemUsed", path); > + return 0; > + } > > if (sscanf(buf, "%*s %*i FilePages: %li", &val) == 1) > file_pages = val; > @@ -161,10 +169,6 @@ static int node_has_enough_memory(int node, size_t min_kb) > > fclose(fp); > > - if (!mem_total || !mem_used) { > - tst_res(TWARN, "Failed to parse '%s'", path); > - return 0; > - } > > mem_avail = mem_total - mem_used + (9 * file_pages)/10; > > > -- > Cyril Hrubis > chrubis@suse.cz
Hi! > # ./get_mempolicy01 > tst_test.c:1560: TINFO: Timeout per run is 0h 00m 30s > tst_numa.c:208: TINFO: Found 3 NUMA memory nodes > tst_numa.c:162: TWARN: Faield to parse '/sys/devices/system/node/node0/meminfo' MemUsed:65220292 > tst_numa.c:162: TWARN: Faield to parse '/sys/devices/system/node/node1/meminfo' MemUsed:65167784 > tst_numa.c:162: TWARN: Faield to parse '/sys/devices/system/node/node2/meminfo' MemUsed:4194304 > get_mempolicy01.c:149: TCONF: test requires at least one NUMA memory node > Node 0 and 1 could not be tested with follow patch installed, seems when > node2/meminfo "MemUsed: 0"，sscanf("...%li", &val) will return 0 if only 0 value > will be scaned, and the while loop will scan the char one by one and impact all > other node meminfo scan result, it's strange. > > If used my patch, I didn't meet this issue. Ah, right the file is multiline and we match only one of the lines per iteration... Then we need: diff --git a/libs/libltpnuma/tst_numa.c b/libs/libltpnuma/tst_numa.c index ef4c8e879..c3297013b 100644 --- a/libs/libltpnuma/tst_numa.c +++ b/libs/libltpnuma/tst_numa.c @@ -127,8 +127,8 @@ static int node_has_enough_memory(int node, size_t min_kb) { char path[1024]; char buf[1024]; - long mem_total = 0; - long mem_used = 0; + long mem_total = -1; + long mem_used = -1; long file_pages = 0; long mem_avail; @@ -161,7 +161,7 @@ static int node_has_enough_memory(int node, size_t min_kb) fclose(fp); - if (!mem_total || !mem_used) { + if (mem_total == -1 || mem_used == -1) { tst_res(TWARN, "Failed to parse '%s'", path); return 0; }
Hi Cyril, On 2023-05-09 at 16:10:35 +0200, Cyril Hrubis wrote: > Hi! > > # ./get_mempolicy01 > > tst_test.c:1560: TINFO: Timeout per run is 0h 00m 30s > > tst_numa.c:208: TINFO: Found 3 NUMA memory nodes > > tst_numa.c:162: TWARN: Faield to parse '/sys/devices/system/node/node0/meminfo' MemUsed:65220292 > > tst_numa.c:162: TWARN: Faield to parse '/sys/devices/system/node/node1/meminfo' MemUsed:65167784 > > tst_numa.c:162: TWARN: Faield to parse '/sys/devices/system/node/node2/meminfo' MemUsed:4194304 > > get_mempolicy01.c:149: TCONF: test requires at least one NUMA memory node > > Node 0 and 1 could not be tested with follow patch installed, seems when > > node2/meminfo "MemUsed: 0"，sscanf("...%li", &val) will return 0 if only 0 value > > will be scaned, and the while loop will scan the char one by one and impact all > > other node meminfo scan result, it's strange. > > > > If used my patch, I didn't meet this issue. > > Ah, right the file is multiline and we match only one of the lines per > iteration... > > Then we need: > Yes, below patch works well and as expected for all nodes memory check now. Below way is better and correct. Thanks! BR. > diff --git a/libs/libltpnuma/tst_numa.c b/libs/libltpnuma/tst_numa.c > index ef4c8e879..c3297013b 100644 > --- a/libs/libltpnuma/tst_numa.c > +++ b/libs/libltpnuma/tst_numa.c > @@ -127,8 +127,8 @@ static int node_has_enough_memory(int node, size_t min_kb) > { > char path[1024]; > char buf[1024]; > - long mem_total = 0; > - long mem_used = 0; > + long mem_total = -1; > + long mem_used = -1; > long file_pages = 0; > long mem_avail; > > @@ -161,7 +161,7 @@ static int node_has_enough_memory(int node, size_t min_kb) > > fclose(fp); > > - if (!mem_total || !mem_used) { > + if (mem_total == -1 || mem_used == -1) { > tst_res(TWARN, "Failed to parse '%s'", path); > return 0; > } > > -- > Cyril Hrubis > chrubis@suse.cz
Hi! > Yes, below patch works well and as expected for all nodes memory check now. > > Below way is better and correct. I've pushed these changes along with your description, thanks!
On 2023-05-10 at 17:20:45 +0200, Cyril Hrubis wrote: > Hi! > > Yes, below patch works well and as expected for all nodes memory check now. > > > > Below way is better and correct. > > I've pushed these changes along with your description, thanks! LGTM, it's great! I'm glad that this issue was fixed. Thanks! BR. > > -- > Cyril Hrubis > chrubis@suse.cz
diff --git a/libs/libltpnuma/tst_numa.c b/libs/libltpnuma/tst_numa.c index ef4c8e879..f6f577657 100644 --- a/libs/libltpnuma/tst_numa.c +++ b/libs/libltpnuma/tst_numa.c @@ -161,8 +161,11 @@ static int node_has_enough_memory(int node, size_t min_kb) fclose(fp); - if (!mem_total || !mem_used) { - tst_res(TWARN, "Failed to parse '%s'", path); + tst_res(TINFO,"Node%i: mem_total:%ld kB, mem_used:%ld kB", node, + mem_total, mem_used); + + if (!mem_total) { + tst_res(TWARN, "Failed to parse '%s', MemTotal is 0", path); return 0; }
On Intel sapphire rapids server, BIOS could allocate one memory block for CXL node when the server boot up, and this node "MemUsed" is 0 when CXL is not used like as follow: " cat /sys/devices/system/node/node2/meminfo Node 2 MemTotal: 4194304 kB Node 2 MemFree: 4194304 kB Node 2 MemUsed: 0 kB ... " And it caused get_mempolicy01/02 and set_mempolicy01/02/03/04 cases to fail like as follow sample: " tag=get_mempolicy01 stime=1683272855 cmdline="get_mempolicy01" contacts="" analysis=exit <<<test_output>>> incrementing stop tst_test.c:1560: TINFO: Timeout per run is 0h 00m 30s tst_numa.c:200: TINFO: Found 3 NUMA memory nodes tst_numa.c:165: TWARN: Failed to parse '/sys/devices/system/node/node2/meminfo' get_mempolicy01.c:188: TINFO: test #1: policy: MPOL_DEFAULT, no target get_mempolicy01.c:191: TPASS: policy: MPOL_DEFAULT, no target passed get_mempolicy01.c:188: TINFO: test #2: policy: MPOL_BIND get_mempolicy01.c:191: TPASS: policy: MPOL_BIND passed get_mempolicy01.c:188: TINFO: test #3: policy: MPOL_INTERLEAVE get_mempolicy01.c:191: TPASS: policy: MPOL_INTERLEAVE passed get_mempolicy01.c:188: TINFO: test #4: policy: MPOL_PREFERRED, no target get_mempolicy01.c:191: TPASS: policy: MPOL_PREFERRED, no target passed get_mempolicy01.c:188: TINFO: test #5: policy: MPOL_PREFERRED get_mempolicy01.c:191: TPASS: policy: MPOL_PREFERRED passed get_mempolicy01.c:188: TINFO: test #6: policy: MPOL_DEFAULT, flags: MPOL_F_ADDR, no target get_mempolicy01.c:191: TPASS: policy: MPOL_DEFAULT, flags: MPOL_F_ADDR, no target passed get_mempolicy01.c:188: TINFO: test #7: policy: MPOL_BIND, flags: MPOL_F_ADDR get_mempolicy01.c:191: TPASS: policy: MPOL_BIND, flags: MPOL_F_ADDR passed get_mempolicy01.c:188: TINFO: test #8: policy: MPOL_INTERLEAVE, flags: MPOL_F_ADDR get_mempolicy01.c:191: TPASS: policy: MPOL_INTERLEAVE, flags: MPOL_F_ADDR passed get_mempolicy01.c:188: TINFO: test #9: policy: MPOL_PREFERRED, flags: MPOL_F_ADDR, no target get_mempolicy01.c:191: TPASS: policy: MPOL_PREFERRED, flags: MPOL_F_ADDR, no target passed get_mempolicy01.c:188: TINFO: test #10: policy: MPOL_PREFERRED, flags: MPOL_F_ADDR get_mempolicy01.c:191: TPASS: policy: MPOL_PREFERRED, flags: MPOL_F_ADDR passed Summary: passed 10 failed 0 broken 0 skipped 0 warnings 1 ... -------- ------ ---------- get_mempolicy01 FAIL 4 " So fixed the fake failure when CXL node memory is not being used. Signed-off-by: Pengfei Xu <pengfei.xu@intel.com> --- libs/libltpnuma/tst_numa.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)