Bug 209186

Created attachment 393768 [details] patch

Comment on attachment 393768 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=393768&action=review > Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:145 > + line = fgets(buffer, 128, file); > + token = strtok(line, "\0"); Is this really needed ? Wouldn't it suffice with: token = fgets(buffer, 128, file); ? > Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:153 > + line = fgets(buffer, 128, file); > + token = strtok(line, "\0"); ditto > Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:163 > + line = fgets(buffer, 128, file); > + token = strtok(line, "\0"); ditto > Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:171 > + line = fgets(buffer, 128, file); > + token = strtok(line, "\0"); ditto > Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:239 > + while (char* line = fgets(buffer, 128, file)) { > + char* token = strtok(line, "\n"); > + if (!token) > + break; 128 seems low value here? What happens if there is a line with more than 128 characters? > Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:244 > + if (!strcmp(token, controllerName)) { > + if (nullptr != token) { is this "if (nullptr != token)" really needed given the previous if? > Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:246 > + memoryControllerName = (char*) malloc(sizeof(char) * strlen(token)); This its perhaps leaking the (previously) dynamic allocated char[] on each loop iteration that doesn't break the loop? or the loop assumes to enter here only once? Maybe its better to use a C++ WTF::CString() instead a char* for storing and passing memoryControllerName between function calls? > Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:258 > + snprintf(cgroupPath, maxCgroupPath, s_cgroupMemoryPath, memoryControllerName, "memory.limit_in_bytes"); > + FILE* fileMemoryTotal = fopen(cgroupPath, "r"); > + if (!fileMemoryTotal) > + break; > + fclose(fileMemoryTotal); > + snprintf(cgroupPath, maxCgroupPath, s_cgroupMemoryPath, memoryControllerName, "memory.usage_in_bytes"); > + FILE* fileMemoryUsage = fopen(cgroupPath, "r"); > + if (!fileMemoryUsage) > + break; > + fclose(fileMemoryUsage); What is the purpose of opening a file and then closing it? To simply check for the existence of a file (and about if its readable) I would use access() or something like that

Comment on attachment 393768 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=393768&action=review >> Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:258 >> + fclose(fileMemoryUsage); > > What is the purpose of opening a file and then closing it? > To simply check for the existence of a file (and about if its readable) I would use access() or something like that I think stat() is used more often for this purpose

Created attachment 393830 [details] patch

