Patch for adding SO_REUSEPORT socket option

Mon Jan 4 19:24:04 UTC 2016

On Mon, Jan 4, 2016 at 8:02 PM, Lu, Yingqi <yingqi.lu at intel.com> wrote:
> Hi Volker,
>
> Happy New Year and welcome back! Based on the feedback from you and Alan, here is what I plan to do. Please let me know if you have any concerns.
>
> 1. I will make HPUX, MACOSX and AIX as known and set them to 0x0200. As previous, Linux is set to 15 and Solaris is set to 0x100. The rest of the unix based OSes will be treated as unknown and will be set to 0. Windows will be set to 0 as well.
>
> 2. Regarding to the code snippet in net_util_md.c, the reason I check for ENOPROTOOPT is to enable SO_REUSEPORT in the situation that the feature is supported but something else happens during setsocksopts call. I totally agree this is much less safer than just disable the feature whenever the setsockopts returns error. Alan, please let me know what your take on this is. I can quick change it if you both think it is more appropriate to remove the ENOPROTOOPT check.
>

I just think we should play safe here. You also return JNI_FALSE when
the call to socket() just before the one to setsockopt() fails. But
lets here what's Alan's opinion.

> Again, really appreciate the quick responses and helpful feedback from both of you :-)
>
> Thanks,
> Lucy
>
> -----Original Message-----
> From: Volker Simonis [mailto:volker.simonis at gmail.com]
> Sent: Monday, January 04, 2016 10:43 AM
> To: Alan Bateman <Alan.Bateman at oracle.com>
> Cc: Lu, Yingqi <yingqi.lu at intel.com>; Kaczmarek, Eric <eric.kaczmarek at intel.com>; Viswanathan, Sandhya <sandhya.viswanathan at intel.com>; Kharbas, Kishor <kishor.kharbas at intel.com>; Aundhe, Shirish <shirish.aundhe at intel.com>; net-dev at openjdk.java.net
> Subject: Re: Patch for adding SO_REUSEPORT socket option
>
> Hi Lucy, Alan,
>
> I'm back from vacation so here we go :)
>
> On Fri, Jan 1, 2016 at 12:18 PM, Alan Bateman <Alan.Bateman at oracle.com> wrote:
>>
>> On 21/12/2015 17:53, Lu, Yingqi wrote:
>>>
>>> Hi All,
>>>
>>> Sorry for the late reply. Here is the link to the most recent
>>> reversion of the patch (version #6).
>>> http://cr.openjdk.java.net/~mcberg/jdk/6432031/webrev.06/
>>>
>>> In this version, we have done following modifications based on the feedback we received. Please review and let us know if there is anything missing. Thank you very much!
>>>
>>>
>> I went through the latest webrev and it is look quite good.
>>
>> One thing that doesn't seem quite right is SocketImpl::isReusePortAvailable. Whether SO_REUSEPORT is supported or not depends on the concrete implementation of the SocketImpl. So I think this needs to be moved down to AbstractPlainSocketImpl.
>>
>> In net_util_md.h then SO_REUSEPORT is defined as 0x0200 on "unknown" platforms. I wonder if it would be better to not set it and instead have reuseport_supported return JNI_FALSE when SO_REUSEPORT is not defined.
>>
>>
>> StandardSocketOptions.SO_REUSEPORT is referenced in shared code (sun/nio/ch/*Impl.java) so shouldn't it be defined to something on all platforms? I ask because genSocketOptionRegistry.c open emits a definition on non-Windows builds. Maybe it needs to emit it with a value of 0 on Windows. Also like net_util_md.c then maybe it should be emitted as 0 rather than 0x0200 when the value is not known.
>>
>
> Alan, what do you mean by "unknown" platform? Currently, as far as I know, 0x0200 is used by AIX and MacOS X. Do you suggest to name these platforms explicitly and set it to 0 otherwise? Leaving the default at
> 0x0200 has the advantage that it would implicitly work on other platforms like HPUX at the risk that it may be the wrong value on other, yet still unknown platforms. So in general I'm not against setting it to 0 on unknown platfroms (as this is the most secure
> setting) but in that case we should definitely name AIX and MacOS X as "known" platforms.
>
>
> I'm also a bit concerned about the following code in net_util_md.c
>
>  451     rv = setsockopt(s, SOL_SOCKET, SO_REUSEPORT, (void *)&one,
> sizeof(one));
>  452     if (rv != 0 && errno == ENOPROTOOPT) {
>  453         rv = JNI_FALSE;
>  454     } else {
>  455         rv = JNI_TRUE;
>  456     }
>
> Why do we need the extra check for 'ENOPROTOOPT'? If the setsockopt() call, for whatever reason, returns with an error code other than 'ENOPROTOOPT' we will still return JNI_TRUE which I think would be bad. I think it would be more robust to always return JNI_FALSE if
> setsockopt() returns an error.
>
> Some tests I'm just running are still pending, but I think the rest looks pretty good.
>
> Regards,
> Volker
>
>> The javadoc looks good, a minor nit is that SocketOptions.SO_REUSEPORT needs @since 9.
>>
>> A few other minor nits in passing:
>>
>> - src/java.base/share/classes/java/net/AbstractPlainDatagramSocketImpl.java indentation doesn't look right.
>>
>> - sun.nio.ch.Net.checkedReusePort - shouldn't be a need to initialize the volatile boolean to its default value (there is a patch in progress to remove many of these).
>>
>> - SdpSupport.convert0 - I assume this should only use getsockopt when the socket option is supported. I think this means it will need #ifdef SO_REUSEPORT.
>>
>> - A place uses have "//SO.." when "// SO..." would be more consistent.
>>
>> I think that's it.
>>
>> -Alan
>>
>>
>>