Re: Patch for adding SO_REUSEPORT socket option

[Michael McMahon]

Hi Lucy,

Sorry for the delay in getting back. I think it is getting closer now.
I should have mentioned this before, but now that all socket types
support the setOption/getOption methods, I'm not sure it is necessary
to have explicit per option set/get methods. These methods were only
added in JDK9. So, I think this is the first time we have to consider this.

I think it will simplify the API change (and patch) quite a bit, if all 
of the

details of SO_REUSEPORT are specified in StandardSocketOptions.
I'm not sure that any change is needed in the other API classes.

- Michael

On 01/12/15 07:25, Lu, Yingqi wrote:


Hi All,

Here is the most recent version of the patch. 
http://cr.openjdk.java.net/~mcberg/jdk/6432031/webrev.05/ 



We already tested patch on Linux 3.4.110 kernel and 3.18 kernel with 
regression tests from java.net and java.nio, and a simple program for 
reuseport. We also tested the same thing on Windows platform. Results 
are as expected.


For the simple program, we tested Sockets.setOption, 
setOption/getOption and set/getReusePort methods from java.net.


Please review this version and let us know your feedback.

Thanks very much for your help!

Lucy

*From:*net-dev [mailto:net-dev-boun...@openjdk.java.net] *On Behalf Of 
*Lu, Yingqi

*Sent:* Wednesday, November 25, 2015 5:12 PM
*To:* Volker Simonis 
*Cc:* Kaczmarek, Eric ; 
net-dev@openjdk.java.net; Kharbas, Kishor 

*Subject:* RE: Patch for adding SO_REUSEPORT socket option

Here is an update.

Changes are already completed locally. All the tests passed on an old 
Linux kernel 3.4.110 which does not have SO_REUSEPORT. Same tests are 
done on Linux 4.2 kernel before.


Here are the quick information on the current implementation:

1.For multicast socket, SO_REUSEPORT will be set by default if 
supported. We use Net.reuseportSupported method to check before 
calling setReusePort(). If not supported, will silently continue 
without setting it.


2.For socket, serversocket and datagramsocket, we check if 
SO_REUSEPORT is supported before calling setOption/getOption and 
setReusePort/ getReusePort methods. If not supported, UOE exception 
will be thrown.


3.We modified a bug in the OptionsTest.java file.

We will test Windows environment to see if it behaves the expected 
way. However, we need help to test other OSes from the community J


Due to the Thanksgiving holiday schedule, we will send updated webrev 
package on Monday.


Thank you all for your help and happy thanksgiving!

Lucy

*From:*Lu, Yingqi
*Sent:* Wednesday, November 25, 2015 1:23 PM
*To:* 'Volker Simonis' >
*Cc:* Michael McMahon >; Alan Bateman 
mailto:alan.bate...@oracle.com>>; Kharbas, 
Kishor mailto:kishor.khar...@intel.com>>; 
net-dev@openjdk.java.net ; Kaczmarek, 
Eric mailto:eric.kaczma...@intel.com>>

*Subject:* RE: Patch for adding SO_REUSEPORT socket option

Yes, it should work! I already located the issues. Good catch!

I will submit an update as soon as possible.

Thanks,

Lucy

*From:*Volker Simonis [mailto:volker.simo...@gmail.com]
*Sent:* Wednesday, November 25, 2015 12:17 PM
*To:* Lu, Yingqi mailto:yingqi...@intel.com>>
*Cc:* Michael McMahon >; Alan Bateman 
mailto:alan.bate...@oracle.com>>; Kharbas, 
Kishor mailto:kishor.khar...@intel.com>>; 
net-dev@openjdk.java.net ; Kaczmarek, 
Eric mailto:eric.kaczma...@intel.com>>

*Subject:* Re: Patch for adding SO_REUSEPORT socket option

Yes, I indeed tested on an old kernel - but that should still work 
after your change!


On Wednesday, November 25, 2015, Lu, Yingqi > wrote:


Hi Volker,

Thanks very much for letting me know. I actually took the related
regression tests and they all passed. I think you tested on an old
kernel which does not have SO_REUSEPORT enabled. In this case, it
should not set the flag.

Let me double check. I will get back to you as soon as I can.

Thanks,
Lucy

-Original Message-
From: Volker Simonis [mailto:volker.simo...@gmail.com ]
Sent: Wednesday, November 25, 2015 10:46 AM
To: Lu, Yingqi >
Cc: Michael McMahon >;
Alan Bateman >; Kharbas,
Kishor >;
net-dev@openjdk.java.net ; Kaczmarek, Eric
>
Subject: Re: Patch for adding SO_REUSEPORT socket option

Hi Lucy,

I took a brief look at your changes but there seems to be
something not right. I can't understand for example why you
unconditionally try to set SO_REUSEPORT on all sockets in
Java_sun_nio_ch_Net_socket0() ?

Also which your changes applied, simple regression tests like
test/java/net/SocketOption/OptionsTest.java start to fail even on
Linux/x86_64:

java.net.SocketException: Protocol not available
at
java.net.PlainDatagramSocketImpl.socketSet

Re: Patch for adding SO_REUSEPORT socket option

[Alan Bateman]

On 02/12/2015 17:08, Lu, Yingqi wrote:


Hi All,

