Bug 150095 - [kernel] [patch] Account for reserved itimers which shouldn't count against _SC_TIMER_MAX
Summary: [kernel] [patch] Account for reserved itimers which shouldn't count against _...
Status: Open
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: unspecified
Hardware: Any Any
: Normal Affects Only Me
Assignee: freebsd-bugs mailing list
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-29 23:00 UTC by Enji Cooper
Modified: 2017-12-31 22:23 UTC (History)
0 users

See Also:


Attachments
file.diff (2.60 KB, patch)
2010-08-29 23:00 UTC, Enji Cooper
no flags Details | Diff
account-for-reserved-itimers.diff.txt (2.23 KB, text/plain; charset=US-ASCII)
2010-08-29 23:17 UTC, Enji Cooper
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Enji Cooper freebsd_committer 2010-08-29 23:00:04 UTC
Currently 3 reserve itimers are allocated purely for kernel use, but unfortunately this breaks the SUSv4 requirement which states:

{TIMER_MAX}
    Maximum number of timers per process supported by the implementation.
    Minimum Acceptable Value: {_POSIX_TIMER_MAX}

..

{_POSIX_TIMER_MAX}
    The per-process number of timers.
    Value: 32

Example (from testcases/open_posix_testsuite/conformance/behavior/timers in the LTP package):

$ ./1-1.run-test 
[29] timer_create() did not return success: Resource temporarily unavailable

After applying this patch, things function as expected:

$ ./1-1.run-test 
Test PASSED

Fix: Patch attached with submission follows:
How-To-Repeat: 1. Grab LTP: the package needs to be July or later as I didn't apply many fixes to the branch until post that date -- this may require grabbing the source from git. See the sourceforge page for more info (http://sf.net/projects/ltp). Use ltp/ltp-dev instead of ltp/ltp in the directions, if ltp-dev.git still exists (I'm working on fixing that, because the directions on the webpage are incorrect).
2. Go to the open_posix_testsuite directory: cd $LTP_DIR/testcases/open_posix_testsuite
3. Generate the Makefiles: make generate-makefiles
4. Build the test: cd conformance/behavior/timers; make
5. Execute the test like: ./1-1.run-test
Comment 1 Enji Cooper freebsd_committer 2010-08-29 23:17:44 UTC
    Sorry... some noise from nanosleep was in the last patch (another
bug I filed -- kern/149980). This patch only deals with the items I
described.
Thanks,
-Garrett
Comment 2 Alexander Best freebsd_committer 2010-09-05 20:48:04 UTC
Responsible Changed
From-To: freebsd-bugs->mav

If I remember correctly Alexander Motin is working on timers. He might know how 
to handle this PR.
Comment 3 Alexander Motin freebsd_committer 2010-09-06 09:35:18 UTC
Hi.

Sorry, but this is not my area of knowledge yet. I am working on kernel
time event producing, while this is consuming, half way to user-level.

Still few comments from quick review:
 - patch seems slightly incomplete. At least in itimers_alloc() loop
condition need to be changed.
 - To reduce chance of alike mistakes I would implement that patch in
opposite way, by defining TIMER_MAX as (32 + TIMER_RESERVED), but
reporting to user-level as TIMER_MAX - TIMER_RESERVED.
 - I have looked on setitimer() implementation referenced here and found
no any interaction with timer_create() API and TIMER_MAX. There is
indeed many loose ends which make me thing there was or intended to be
something, but I haven't found it. Have I missed something?
 - this code was written by David Xu. I would asked him.

-- 
Alexander Motin
Comment 4 David Xu freebsd_committer 2010-09-06 18:48:03 UTC
Alexander Motin wrote:
> Hi.
> 
> Sorry, but this is not my area of knowledge yet. I am working on kernel
> time event producing, while this is consuming, half way to user-level.
> 
> Still few comments from quick review:
>  - patch seems slightly incomplete. At least in itimers_alloc() loop
> condition need to be changed.
>  - To reduce chance of alike mistakes I would implement that patch in
> opposite way, by defining TIMER_MAX as (32 + TIMER_RESERVED), but
> reporting to user-level as TIMER_MAX - TIMER_RESERVED.
>  - I have looked on setitimer() implementation referenced here and found
> no any interaction with timer_create() API and TIMER_MAX. There is
> indeed many loose ends which make me thing there was or intended to be
> something, but I haven't found it. Have I missed something?
>  - this code was written by David Xu. I would asked him.
> 

