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

[Thomas Stüfe]

Ping...

Could I have reviewer and a sponsor, please?

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
>>>
>>
>>
>