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

Tue Mar 1 13:51:21 UTC 2016

Dmitry,

On Tue, Mar 1, 2016 at 2:49 PM, Dmitry Samersoff <
dmitry.samersoff at oracle.com> wrote:

> Thomas,
>
> > For fd < 65535, I effectively fall back to a plain array lookup by
> > setting the size of the base table to 1. So, for this case the sparse
> > array degenerates to a one-dimensional plain array.
>
> It might be good to make it more explicit: just allocate a separate
> array for values less than 65535 and skip other machinery if
> nbr_files.rlim_max less than 65536.
>
>
Yes, maybe it makes the code more readable. My code is clever, but I am not
a big fan of cleverness if it costs readability. I will prepare a new
change.

Thanks for reviewing!

..Thomas

> But it's just a cosmetic, so feel free to leave the code as is.
>
> -Dmitry
>
>
>
> On 2016-03-01 16:33, Thomas Stüfe wrote:
> > Hi Dmitry,
> >
> > On Tue, Mar 1, 2016 at 1:44 PM, Dmitry Samersoff
> > <dmitry.samersoff at oracle.com <mailto:dmitry.samersoff at oracle.com>>
> wrote:
> >
> >     Christoph,
> >
> >     > Dmitry, I think you are referring to an outdated version of the
> >     > webrev, the current one is this:
> >
> >     Yes. Sorry!
> >
> >     You may consider a bit different approach to save memory:
> >
> >     Allocate multiple baseTables for different ranges of fd's with
> >     plain array of 32 * (fdEntry_t*) for simple case.
> >
> >     i.e. if (fd < 32)
> >              do plain array lookup
> >
> >          if (fd < N1)
> >              do two steps lookup in baseTable1
> >
> >          if (fd < N2)
> >              do two steps lookup in baseTable2
> >
> >
> > How does this differ from my approach
> > in
> http://cr.openjdk.java.net/~stuefe/webrevs/8150460-linux_close-fdTable/webrev.01/webrev/
> ?
> >
> > For fd < 65535, I effectively fall back to a plain array lookup by
> > setting the size of the base table to 1. So, for this case the sparse
> > array degenerates to a one-dimensional plain array.
> >
> > Kind Regards, Thomas
> >
> >
> >
> >          ...
> >
> >     -Dmitry
> >
> >
> >
> >     On 2016-03-01 13:47, Langer, Christoph 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.
>