A quick check here. Does anyone get a chance to try the most recent 
patch? Any feedback and comments?


Thanks,

Lucy



I looked through the latest webrev (webrev.05) and it's looking quite good.

I agree with Michael, we should decide whether it make sense to add 
specific methods to Socket/ServerSocket/DatagramSocket. They aren't 
strictly needed and given that the socket option is essentially optional 
then setOption/getOption should be fine.


The updated wording in StandardSocketOptions looks okay but one 
suggestion is to include the word "usually" as it can't mandate the 
behavior on all platforms where it is supported - for example "the 
socket option will usually allow ...". A minor point here is that they 
needs @since 9.


The updates to the NIO implementation classes mostly look okay, except 
sun.nio.ch.Net where I have a number of comments:


1. In Net.java then you'll see where it caches several capabilities, 
that could be used to cache whether this socket option is supported or 
not, no need to do it in native code (Net.c).


2. I see reuseportSupported() is public so that it can be in several 
places (including code in java.net) but would be nice to avoid that.


3. For #1 and #3 then maybe the simplest is to add a native function in 
libnet like we have for IPv6 and that would allow the net and nio code 
to use the same implementation that is consistent with the existing code.


4. Minor comment on Net.c is that you use UNUSED whereas the existing 
code uses "this".


I wonder if it might be simpler to leave the debugger agent out of this, 
I don't see a big reason to change it.


For the tests then it would be good to avoid using 
sun.net.ch.Net.reuseportSupported(). Instead I think it should check 
that supportedOptions returns the right value for the platform.


I assume tests such as MulticastSendReceiveTests don't need to set this 
option.


-Alan.

RE: Patch for adding SO_REUSEPORT socket option

[Lu, Yingqi]

Hi Alan and Michael,

Thank you very much for your detailed feedback. Before I start modify the code, 
I want to make sure I understand correctly. Regarding the following comment, do 
you mean that we should remove the setReusePort and getReusePort methods? If 
yes, we can easily remove them, and keep all the setOption/getOption methods 
the same way for SO_REUSEPORT.
I agree with Michael, we should decide whether it make sense to add specific 
methods to Socket/ServerSocket/DatagramSocket. They aren't strictly needed and 
given that the socket option is essentially optional then setOption/getOption 
should be fine.

I will work on this today. Hopefully can send out an update tomorrow.

Thanks,
Lucy

From: Alan Bateman [mailto:alan.bate...@oracle.com]
Sent: Monday, December 07, 2015 7:04 AM
To: Lu, Yingqi 
Cc: net-dev@openjdk.java.net; Kharbas, Kishor ; 
Kaczmarek, Eric 
Subject: Re: Patch for adding SO_REUSEPORT socket option


On 02/12/2015 17:08, Lu, Yingqi wrote:
Hi All,

A quick check here. Does anyone get a chance to try the most recent patch? Any 
feedback and comments?

Thanks,
Lucy

I looked through the latest webrev (webrev.05) and it's looking quite good.

I agree with Michael, we should decide whether it make sense to add specific 
methods to Socket/ServerSocket/DatagramSocket. They aren't strictly needed and 
given that the socket option is essentially optional then setOption/getOption 
should be fine.

The updated wording in StandardSocketOptions looks okay but one suggestion is 
to include the word "usually" as it can't mandate the behavior on all platforms 
where it is supported - for example "the socket option will usually allow ...". 
A minor point here is that they needs @since 9.

The updates to the NIO implementation classes mostly look okay, except 
sun.nio.ch.Net where I have a number of comments:

1. In Net.java then you'll see where it caches several capabilities, that could 
be used to cache whether this socket option is supported or not, no need to do 
it in native code (Net.c).

2. I see reuseportSupported() is public so that it can be in several places 
(including code in java.net) but would be nice to avoid that.

3. For #1 and #3 then maybe the simplest is to add a native function in libnet 
like we have for IPv6 and that would allow the net and nio code to use the same 
implementation that is consistent with the existing code.

4. Minor comment on Net.c is that you use UNUSED whereas the existing code uses 
"this".

I wonder if it might be simpler to leave the debugger agent out of this, I 
don't see a big reason to change it.

For the tests then it would be good to avoid using 
sun.net.ch.Net.reuseportSupported(). Instead I think it should check that 
supportedOptions returns the right value for the platform.

I assume tests such as MulticastSendReceiveTests don't need to set this option.

-Alan.

Re: Patch for adding SO_REUSEPORT socket option

[Michael McMahon]

Yes, since the functionality is basically duplicated across the two 
mechanisms

and since setOption/getOption already exists, the only new part of the API
is the SO_REUSEPORT option itself in StandardSocketOptions.

- Michael

On 07/12/15 17:04, Lu, Yingqi wrote:


Hi Alan and Michael,

Thank you very much for your detailed feedback. Before I start modify 
the code, I want to make sure I understand correctly. Regarding the 
following comment, do you mean that we should remove the setReusePort 
and getReusePort methods? If yes, we can easily remove them, and keep 
all the setOption/getOption methods the same way for SO_REUSEPORT.


I agree with Michael, we should decide whether it make sense to add 
specific methods to Socket/ServerSocket/DatagramSocket. They aren't 
strictly needed and given that the socket option is essentially 
optional then setOption/getOption should be fine.


