| Message ID | 20240301062716.3023-1-wegao@suse.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v1] libswap.c: Improve caculate swap dev number | expand |
Hi Wei > I encounter a dead loop on following code in our test on ppc64 machine: > while ((c = fgetc(fp)) != EOF) > > I think "/proc/swaps" is not normal file and can not get EOF in some situation, > so i use fgets a line and caculate swap dev number. > > Signed-off-by: Wei Gao <wegao@suse.com> > --- > libs/libltpswap/libswap.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/libs/libltpswap/libswap.c b/libs/libltpswap/libswap.c > index c8cbb8ea1..6924066b7 100644 > --- a/libs/libltpswap/libswap.c > +++ b/libs/libltpswap/libswap.c > @@ -13,6 +13,7 @@ > > #define TST_NO_DEFAULT_MAIN > #define DEFAULT_MAX_SWAPFILE 32 > +#define MAX_LINE_LEN 256 > > #include "tst_test.h" > #include "libswap.h" > @@ -274,16 +275,17 @@ int tst_max_swapfiles(void) > int tst_count_swaps(void) > { > FILE *fp; > - int used = -1; > - char c; > + int used = 0; > > fp = SAFE_FOPEN("/proc/swaps", "r"); > if (fp == NULL) > return -1; > > - while ((c = fgetc(fp)) != EOF) { > - if (c == '\n') > + char line[MAX_LINE_LEN]; > + while (fgets(line, MAX_LINE_LEN, fp) != NULL) { > + if (strstr(line, "/dev/") != NULL) { > used++; > + } > } You are not the first person to meet this deadloop problem, Petr also met this[1] in my v4 patch.. But I don't think it related to /proc/swapfiles, I doubot libc wrapper for fgetc problem on ppc64 machine. Can you try fgetc problem by using fgetc api in ipc library[2]? Then we can know the right reason whether is /proc/swaps or getc problem. If so, I think we can change this as my v2 way[3]. [1]https://patchwork.ozlabs.org/project/ltp/patch/20240220074218.13487-3-xuyang2018.jy@fujitsu.com/ [2]https://github.com/linux-test-project/ltp/blob/master/libs/libltpnewipc/libnewipc.c#L58 [3]https://patchwork.ozlabs.org/project/ltp/patch/20231222050006.148845-2-xuyang2018.jy@fujitsu.com/ Best Regards Yang Xu > > SAFE_FCLOSE(fp);
Hi Wei, Xu, > Hi Wei > > I encounter a dead loop on following code in our test on ppc64 machine: > > while ((c = fgetc(fp)) != EOF) > > I think "/proc/swaps" is not normal file and can not get EOF in some situation, > > so i use fgets a line and caculate swap dev number. I guess the problem has been fixed by another patch [1], thus closing it in patchwork. Please let me know if not. Kind regards, Petr [1] https://github.com/linux-test-project/ltp/commit/0f5d8c520d4e5b22325526eab41ed6bcad1847fc
On Sun, Mar 3, 2024 at 9:14 PM Petr Vorel <pvorel@suse.cz> wrote: > Hi Wei, Xu, > > > Hi Wei > > > > I encounter a dead loop on following code in our test on ppc64 machine: > > > while ((c = fgetc(fp)) != EOF) > > > > I think "/proc/swaps" is not normal file and can not get EOF in some > situation, > > > so i use fgets a line and caculate swap dev number. > > I guess the problem has been fixed by another patch [1], thus closing it in > patchwork. Please let me know if not. > Seems not, the patch [1] below is mainly to count the free disk size. But Wei's work here is to calculate swap-dev numbers correctly (especially get rid of EOF affection to some degree). Xu Yang, what do you think? or did I miss anything here?
> On Sun, Mar 3, 2024 at 9:14 PM Petr Vorel <pvorel@suse.cz> wrote: > > Hi Wei, Xu, > > > Hi Wei > > > > I encounter a dead loop on following code in our test on ppc64 machine: > > > > while ((c = fgetc(fp)) != EOF) > > > > I think "/proc/swaps" is not normal file and can not get EOF in some > > situation, > > > > so i use fgets a line and caculate swap dev number. > > I guess the problem has been fixed by another patch [1], thus closing it in > > patchwork. Please let me know if not. > Seems not, the patch [1] below is mainly to count the free disk size. > But Wei's work here is to calculate swap-dev numbers correctly > (especially get rid of EOF affection to some degree). OK, setting it as "New" again. Kind regards, Petr > Xu Yang, what do you think? or did I miss anything here?
Hi Li， > > > On Sun, Mar 3, 2024 at 9:14 PM Petr Vorel <pvorel@suse.cz > <mailto:pvorel@suse.cz>> wrote: > > Hi Wei, Xu, > > > Hi Wei > > > > I encounter a dead loop on following code in our test on ppc64 > machine: > > > while ((c = fgetc(fp)) != EOF) > > > > I think "/proc/swaps" is not normal file and can not get EOF in > some situation, > > > so i use fgets a line and caculate swap dev number. > > I guess the problem has been fixed by another patch [1], thus > closing it in > patchwork. Please let me know if not. > > > Seems not, the patch [1] below is mainly to count the free disk size. > > But Wei's work here is to calculate swap-dev numbers correctly > (especially get rid of EOF affection to some degree). > > Xu Yang, what do you think? or did I miss anything here? > > I still think we can use the same way in ipc libs to get rid of the EOF problem instead of searching "dev" keyword. We just don't need to calcualte "/proc/swaps" header. int get_used_sysvipc(const char *file, const int lineno, const char *sysvipc_file) { FILE *fp; int used = -1; char buf[BUFSIZE]; fp = safe_fopen(file, lineno, NULL, sysvipc_file, "r"); while (fgets(buf, BUFSIZE, fp) != NULL) used++; fclose(fp); if (used < 0) { tst_brk(TBROK, "can't read %s to get used sysvipc resource total at " "%s:%d", sysvipc_file, file, lineno); } return used; } But we don't get the actual reason, I still wonder why this deadloop exists o ppcc64 instead of other architecture(ie x86_64). Best Regards Yang Xu > > -- > Regards, > Li Wang
On Mon, Mar 4, 2024 at 5:19 PM Yang Xu (Fujitsu) <xuyang2018.jy@fujitsu.com> wrote: > Hi Li， > > > > > > > On Sun, Mar 3, 2024 at 9:14 PM Petr Vorel <pvorel@suse.cz > > <mailto:pvorel@suse.cz>> wrote: > > > > Hi Wei, Xu, > > > > > Hi Wei > > > > > > I encounter a dead loop on following code in our test on ppc64 > > machine: > > > > while ((c = fgetc(fp)) != EOF) > > > > > > I think "/proc/swaps" is not normal file and can not get EOF in > > some situation, > > > > so i use fgets a line and caculate swap dev number. > > > > I guess the problem has been fixed by another patch [1], thus > > closing it in > > patchwork. Please let me know if not. > > > > > > Seems not, the patch [1] below is mainly to count the free disk size. > > > > But Wei's work here is to calculate swap-dev numbers correctly > > (especially get rid of EOF affection to some degree). > > > > Xu Yang, what do you think? or did I miss anything here? > > > > > > I still think we can use the same way in ipc libs to get rid of the EOF > problem instead of > searching "dev" keyword. We just don't need to calcualte "/proc/swaps" > header. > Sounds good to me. At least it counts right lines. It'd be great to have a patch by that way. > > int get_used_sysvipc(const char *file, const int lineno, const char > *sysvipc_file) > { > FILE *fp; > int used = -1; > char buf[BUFSIZE]; > > fp = safe_fopen(file, lineno, NULL, sysvipc_file, "r"); > > while (fgets(buf, BUFSIZE, fp) != NULL) > used++; > > fclose(fp); > > if (used < 0) { > tst_brk(TBROK, "can't read %s to get used sysvipc resource > total at " > "%s:%d", sysvipc_file, file, lineno); > } > > return used; > } > > But we don't get the actual reason, I still wonder why this deadloop > exists o ppcc64 instead of > other architecture(ie x86_64). > Ok, I think we can just apply your suggested method to see if that deadloop disappears.
On Mon, Mar 04, 2024 at 06:04:08PM +0800, Li Wang wrote: > On Mon, Mar 4, 2024 at 5:19 PM Yang Xu (Fujitsu) <xuyang2018.jy@fujitsu.com> > wrote: > > > Hi Li， > > > > > > > > > > > On Sun, Mar 3, 2024 at 9:14 PM Petr Vorel <pvorel@suse.cz > > > <mailto:pvorel@suse.cz>> wrote: > > > > > > Hi Wei, Xu, > > > > > > > Hi Wei > > > > > > > > I encounter a dead loop on following code in our test on ppc64 > > > machine: > > > > > while ((c = fgetc(fp)) != EOF) > > > > > > > > I think "/proc/swaps" is not normal file and can not get EOF in > > > some situation, > > > > > so i use fgets a line and caculate swap dev number. > > > > > > I guess the problem has been fixed by another patch [1], thus > > > closing it in > > > patchwork. Please let me know if not. > > > > > > > > > Seems not, the patch [1] below is mainly to count the free disk size. > > > > > > But Wei's work here is to calculate swap-dev numbers correctly > > > (especially get rid of EOF affection to some degree). > > > > > > Xu Yang, what do you think? or did I miss anything here? > > > > > > > > > > I still think we can use the same way in ipc libs to get rid of the EOF > > problem instead of > > searching "dev" keyword. We just don't need to calcualte "/proc/swaps" > > header. > > > > Sounds good to me. At least it counts right lines. > > It'd be great to have a patch by that way. > > > > > > > int get_used_sysvipc(const char *file, const int lineno, const char > > *sysvipc_file) > > { > > FILE *fp; > > int used = -1; > > char buf[BUFSIZE]; > > > > fp = safe_fopen(file, lineno, NULL, sysvipc_file, "r"); > > > > while (fgets(buf, BUFSIZE, fp) != NULL) > > used++; > > > > fclose(fp); > > > > if (used < 0) { > > tst_brk(TBROK, "can't read %s to get used sysvipc resource > > total at " > > "%s:%d", sysvipc_file, file, lineno); > > } > > > > return used; > > } > > > > But we don't get the actual reason, I still wonder why this deadloop > > exists o ppcc64 instead of > > other architecture(ie x86_64). > > > > Ok, I think we can just apply your suggested method to see if that deadloop > disappears. > Hi Petr, Xu, Li Deadloop will disappear if you use fgets, fgetc can not get EOF on ppc64.(Suspect an bug) So either use my patch or Xu's suggestion both can work. > > > -- > Regards, > Li Wang > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp
> On Mon, Mar 04, 2024 at 06:04:08PM +0800, Li Wang wrote: > > On Mon, Mar 4, 2024 at 5:19 PM Yang Xu (Fujitsu) <xuyang2018.jy@fujitsu.com> > > wrote: > > > Hi Li， > > > > On Sun, Mar 3, 2024 at 9:14 PM Petr Vorel <pvorel@suse.cz > > > > <mailto:pvorel@suse.cz>> wrote: > > > > Hi Wei, Xu, > > > > > Hi Wei > > > > > > I encounter a dead loop on following code in our test on ppc64 > > > > machine: > > > > > > while ((c = fgetc(fp)) != EOF) > > > > > > I think "/proc/swaps" is not normal file and can not get EOF in > > > > some situation, > > > > > > so i use fgets a line and caculate swap dev number. > > > > I guess the problem has been fixed by another patch [1], thus > > > > closing it in > > > > patchwork. Please let me know if not. > > > > Seems not, the patch [1] below is mainly to count the free disk size. > > > > But Wei's work here is to calculate swap-dev numbers correctly > > > > (especially get rid of EOF affection to some degree). > > > > Xu Yang, what do you think? or did I miss anything here? > > > I still think we can use the same way in ipc libs to get rid of the EOF > > > problem instead of > > > searching "dev" keyword. We just don't need to calcualte "/proc/swaps" > > > header. > > Sounds good to me. At least it counts right lines. > > It'd be great to have a patch by that way. > > > int get_used_sysvipc(const char *file, const int lineno, const char > > > *sysvipc_file) > > > { > > > FILE *fp; > > > int used = -1; > > > char buf[BUFSIZE]; > > > fp = safe_fopen(file, lineno, NULL, sysvipc_file, "r"); > > > while (fgets(buf, BUFSIZE, fp) != NULL) > > > used++; > > > fclose(fp); > > > if (used < 0) { > > > tst_brk(TBROK, "can't read %s to get used sysvipc resource > > > total at " > > > "%s:%d", sysvipc_file, file, lineno); > > > } > > > return used; > > > } > > > But we don't get the actual reason, I still wonder why this deadloop > > > exists o ppcc64 instead of > > > other architecture(ie x86_64). FYI also other architectures fails (eg.g least aarch64, s390x and ppc64le which was in the original report; it looks to me it works on x86_64). Problem is somewhere in c1b8c011e447088b08787462e0c2ed50cd8c43f3..8fd941649afeecc5f87508c9f94e9a840a84e44d which contains 319693d0b ("libltpswap: alter tst_count_swaps API"), which is Wei trying to fix. > > Ok, I think we can just apply your suggested method to see if that deadloop > > disappears. > Hi Petr, Xu, Li > Deadloop will disappear if you use fgets, fgetc can not get EOF on ppc64.(Suspect an bug) > So either use my patch or Xu's suggestion both can work. I would vote for Xu's suggestion (any line except the first header is valid), actually this implementation is not correct when it counts only /dev: # dd if=/dev/zero of=/root/swap bs=100M count=1 # mkswap -f /root/swap # swapon /root/swap # cat /proc/swaps Filename Type Size Used Priority /dev/dm-0 partition 192508 4048 -2 /root/swap file 102396 0 -3 (second line without /dev) Also typo in the subject: s/caculate/calculate/ Wei, please send new version and Cc us. Kind regards, Petr
diff --git a/libs/libltpswap/libswap.c b/libs/libltpswap/libswap.c index c8cbb8ea1..6924066b7 100644 --- a/libs/libltpswap/libswap.c +++ b/libs/libltpswap/libswap.c @@ -13,6 +13,7 @@ #define TST_NO_DEFAULT_MAIN #define DEFAULT_MAX_SWAPFILE 32 +#define MAX_LINE_LEN 256 #include "tst_test.h" #include "libswap.h" @@ -274,16 +275,17 @@ int tst_max_swapfiles(void) int tst_count_swaps(void) { FILE *fp; - int used = -1; - char c; + int used = 0; fp = SAFE_FOPEN("/proc/swaps", "r"); if (fp == NULL) return -1; - while ((c = fgetc(fp)) != EOF) { - if (c == '\n') + char line[MAX_LINE_LEN]; + while (fgets(line, MAX_LINE_LEN, fp) != NULL) { + if (strstr(line, "/dev/") != NULL) { used++; + } } SAFE_FCLOSE(fp);
I encounter a dead loop on following code in our test on ppc64 machine: while ((c = fgetc(fp)) != EOF) I think "/proc/swaps" is not normal file and can not get EOF in some situation, so i use fgets a line and caculate swap dev number. Signed-off-by: Wei Gao <wegao@suse.com> --- libs/libltpswap/libswap.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)