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

[Robbin Ehn]

Hi David,

Fixed, sending new version to your other mail.

Thanks, Robbin

On 11/30/18 5:39 AM, David Holmes wrote:
> 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
>>>>>>
>>>>