I will work on this today. Hopefully can send out an update tomorrow.

Thanks,

Lucy

*From:*Alan Bateman [mailto:alan.bate...@oracle.com]
*Sent:* Monday, December 07, 2015 7:04 AM
*To:* Lu, Yingqi 
*Cc:* net-dev@openjdk.java.net; Kharbas, Kishor 
; Kaczmarek, Eric 

*Subject:* Re: Patch for adding SO_REUSEPORT socket option

On 02/12/2015 17:08, Lu, Yingqi wrote:

Hi All,

A quick check here. Does anyone get a chance to try the most
recent patch? Any feedback and comments?

Thanks,

Lucy

I looked through the latest webrev (webrev.05) and it's looking quite 
good.


I agree with Michael, we should decide whether it make sense to add 
specific methods to Socket/ServerSocket/DatagramSocket. They aren't 
strictly needed and given that the socket option is essentially 
optional then setOption/getOption should be fine.


The updated wording in StandardSocketOptions looks okay but one 
suggestion is to include the word "usually" as it can't mandate the 
behavior on all platforms where it is supported - for example "the 
socket option will usually allow ...". A minor point here is that they 
needs @since 9.


The updates to the NIO implementation classes mostly look okay, except 
sun.nio.ch.Net where I have a number of comments:


1. In Net.java then you'll see where it caches several capabilities, 
that could be used to cache whether this socket option is supported or 
not, no need to do it in native code (Net.c).


2. I see reuseportSupported() is public so that it can be in several 
places (including code in java.net) but would be nice to avoid that.


3. For #1 and #3 then maybe the simplest is to add a native function 
in libnet like we have for IPv6 and that would allow the net and nio 
code to use the same implementation that is consistent with the 
existing code.


4. Minor comment on Net.c is that you use UNUSED whereas the existing 
code uses "this".


I wonder if it might be simpler to leave the debugger agent out of 
this, I don't see a big reason to change it.


For the tests then it would be good to avoid using 
sun.net.ch.Net.reuseportSupported(). Instead I think it should check 
that supportedOptions returns the right value for the platform.


I assume tests such as MulticastSendReceiveTests don't need to set 
this option.


-Alan.

RE: Patch for adding SO_REUSEPORT socket option

[Lu, Yingqi]

Michael,

Thank you for confirming this. I will drop 
test/java/net/Socket/setReusePort/Basic.java as well since that was added for 
testing the setReusePort and getReusePort ☺

Thanks,
Lucy

From: Michael McMahon [mailto:michael.x.mcma...@oracle.com]
Sent: Monday, December 07, 2015 9:13 AM
To: Lu, Yingqi ; Alan Bateman 
Cc: Kaczmarek, Eric ; Kharbas, Kishor 
; net-dev@openjdk.java.net
Subject: Re: Patch for adding SO_REUSEPORT socket option

Yes, since the functionality is basically duplicated across the two mechanisms
and since setOption/getOption already exists, the only new part of the API
is the SO_REUSEPORT option itself in StandardSocketOptions.

- Michael

On 07/12/15 17:04, Lu, Yingqi wrote:
Hi Alan and Michael,

Thank you very much for your detailed feedback. Before I start modify the code, 
I want to make sure I understand correctly. Regarding the following comment, do 
you mean that we should remove the setReusePort and getReusePort methods? If 
yes, we can easily remove them, and keep all the setOption/getOption methods 
the same way for SO_REUSEPORT.
I agree with Michael, we should decide whether it make sense to add specific 
methods to Socket/ServerSocket/DatagramSocket. They aren't strictly needed and 
given that the socket option is essentially optional then setOption/getOption 
should be fine.

I will work on this today. Hopefully can send out an update tomorrow.

Thanks,
Lucy

From: Alan Bateman [mailto:alan.bate...@oracle.com]
Sent: Monday, December 07, 2015 7:04 AM
To: Lu, Yingqi 
Cc: net-dev@openjdk.java.net; Kharbas, Kishor 
; Kaczmarek, Eric 

Subject: Re: Patch for adding SO_REUSEPORT socket option


On 02/12/2015 17:08, Lu, Yingqi wrote:
Hi All,

A quick check here. Does anyone get a chance to try the most recent patch? Any 
feedback and comments?

Thanks,
Lucy

I looked through the latest webrev (webrev.05) and it's looking quite good.

I agree with Michael, we should decide whether it make sense to add specific 
methods to Socket/ServerSocket/DatagramSocket. They aren't strictly needed and 
given that the socket option is essentially optional then setOption/getOption 
should be fine.

The updated wording in StandardSocketOptions looks okay but one suggestion is 
to include the word "usually" as it can't mandate the behavior on all platforms 
where it is supported - for example "the socket option will usually allow ...". 
A minor point here is that they needs @since 9.

The updates to the NIO implementation classes mostly look okay, except 
sun.nio.ch.Net where I have a number of comments:

1. In Net.java then you'll see where it caches several capabilities, that could 
be used to cache whether this socket option is supported or not, no need to do 
it in native code (Net.c).

2. I see reuseportSupported() is public so that it can be in several places 
(including code in java.net) but would be nice to avoid that.

