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

[David Holmes]

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