RFR(s): 8150460: (linux|bsd|aix)_close.c: file descriptor table may become large or may not work at all
Thomas Stüfe
thomas.stuefe at gmail.com
Wed Mar 2 08:09:53 UTC 2016
Hi Hans,
thanks for the hint!
But how would I do this for my problem:
Allocate memory, zero it out and then store the pointer into a variable
seen by other threads, while preventing the other threads from seeing . I
do not understand how atomics would help: I can make the pointer itself an
atomic, but that only guarantees memory ordering in regard to this
variable, not to the allocated memory.
Kind Regards, Thomas
On Wed, Mar 2, 2016 at 2:27 AM, Hans Boehm <hboehm at google.com> wrote:
> 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.
>>>>
>>>>
>>>>
>>>
>>>
>
More information about the core-libs-dev
mailing list