[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