Opened 18 years ago

Closed 11 years ago

#1069 closed enhancement (fixed)

Create thread scheduler with CPU affinity

Reported by: axeld Owned by: pdziepak
Priority: normal Milestone: R1
Component: System/Kernel Version: R1/pre-alpha1
Keywords: Cc: kaoutsis, fredrik.holmqvist@…, scorbett@…
Blocked By: Blocking:
Platform: All

Description

Create an O(1) thread scheduler with CPU affinity and soft real-time support which targets desktop responsiveness.

Attachments (3)

scheduler.diff (15.5 KB ) - added by Duggan 13 years ago.
This is an intermediate patch to some work I'm doing on the extant affine scheduler. Theoretically hyperthreaded cores should share ready queues now and there's a little support for soft affinities for teams. I can't test the hyperthreading code so if you guys can test this on various Intel and AMD processors and provide some feedback (mostly syslogs, bt's if it crashes) I would appreciate it. Future work includes proper load balancing and affinities, API functions to support affinities, possibly a thread class (a la BeAPI) that provides an OO wrapper to the API functions. Other changes I would like to make as well include shutting down unnecessary cores when the load is light and reenabling them as the load increases as well as potentially doing away with the global scheduler lock in favour of per-queue locks. Again, this is an intermediate patch so there's likely alot of problems with it besides bugs (but I did run it through the style checker and did the diff from trunk this time ;) ... not saying there's not still style errors, just not in my code). Cheers!
syslog.txt (135.5 KB ) - added by luroh 13 years ago.
scheduler1.diff (16.7 KB ) - added by Duggan 13 years ago.
The code that gathers the topology information has been rewritten and moved around a few times, and this is what I've arrived at so far. Theoretically Intel processors with HT/SMT will have it enabled now, but I need someone to test to make sure.

Download all attachments as: .zip

Change History (17)

comment:1 by kaoutsis, 18 years ago

Cc: kaoutsis added

comment:2 by axeld, 17 years ago

Owner: changed from axeld to meianoite

comment:3 by tqh, 16 years ago

Cc: fredrik.holmqvist@… added

Any news?

comment:4 by tegaru, 15 years ago

Cc: scorbett@… added

by Duggan, 13 years ago

Attachment: scheduler.diff added

This is an intermediate patch to some work I'm doing on the extant affine scheduler. Theoretically hyperthreaded cores should share ready queues now and there's a little support for soft affinities for teams. I can't test the hyperthreading code so if you guys can test this on various Intel and AMD processors and provide some feedback (mostly syslogs, bt's if it crashes) I would appreciate it. Future work includes proper load balancing and affinities, API functions to support affinities, possibly a thread class (a la BeAPI) that provides an OO wrapper to the API functions. Other changes I would like to make as well include shutting down unnecessary cores when the load is light and reenabling them as the load increases as well as potentially doing away with the global scheduler lock in favour of per-queue locks. Again, this is an intermediate patch so there's likely alot of problems with it besides bugs (but I did run it through the style checker and did the diff from trunk this time ;) ... not saying there's not still style errors, just not in my code). Cheers!

comment:5 by Duggan, 13 years ago

patch: 01

comment:6 by Duggan, 13 years ago

I also need to go back and check variable naming conventions. Any ideas / input / feedback are welcome.

comment:7 by luroh, 13 years ago

Booting an Intel T7400, hrev42889, gcc2.

by luroh, 13 years ago

Attachment: syslog.txt added

comment:8 by Duggan, 13 years ago

Things look a bit screwy, I'll have to get to the bottom of that... I'm glad it works though

comment:9 by bonefish, 13 years ago

Great to see someone taking interest in this stuff!

I haven't really read through the code. Just a few remarks:

  • You introduce Thread::affinity_hard, but never use it. And yes pinned_to_cpu, if > 0 means the thread will only be scheduled on the CPU it ran previously. There are symmetric functions to pin/unpin a thread. The member being an int32 allows them to be nested.
  • get_current_cpuid_ex(): The usual function naming on Haiku in this case would be get_current_cpuid_etc(). If you specify a default value for the new parameter anyway, there's no reason not just to extend get_current_cpuid() instead, though.
  • round_to_pwr_of_2(): Name should be "..._power_...". The function should be static. It's a bit ugly that it only works for number <= 128. It isn't performance critical in this case, so there's no reason not to implement it generally.
  • RunQueue() and RunQueueSize() should follow the macro naming scheme (all upper case, words separated by underscore).
  • display_topology(): Should be static. A better name would be dump_topology().
  • It's generally preferable not to mix coding style changes with functional changes.

