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 11:39 PM, Dmitry Samersoff wrote:
> Thomas,
>
> We probably can do:
>
> if (fdTable[rootArrayIndex] != NULL) {
>     entryTable = fdTable[rootArrayIndex];
> }
> else { // existing code
>    pthread_mutex_lock(&fdTableLock);
>    if (fdTable[rootArrayIndex] == NULL) {
>        ....
>    }
> }

This is double-checked locking and it requires memory barriers to be 
correct - as Thomas already discussed.

David

> -Dmitry
>
>
> On 2016-03-01 16:13, Thomas Stüfe wrote:
>> Dmitry, Christoph,
>>
>> I am not 100% sure this would work for weak ordering platforms.
>>
>> If I understand you correctly you suggest the double checking pattern:
>>
>> if (basetable[index] == NULL) {
>>      lock
>>          if (basetable[index] == NULL) {
>>              basetable[index] = calloc(size);
>>          }
>>       unlock
>> }
>>
>> The problem I cannot wrap my head around is how to make this safe for
>> all platforms. Note: I am not an expert for this.
>>
>> How do you prevent the "reading thread reads partially initialized
>> object" problem?
>>
>> Consider this: We need to allocate memory, set it completely to zero and
>> then store a pointer to it in basetable[index]. This means we have
>> multiple stores - one store for the pointer, n stores for zero-ing out
>> the memory, and god knows how many stores the C-Runtime allcoator needs
>> to update its internal structures.
>>
>> On weak ordering platforms like ppc (and arm?), the store for
>> basetable[index] may be visible before the other stores, so the reading
>> threads, running on different CPUs, may read a pointer to partially
>> initialized memory. What you need is a memory barrier between the
>> calloc() and store of basetable[index], to prevent the latter store from
>> floating above the other stores.
>>
>> I did not find anything about multithread safety in the calloc() docs,
>> or guaranteed barrier behaviour, nor did I expect anything. In the
>> hotspot we have our memory barrier APIs, but in the JDK I am confined to
>> basic C and there is no way that I know of to do memory barriers with
>> plain Posix APIs.
>>
>> Bottomline, I am not sure. Maybe I am too cautious here, but I do not
>> see a way to make this safe without locking the reader thread too.
>>
>> Also, we are about to do an IO operation - is a mutex really that bad
>> here? Especially with the optimization Roger suggested of pre-allocating
>> the basetable[0] array and omitting lock protection there?
>>
>> Kind Regards,
>>
>> Thomas
>>
>>
>>
>>
>> On Tue, Mar 1, 2016 at 11:47 AM, Langer, Christoph
>> <christoph.langer at sap.com <mailto:christoph.langer at sap.com>> wrote:
>>
>>      Hi Dmitry, Thomas,
>>
>>      Dmitry, I think you are referring to an outdated version of the
>>      webrev, the current one is this:
>>      http://cr.openjdk.java.net/~stuefe/webrevs/8150460-linux_close-fdTable/webrev.01/webrev/
>>
>>      However, I agree - the lock should probably not be taken every time
>>      but only in the case where we find the entry table was not yet
>>      allocated.
>>
>>      So, maybe getFdEntry should always do this:
>>      entryTable = fdTable[rootArrayIndex]; // no matter if rootArrayIndex
>>      is 0
>>
>>      Then check if entryTable is NULL and if yes then enter a guarded
>>      section which does the allocation and before that checks if another
>>      thread did it already.
>>
>>      Also I'm wondering if the entryArrayMask and the rootArrayMask
>>      should be calculated once in the init() function and stored in a
>>      static field? Because right now it is calculated every time
>>      getFdEntry() is called and I don't think this would be optimized by
>>      inlining...
>>
>>      Best regards
>>      Christoph
>>
>>      -----Original Message-----
>>      From: core-libs-dev [mailto:core-libs-dev-bounces at openjdk.java.net
>>      <mailto:core-libs-dev-bounces at openjdk.java.net>] On Behalf Of Dmitry
>>      Samersoff
>>      Sent: Dienstag, 1. März 2016 11:20
>>      To: Thomas Stüfe <thomas.stuefe at gmail.com
>>      <mailto:thomas.stuefe at gmail.com>>; Java Core Libs
>>      <core-libs-dev at openjdk.java.net <mailto:core-libs-dev at openjdk.java.net>>
>>      Subject: Re: RFR(s): 8150460: (linux|bsd|aix)_close.c: file
>>      descriptor table may become large or may not work at all
>>
>>      Thomas,
>>
>>      Sorry for being later.
>>
>>      I'm not sure we should take a lock at ll. 131 for each fdTable lookup.
>>
>>      As soon as we never deallocate fdTable[base_index] it's safe to try to
>>      return value first and then take a slow path (take a lock and check
>>      fdTable[base_index] again)
>>
>>      -Dmitry
>>
>>
>>      On 2016-02-24 20:30, 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.
>>      >
>>      > 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.
>>      >
>>      > Thank you and 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.
>>
>>
>
>