RFR: 8203227: Introduce os::processor_id() for Linux and Solaris

[Per Liden]

Hi David,

Thanks for looking at this.

On 05/16/2018 04:24 AM, David Holmes wrote:
> Hi Per,
> 
> On 16/05/2018 1:07 AM, Per Liden wrote:
>> This patch introduces os::processor_id() to get the id of the CPU that 
>> the caller is currently executing on. Obviously, the returned id 
>> should only be viewed as a strong hint, since the information might be 
>> instantly out-of-date.
> 
> Or even wrong to begin with:
> 
> https://stackoverflow.com/questions/36934736/is-sched-getcpu-reliable-on-linux 
> 
> 
> caveat-emptor! :)
> 
>> We're using this in ZGC to do various CPU-local operations. We currently
> 
> So there's a requirement that the CPU value returned by processor_id() 
> is compatible with some other API's that take a processor id? Does that 
> need documenting in some form?

Good point, this should be made more clear. We currently assume that 
os::processor_id() returns a value from 0 to (os::processor_count() - 
1). Adding a comment in os.hpp clarifying that.

> 
>> only need this for Linux and Solaris, which is what this patch adds. 
> 
> Are the API's that take the processor id defined in os as well? 
> Otherwise it seems this functionality should just be localized to the 
> specific os_<os>.cpp file from which it is used (and/or the os::<OS> 
> class).

The os::processor_id() call-sites we have are in shared os-agnostic 
code. This function is also fairly closely related to 
os::processor_count(), so it seems like os:: would be the natural place 
for this.

> 
>> Support for additional platforms can be added when needed. I didn't 
>> add dummy os::processor_id() to the not-yet-implemented platforms, 
>> since I think it's better to get a early compile-time error instead of 
>> a Unimplemented() at runtime, if you try to use them.
> 
> I'd rather not see the runtime support of our primary platforms of 
> interest fragmented like this. Are there technical challenges to doing 
> this?

I'm a bit reluctant to add this for other platforms at this point, since 
some platforms might require non-trivial implementations, which would go 
unused/untested. For macOS I believe we would need something like this:

http://cr.openjdk.java.net/~gziemski/zgc_os_processor_id/webrev/src/hotspot/os/bsd/os_bsd.cpp.udiff.html

(I should add that I don't know if this patch does the right thing, but 
Gerard apparently had some success with it)

For Windows, I believe a call to GetCurrentProcessorNumber() should be 
enough (we don't seem to handle/support logical processor groups in 
os::processor_count() so should be safe to do the same thing here).

The plan has been to add support for these if/when ZGC supports these 
platforms (unless someone else needs and adds them before that). But 
adding them now makes me a bit uneasy, especially given the non-trivial 
nature of the macOS implementation.

> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8203227
>> Webrev: http://cr.openjdk.java.net/~pliden/8203227/webrev.0
> 
> For Solaris there is no need to introduce Solaris::getcpuid() just 
> because Linux already has that. (Unless this ends up not being needed in 
> the os class of course :) ).

Check.

> 
> Are you allowing for the -1 return value on Linux or assuming it will 
> never happen in practice? If the former then that should be documented 
> as part of the semantics of os::processor_id(). If the latter then you 
> should probably add an assert or guarantee. I suspect this is a 
> historical restriction no longer relevant to us - in which case we can 
> probably clean out the dynamic lookup/linking!).

We're assuming it will never happen in practice, and this is currently 
guarded with a guarantee() during initialization of ZGC. sched_getcpu() 
has "only" been around since glibc 2.14 (June 2011). However, we can 
also receive -1 if the kernel doesn't have the getcpu() syscall (first 
introduced in Linux 2.6.19, Nov 2006).

So, it seems safe to at least assume that the getcpu() syscall is 
available, and only have an single check for that during startup (with a 
vm_exit_during_initialization() if it's unavailable).

Here's an updated webrev for continued discussion:

http://cr.openjdk.java.net/~pliden/8203227/webrev.1

cheers,
Per

> 
> Thanks,
> David
> 
>> /Per