8242480: Negative value may be returned by getFreeSwapSpaceSize() in the docker(Internet mail)

[Severin Gehwolf]

Hi Jie,

On Thu, 2020-04-16 at 13:23 +0000, jiefu(傅杰) wrote:
> Hi Severin,
> 
> Thanks for your review and very nice suggestions.

Thanks for adding a test!

> Updated: http://cr.openjdk.java.net/~jiefu/8242480/webrev.01/
> 
> test/hotspot/jtreg/containers/docker/TestGetFreeSwapSpaceSize.java is
> added to reproduce the bug.

Since you've added a new test, please move them to the jdk docker tests
in: 
test/jdk/jdk/internal/platform/docker/

This is because the OperatingSystemMXBean uses the Metrics.java
internal API (not hotspot cgroup support) and the tests really belong
there alongside other Metrics docker tests.

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

+ * @build sun.hotspot.WhiteBox GetFreeSwapSpaceSize
+ * @run driver ClassFileInstaller -jar whitebox.jar sun.hotspot.WhiteBox sun.hotspot.WhiteBox$WhiteBoxPermission

I don't see any reason why WhiteBox would be needed for this test. Is
that an oversight or am I missing something?

Thanks,
Severin

> Thanks a lot.
> Best regards,
> Jie
> 
> 
> On 2020/4/16, 4:40 PM, "Severin Gehwolf" <sgehwolf at redhat.com>
> wrote:
> 
>     Hi Jie,
>     
>     On Fri, 2020-04-10 at 01:49 +0000, jiefu(傅杰) wrote:
>     > Hi all,
>     >  
>     > JBS:    https://bugs.openjdk.java.net/browse/JDK-8242480
>     > Webrev: http://cr.openjdk.java.net/~jiefu/8242480/webrev.00/
>     >  
>     > Negative values were returned by getFreeSwapSpaceSize() in our
> docker testing.
>     > The reason is that current implementation doesn't consider the
> situation when memory.limit_in_bytes == memory.memsw.limit_in_bytes,
> which means do not use the swap space at all.
>     >  
>     > In
> src/jdk.management/unix/classes/com/sun/management/internal/Operating
> SystemImpl.java, let's see
>     > ------------------------------------------------
>     >  71     public long getFreeSwapSpaceSize() {
>     >  72         if (containerMetrics != null) {
>     >  73             long memSwapLimit =
> containerMetrics.getMemoryAndSwapLimit();
>     >  74             long memLimit =
> containerMetrics.getMemoryLimit();
>     >  75             if (memSwapLimit >= 0 && memLimit >= 0) {
>     >  76                 for (int attempt = 0; attempt <
> MAX_ATTEMPTS_NUMBER; attempt++) {
>     >  77                     long memSwapUsage =
> containerMetrics.getMemoryAndSwapUsage();
>     >  78                     long memUsage =
> containerMetrics.getMemoryUsage();
>     >  79                     if (memSwapUsage > 0 && memUsage > 0) {
>     >  80                         // We read "memory usage" and
> "memory and swap usage" not atomically,
>     >  81                         // and it's possible to get the
> negative value when subtracting these two.
>     >  82                         // If this happens just retry the
> loop for a few iterations.
>     >  83                         if ((memSwapUsage - memUsage) >= 0)
> {
>     >  84                             return memSwapLimit - memLimit
> - (memSwapUsage - memUsage);
>     >  85                         }
>     >  86                     }
>     >  87                 }
>     >  88             }
>     >  89         }
>     >  90         return getFreeSwapSpaceSize0();
>     >  91     }
>     > ------------------------------------------------
>     > If memSwapLimit (@line 73) equals memLimit (@line 74), then
> getFreeSwapSpaceSize() may return a negative value @line 84.
>     >  
>     > It would be better to fix it.
>     > Could you please review it and give me some advice?
>     
>     Would this be reproducible via a test? There is
>     test/hotspot/jtreg/containers/docker/TestMemoryAwareness.java
> which
>     contains testOperatingSystemMXBeanAwareness() tests.
>     
>     It would be good to capture this in a test somehow.
>     
>     Thanks,
>     Severin
>     
>     
>     
>