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

Wed Mar 2 01:27:05 UTC 2016

The preferred C11 solution is to use atomics.  Using just memory fences
here is tricky, and not fully correct, since data races have undefined
semantics in C (and Posix).

On Tue, Mar 1, 2016 at 12:56 PM, David Holmes <david.holmes at oracle.com>
wrote:

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