RFR(s): 8214180: Need better granularity for sleeping

[Robbin Ehn]

Hi,

I added a new version of the comment to yield:
http://cr.openjdk.java.net/~rehn/8214180/v2/inc/webrev/

Let me know if that seem like a good way.

/Robbin

On 2018-11-26 23:45, David Holmes wrote:
> On 27/11/2018 12:32 am, Daniel D. Daugherty wrote:
>> On 11/24/18 3:38 PM, Robbin Ehn wrote:
>>> Hi Dan,
>>>
>>> On 2018-11-24 15:28, Daniel D. Daugherty wrote:
>>>> src/hotspot/os/linux/os_linux.cpp
>>>>      old L4030 // Short sleep, direct OS call.
>>>>      old L4031 //
>>>>      old L4032 // Note: certain versions of Linux CFS scheduler (since 2.6.23) do not guarantee
>>>>      old L4033 // sched_yield(2) will actually give up the CPU:
>>>>      old L4034 //
>>>>      old L4035 //   * Alone on this pariticular CPU, keeps running.
>>>>      old L4036 //   * Before the introduction of "skip_buddy" with "compat_yield" disabled
>>>>      old L4037 //     (pre 2.6.39).
>>>>      old L4038 //
>>>>      old L4039 // So calling this with 0 is an alternative.
>>>>      old L4040 //
>>>>          I noticed this comment didn't get copied over to os_posix.cpp.
>>>>          Is the comment about an old enough Linux kernel that it is no
>>>>          longer useful?
>>>>
>>>
>>> At least to me it is not useful. Having it in the generic os_posix.cpp seems
>>> miss-placed ? Let me know if you want it somewhere?
>>
>> Looks like the comment was added by this changeset:
>>
>> $ hg annot src/os/linux/vm/os_linux.cpp | grep 'certain versions of Linux CFS scheduler'
>>   5840: // Note: certain versions of Linux CFS scheduler (since 2.6.23) do not guarantee
>>
>> $ hg log -r 5840
>> changeset:   5840:5944dba4badc
>> user:        dsimms
>> date:        Fri Jan 24 09:28:47 2014 +0100
>> summary:     8028280: ParkEvent leak when running modified runThese which only loads classes
>>
>> $ hg diff -r 5839 -r 5840 src/os/linux/vm/os_linux.cpp
>> diff -r d050fbf914d8 -r 5944dba4badc src/os/linux/vm/os_linux.cpp
>> --- a/src/os/linux/vm/os_linux.cpp      Thu Jan 23 16:02:14 2014 -0500
>> +++ b/src/os/linux/vm/os_linux.cpp      Fri Jan 24 09:28:47 2014 +0100
>> @@ -3871,9 +3871,33 @@
>>     }
>>   }
>>
>> -int os::naked_sleep() {
>> -  // %% make the sleep time an integer flag. for now use 1 millisec.
>> -  return os::sleep(Thread::current(), 1, false);
>> +//
>> +// Short sleep, direct OS call.
>> +//
>> +// Note: certain versions of Linux CFS scheduler (since 2.6.23) do not guarantee
>> +// sched_yield(2) will actually give up the CPU:
>> +//
>> +//   * Alone on this pariticular CPU, keeps running.
>> +//   * Before the introduction of "skip_buddy" with "compat_yield" disabled
>> +//     (pre 2.6.39).
>> +//
>> +// So calling this with 0 is an alternative.
>> +//
>> +void os::naked_short_sleep(jlong ms) {
>> +  struct timespec req;
>> +
>> +  assert(ms < 1000, "Un-interruptable sleep, short time use only");
>> +  req.tv_sec = 0;
>> +  if (ms > 0) {
>> +    req.tv_nsec = (ms % 1000) * 1000000;
>> +  }
>> +  else {
>> +    req.tv_nsec = 1;
>> +  }
>> +
>> +  nanosleep(&req, NULL);
>> +
>> +  return;
>>   }
>>
>>   // Sleep forever; naked call to OS-specific sleep; use with CAUTION
>>
>>
>> Since Mr Simms is local to you, I'll let you ping him when you
>> get a chance. Don't let that hold up your fix. If Mr Simms wants
>> the comment restored (somewhere), then we can do that with a
>> different fix.
> 
> I think Mr Simms is still on vacation. Regardless that comment seems better applied to:
> 
> 4042 void os::naked_yield() {
> 4043   sched_yield();
> 4044 }
> 
> with a reference to os::naked_sleep, than the other way around.
> 
> Cheers,
> David
> -----
> 
>>
>> Dan
>>
>>
>>>
>>>>
>>>> Thumbs up.
>>>
>>> Thanks Dan!
>>>
>>> /Robbin
>>>
>>>
>>>>
>>>> Dan
>>>>
>>>>
>>>>>
>>>>> Passes t1-3.
>>>>>
>>>>> Thanks, Robbin
>>>>
>>