Yes, the code was written by me long time ago, my original intention
was to not only implement timer_create() but also merge existing
setitimer() code into unique module, but I had never done it
because lack of time, so three slots were reserved for setitimer.
I can see that there is inconsistent between TIMER_MAX and maximum
timers user can create, your both methods will work.

Thanks,
David Xu
Comment 5 Enji Cooper freebsd_committer 2010-09-08 08:34:21 UTC
2010/9/6 Alexander Motin <mav@freebsd.org>:
> Hi.
>
> Sorry, but this is not my area of knowledge yet. I am working on kernel
> time event producing, while this is consuming, half way to user-level.
>
> Still few comments from quick review:
> =A0- patch seems slightly incomplete. At least in itimers_alloc() loop
> condition need to be changed.

Good catch. I'll need to look around for more potential issues with TIMER_M=
AX.

> =A0- To reduce chance of alike mistakes I would implement that patch in
> opposite way, by defining TIMER_MAX as (32 + TIMER_RESERVED), but
> reporting to user-level as TIMER_MAX - TIMER_RESERVED.

Why? A timer ID is a timer ID... Linux reports whacky values for timer
IDs, and while we report fairly sensical ones (0-TIMER_MAX), people
need to check the timer ID value, and don't assume that the return
value is correct. There is one case that I can think of that you're
guarding against (poorly written code, where timer ID =3D=3D 0, someone
whacks a kernel timer by accident as root, and things don't go so
well). That's one thing that I suppose the random timer ID values in
Linux protect against (they aren't really deterministic from what I
saw, so people either get things spot on by a stroke of luck, or their
app goes south with a segfault, etc).

Thanks,
-Garrett
Comment 6 Enji Cooper freebsd_committer 2010-09-08 08:35:48 UTC
2010/9/6 David Xu <davidxu@freebsd.org>:
> Alexander Motin wrote:
>>
>> Hi.
>>
>> Sorry, but this is not my area of knowledge yet. I am working on kernel
>> time event producing, while this is consuming, half way to user-level.
>>
>> Still few comments from quick review:
>> =A0- patch seems slightly incomplete. At least in itimers_alloc() loop
>> condition need to be changed.
>> =A0- To reduce chance of alike mistakes I would implement that patch in
>> opposite way, by defining TIMER_MAX as (32 + TIMER_RESERVED), but
>> reporting to user-level as TIMER_MAX - TIMER_RESERVED.
>> =A0- I have looked on setitimer() implementation referenced here and fou=
nd
>> no any interaction with timer_create() API and TIMER_MAX. There is
>> indeed many loose ends which make me thing there was or intended to be
>> something, but I haven't found it. Have I missed something?
>> =A0- this code was written by David Xu. I would asked him.
>>
>
> Yes, the code was written by me long time ago, my original intention
> was to not only implement timer_create() but also merge existing
> setitimer() code into unique module, but I had never done it
> because lack of time, so three slots were reserved for setitimer.
> I can see that there is inconsistent between TIMER_MAX and maximum
> timers user can create, your both methods will work.

    Understood. Are there any other spots in the code that we need to
