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

[David Holmes]

On 1/03/2016 7:27 PM, Thomas Stüfe wrote:
> Ping...
>
> Could I have reviewer and a sponsor, please?

You don't need a sponsor for this JDK change - you are a Committer. :)

Cheers,
David

> Thanks you!
>
> Thomas
>
> On Thu, Feb 25, 2016 at 5:51 PM, Thomas Stüfe <thomas.stuefe at gmail.com>
> wrote:
>
>> 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
>>>>
>>>
>>>
>>