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

[Roger Riggs]

Hi Thomas,

When returning the address of the fdentry, the style using &fdTable[fd] 
is preferred over
the implicit pointer arithmetic (as it was in the previous version).

Occurs in all 3 deltas:

src/java.base/*/native/libnet/*_close.c:
+        result = fdTable + fd;

and
+        result = fdOverflowTable[rootindex] + slabindex;

The rest looks fine.

Thanks, Roger



On 3/10/2016 7:59 AM, Thomas Stüfe wrote:
> Thank you Dmitry!
>
> I will fix the typo before comitting.
>
> Kind Regards, Thomas
>
> On Thu, Mar 10, 2016 at 9:19 AM, Dmitry Samersoff <
> dmitry.samersoff at oracle.com> wrote:
>
>> Thomas,
>>
>> Looks good for me. But please wait for someone from core-libs team.
>>
>> PS: Could you also fix a typeo at 79, 51, 53?
>>
>>      s/initialized/initialization/
>>
>>   51  * Heap allocated during initialization - one entry per fd
>>
>> -Dmitry
>>
>> On 2016-03-10 10:59, Thomas Stüfe wrote:
>>> Hi,
>>>
>>> may I have a review of this new iteration for this fix?
>>>
>>> Thank you!
>>>
>>> Thomas
>>>
>>> On Thu, Mar 3, 2016 at 5:26 PM, Thomas Stüfe <thomas.stuefe at gmail.com>
>>> wrote:
>>>
>>>> Hi all,
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8150460
>>>>
>>>> thanks to all who took the time to review the first version of this fix!
>>>>
>>>> This is the new version:
>>>>
>>>>
>>>>
>> http://cr.openjdk.java.net/~stuefe/webrevs/8150460-linux_close-fdTable/webrev.02/webrev/
>>>> I reworked the fix, trying to add in all the input I got: This fix uses
>> a
>>>> simple one-dimensional array, preallocated at startup, for low-value
>> file
>>>> descriptors. Like the code did before. Only for large values of file
>>>> descriptors it switches to an overflow table, organized as two
>> dimensional
>>>> sparse array of fixed sized slabs, which are allocated on demand. Only
>> the
>>>> overflow table is protected by a lock.
>>>>
>>>> For 99% of all cases we will be using the plain simple fdTable structure
>>>> as before. Only for unusually large file descriptor values we will be
>> using
>>>> this overflow table.
>>>>
>>>> Memory footprint is kept low: for small values of RLIMIT_NOFILE, we will
>>>> only allocate as much space as we need. Only if file descriptor values
>> get
>>>> large, memory is allocated in the overflow table.
>>>>
>>>> Note that I avoided the proposed double-checked locking solution: I find
>>>> it too risky in this place and also unnecessary. When calling
>> getFdEntry(),
>>>> we will be executing a blocking IO operation afterwards, flanked by two
>>>> mutex locks (in startOp and endOp). So, I do not think the third mutex
>> lock
>>>> in getFdEntry will add much, especially since it is only used in case of
>>>> larger file descriptor values.
>>>>
>>>> I also added the fix to bsd_close.c and aix_close.c. I do not like this
>>>> code triplication. I briefly played around with unifying this code, but
>>>> this is more difficult than it seems: implementations subtly differ
>> between
>>>> the three platforms, and solaris implementation is completely
>> different. It
>>>> may be a worthwhile cleanup, but that would be a separate issue.
>>>>
>>>> I did some artificial tests to check how the code does with many and
>> large
>>>> file descriptor values, all seemed to work well. I also ran java/net
>> jtreg
>>>> tests on Linux and AIX.
>>>>
>>>> Kind Regards, Thomas
>>>>
>>>>
>>
>> --
>> Dmitry Samersoff
>> Oracle Java development team, Saint Petersburg, Russia
>> * I would love to change the world, but they won't give me the sources.
>>