be aware of in this particular area which are incomplete (I've seen
that you've done a fair amount of work in libc/compat43 and libthr in
the signal handling sections as well)?
Thanks,
-Garrett
Comment 7 Alexander Motin freebsd_committer 2010-09-08 08:44:06 UTC
Garrett Cooper wrote:
> 2010/9/6 Alexander Motin <mav@freebsd.org>:
>> Sorry, but this is not my area of knowledge yet. I am working on kernel
>> time event producing, while this is consuming, half way to user-level.
>>
>> Still few comments from quick review:
>>  - patch seems slightly incomplete. At least in itimers_alloc() loop
>> condition need to be changed.
> 
> Good catch. I'll need to look around for more potential issues with TIMER_MAX.
> 
>>  - To reduce chance of alike mistakes I would implement that patch in
>> opposite way, by defining TIMER_MAX as (32 + TIMER_RESERVED), but
>> reporting to user-level as TIMER_MAX - TIMER_RESERVED.
> 
> Why? A timer ID is a timer ID... Linux reports whacky values for timer
> IDs, and while we report fairly sensical ones (0-TIMER_MAX), people
> need to check the timer ID value, and don't assume that the return
> value is correct. There is one case that I can think of that you're
> guarding against (poorly written code, where timer ID == 0, someone
> whacks a kernel timer by accident as root, and things don't go so
> well). That's one thing that I suppose the random timer ID values in
> Linux protect against (they aren't really deterministic from what I
> saw, so people either get things spot on by a stroke of luck, or their
> app goes south with a segfault, etc).

While I indeed worried about some possible ID constraints in user-level,
I was mostly speaking about kernel code clearness. My point was
depending on assumption that from kernel side all usual and reserved
timers handled same way and so it would be nice to have dedicated
constant defining their count instead of use addition every time, having
a constant chance of mistake. This point is surely not mandatory and may
be ungrounded, but I would at least added one more constant describing
full number of timers, including reserved. Also, as soon as reserved
timers are never really used, we could probably choose which one to
reserve - first or last 3, if it could make code more clear.

-- 
Alexander Motin
Comment 8 David Xu freebsd_committer 2010-09-08 17:49:56 UTC
Garrett Cooper wrote:
> 2010/9/6 David Xu <davidxu@freebsd.org>:
>> Alexander Motin wrote:
>>> Hi.
>>>
>>> Sorry, but this is not my area of knowledge yet. I am working on kernel
>>> time event producing, while this is consuming, half way to user-level.
>>>
>>> Still few comments from quick review:
>>>  - patch seems slightly incomplete. At least in itimers_alloc() loop
>>> condition need to be changed.
>>>  - To reduce chance of alike mistakes I would implement that patch in
>>> opposite way, by defining TIMER_MAX as (32 + TIMER_RESERVED), but
>>> reporting to user-level as TIMER_MAX - TIMER_RESERVED.
>>>  - I have looked on setitimer() implementation referenced here and found
>>> no any interaction with timer_create() API and TIMER_MAX. There is
>>> indeed many loose ends which make me thing there was or intended to be
>>> something, but I haven't found it. Have I missed something?
>>>  - this code was written by David Xu. I would asked him.
>>>
>> Yes, the code was written by me long time ago, my original intention
>> was to not only implement timer_create() but also merge existing
>> setitimer() code into unique module, but I had never done it
>> because lack of time, so three slots were reserved for setitimer.
>> I can see that there is inconsistent between TIMER_MAX and maximum
>> timers user can create, your both methods will work.
> 
>     Understood. Are there any other spots in the code that we need to
> be aware of in this particular area which are incomplete (I've seen
> that you've done a fair amount of work in libc/compat43 and libthr in
> the signal handling sections as well)?
> Thanks,
> -Garrett
> 

Note that there is a userland library called librt which wraps around
the kernel system calls, the reason is specification requires
SEGEV_THREAD notification type, and struct sigevent
contains pthread_attr_t pointer which is used for creating
service thread, the pthread is unknown to kernel, so the library
is created, translating SIGEV_THREAD_ID into SIGEV_THREAD notification.
the timer id is not directly accessed by user code, instead user code
uses timer_t which is a pointer of structure __timer, however the timer
id is saved in the structure.

Regards,
David Xu
Comment 9 Eitan Adler freebsd_committer freebsd_triage 2017-12-31 08:00:34 UTC
For bugs matching the following criteria:

Status: In Progress Changed: (is less than) 2014-06-01

Reset to default assignee and clear in-progress tags.

Mail being skipped