346376 - [OS/2] Don't use PRTYC_IDLETIME class for thread priorities

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Peter Weilbacher]

I often hear that people have problems shutting down Mozilla Browsers on OS/2 when they are running SETI@Home or similarly CPU intensive apps at the same time.

I think this is because the browsers use PR_PRIORITY_LOW to delete the cache dir after a crash and on OS/2 this is reflected by using the priority class PRTYC_IDLETIME in _PR_MD_SET_PRIORITY(). According to the (OS/2 Toolkit) documentation and some comments in the OS/2 newsgroups IDLETIME threads may not get _any_ CPU cycles, so using this in any NSPR-based app is probably not a good idea.

Comment 1

[Peter Weilbacher]

Attachment: Make the change

Instead of IDLETIME this patch now uses REGULAR with a low delta (I hope that negative numbers mean lower priority!), and also fixes a compile warning by initializing nativePri (and changes the only tab in the file to 8 chars).

Comment 2

[Mike Kaply [:mkaply]]

Scott, what do you think of this one?

Comment 3

[Scott Garfinkle]

It sounds to me like both the analysis and proposed fix are on target. -scott

Comment 4

[Mike Kaply [:mkaply]]

Comment on attachment 231187 [details] [diff] [review]
Make the change

sr=mkaply

Comment 5

[Mike Kaply [:mkaply]]

Peter - you can't check this in - I'll take care of it.

Comment 6

[Mike Kaply [:mkaply]]

Should I try to get this on the branches? I checked it into the NSPR client branch and the NSPR trunk.

Comment 7

[Wan-Teh Chang]

Mike,

If you want this fix in Firefox 2.0 (Gecko 1.8.1), you also need to
check it into the MOZILLA_1_8_BRANCH and the NSPR_4_6_BRANCH (and
change the target milestone to 4.6.3).  Thanks.

Comment 8

[Peter Weilbacher]

Comment on attachment 231187 [details] [diff] [review]
Make the change

mkaply, yes I think it should go onto the branches, too.

Asking for approval for this small OS/2 only improvement.

Comment 9

[knut st. osmundsen]

Just my two euro cents on your hopes about how the DosSetPriority() API work. 
The priority level within a priority class goes from 0 to 31 (inclusivly). Unless you specify PRTYC_NOCHANGE negative deltas are all rounded up to 0. So, rv = DosSetPriority(PRTYS_THREAD, PRTYC_REGULAR, -25, thread->handle) becomes DosSetPriority(PRTYS_THREAD, PRTYC_REGULAR, 0, thread->handle), which means that PR_PRIORITY_NORMAL and PR_PRIORITY_LOW will end up with the same result.

Comment 10

[Peter Weilbacher]

Is that documented anywhere or did you just find out through testing it?

Should the solution then be to call DosSetPriority() a second time if delta<0, but then with PRTYC_NOCHANGE?

Comment 11

[Peter Weilbacher]

Comment on attachment 231187 [details] [diff] [review]
Make the change

Killing approvals for now. Perhaps we should wait for  some reports of SETI@Home users before we go further.

Comment 12

[knut st. osmundsen]

Perhaps I wasn't clear enough, so let me try explain again. OS/2 has several priority classes, like PRTYC_IDLETIME and PRTYC_REGULAR. Each class is divided into priority levels ranging from 0 thru 31, with 31 being the highest. The DosSetPriority API uses the negative delta together with PRTYC_NOCHANGE to adjust the current priority. That is, the thread priority is 0x081f (time critical, level 31) you can use a delta of -16 together with PRTYC_NOCHANGE to adjust the priority down to 0x080f (still time critical, but level 15). If you then try modify it again by -16, you'll not end up at -1 but 0. When the priority class isn't PRTYC_NOCHANGE, the delta is no longer a 'delta' but actually the new priority level.

If you want to check this, you can use DosGetInfoBlocks and watch the pTib->tib_ptib2->tib2_ulpri field.

Btw, if you think it isn't ok to run what NSPR terms low-priority at regular priority, you might experiment with PRTYC_IDLETIME and high deltas, like 31, but I'm not sure that'll solve the starvation issues you seems to be having - I haven't experimented that much with the OS/2 scheduling.

The priority and scheduling of OS/2 is documented in several places. For instance in the cpref: http://www.warpspeed.com.au/cgi-bin/inf2html.cmd?..\html\book\Toolkt40\CP3.INF+90
You'll also find it described in section "5.4 Scheduling" starting at page 115 in "The design of OS/2" by M.S.Kogan. There should also be some explanation about priorities in the kernel debugging handbooks I think (that sg<something>.inf you find with later toolkits). If you're really into it, you can use the kernel debugger (or a disassembler like IDA pro) and step the DosSetPrty API call.

Comment 13

[Peter Weilbacher]

OK, you are right that I was mistaken. I thought that "delta" directly referred to the priority levels within each class (the docs on DosSetPriority() seem to suggest this), but instead it is the value that is added to the priority level of the given priority class (added to 0 if changing the priority class at the same time, even when not specifying PRTYC_NOCHANGE).