(In reply to Carlos Alberto Lopez Perez from comment #2) > Comment on attachment 393768 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=393768&action=review > > > Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:145 > > + line = fgets(buffer, 128, file); > > + token = strtok(line, "\0"); > > Is this really needed ? > Wouldn't it suffice with: > token = fgets(buffer, 128, file); > ? Done. > 128 seems low value here? What happens if there is a line with more than 128 > characters? Adjusted to PAGE_SIZE=4096 like `int proc_read_strstr()` in Linux kernel `tools/testing/selftests/cgroup/cgroup_util.c` > > Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:244 > > + if (!strcmp(token, controllerName)) { > > + if (nullptr != token) { > > is this "if (nullptr != token)" really needed given the previous if? > Fixed. > > Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:246 > > + memoryControllerName = (char*) malloc(sizeof(char) * strlen(token)); > > This its perhaps leaking the (previously) dynamic allocated char[] on each > loop iteration that doesn't break the loop? or the loop assumes to enter > here only once? > Maybe its better to use a C++ WTF::CString() instead a char* for storing and > passing memoryControllerName between function calls? bug. Added a break at the end of the `if`. > > Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:258 > > + snprintf(cgroupPath, maxCgroupPath, s_cgroupMemoryPath, memoryControllerName, "memory.limit_in_bytes"); > > + FILE* fileMemoryTotal = fopen(cgroupPath, "r"); > > + if (!fileMemoryTotal) > > + break; > > + fclose(fileMemoryTotal); > > + snprintf(cgroupPath, maxCgroupPath, s_cgroupMemoryPath, memoryControllerName, "memory.usage_in_bytes"); > > + FILE* fileMemoryUsage = fopen(cgroupPath, "r"); > > + if (!fileMemoryUsage) > > + break; > > + fclose(fileMemoryUsage); > > What is the purpose of opening a file and then closing it? > To simply check for the existence of a file (and about if its readable) I > would use access() or something like that Not needed anymore since these are already checked the other functions where they are actually used. Removed in the new patch

(In reply to Konstantin Tokarev from comment #3) > Comment on attachment 393768 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=393768&action=review > > >> Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:258 > >> + fclose(fileMemoryUsage); > > > > What is the purpose of opening a file and then closing it? > > To simply check for the existence of a file (and about if its readable) I would use access() or something like that > > I think stat() is used more often for this purpose I see.

(In reply to Pablo Saavedra from comment #5) > (In reply to Carlos Alberto Lopez Perez from comment #2) > > Comment on attachment 393768 [details] > > patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=393768&action=review > > > > > Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:145 > > > + line = fgets(buffer, 128, file); > > > + token = strtok(line, "\0"); > > > > Is this really needed ? > > Wouldn't it suffice with: > > token = fgets(buffer, 128, file); > > ? > > Done. > > > 128 seems low value here? What happens if there is a line with more than 128 > > characters? > > Adjusted to PAGE_SIZE=4096 like `int proc_read_strstr()` in Linux kernel > `tools/testing/selftests/cgroup/cgroup_util.c` > The value 4096 seems right, but I think it should be PATH_MAX and not PAGE_SIZE. I can see how PATH_MAX limits the maximum length of a line on /proc/self/cgroups, but I can't make the same connection with PAGE_SIZE. Can you?

Page size must not be assumed to be 4K everywhere, it can easily be 64K on some systems. Actual page size can be found via sysconf(_SC_PAGESIZE), as it's done in other places in WebKit

Comment on attachment 393830 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=393830&action=review > Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:52 > +static const unsigned maxCgroupPath = 4096; // PATH_MAX from (Linux) include/uapi/linux/limits.h > +static const unsigned maxCgroupLine = 4096; // PAGE_SIZE 4096 from (Linux) I would use only one variable for this, since its the same value. I think the one right its PATH_MAX. Also, since the value of this variable its only needed at compilation time I would use a #define instead of a variable. > Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:241 > + memoryControllerName = (char*) malloc(sizeof(char) * strlen(token)); I'm still not happy with this. I think it continues to leak because (even when the loop now only enters here once) this function gets called each few seconds by the thread monitor (it calls systemMemoryUsedAsPercentage(), which now calls this), so it leaks strlen(token) bytes per call. If you want to allocate dynamic memory then I suggest to use a C++ String object with automatic memory lifetime cleanup. Other option (if you can define a maximum possible length for the length of token) is to use a stack variable (like you are doing with buffer) and return the address to it.

Comment on attachment 393830 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=393830&action=review >> Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:52 >> +static const unsigned maxCgroupLine = 4096; // PAGE_SIZE 4096 from (Linux) > > I would use only one variable for this, since its the same value. > I think the one right its PATH_MAX. > Also, since the value of this variable its only needed at compilation time I would use a #define instead of a variable. No, you should use C++ constants instead of preprocessor macros, because they are typed and generally safer to use. "static" keyword, though, is an overkill here - all global constants in C++ have internal linkage by standard, even without static keyword.

(In reply to Carlos Alberto Lopez Perez from comment #7) > (In reply to Pablo Saavedra from comment #5) > > (In reply to Carlos Alberto Lopez Perez from comment #2) > > > Comment on attachment 393768 [details] > > > patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=393768&action=review > > > > > > > Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:145 > > > > + line = fgets(buffer, 128, file); > > > > + token = strtok(line, "\0"); > > > > > > Is this really needed ? > > > Wouldn't it suffice with: > > > token = fgets(buffer, 128, file); > > > ? > > > > Done. > > > > > 128 seems low value here? What happens if there is a line with more than 128 > > > characters? > > > > Adjusted to PAGE_SIZE=4096 like `int proc_read_strstr()` in Linux kernel > > `tools/testing/selftests/cgroup/cgroup_util.c` > > > > The value 4096 seems right, but I think it should be PATH_MAX and not > PAGE_SIZE. > I can see how PATH_MAX limits the maximum length of a line on > /proc/self/cgroups, > but I can't make the same connection with PAGE_SIZE. Can you? For example, in Linux, in the `migrating_thread_fn()` in `tools/testing/selftests/cgroup/test_core.c` uses: proc_read_strstr(0, 1, "cgroup", lines[(i % 2) + 1])) and `proc_read_strstr()` uses `char buf[PAGE_SIZE]`

(In reply to Pablo Saavedra from comment #11) > (In reply to Carlos Alberto Lopez Perez from comment #7) > > (In reply to Pablo Saavedra from comment #5) > > > (In reply to Carlos Alberto Lopez Perez from comment #2) > > > > Comment on attachment 393768 [details] > > > > patch > > > > > > > > View in context: > > > > https://bugs.webkit.org/attachment.cgi?id=393768&action=review > > > > > > > > > Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:145 > > > > > + line = fgets(buffer, 128, file); > > > > > + token = strtok(line, "\0"); > > > > > > > > Is this really needed ? > > > > Wouldn't it suffice with: > > > > token = fgets(buffer, 128, file); > > > > ? > > > > > > Done. > > > > > > > 128 seems low value here? What happens if there is a line with more than 128 > > > > characters? > > > > > > Adjusted to PAGE_SIZE=4096 like `int proc_read_strstr()` in Linux kernel > > > `tools/testing/selftests/cgroup/cgroup_util.c` > > > > > > > The value 4096 seems right, but I think it should be PATH_MAX and not > > PAGE_SIZE. > > I can see how PATH_MAX limits the maximum length of a line on > > /proc/self/cgroups, > > but I can't make the same connection with PAGE_SIZE. Can you? > > For example, in Linux, in the `migrating_thread_fn()` in > `tools/testing/selftests/cgroup/test_core.c` uses: > > proc_read_strstr(0, 1, "cgroup", lines[(i % 2) + 1])) > > and `proc_read_strstr()` uses `char buf[PAGE_SIZE]` I saw that, but I think that this is either a mistake or in their use case they don't care about reading whole lines at once. Why would the length of a line in the file /proc/self/cgroup depend on the value of PAGE_SIZE? Which one is the explanation for that dependency?

Created attachment 393878 [details] patch

Created attachment 393891 [details] patch

Created attachment 393893 [details] patch

Created attachment 393898 [details] patch

Created attachment 393902 [details] patch

Comment on attachment 393902 [details] patch This needs a bit of testing, which I did and doesn't seem to be working: I applied this patch over this: http://sprunge.us/Siqbdt and ran: sudo cgcreate -a $USER:$USER -s 777 -g memory:/testmemorycontroller echo 2G > /sys/fs/cgroup/memory/testmemorycontroller/memory.limit_in_bytes echo 3G > /sys/fs/cgroup/memory/testmemorycontroller/memory.memsw.limit_in_bytes cgexec -g memory:testmemorycontroller Tools/Scripts/run-minibrowser --gtk Starting MiniBrowser. GLib-GIO-Message: 23:05:13.451: Using the 'memory' GSettings backend. Your settings will not be saved or shared with other applications. returning controller value /testmemorycontrolle memory usage with cgroup is 0 memoryUsagePercentageWithCgroup is -2147483648 and memoryUsagePercentage was 52 final memoryUsagePercentage is 52 returning controller value /testmemorycontrolle memory usage with cgroup is 0 memoryUsagePercentageWithCgroup is -2147483648 and memoryUsagePercentage was 52 final memoryUsagePercentage is 52 [...] Notice how it gets wrong the name of the controller (it eats the last char, the "r"). I also noticed this warnings when building the patch (with clang): In file included from DerivedSources/WebKit/unified-sources/UnifiedSource-88d1702b-28.cpp:6: ../../Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:175:8: warning: 'const' type qualifier on return type has no effect [-Wignored-qualifiers] static const size_t getMemoryUsageWithCgroup(const char* memoryControllerName) ^~~~~~ ../../Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:245:20: warning: address of stack memory associated with local variable 'memoryControllerName' returned [-Wreturn-stack-address] return memoryControllerName; ^~~~~~~~~~~~~~~~~~~~ It seems returning the addres of a stack variable its not ok, so I guess its better to use a c++ string object and dynamic allocate it.

Created attachment 393919 [details] patch

Comment on attachment 393919 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=393919&action=review > Source/WebKit/ChangeLog:28 > +2020-03-18 Pablo Saavedra <psaavedra@igalia.com> > + > + [GTK][WPE] Check the cgroups memory limits (v1 and v2) to calculate the systemMemoryUsedAsPercentage() in the MemoryPressureMonitor > + https://bugs.webkit.org/show_bug.cgi?id=209186 > + > + Reviewed by NOBODY (OOPS!). > + > + * UIProcess/linux/MemoryPressureMonitor.cpp: > + (WebKit::getMemoryTotalWithCgroup): > + (WebKit::getMemoryUsageWithCgroup): > + (WebKit::getCgroupController): > + (WebKit::systemMemoryUsedAsPercentage): > + > +2020-03-17 Pablo Saavedra <psaavedra@igalia.com> > + > + [GTK][WPE] Check the cgroups memory limits (v1 and v2) to calculate the systemMemoryUsedAsPercentage() in the MemoryPressureMonitor > + https://bugs.webkit.org/show_bug.cgi?id=209186 > + > + Reviewed by NOBODY (OOPS!). > + > + No new tests. > + > + * UIProcess/linux/MemoryPressureMonitor.cpp: > + (WebKit::getMemoryTotalWithCgroup): > + (WebKit::getMemoryUsageWithCgroup): > + (WebKit::getCgroupController): > + (WebKit::systemMemoryUsedAsPercentage): > + Double changelog. Also, if possible please explain a bit what this patch does in the changelog. > Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:134 > +size_t getMemoryTotalWithCgroup(CString memoryControllerName) Please add an "#include <wtf/text/CString.h>" at the top of the file. Otherwise this will likely fail to build without unified builds. > Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:321 > + memoryTotal = getMemoryTotalWithCgroup(memoryControllerName); > + size_t memoryUsage = getMemoryUsageWithCgroup(memoryControllerName); > + int memoryUsagePercentageWithCgroup = 100 * (float) memoryUsage / (float) memoryTotal; What happens if getMemoryTotalWithCgroup(memoryControllerName) returns 0 (for example, because no permission to read the cgroup memory info files) ? Add a if-check here for that to avoid a division by zero. Also: I don't think the casts to float are needed. > Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:322 > + if (memoryTotal && (memoryUsagePercentageWithCgroup > memoryUsagePercentage)) I see you check here for memoryTotal to be non-zero, so just move that check above to avoid the division by zero in that case

Created attachment 393956 [details] patch

Comment on attachment 393956 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=393956&action=review > Source/WebKit/ChangeLog:31 > + > + Reviewed by NOBODY (OOPS!). > + This line has to go just below the bug URL. See other changelogs for how we do it. > Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:323 > + int memoryUsagePercentageWithCgroup = 100 * (float) memoryUsage / (float) memoryTotal; Are the float casts needed given that the result its stored in an int? This gives the same result to me: int x = 10 * 3 / 5; int x = 10 * (float) 3 / (float) 5;

(In reply to Carlos Alberto Lopez Perez from comment #22) > Are the float casts needed given that the result its stored in an int? > This gives the same result to me: > int x = 10 * 3 / 5; > int x = 10 * (float) 3 / (float) 5; I correct myself, it doesn't give the same result, but you have to use parenthesis to make the division happen first. int x = 10* (5 / 3); // gives 10 int x = 10* ((float) 5 / (float) 3); // gives 16 In this specific case it seems it didn't mattered because we are multiplying by 100 before doing the division

Created attachment 393996 [details] patch

(In reply to Carlos Alberto Lopez Perez from comment #23) > (In reply to Carlos Alberto Lopez Perez from comment #22) > > > Are the float casts needed given that the result its stored in an int? > > This gives the same result to me: > > int x = 10 * 3 / 5; > > int x = 10 * (float) 3 / (float) 5; > > I correct myself, it doesn't give the same result, but you have to use > parenthesis to make the division happen first. > > int x = 10* (5 / 3); // gives 10 > int x = 10* ((float) 5 / (float) 3); // gives 16 > > In this specific case it seems it didn't mattered because we are multiplying > by 100 before doing the division The last patch reflects your ideas. Could you confirm this is ok for you? Thanks in advance.

Comment on attachment 393996 [details] patch thanks!

Committed r258835: <https://trac.webkit.org/changeset/258835> All reviewed patches have been landed. Closing bug and clearing flags on attachment 393996 [details].