Regarding your future work suggestions:

  • You can't do away with the scheduler lock. It is needed outside the scheduler to implement locking primitives.
  • The most important change in the kernel regarding scheduling is to get rid of B_MAX_CPU_COUNT. It's currently 8 which is already insufficient for some of todays desktop machines. Given the general direction the CPU manufactures are headed just raising the number (which is a binary compatibility concern, BTW) wouldn't help for long either. The CPU array needs to be allocated dynamically. Getting rid of the static B_MAX_CPU_COUNT in kernel and drivers is probably quite a bit of work.
  • I wonder, if a tree/forest representation of the CPU structure would be better than the extra fields you added to cpu_ent.

comment:10 by Duggan, 13 years ago

Thanks alot for the input! In response:

  • Some changes (like Thread::affinity_hard) are preparatory steps for future work. I'll look into those functions you mention as I haven't run across them myself yet and get rid of the flag once I understand it.
  • I considered the issues with modifying get_current_cpuid() but didn't want to screw too much up at first, I'll replace it in the next patch.
  • As far as round_to_pwr_of_2(), it's inelegant but it's really only a hack... I don't think it's needed (at this point) for anything more than 8 bits.
  • I'll change the macro names... I wasn't sure if it was a big deal or not. I was more interested in fewer changes at the time heh.
  • I'll change the display_topology() function next patch too.
  • I guess I'll go back and undo some of the coding style stuff keeping it purely functional (fixing things in my code of course).

As far as the future work:

  • I'll have to look into that more... It's nothing I've committed to at this point but I see more benefit in locking only the queues that are being modified at the time versus all queues causing other processors to block even if they're not working with the same queues. Would it be possible to simply not lock it when the scheduler is entered then? I haven't looked extensively at the overall scheduling system but focused primarily on the scheduler itself.
  • I bet that would be alot of work, but there might be a way to work around it that isn't too involved so it can be removed post-R1, keeping only the dynamic array. I'm not that concerned with it right now, maybe later. Since the constant is only used for declaring array sizes and one loop (which isn't even necessary) I don't think it would make that big of an impact at least in the scheduler since no where else (in the scheduler) are we iterating through unused items. I could always just replace those arrays with dynamic ones and be done with it.
  • I considered that at first but thought it would end up being easier just to do it this way at first. I'm considering replacing the queues with trees, but if I do, that would likely be the last thing I do.

Again this was just an intermediate patch and any more feedback like this on this and future patches would be greatly appreciated.

Thank you very much!

by Duggan, 13 years ago

Attachment: scheduler1.diff added

The code that gathers the topology information has been rewritten and moved around a few times, and this is what I've arrived at so far. Theoretically Intel processors with HT/SMT will have it enabled now, but I need someone to test to make sure.

comment:11 by Duggan, 13 years ago

This is an INTEL ONLY patch! If you run this patch on an AMD processor your machine WILL PANIC!!! Please don't attach any crash reports for AMD processors**

Anyone is welcome to test, but please only attach topology information if you know your processor has HT/SMT capabilities.

Crash reports are welcome for any Intel processor, but please no crash reports for AMD processors. This patch will panic on all AMD processors, so I already know it's broken, thanks! :)

When testing: If you make it to the desktop, please enter KDL by pressing alt + sys rq + D and then type "topology". Please attach the information that comes up here.

Thanks!

comment:12 by Duggan, 13 years ago

The two rather large and redundant functions are constructed that way for speed. They're also not architecture specific and should be placed elsewhere. I'll move them in the next patch. Thanks John Scipione for pointing it out!

comment:13 by pdziepak, 11 years ago

Owner: changed from meianoite to pdziepak
Status: newassigned

comment:14 by pdziepak, 11 years ago

Resolution: fixed
Status: assignedclosed

New scheduler has been merged in hrev46690.

Note: See TracTickets for help on using tickets.