(Round 2) RFR(s): 8150460: (linux|bsd|aix)_close.c: file descriptor table may become large or may not work at all
Roger Riggs
Roger.Riggs at Oracle.com
Thu Apr 14 18:18:28 UTC 2016
+1, looks fine to me also.
Roger
On 4/13/2016 6:46 AM, Thomas Stüfe wrote:
> Thanks Dmitry!
>
> On Wed, Apr 13, 2016 at 12:00 PM, Dmitry Samersoff
> <dmitry.samersoff at oracle.com <mailto:dmitry.samersoff at oracle.com>> wrote:
>
> Thomas,
>
> Looks good for me!
>
> -Dmitry
>
>
> On 2016-04-13 12:12, Thomas Stüfe wrote:
> > Hi Roger, Dmitry,
> >
> > May I ask you both to have a last look at this change before I
> commit?
> > It took a while for this to go through our internal tests, hence
> the delay.
> >
> > New
> > version:
> http://cr.openjdk.java.net/~stuefe/webrevs/8150460-linux_close-fdTable/webrev.03/webrev/
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8150460-linux_close-fdTable/webrev.03/webrev/>
> > Delta to last
> > version:
> http://cr.openjdk.java.net/~stuefe/webrevs/8150460-linux_close-fdTable/webrev.02-webrev.03/webrev/
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8150460-linux_close-fdTable/webrev.02-webrev.03/webrev/>
> >
> > The changes are mostly cosmetic:
> >
> > - I tweaked a number of comments to make them clearer
> > - If getrlimit(RLIMIT_NOFILE) returns an error, I now handle
> this correctly.
> > - Just for readability, I explicitly initialize global variables
> instead
> > of relying on static zero-initialization.
> > - As Roger requested, I changed accesses to the entry table elements
> > from using implicit pointer arithmetic to explicit array
> accesses with "&".
> >
> > Thank you for your time!
> >
> > Kind Regards, Thomas
> >
> > On Sat, Mar 12, 2016 at 8:38 AM, Thomas Stüfe
> <thomas.stuefe at gmail.com <mailto:thomas.stuefe at gmail.com>
> > <mailto:thomas.stuefe at gmail.com <mailto:thomas.stuefe at gmail.com>>> wrote:
> >
> > Thank you Roger!
> >
> > On Fri, Mar 11, 2016 at 4:26 PM, Roger Riggs
> <Roger.Riggs at oracle.com <mailto:Roger.Riggs at oracle.com>
> > <mailto:Roger.Riggs at oracle.com <mailto:Roger.Riggs at oracle.com>>> wrote:
> >
> > Hi Thomas,
> >
> > When returning the address of the fdentry, the style using
> > &fdTable[fd] is preferred over
> > the implicit pointer arithmetic (as it was in the
> previous version).
> >
> > Occurs in all 3 deltas:
> >
> > src/java.base/*/native/libnet/*_close.c:
> > + result = fdTable + fd;
> >
> > and
> > + result = fdOverflowTable[rootindex] + slabindex;
> >
> > The rest looks fine.
> >
> > Thanks, Roger
> >
> >
> >
> >
> > On 3/10/2016 7:59 AM, Thomas Stüfe wrote:
> >
> > Thank you Dmitry!
> >
> > I will fix the typo before comitting.
> >
> > Kind Regards, Thomas
> >
> > On Thu, Mar 10, 2016 at 9:19 AM, Dmitry Samersoff <
> > dmitry.samersoff at oracle.com <mailto:dmitry.samersoff at oracle.com>
> > <mailto:dmitry.samersoff at oracle.com <mailto:dmitry.samersoff at oracle.com>>>
> wrote:
> >
> > Thomas,
> >
> > Looks good for me. But please wait for someone from
> > core-libs team.
> >
> > PS: Could you also fix a typeo at 79, 51, 53?
> >
> > s/initialized/initialization/
> >
> > 51 * Heap allocated during initialization -
> one entry
> > per fd
> >
> > -Dmitry
> >
> > On 2016-03-10 10:59, Thomas Stüfe wrote:
> >
> > Hi,
> >
> > may I have a review of this new iteration
> for this fix?
> >
> > Thank you!
> >
> > Thomas
> >
> > On Thu, Mar 3, 2016 at 5:26 PM, Thomas Stüfe
> > <thomas.stuefe at gmail.com
> <mailto:thomas.stuefe at gmail.com>
> > <mailto:thomas.stuefe at gmail.com
> <mailto:thomas.stuefe at gmail.com>>>
> > wrote:
> >
> > Hi all,
> >
> > https://bugs.openjdk.java.net/browse/JDK-8150460
> >
> > thanks to all who took the time to
> review the
> > first version of this fix!
> >
> > This is the new version:
> >
> >
> >
> >
> http://cr.openjdk.java.net/~stuefe/webrevs/8150460-linux_close-fdTable/webrev.02/webrev/
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8150460-linux_close-fdTable/webrev.02/webrev/>
> >
> > I reworked the fix, trying to add in all the
> > input I got: This fix uses
> >
> > a
> >
> > simple one-dimensional array,
> preallocated at
> > startup, for low-value
> >
> > file
> >
> > descriptors. Like the code did before.
> Only for
> > large values of file
> > descriptors it switches to an overflow
> table,
> > organized as two
> >
> > dimensional
> >
> > sparse array of fixed sized slabs, which are
> > allocated on demand. Only
> >
> > the
> >
> > overflow table is protected by a lock.
> >
> > For 99% of all cases we will be using
> the plain
> > simple fdTable structure
> > as before. Only for unusually large file
> > descriptor values we will be
> >
> > using
> >
> > this overflow table.
> >
> > Memory footprint is kept low: for small
> values
> > of RLIMIT_NOFILE, we will
> > only allocate as much space as we need.
> Only if
> > file descriptor values
> >
> > get
> >
> > large, memory is allocated in the
> overflow table.
> >
> > Note that I avoided the proposed
> double-checked
> > locking solution: I find
> > it too risky in this place and also
> unnecessary.
> > When calling
> >
> > getFdEntry(),
> >
> > we will be executing a blocking IO operation
> > afterwards, flanked by two
> > mutex locks (in startOp and endOp). So,
> I do not
> > think the third mutex
> >
> > lock
> >
> > in getFdEntry will add much, especially
> since it
> > is only used in case of
> > larger file descriptor values.
> >
> > I also added the fix to bsd_close.c and
> > aix_close.c. I do not like this
> > code triplication. I briefly played
> around with
> > unifying this code, but
> > this is more difficult than it seems:
> > implementations subtly differ
> >
> > between
> >
> > the three platforms, and solaris
> implementation
> > is completely
> >
> > different. It
> >
> > may be a worthwhile cleanup, but that
> would be a
> > separate issue.
> >
> > I did some artificial tests to check how the
> > code does with many and
> >
> > large
> >
> > file descriptor values, all seemed to
> work well.
> > I also ran java/net
> >
> > jtreg
> >
> > tests on Linux and AIX.
> >
> > 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.
>
>
More information about the core-libs-dev
mailing list