3. For #1 and #3 then maybe the simplest is to add a native function in libnet 
like we have for IPv6 and that would allow the net and nio code to use the same 
implementation that is consistent with the existing code.

4. Minor comment on Net.c is that you use UNUSED whereas the existing code uses 
"this".

I wonder if it might be simpler to leave the debugger agent out of this, I 
don't see a big reason to change it.

For the tests then it would be good to avoid using 
sun.net.ch.Net.reuseportSupported(). Instead I think it should check that 
supportedOptions returns the right value for the platform.

I assume tests such as MulticastSendReceiveTests don't need to set this option.

-Alan.

Re: Patch for adding SO_REUSEPORT socket option

[Volker Simonis]

Hi Lucy,

thanks for updating the change and sorry for the late review. It looks
pretty good now. Please find my comments below:

genSocketOptionRegistry.c
net_util_md.h
nio_util.h
socket_md.h


I'm a little concerned about the following code:

  37 /* Defines SO_REUSEPORT */
  38 #ifndef SO_REUSEPORT
  39 #define SO_REUSEPORT 15
  40 #endif

because the value of SO_REUSPORT is platform dependent. It's 15 on
Linux, but 0x100e on Solaris and 0x0200 on MacOS X, AIX and HPUX. So
we should make this platform dependent otherwise this code will break
if the JDK will be compiled lets say on Solaris 10 which doesn't have
SO_REUSEPORT but later runs on Solaris 11 where it will set the socket
option '15' which is undefined there.

Net.c

- you can call initialize 'reuseportsupported'  by calling
'reuseportSupported0()' from Java_sun_nio_ch_Net_initIDs() which is
called exactly once during class initialization (so no need for the
'reuseportsupported_set' guard). In
Java_sun_nio_ch_Net_reuseportSupported()' you can then simply return
'reuseportsupported'.

- why do we unconditionally set SO_REUSEPORT for every socket created
in 'Java_sun_nio_ch_Net_socket0()'? I don't think that's right.

src/java.management/share/classes/sun/management/jdp
src/jdk.jdwp.agent/unix/native/libdt_socket
===
- I don't think we should change these socket options. I think there's
not much to win but we can get problems on platforms where
SO_REUSEPORT has a different semantics.

DatagramSocket.java


- that's probably another clean-up change, but do we really still need
to support pre-1.4 DatagramSocketImpl?


And as always, the copyright of all the files you've touched has to be
updated to 2015 (or 2016 :)

Regards,
Volker

On Wed, Dec 2, 2015 at 6:08 PM, Lu, Yingqi  wrote:
> Hi All,
>
>
>
> A quick check here. Does anyone get a chance to try the most recent patch?
> Any feedback and comments?
>
>
>
> Thanks,
>
> Lucy
>
>
>
> From: Lu, Yingqi
> Sent: Monday, November 30, 2015 11:25 PM
> To: Lu, Yingqi ; Volker Simonis
> 
>
>
> Cc: Kaczmarek, Eric ; net-dev@openjdk.java.net;
> Kharbas, Kishor 
> Subject: RE: Patch for adding SO_REUSEPORT socket option
>
>
>
> Hi All,
>
>
>
> Here is the most recent version of the patch.
> http://cr.openjdk.java.net/~mcberg/jdk/6432031/webrev.05/
>
>
>
> We already tested patch on Linux 3.4.110 kernel and 3.18 kernel with
> regression tests from java.net and java.nio, and a simple program for
> reuseport. We also tested the same thing on Windows platform. Results are as
> expected.
>
>
>
> For the simple program, we tested Sockets.setOption, setOption/getOption and
> set/getReusePort methods from java.net.
>
>
>
> Please review this version and let us know your feedback.
>
>
>
> Thanks very much for your help!
>
> Lucy
>
>
>
> From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of Lu,
> Yingqi
> Sent: Wednesday, November 25, 2015 5:12 PM
> To: Volker Simonis 
> Cc: Kaczmarek, Eric ; net-dev@openjdk.java.net;
> Kharbas, Kishor 
> Subject: RE: Patch for adding SO_REUSEPORT socket option
>
>
>
> Here is an update.
>
>
>
> Changes are already completed locally. All the tests passed on an old Linux
> kernel 3.4.110 which does not have SO_REUSEPORT. Same tests are done on
> Linux 4.2 kernel before.
>
>
>
> Here are the quick information on the current implementation:
>
>
>
> 1.   For multicast socket, SO_REUSEPORT will be set by default if
> supported. We use Net.reuseportSupported method to check before calling
> setReusePort(). If not supported, will silently continue without setting it.
>
> 2.   For socket, serversocket and datagramsocket, we check if
> SO_REUSEPORT is supported before calling setOption/getOption and
> setReusePort/ getReusePort methods. If not supported, UOE exception will be
> thrown.
>
> 3.   We modified a bug in the OptionsTest.java file.
>
>
>
> We will test Windows environment to see if it behaves the expected way.
> However, we need help to test other OSes from the community J
>
>
>
> Due to the Thanksgiving holiday schedule, we will send updated webrev
> package on Monday.
>
>
>
> Thank you all for your help and happy thanksgiving!
>
> Lucy
>
>
>
>
>
> From: Lu, Yingqi
> Sent: Wednesday, November 25, 2015 1:23 PM
> To: 'Volker Simonis' 
> Cc: Michael McMahon ; Alan Bateman
> ; Kharbas, Kishor ;
> net-dev@openjdk.java.net; Kaczmarek, Eric 
> Subject: RE: Patch for adding SO_REUSEPORT socket option
>
>
>
> Yes, it should work! I already located the issues. Good catch!
>
>
>
> I will submit an update as soon as possible.
>
>
>
> Thanks,
>
> Lucy
>
>
>
> From: Volker Simonis [mailto:volker.simo...@gmail.com]
> Sent: Wednesday, November 25, 2015 12:17 PM
> To: Lu, Yingqi 
> Cc: Michael McMahon ; Alan Bateman
> ; Kharbas, Kishor ;
> net-dev@openjdk.java.net; Kaczmarek, Eric 
> Subject: Re: Patch for adding SO_REUSEPORT socket option
>
>
>
> Yes, I indeed tested on an old kernel - but that should still

