RFR(s): 8150460: (linux|bsd|aix)_close.c: file descriptor table may become large or may not work at all

[Thomas Stüfe]

Hi Roger,

thank you for the review!

New webrev:
http://cr.openjdk.java.net/~stuefe/webrevs/8150460-linux_close-fdTable/webrev.01/webrev/

Please find my comments inline.

On Wed, Feb 24, 2016 at 8:28 PM, Roger Riggs <Roger.Riggs at oracle.com> wrote:

> Hi Thomas,
>
> On 2/24/2016 12:30 PM, Thomas Stüfe wrote:
>
>> Hi all,
>>
>> please take a look at this proposed fix.
>>
>> The bug: https://bugs.openjdk.java.net/browse/JDK-8150460
>> The Webrev:
>>
>> http://cr.openjdk.java.net/~stuefe/webrevs/8150460-linux_close-fdTable/webrev.00/webrev/
>>
>> Basically, the file descriptor table implemented in linux_close.c may not
>> work for RLIMIT_NO_FILE=infinite or may grow very large (I saw a 50MB
>> table) for high values for RLIMIT_NO_FILE. Please see details in the bug
>> description.
>>
>> The proposed solution is to implement the file descriptor table not as
>> plain array, but as a twodimensional sparse array, which grows on demand.
>> This keeps the memory footprint small and fixes the corner cases described
>> in the bug description.
>>
>> Please note that the implemented solution is kept simple, at the cost of
>> somewhat higher (some kb) memory footprint for low values of
>> RLIMIT_NO_FILE.
>> This can be optimized, if we even think it is worth the trouble.
>>
>> Please also note that the proposed implementation now uses a mutex lock
>> for
>> every call to getFdEntry() - I do not think this matters, as this is all
>> in
>> preparation for an IO system call, which are usually way more expensive
>> than a pthread mutex. But again, this could be optimized.
>>
> I would suggest preallocating the index[0] array and then skip the mutex
> for that case.
> That would give the same as current performance.
>
>
I did this.


> And I'd suggest a different hi/low split of the indexes to reduce the size
> of pre-allocated memory.
> Most processes are going to use a lot fewer than 16384 fd's.  How about
> 2048?
>

I did this too. Now I calculate the split point based on RLIMIT_NO_FILE.
For small values of RLIMIT_NO_FILE
(<64K), I effectivly fall back to a one-dimensional array by making the
first level table a size 1. For larger values,
multiple second level tables, each 64K size, will be allocated on demand
(save for the first one which is preallocated).


> I have my doubts about needing to cover fd's  up to the full range of 32
> bits.
> Can the RLIMIT_NO_FILE be used too parametrize the allocation of the first
> level table?
>
>
I did this.

Interesting note, I found nowhere in the Posix specs a mentioning that
socked descriptors have to be handed out
sequentially and therefore cannot be larger than RLIMIT_NO_FILE. But in
reality on all operating systems file descriptors
seem to be [0, RLIMIT_NO_FILE).


Not specific to your change but it would nice to see consistency between
> libnio and libnet on
> the name of the sigWakeup/INTERRUPT_SIGNAL constant.


I agree, but this is out of the scope of this bug fix.


>
>
>> This is an implementation proposal for Linux; the same code found its way
>> to BSD and AIX. Should you approve of this fix, I will modify those files
>> too.
>>
> yes please.
>
> $.02, Roger
>
>
>
Thanks, Thomas


>
>> Thank you and Kind Regards, Thomas
>>
>
>