8300658: memory_and_swap_limit() reporting wrong values on systems with swapaccount=0

Reviewed-by: jsjolen, iklam

Author: jerboaa

src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp

@@ -111,7 +111,19 @@ jlong CgroupV1Subsystem::read_memory_limit_in_bytes() {
   }
 }
 
-jlong CgroupV1Subsystem::memory_and_swap_limit_in_bytes() {
+/* read_mem_swap
+ *
+ * Determine the memory and swap limit metric. Returns a positive limit value strictly
+ * lower than the physical memory and swap limit iff there is a limit. Otherwise a
+ * negative value is returned indicating the determined status.
+ *
+ * returns:
+ *    * A number > 0 if the limit is available and lower than a physical upper bound.
+ *    * OSCONTAINER_ERROR if the limit cannot be retrieved (i.e. not supported) or
+ *    * -1 if there isn't any limit in place (note: includes values which exceed a physical
+ *      upper bound)
+ */
+jlong CgroupV1Subsystem::read_mem_swap() {
   julong host_total_memsw;
   GET_CONTAINER_INFO(julong, _memory->controller(), "/memory.memsw.limit_in_bytes",
                      "Memory and Swap Limit is: ", JULONG_FORMAT, JULONG_FORMAT, memswlimit);
@@ -126,29 +138,36 @@ jlong CgroupV1Subsystem::memory_and_swap_limit_in_bytes() {
       if (hier_memswlimit >= host_total_memsw) {
         log_trace(os, container)("Hierarchical Memory and Swap Limit is: Unlimited");
       } else {
-        jlong swappiness = read_mem_swappiness();
-        if (swappiness == 0) {
-            const char* matchmemline = "hierarchical_memory_limit";
-            GET_CONTAINER_INFO_LINE(julong, _memory->controller(), "/memory.stat", matchmemline,
-                             "Hierarchical Memory Limit is : " JULONG_FORMAT, JULONG_FORMAT, hier_memlimit)
-            log_trace(os, container)("Memory and Swap Limit has been reset to " JULONG_FORMAT " because swappiness is 0", hier_memlimit);
-            return (jlong)hier_memlimit;
-        }
         return (jlong)hier_memswlimit;
       }
     }
     return (jlong)-1;
   } else {
-    jlong swappiness = read_mem_swappiness();
-    if (swappiness == 0) {
-      jlong memlimit = read_memory_limit_in_bytes();
-      log_trace(os, container)("Memory and Swap Limit has been reset to " JULONG_FORMAT " because swappiness is 0", memlimit);
-      return memlimit;
-    }
     return (jlong)memswlimit;
   }
 }
 
