[crac] RFR: RFC: -XX:CPUFeatures=0xnumber for CPU migration [v2]

Jan Kratochvil duke at openjdk.org
Thu Feb 2 06:06:31 UTC 2023


On Wed, 1 Feb 2023 14:06:01 GMT, Dan Heidinga <heidinga at openjdk.org> wrote:

>> Jan Kratochvil has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - +comment; found by Dan Heidinga.
>>  - Rename signalled->retry; found by Dan Heidinga.
>
> src/hotspot/os/linux/os_linux.cpp line 6050:
> 
>> 6048:         unsigned long l = strtoul(name, &endptr, 10);
>> 6049:         pid_t ent_tid = l;
>> 6050:         if (l >= LONG_MAX || ent_tid != (long) l) {
> 
> `l` is an `unsigned long` and I think `LONG_MAX` is a signed value as there's also a `ULONG_MAX`.  Are we using this to validate that `l` is a positive value?  Otherwise, I'm not sure I understand this condition.

The number must fit in `pid_t` (otherwise it is a weird Linux kernel). Therefore `int`. But there is no `strtoi`. Also PID must not be negative (otherwise it is a weird Linux kernel again). So I found this as the most simple and safe code to validate `name` contains a non-negative number which can fit in `pid_t`.

> src/hotspot/os/linux/os_linux.cpp line 6074:
> 
>> 6072:   static pthread_mutex_t signalled_mutex;
>> 6073:   static pthread_cond_t signalled_cond;
>> 6074:   static size_t signalled;
> 
> The above `template<class Callback> bool all_threads` also uses a "signalled" local variable.  It might be clearer to rename one of these so future readers don't have to think about scoping and shadowing rules

OK, yes, renamed the local variable to `retry`.

> src/hotspot/os/linux/os_linux.cpp line 6086:
> 
>> 6084:     err = pthread_cond_signal(&signalled_cond);
>> 6085:     assert(!err, "pthread error");
>> 6086:     pthread_mutex_t unused_mutex = PTHREAD_MUTEX_INITIALIZER;
> 
> Waiting on a local mutex deserves a comment to explain the approach.  I think we're waiting until the thaw operation signals the resume condition but better to be explicit in the code on the intended interactions between the waiters

Added: `// Just wait until thaw() is called. No mutex is needed for that.`

-------------

PR: https://git.openjdk.org/crac/pull/41


More information about the crac-dev mailing list