But PRTYC_IDLETIME is definitely a problem, even with high priority levels within the class. There is just no good representation of what under Linux would be a nice level of e.g. 5. So the best we can do for PR_PRIORITY_LOW is set PRTYC_REGULAR and with a low delta ensure that the process really gets as little CPU time as possible within that class. Perhaps we should use PRTYD_MINIMUM (which is -31) instead of -25 to ensure that even if the thread was changed to have a higher priority level somewhere outside NSPR that it will definitely get level=0 after this call.

Comment 14

[knut st. osmundsen]

I didn't think I was that hard to understand... Using negative deltas with any priority class but PRTYC_NOCHANGE is fooling yourself. 

If you can't use PRTYC_IDLETIME for PR_PRIORITY_LOW, then you can simply join it with the PR_PRIORITY_NORMAL case, which means DosSetPriority(PRTYS_THREAD, nativePri=PRTYC_REGULAR, 0, thread->handle) [this is the default priority of an OS/2 thread].

Comment 15

[Wan-Teh Chang]

It is fine for PR_PRIORITY_LOW and PR_PRIORITY_NORMAL to map
to the same native thread priority.

Comment 16

[Peter Weilbacher]

(In reply to comment #14)
> I didn't think I was that hard to understand... Using negative deltas with any
> priority class but PRTYC_NOCHANGE is fooling yourself. 

Knut, I _did_ understand. Perhaps you should again read comment 13.

(In reply to comment #15)
> It is fine for PR_PRIORITY_LOW and PR_PRIORITY_NORMAL to map
> to the same native thread priority.

That would be OK for me, too. But before making another patch I would like to wait for some reports if the current change actually accomplished something for the original problem.

Comment 17

[knut st. osmundsen]

Peter, in comment #13, you are suggesting using using PRTYD_MINIMUM instead of -25. That indicates that my explanation in comment #11 wasn't good enough.

Comment 18

[Peter Weilbacher]

Well, after comment #12 (I guess that's the one you meant) I tested this some time last week and it seemed that if I specify class=PRTYC_REGULAR when the thread was already PRTYC_REGULAR before does not necessarily reset the priority level to 0. I am perfectly willing to accept that my testing was flawed in some way, especially since I have lost or deleted the test program in the meantime (and my current test program just crashes).

Comment 19

[Peter Weilbacher]

Attachment: Map both low PR priorities to REGULAR

OK, I finally managed to get my test program running again, and I am now convinced that Knut is right. So this patch partly reverts the earlier patch and maps both PR_PRIORITY_LOW and PR_PRIORITY_NORMAL to OS/2's PRTYC_REGULAR.

Comment 20

[Wan-Teh Chang]

Comment on attachment 238310 [details] [diff] [review]
Map both low PR priorities to REGULAR

r=wtc.

Comment 21

[knut st. osmundsen]

Ah, you got the point. ;-)

Comment 22

[Mike Kaply [:mkaply]]

Comment on attachment 238310 [details] [diff] [review]
Map both low PR priorities to REGULAR

r=mkaply

Comment 23

[Peter Weilbacher]

Comment on attachment 238310 [details] [diff] [review]
Map both low PR priorities to REGULAR

mkaply, wtc, could one of you please check this into NSPR for trunk? (I guess branch would need separate patch and approval?)

Comment 24

[Peter Weilbacher]

Second patch checked into NSPRPUB_PRE_4_2_CLIENT_BRANCH (Mozilla trunk) and NSPR trunk.

Comment 25

[Peter Weilbacher]

Hmm, I think this is another fix that I always shipped with my unofficial builds but forgot to ever get into the 1.8 branch (or the NSPR version that the 1.8 branch uses). As this is in an OS/2 only file I would normally just check it in without extra approval. Is there a different procedure for NSPR?

Comment 26

[Wan-Teh Chang]

Peter, you need to do the following.
1. Check in the patch on the NSPR_4_6_BRANCH.
2. Work with the NSPR team to release a new NSPR 4.6.x version.
3. Update mozilla/client.mk on the MOZILLA_1_8_BRANCH to check out NSPR with the new NSPR_4_6_x_RTM tag.

Comment 27

[Peter Weilbacher]

OK, item 1 was easy, I have checked it into the NSPR branch. (Both patches so that os2thred.c is now in sync with NSPR HEAD.)

Checking in nsprpub/pr/src/md/os2/os2thred.c;
/cvsroot/mozilla/nsprpub/pr/src/md/os2/os2thred.c,v  <--  os2thred.c
new revision: 3.17.2.1; previous revision: 3.17
done

I have no clue what item 2 entails, please let me know if I can help. As this small patch is entirely inside an OS/2 file and you don't release OS/2 binaries, I don't think there would be much to do. This bug alone does not validate a new NSPR release anyway...

Comment 28

[Wan-Teh Chang]

Peter: since this bug alone doesn't warrant a new NSPR release,
you can just wait for the next "natural" NSPR release.  We usually
make a new NSPR 4.6.x release when we make a new NSS 3.11.x release.