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

[Dmitry Samersoff]

Thomas,

We probably can do:

if (fdTable[rootArrayIndex] != NULL) {
   entryTable = fdTable[rootArrayIndex];
}
else { // existing code
  pthread_mutex_lock(&fdTableLock);
  if (fdTable[rootArrayIndex] == NULL) {
      ....
  }
}

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


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.