RE: Patch for adding SO_REUSEPORT socket option

[Lu, Yingqi]

Hi Volker,

Thank you very much for your feedback.

1. For the following code, I will do it as platform specific as you suggested. 
  37 /* Defines SO_REUSEPORT */
  38 #ifndef SO_REUSEPORT
  39 #define SO_REUSEPORT 15
  40 #endif

2. I will remove the setReusePort and getReusePort methods from 
Socket/ServerSocket/DiagramSocket.

3. I will remove SO_REUSEPORT from following, since they do not really have 
performance impact here.
src/java.management/share/classes/sun/management/jdp
src/jdk.jdwp.agent/unix/native/libdt_socket

4. I will follow the example of IPv6 check for reuseportSupported(), adding 
caching (check once) as well as relocate the native check to libnet.

5. Regarding to your following comment, I think set SO_REUSEPORT is not for 
every socket. The "support or not check" has already been done before going to 
this native function. If not supported, it won’t go here. Please let me know if 
my understanding is correct.
- why do we unconditionally set SO_REUSEPORT for every socket created in 
'Java_sun_nio_ch_Net_socket0()'? I don't think that's right.

Thanks,
Lucy

-Original Message-
From: Volker Simonis [mailto:volker.simo...@gmail.com] 
Sent: Monday, December 07, 2015 9:51 AM
To: Lu, Yingqi ; Alan Bateman ; 
Michael McMahon 
Cc: Kaczmarek, Eric ; net-dev@openjdk.java.net; 
Kharbas, Kishor 
Subject: Re: Patch for adding SO_REUSEPORT socket option

Hi Lucy,

thanks for updating the change and sorry for the late review. It looks pretty 
good now. Please find my comments below:

genSocketOptionRegistry.c
net_util_md.h
nio_util.h
socket_md.h


I'm a little concerned about the following code:

  37 /* Defines SO_REUSEPORT */
  38 #ifndef SO_REUSEPORT
  39 #define SO_REUSEPORT 15
  40 #endif

because the value of SO_REUSPORT is platform dependent. It's 15 on Linux, but 
0x100e on Solaris and 0x0200 on MacOS X, AIX and HPUX. So we should make this 
platform dependent otherwise this code will break if the JDK will be compiled 
lets say on Solaris 10 which doesn't have SO_REUSEPORT but later runs on 
Solaris 11 where it will set the socket option '15' which is undefined there.

Net.c

- you can call initialize 'reuseportsupported'  by calling 
'reuseportSupported0()' from Java_sun_nio_ch_Net_initIDs() which is called 
exactly once during class initialization (so no need for the 
'reuseportsupported_set' guard). In Java_sun_nio_ch_Net_reuseportSupported()' 
you can then simply return 'reuseportsupported'.

- why do we unconditionally set SO_REUSEPORT for every socket created in 
'Java_sun_nio_ch_Net_socket0()'? I don't think that's right.

src/java.management/share/classes/sun/management/jdp
src/jdk.jdwp.agent/unix/native/libdt_socket
===
- I don't think we should change these socket options. I think there's not much 
to win but we can get problems on platforms where SO_REUSEPORT has a different 
semantics.

DatagramSocket.java


- that's probably another clean-up change, but do we really still need to 
support pre-1.4 DatagramSocketImpl?


And as always, the copyright of all the files you've touched has to be updated 
to 2015 (or 2016 :)

Regards,
Volker

