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

[jiefu(傅杰)]

Thanks Severin and David for your review.
Will push it tomorrow.

Best regards,
Jie


On 2020/4/17, 8:56 PM, "David Holmes" <david.holmes at oracle.com> wrote:

    On 17/04/2020 5:00 pm, jiefu(傅杰) wrote:
    > Hi David,
    > 
    > Updated: http://cr.openjdk.java.net/~jiefu/8242480/webrev.02/
    > 
    > The file header had been fixed. Please review it.
    
    File header update looks good.
    
    Thanks,
    David
    
    > Thanks a lot.
    > Best regards,
    > Jie
    > 
    > On 2020/4/17, 11:59 AM, "David Holmes" <david.holmes at oracle.com> wrote:
    > 
    >      Hi Jie,
    >      
    >      On 16/04/2020 11:23 pm, jiefu(傅杰) wrote:
    >      > Hi Severin,
    >      >
    >      > Thanks for your review and very nice suggestions.
    >      >
    >      > Updated: http://cr.openjdk.java.net/~jiefu/8242480/webrev.01/
    >      >
    >      > test/hotspot/jtreg/containers/docker/TestGetFreeSwapSpaceSize.java is added to reproduce the bug.
    >      
    >      
    >      Can you please use the standard OpenJDK file header after your Tencent
    >      copyright line:
    >      
    >        * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
    >        *
    >        * This code is free software; you can redistribute it and/or modify it
    >        * under the terms of the GNU General Public License version 2 only, as
    >        * published by the Free Software Foundation.
    >        *
    >        * This code is distributed in the hope that it will be useful, but WITHOUT
    >        * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
    >        * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
    >        * version 2 for more details (a copy is included in the LICENSE file that
    >        * accompanied this code).
    >        *
    >        * You should have received a copy of the GNU General Public License
    >      version
    >        * 2 along with this work; if not, write to the Free Software Foundation,
    >        * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
    >        *
    >        * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
    >        * or visit www.oracle.com if you need additional information or have any
    >        * questions.
    >        */
    >      
    >      I don't think the "classpath exception" is relevant to tests - certainly
    >      other tests I checked do not have it.
    >      
    >      Thanks,
    >      David
    >      -----
    >      
    >      > 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/OperatingSystemImpl.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
    >      >
    >      >
    >      >
    >      >
    >      
    >      
    >