Question about JDK-8043780 (was: RFR: 8237830: support O_CLOEXEC in os::open on other OS than Linux)

[Baesken, Matthias]

>I wonder whether we could shorten the coding to a simple, unconditional
>
>::fcntl(fd, F_SETFD, flags | FD_CLOEXEC);


Looks like we would need to call first  fcntl with   F_GETFD  AND then F_SETFD   (or where  would flags come from?) :

    int flags = ::fcntl(fd, F_GETFD);
    ::fcntl(fd, F_SETFD, flags | FD_CLOEXEC);

That’s 2  calls  that are not really needed .


Best regards, Matthias


From: Baesken, Matthias
Sent: Dienstag, 11. Februar 2020 10:09
To: 'Thomas Stüfe' <thomas.stuefe at gmail.com>; Martin Buchholz <martinrb at google.com>
Cc: David Holmes <david.holmes at oracle.com>; hotspot-dev at openjdk.java.net
Subject: RE: Question about JDK-8043780 (was: RFR: 8237830: support O_CLOEXEC in os::open on other OS than Linux)

Ping – any comments on this ?

https://bugs.openjdk.java.net/browse/JDK-8043780

mentions :  „Backward compatibility needs to be provided via #ifdef and checking (once) whether O_CLOEXEC works.”

So it was intentional that just ***once*** it is checked whether  O_CLOEXEC   works (in the linux version) .


Best regards, Matthias


From: Thomas Stüfe <thomas.stuefe at gmail.com<mailto:thomas.stuefe at gmail.com>>
Sent: Donnerstag, 6. Februar 2020 11:30
To: Martin Buchholz <martinrb at google.com<mailto:martinrb at google.com>>
Cc: David Holmes <david.holmes at oracle.com<mailto:david.holmes at oracle.com>>; hotspot-dev at openjdk.java.net<mailto:hotspot-dev at openjdk.java.net>; Baesken, Matthias <matthias.baesken at sap.com<mailto:matthias.baesken at sap.com>>
Subject: Question about JDK-8043780 (was: RFR: 8237830: support O_CLOEXEC in os::open on other OS than Linux)

Hi Martin,

do you remember https://bugs.openjdk.java.net/browse/JDK-8043780 ?

Some questions came up about your patch:

Your original coding does this check-O_CLOEXEC-after-open-and-set-if-missing-dance, but if it worked ar least once it will stop doing that. However, if it fails to work it will retest over and over again. Was this intentional? Do we expect it to start working at some point? If yes, could it also stop working at some point?

I wonder whether we could shorten the coding to a simple, unconditional

::fcntl(fd, F_SETFD, flags | FD_CLOEXEC);

to be done after open.

If open(CLOEXEC) worked, this would be a unnecessary noop. If it did not, we set it now. We'd always pay for a fcntl call but I think it is cheap and open is not called that often anyway.

--

But then, I wonder whether we can scratch that coding altogether. O_CLOEXEC has been in the kernel since 2.6.23. This is 13 years old. 2.6 line topped out at 2.6.39 in 2011, and I would expect anyone still running on 2.6 to have at least a recent kernel.

What do you think?

Thank you!

Cheers, Thomas


On Thu, Feb 6, 2020 at 8:55 AM Baesken, Matthias <matthias.baesken at sap.com<mailto:matthias.baesken at sap.com>> wrote:
Hi Thomas , the coding was borrowed from linux, see :

https://hg.openjdk.java.net/jdk/jdk/file/7b57401deb0c/src/hotspot/os/linux/os_linux.cpp#l5616

I would be fine with  always calling fcntl (your suggestion 1) ,  do you think  it was done otherwise  back then   in os_linux because of performance issues ?
Probably os::open in os_linux  should be adjusted then  to the  new “always call fcntl” approach too, right ?


Best regards, Matthias



Hi Matthias:

+ // Validate that the use of the O_CLOEXEC flag on open above worked.

+  static sig_atomic_t O_CLOEXEC_is_known_to_work = 0;

+  if (!O_CLOEXEC_is_known_to_work) {

     int flags = ::fcntl(fd, F_GETFD);

-    if (flags != -1)

+    if (flags != -1) {

+      if ((flags & FD_CLOEXEC) != 0) {

+        O_CLOEXEC_is_known_to_work = 1;

+      } else {

       ::fcntl(fd, F_SETFD, flags | FD_CLOEXEC);

   }

-#endif

+    }

+  }

This is either too complicated or not complicated enough :)

if we know for a fact that open(..O_CLOEXEC) did work at least once, we never again check.
if we know for a fact that open(..O_CLOEXEC) failed to work, we recheck again and again. Do we expect it to suddenly start working? If yes, could it also suddenly start failing? Should we then not check always?

I propose the following change:

1) If open(..O_CLOEXEC) could sometimes fail and sometimes not, or we are just not sure, I would do the fcntl test every time. Or, even just always set the flag again with fcntl. So, just remove the O_CLOEXEC_is_known_to_work conditional variable.

Which would reduce the code to a one liner:

+ ::fcntl(fd, F_SETFD, flags | FD_CLOEXEC);

2) If you are sure that it either always works or fails, you need a conditional with three states: unknown, works, does not work. And only check if unknown.

I personally would prefer (1) because it is way simpler and fcntl is not such an expensive operation that we have to save time (also, how often do we open files anyway?).
----

Otherwise looks fine. This is a useful fix.


Cheers, Thomas







On Tue, Jan 28, 2020 at 12:32 PM Baesken, Matthias <matthias.baesken at sap.com<mailto:matthias.baesken at sap.com>> wrote:
New webrev :

http://cr.openjdk.java.net/~mbaesken/webrevs/8237830.1/

(one 'c' got lost somehow in the older webrev)

Best regards, Matthias