Question about JDK-8043780

[David Holmes]

Hi Matthias,

On 12/02/2020 12:39 am, Baesken, Matthias wrote:
> Hello,  new webrev :

Can you do this on the actual review thread please. It is very hard to 
keep track of this one.

Thanks,
David

> http://cr.openjdk.java.net/~mbaesken/webrevs/8237830.3/
> 
>   * Only touching  os_aix.cpp  ( I leave os_bsd.cpp as it is , might
>     make the discussion   more  simple )
>   * Added handling of  failing O_CLOEXEC   ( O_CLOEXEC_is_known_to_work
>     == -1  … case )
> 
> Best regards, Matthias
> 
> *From:*Baesken, Matthias
> *Sent:* Dienstag, 11. Februar 2020 14:06
> *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' <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)
> 
>>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 
> <mailto:thomas.stuefe at gmail.com>>; 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>
> *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
>