+jlong CgroupV1Subsystem::memory_and_swap_limit_in_bytes() {
+  jlong memory_swap = read_mem_swap();
+  if (memory_swap == -1) {
+    return memory_swap;
+  }
+  // If there is a swap limit, but swappiness == 0, reset the limit
+  // to the memory limit. Do the same for cases where swap isn't
+  // supported.
+  jlong swappiness = read_mem_swappiness();
+  if (swappiness == 0 || memory_swap == OSCONTAINER_ERROR) {
+    jlong memlimit = read_memory_limit_in_bytes();
+    if (memory_swap == OSCONTAINER_ERROR) {
+      log_trace(os, container)("Memory and Swap Limit has been reset to " JLONG_FORMAT " because swap is not supported", memlimit);
+    } else {
+      log_trace(os, container)("Memory and Swap Limit has been reset to " JLONG_FORMAT " because swappiness is 0", memlimit);
+    }
+    return memlimit;
+  }
+  return memory_swap;
+}
+
 jlong CgroupV1Subsystem::read_mem_swappiness() {
   GET_CONTAINER_INFO(julong, _memory->controller(), "/memory.swappiness",
                      "Swappiness is: ", JULONG_FORMAT, JULONG_FORMAT, swappiness);

src/hotspot/os/linux/cgroupV1Subsystem_linux.hpp

@@ -114,6 +114,7 @@ class CgroupV1Subsystem: public CgroupSubsystem {
     char * pids_max_val();
 
     jlong read_mem_swappiness();
+    jlong read_mem_swap();
 
   public:
     CgroupV1Subsystem(CgroupV1Controller* cpuset,

src/hotspot/os/linux/cgroupV2Subsystem_linux.cpp

@@ -152,6 +152,12 @@ char* CgroupV2Subsystem::mem_soft_limit_val() {
 // without also setting a memory limit is not allowed.
 jlong CgroupV2Subsystem::memory_and_swap_limit_in_bytes() {
   char* mem_swp_limit_str = mem_swp_limit_val();
+  if (mem_swp_limit_str == nullptr) {
+    // Some container tests rely on this trace logging to happen.
+    log_trace(os, container)("Memory and Swap Limit is: %d", OSCONTAINER_ERROR);
+    // swap disabled at kernel level, treat it as no swap
+    return read_memory_limit_in_bytes();
+  }
   jlong swap_limit = limit_from_str(mem_swp_limit_str);
   if (swap_limit >= 0) {
     jlong memory_limit = read_memory_limit_in_bytes();

test/hotspot/jtreg/containers/docker/TestMemoryAwareness.java

@@ -77,6 +77,8 @@ public static void main(String[] args) throws Exception {
             testMemorySoftLimit("1g", "1073741824");
             testMemorySwapLimitSanity();
 
+            testMemorySwapNotSupported("500m", "520m", "512000 k", "532480 k");
+
             // Add extra 10 Mb to allocator limit, to be sure to cause OOM
             testOOM("256m", 256 + 10);
 
@@ -154,6 +156,29 @@ private static void testMemorySoftLimit(String valueToSet, String expectedTraceV
             .shouldMatch("Memory Soft Limit.*" + expectedTraceValue);
     }
 
+    /*
+     * Verifies that PrintContainerInfo prints the memory
+     * limit - without swap - iff swap is disabled (e.g. via swapaccount=0). It must
+     * not print 'not supported' for that value in that case. It'll always pass
+     * on systems with swap accounting enabled.
+     */
+    private static void testMemorySwapNotSupported(String valueToSet, String swapToSet, String expectedMem, String expectedSwap)
+            throws Exception {
+        Common.logNewTestCase("memory swap not supported: " + valueToSet);
+
+        DockerRunOptions opts = Common.newOpts(imageName, "PrintContainerInfo");
+        Common.addWhiteBoxOpts(opts);
+        opts.addDockerOpts("--memory=" + valueToSet);
+        opts.addDockerOpts("--memory-swap=" + swapToSet);
+
+        Common.run(opts)
+            .shouldMatch("memory_limit_in_bytes:.*" + expectedMem)
+            .shouldNotMatch("memory_and_swap_limit_in_bytes:.*not supported")
+            // On systems with swapaccount=0 this returns the memory limit.
+            // On systems with swapaccount=1 this returns the set memory+swap value.
+            .shouldMatch("memory_and_swap_limit_in_bytes:.*(" + expectedMem + "|" + expectedSwap + ")");
+    }
+
     /*
      * This test verifies that no confusingly large positive numbers get printed on
      * systems with swapaccount=0 kernel option. On some systems -2 were converted

test/hotspot/jtreg/containers/docker/TestMemoryWithCgroupV1.java

@@ -85,8 +85,8 @@ private static void testMemoryLimitWithSwappiness(String dockerMemLimit, String
         // capabilities or the cgroup is not mounted. Memory limited without swap."
         // we only have 'Memory and Swap Limit is: -2' in the output
         try {
-            if (out.getOutput().contains("memory_and_swap_limit_in_bytes: not supported")) {
-                System.out.println("memory_and_swap_limit_in_bytes not supported, avoiding Memory and Swap Limit check");
+            if (out.getOutput().contains("Memory and Swap Limit is: -2")) {
+                System.out.println("System doesn't seem to allow swap, avoiding Memory and Swap Limit check");
             } else {
                 out.shouldContain("Memory and Swap Limit is: " + expectedReadLimit)
                     .shouldContain(