On Wed, Dec 2, 2015 at 6:08 PM, Lu, Yingqi  wrote:
> Hi All,
>
>
>
> A quick check here. Does anyone get a chance to try the most recent patch?
> Any feedback and comments?
>
>
>
> Thanks,
>
> Lucy
>
>
>
> From: Lu, Yingqi
> Sent: Monday, November 30, 2015 11:25 PM
> To: Lu, Yingqi ; Volker Simonis 
> 
>
>
> Cc: Kaczmarek, Eric ; 
> net-dev@openjdk.java.net; Kharbas, Kishor 
> Subject: RE: Patch for adding SO_REUSEPORT socket option
>
>
>
> Hi All,
>
>
>
> Here is the most recent version of the patch.
> http://cr.openjdk.java.net/~mcberg/jdk/6432031/webrev.05/
>
>
>
> We already tested patch on Linux 3.4.110 kernel and 3.18 kernel with 
> regression tests from java.net and java.nio, and a simple program for 
> reuseport. We also tested the same thing on Windows platform. Results 
> are as expected.
>
>
>
> For the simple program, we tested Sockets.setOption, 
> setOption/getOption and set/getReusePort methods from java.net.
>
>
>
> Please review this version and let us know your feedback.
>
>
>
> Thanks very much for your help!
>
> Lucy
>
>
>
> From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of 
> Lu, Yingqi
> Sent: Wednesday, November 25, 2015 5:12 PM
> To: Volker Simonis 
> Cc: Kaczmarek, Eric ; 
> net-dev@openjdk.java.net; Kharbas, Kishor 
> Subject: RE: Patch for adding SO_REUSEPORT socket option
>
>
>
> Here is an update.
>
>
>
> Changes are already completed locally. All the tests passed on an old 
> Linux kernel 3.4.110 which does not have SO_REUSEPORT. Same tests are 
> done on Linux 4.2 kernel before.
>
>
>
> Here are the quick information on the current implementation:
>
>
>
> 1.   For multicast socket, SO_REUSEPORT will be set by default if
> supported. We use Net.reuseportSupported method to check before 
> calling setReusePort(). If not su

Re: Patch for adding SO_REUSEPORT socket option

[Volker Simonis]

On Mon, Dec 7, 2015 at 7:02 PM, Lu, Yingqi  wrote:
> Hi Volker,
>
> Thank you very much for your feedback.
>
> 1. For the following code, I will do it as platform specific as you suggested.
>   37 /* Defines SO_REUSEPORT */
>   38 #ifndef SO_REUSEPORT
>   39 #define SO_REUSEPORT 15
>   40 #endif
>
> 2. I will remove the setReusePort and getReusePort methods from 
> Socket/ServerSocket/DiagramSocket.
>
> 3. I will remove SO_REUSEPORT from following, since they do not really have 
> performance impact here.
> src/java.management/share/classes/sun/management/jdp
> src/jdk.jdwp.agent/unix/native/libdt_socket
>
> 4. I will follow the example of IPv6 check for reuseportSupported(), adding 
> caching (check once) as well as relocate the native check to libnet.
>
> 5. Regarding to your following comment, I think set SO_REUSEPORT is not for 
> every socket. The "support or not check" has already been done before going 
> to this native function. If not supported, it won’t go here. Please let me 
> know if my understanding is correct.
> - why do we unconditionally set SO_REUSEPORT for every socket created in 
> 'Java_sun_nio_ch_Net_socket0()'? I don't think that's right.

'Java_sun_nio_ch_Net_socket0()' has an argument 'reuse'. This argument
is checked and only if it is true, we set the SO_REUSEADDR option. But
there's no such check for SO_REUSEPORT. We unconditionally set it
always, no difference if 'reuse' was true or false (so we also set it
for every SocketChannelImpl for example, not only for
ServerSocketChannelImpl as with SO_REUSEADDR).

Notice that I don't want you to put the new code inside the "if
(reuse)" block. I think we simply shouldn't implicitly set
SO_REUSEPORT. We should leave that up to the user, if he thinks it is
necessary.

Or am I missing something here?

>
> Thanks,
> Lucy
>
> -Original Message-
> From: Volker Simonis [mailto:volker.simo...@gmail.com]
> Sent: Monday, December 07, 2015 9:51 AM
> To: Lu, Yingqi ; Alan Bateman ; 
> Michael McMahon 
> Cc: Kaczmarek, Eric ; net-dev@openjdk.java.net; 
> Kharbas, Kishor 
> Subject: Re: Patch for adding SO_REUSEPORT socket option
>
> Hi Lucy,
>
> thanks for updating the change and sorry for the late review. It looks pretty 
> good now. Please find my comments below:
>
> genSocketOptionRegistry.c
> net_util_md.h
> nio_util.h
> socket_md.h
> 
>
> I'm a little concerned about the following code:
>
>   37 /* Defines SO_REUSEPORT */
>   38 #ifndef SO_REUSEPORT
>   39 #define SO_REUSEPORT 15
>   40 #endif
>
> because the value of SO_REUSPORT is platform dependent. It's 15 on Linux, but 
> 0x100e on Solaris and 0x0200 on MacOS X, AIX and HPUX. So we should make this 
> platform dependent otherwise this code will break if the JDK will be compiled 
> lets say on Solaris 10 which doesn't have SO_REUSEPORT but later runs on 
> Solaris 11 where it will set the socket option '15' which is undefined there.
>
> Net.c
> 
> - you can call initialize 'reuseportsupported'  by calling 
> 'reuseportSupported0()' from Java_sun_nio_ch_Net_initIDs() which is called 
> exactly once during class initialization (so no need for the 
> 'reuseportsupported_set' guard). In Java_sun_nio_ch_Net_reuseportSupported()' 
> you can then simply return 'reuseportsupported'.
>
> - why do we unconditionally set SO_REUSEPORT for every socket created in 
> 'Java_sun_nio_ch_Net_socket0()'? I don't think that's right.
>
> src/java.management/share/classes/sun/management/jdp
> src/jdk.jdwp.agent/unix/native/libdt_socket
> ===
> - I don't think we should change these socket options. I think there's not 
> much to win but we can get problems on platforms where SO_REUSEPORT has a 
> different semantics.
>
> DatagramSocket.java
> 
>
> - that's probably another clean-up change, but do we really still need to 
> support pre-1.4 DatagramSocketImpl?
>
>
> And as always, the copyright of all the files you've touched has to be 
> updated to 2015 (or 2016 :)
>
> Regards,
> Volker
>
> On Wed, Dec 2, 2015 at 6:08 PM, Lu, Yingqi  wrote:
>> Hi All,
>>
>>
>>
>> A quick check here. Does anyone get a chance to try the most recent patch?
>> Any feedback and comments?
>>
>>
>>
>> Thanks,
>>
>> Lucy
>>
>>
>>
>> From: Lu, Yingqi
>> Sent: Monday, November 30, 2015 11:25 PM
>> To: Lu, Yingqi ; Volker Simonis
>> 
>>
>>
>> Cc: Kaczmarek, Eric ;
>> net-dev@openjdk.java.net; Kharbas, Kishor 
>> Subject: RE: Patch for adding SO_REUSEPORT socket option
>>
>>
>>
>> Hi All,
>>
>>
>>
>> Here is the most recent version of the patch.
>> http://cr.openjdk.java.net/~mcberg/jdk/6432031/webrev.05/
>>
>>
>>
>> We already tested patch on Linux 3.4.110 kernel and 3.18 kernel with
>> regression tests from java.net and java.nio, and a simple program for
>> reuseport. We also tested the same thing on Windows platform. Results
>> are as expected.
>>
>>
>>
>> For the simple program, we tested Sockets.setOption,
>> setOption/getOption and set/getReusePort methods from 

