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

[David Holmes]

On 28/11/2018 9:31 pm, Robbin Ehn wrote:
> 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.

Good placement. Not sure why you rewrote it. There are a number of 
grammatical issues with the new version:

+ // Linux CFS scheduler (since 2.6.23) do not guarantee sched_yield(2) will

s/do/does/

+ // actually give up the CPU. Since skip buddy (v2.6.28):
+ //
+ // * Set the yielding task as skip buddy for current CPU's run queue.

s/Set/Sets/

+ // * Pick next from run queue, if empty, pick a skip buddy (can be the 
yielding task).

s/Pick/Picks/

s/pick/picks/


+ // * Clear skip buddies for this run queue (yielding task no longer a 
skip buddy).

s/Clear/Clears/

+ //
+ // An alternative is calling os::naked_short_nanosleep with a small 
number avoid

s/avoid/to avoid/

+ // getting re-scheduled immediately.
+ //

Thanks,
David

> /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
>>>>>
>>>