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.lan...@sap.com <mailto:christoph.lan...@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-boun...@openjdk.java.net
<mailto:core-libs-dev-boun...@openjdk.java.net>] On Behalf Of Dmitry
Samersoff
Sent: Dienstag, 1. März 2016 11:20
To: Thomas Stüfe <thomas.stu...@gmail.com
<mailto:thomas.stu...@gmail.com>>; Java Core Libs
<core-libs-dev@openjdk.java.net <mailto:core-libs-dev@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.