RE: Patch for adding SO_REUSEPORT socket option

[Lu, Yingqi]

Oh, I got it now. You want SO_REUSEPORT to be disabled by default for both 
ServerSocketChannelImpl and SocketChannelImpl. 

I am actually OK either way. Alan/Michael, what is your take on this? If we all 
think "default off" is the correct/better way to go, we can just simply remove 
that block and leave it to the user.

Please let me know!

Thanks,
Lucy

-Original Message-
From: Volker Simonis [mailto:volker.simo...@gmail.com] 
Sent: Monday, December 07, 2015 10:38 AM
To: Lu, Yingqi 
Cc: Alan Bateman ; Michael McMahon 
; Kaczmarek, Eric ; 
net-dev@openjdk.java.net; Kharbas, Kishor 
Subject: Re: Patch for adding SO_REUSEPORT socket option

On Mon, Dec 7, 2015 at 7:02 PM, Lu, Yingqi  wrote:
> Hi Volker,
>
> Thank you very much for your feedback.
>
> 1. For the following code, I will do it as platform specific as you suggested.
>   37 /* Defines SO_REUSEPORT */
>   38 #ifndef SO_REUSEPORT
>   39 #define SO_REUSEPORT 15
>   40 #endif
>
> 2. I will remove the setReusePort and getReusePort methods from 
> Socket/ServerSocket/DiagramSocket.
>
> 3. I will remove SO_REUSEPORT from following, since they do not really have 
> performance impact here.
> src/java.management/share/classes/sun/management/jdp
> src/jdk.jdwp.agent/unix/native/libdt_socket
>
> 4. I will follow the example of IPv6 check for reuseportSupported(), adding 
> caching (check once) as well as relocate the native check to libnet.
>
> 5. Regarding to your following comment, I think set SO_REUSEPORT is not for 
> every socket. The "support or not check" has already been done before going 
> to this native function. If not supported, it won’t go here. Please let me 
> know if my understanding is correct.
> - why do we unconditionally set SO_REUSEPORT for every socket created in 
> 'Java_sun_nio_ch_Net_socket0()'? I don't think that's right.

'Java_sun_nio_ch_Net_socket0()' has an argument 'reuse'. This argument is 
checked and only if it is true, we set the SO_REUSEADDR option. But there's no 
such check for SO_REUSEPORT. We unconditionally set it always, no difference if 
'reuse' was true or false (so we also set it for every SocketChannelImpl for 
example, not only for ServerSocketChannelImpl as with SO_REUSEADDR).

Notice that I don't want you to put the new code inside the "if (reuse)" block. 
I think we simply shouldn't implicitly set SO_REUSEPORT. We should leave that 
up to the user, if he thinks it is necessary.

Or am I missing something here?

