On Fri, 2019-11-08 at 09:58 +0100, Magnus Ihse Bursie wrote: > On 2019-11-07 20:48, Severin Gehwolf wrote: > > Hi Erik, > > > > On Thu, 2019-11-07 at 10:01 -0800, Erik Joelsson wrote: > > > Hello Severin, > > > > > > Taking ulimit -u into account does seem like a good idea. I don't see > > > why it needs to be limited to just aarch64 though. It should however be > > > limited to OSes that use ulimit (i.e. not windows). > > > > > > I would suggest removing the arbitrary thresholds of 16 and 4096 and try > > > to come up with a plain number based on the ulimit value. You are saying > > > 14 is good for 4096, so the formula for the ULIMIT_DIVIDER is 4096 / 14, > > > which you can just write as: > > > > > > ULIMIT_DIVIER := (4096 / 14) > > > > > > And let the awk script calculate it if needed. In the awk script, you > > > need to put line 275 as conditional of the if statement on line 274. > > > Otherwise ULIMIT=-1 will cause concurrency -1. > > Thanks for the review! Updated webrev: > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8233712/02/webrev/ > I know I'm coming here and making a simple patch more complicated, but > I'm not entirely comfortable with the explicit call to ulimit. Our > principle is that all executables that we call from the makefiles should > be confirmed existing by configure, so the call should be "$(shell > $(ULIMIT) -u)". > > It should be relatively trivial to add this as a required prog in > basics.m4 and spec.gmk.in. Make sure you only add it as required for > non-Windows platforms. If you do this, you can change the check in the > makefile to if $(ULIMIT) has a value, rather than checking on platform.
Sure, will do. Thanks Magnus! Cheers, Severin > /Magnus > > What do you think? > > > > Thanks, > > Severin > > > > > /Erik > > > > > > On 2019-11-07 06:59, Severin Gehwolf wrote: > > > > Hi, > > > > > > > > Could I please get a review of this change for running tests on big > > > > Aarch64 boxes? Currently, only memory and number of cores are taken > > > > into account for the -concurrency setting of jtreg. After this patch > > > > ulimit -u settings are taken into account as well on big Aarch64 boxes > > > > with > 16 cores, yet low ulimit -u setting (<= 4096). This is to avoid > > > > running into the max allowed threads limit causing random test > > > > failures. > > > > > > > > Thoughts? > > > > > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8233712 > > > > webrev: > > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8233712/01/webrev/ > > > > > > > > Testing: Ran tier1 tests on a large Aarch64 box and low ulimit -u > > > > value. Tests run stable. Did the same for a user with high ulimit -u > > > > value, which resulted in concurrency setting as before this patch. > > > > > > > > Thanks, > > > > Severin > > > >
