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

[Thomas Stüfe]

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
> 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] On
> Behalf Of Dmitry Samersoff
> Sent: Dienstag, 1. März 2016 11:20
> To: Thomas Stüfe <thomas.stuefe at gmail.com>; Java Core Libs <
> 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.
>