>
> Thanks,
> Lucy
>
> -Original Message-
> From: Volker Simonis [mailto:volker.simo...@gmail.com]
> Sent: Monday, December 07, 2015 9:51 AM
> To: Lu, Yingqi ; Alan Bateman 
> ; Michael McMahon 
> 
> Cc: Kaczmarek, Eric ; 
> net-dev@openjdk.java.net; Kharbas, Kishor 
> Subject: Re: Patch for adding SO_REUSEPORT socket option
>
> Hi Lucy,
>
> thanks for updating the change and sorry for the late review. It looks pretty 
> good now. Please find my comments below:
>
> genSocketOptionRegistry.c
> net_util_md.h
> nio_util.h
> socket_md.h
> 
>
> I'm a little concerned about the following code:
>
>   37 /* Defines SO_REUSEPORT */
>   38 #ifndef SO_REUSEPORT
>   39 #define SO_REUSEPORT 15
>   40 #endif
>
> because the value of SO_REUSPORT is platform dependent. It's 15 on Linux, but 
> 0x100e on Solaris and 0x0200 on MacOS X, AIX and HPUX. So we should make this 
> platform dependent otherwise this code will break if the JDK will be compiled 
> lets say on Solaris 10 which doesn't have SO_REUSEPORT but later runs on 
> Solaris 11 where it will set the socket option '15' which is undefined there.
>
> Net.c
> 
> - you can call initialize 'reuseportsupported'  by calling 
> 'reuseportSupported0()' from Java_sun_nio_ch_Net_initIDs() which is called 
> exactly once during class initialization (so no need for the 
> 'reuseportsupported_set' guard). In Java_sun_nio_ch_Net_reuseportSupported()' 
> you can then simply return 'reuseportsupported'.
>
> - why do we unconditionally set SO_REUSEPORT for every socket created in 
> 'Java_sun_nio_ch_Net_socket0()'? I don't think that's right.
>
> src/java.management/share/classes/sun/management/jdp
> src/jdk.jdwp.agent/unix/native/libdt_socket
> ===
> - I don't think we should change these socket options. I think there's not 
> much to win but we can get problems on platforms where SO_REUSEPORT has a 
> different semantics.
>
> DatagramSocket.java
> 
>
> - that's probably another clean-up change, but do we really still need to 
> support pre-1.4 DatagramSocketImpl?
>
>
> And as always, the copyright of all the files you've touched has to be 
> updated to 2015 (or 2016 :)
>
> Regards,
> Volker
>
> On Wed, Dec 2, 2015 at 6:08 PM, Lu, Yingqi  wrote:
>> Hi All,
>>
>>
>>
>> A quick check here. Does anyone get a chance to try the most recent patch?
>> Any feedback and comments?
>>
>>
>>
>> Thanks,
>>
>> Lucy
>>
>>
>>
>> From: Lu, Yingqi
>> Sent: Monday, November 30, 2015 11:25 PM
>> To: Lu, Yingqi 

Re: Patch for adding SO_REUSEPORT socket option

[Alan Bateman]

On 07/12/2015 18:59, Lu, Yingqi wrote:

Oh, I got it now. You want SO_REUSEPORT to be disabled by default for both 
ServerSocketChannelImpl and SocketChannelImpl.

I am actually OK either way. Alan/Michael, what is your take on this? If we all think 
"default off" is the correct/better way to go, we can just simply remove that 
block and leave it to the user.


Off by disabled make sense to me.

-Alan

RE: Patch for adding SO_REUSEPORT socket option

[Lu, Yingqi]

OK, will do.

Thanks,
Lucy

-Original Message-
From: Alan Bateman [mailto:alan.bate...@oracle.com] 
Sent: Monday, December 07, 2015 1:01 PM
To: Lu, Yingqi ; Volker Simonis 
Cc: Michael McMahon ; Kaczmarek, Eric 
; net-dev@openjdk.java.net; Kharbas, Kishor 

Subject: Re: Patch for adding SO_REUSEPORT socket option



On 07/12/2015 18:59, Lu, Yingqi wrote:
> Oh, I got it now. You want SO_REUSEPORT to be disabled by default for both 
> ServerSocketChannelImpl and SocketChannelImpl.
>
> I am actually OK either way. Alan/Michael, what is your take on this? If we 
> all think "default off" is the correct/better way to go, we can just simply 
> remove that block and leave it to the user.
>
Off by disabled make sense to me.

-Alan

RE: Patch for adding SO_REUSEPORT socket option

[Lu, Yingqi]

Hi Alan,

I heard that the feature freeze for OpenJDK9 is approaching. Given the 
significant performance impact (up to 1.93x with Hadoop Distributed File 
System) this feature provides and the status of the existing work, we would be 
really interested in see this feature to be approved for OpenJDK9. We will 
surely continue working with the community to address opens/issues until it is 
completely ready to be merged into the source code tree. 

Please let us know what you think.

Thanks,
Lucy

-Original Message-
From: Lu, Yingqi 
Sent: Monday, December 07, 2015 1:02 PM
To: 'Alan Bateman' ; Volker Simonis 

Cc: Michael McMahon ; Kaczmarek, Eric 
; net-dev@openjdk.java.net; Kharbas, Kishor 

Subject: RE: Patch for adding SO_REUSEPORT socket option

OK, will do.

Thanks,
Lucy

-Original Message-
From: Alan Bateman [mailto:alan.bate...@oracle.com] 
Sent: Monday, December 07, 2015 1:01 PM
To: Lu, Yingqi ; Volker Simonis 
Cc: Michael McMahon ; Kaczmarek, Eric 
; net-dev@openjdk.java.net; Kharbas, Kishor 

Subject: Re: Patch for adding SO_REUSEPORT socket option



On 07/12/2015 18:59, Lu, Yingqi wrote:
> Oh, I got it now. You want SO_REUSEPORT to be disabled by default for both 
> ServerSocketChannelImpl and SocketChannelImpl.
>
> I am actually OK either way. Alan/Michael, what is your take on this? If we 
> all think "default off" is the correct/better way to go, we can just simply 
> remove that block and leave it to the user.
>
Off by disabled make sense to me.

-Alan