Align thread priority with Linux defaults #3622
Conversation
|
This is probably because the priorities are inverted on Linux. i.e. higher numbers are higher priority on Solaris while lower numbers are higher priority on Linux. http://lxr.free-electrons.com/source/include/linux/sched/prio.h We probably should change maxclsyspri to 0 for it to match its equivalent on Illumos. |
|
@ryao You mean the other way around, right - "lower numbers are higher priority on Linux"? :) |
|
@ryao that's not a bad idea although it's probably not quite enough. There are places in the code where the priority gets incremented or decremented and those would need to be inverted as well. For the moment, I've just added a I'm sorely tempted to just move everything to the default until we have data for specific taskq's which suggest better values for Linux. There's no great reason to believe the illumos values are good choices for Linux considering how different the schedulers are. |
|
I think the default for filesystem "stuff" is somewhere between |
|
FYI, as a reference: the following is with 4.1 kernel: |
|
Requires openzfs/spl#466 @kernelOfTruth thanks for pointing out what other common Linux filesystems use for process priority. I've refreshed this patch to use those values as a base line. While changing the threads to the default priority was good enough keep user space processes from negatively impacting performance. It occurs to me that it won't be good enough to prevent another active native Linux filesystem from potentially starving ZFS performance. We want them both to be running with the same priority if possible. I've also integrated @ryao's idea of inverting the min/max values. I'd be very interested to see how these patches impact performance on busy systems with real workloads. |
behlendorf
changed the title
|
@behlendorf glad to be of help =) openzfs/spl#466 |
|
Whoops! I corrected the comment to point to the correct issue. |
|
LGTM. The choices seem reasonable and @FransUrbo makes good points about future-proofing this a bit. |
Under Linux filesystem threads responsible for handling I/O are
normally created with the maximum priority. Non-I/O filesystem
processes run with the default priority. ZFS should adopt the
same priority scheme under Linux to maintain good performance
and so that it will complete fairly when other Linux filesystems
are active. The priorities have been updated to the following:
$ ps -eLo rtprio,cls,pid,pri,nice,cmd | egrep 'z_|spl_|zvol|arc|dbu|meta'
- TS 10743 19 -20 [spl_kmem_cache]
- TS 10744 19 -20 [spl_system_task]
- TS 10745 19 -20 [spl_dynamic_tas]
- TS 10764 19 0 [dbu_evict]
- TS 10765 19 0 [arc_prune]
- TS 10766 19 0 [arc_reclaim]
- TS 10767 19 0 [arc_user_evicts]
- TS 10768 19 0 [l2arc_feed]
- TS 10769 39 0 [z_unmount]
- TS 10770 39 -20 [zvol]
- TS 11011 39 -20 [z_null_iss]
- TS 11012 39 -20 [z_null_int]
- TS 11013 39 -20 [z_rd_iss]
- TS 11014 39 -20 [z_rd_int_0]
- TS 11022 38 -19 [z_wr_iss]
- TS 11023 39 -20 [z_wr_iss_h]
- TS 11024 39 -20 [z_wr_int_0]
- TS 11032 39 -20 [z_wr_int_h]
- TS 11033 39 -20 [z_fr_iss_0]
- TS 11041 39 -20 [z_fr_int]
- TS 11042 39 -20 [z_cl_iss]
- TS 11043 39 -20 [z_cl_int]
- TS 11044 39 -20 [z_ioctl_iss]
- TS 11045 39 -20 [z_ioctl_int]
- TS 11046 39 -20 [metaslab_group_]
- TS 11050 19 0 [z_iput]
- TS 11121 38 -19 [z_wr_iss]
Note that under Linux the meaning of a processes priority is inverted
with respect to illumos. High values on Linux indicate a _low_ priority
while high value on illumos indicate a _high_ priority.
In order to preserve the logical meaning of the minclsyspri and
maxclsyspri macros when they are used by the illumos wrapper functions
their values have been inverted. This way when changes are merged
from upstream illumos we won't need to remember to invert the macro.
It could also lead to confusion.
This patch depends on openzfs/spl#466.
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ned Bass <bass6@llnl.gov>
Issue openzfs#3607
|
Refreshed with the follow changes.
@FransUrbo regarding the specific priority of the tasks they'll all listed in the commit message. But the general rule I went by was any task that might perform I/O should have the maximum priority. Other helper threads which are less critical should run with the default. It's important that they run but it's less critical. Incidentally this is far better than the current situation where everything is running with a very low priority. |
|
I still think that Since they're (in one way or the other) associated with a filesystem, I would have expected them to be (slightly) more important… Document why they're not? Also, the additional comment kind'a missed my point (slightly) I was trying to make… |
|
@FransUrbo the following threads are all dbu_evict - Asynchronously evicting user buffers assoicated with a dbuf. |
Something like: I know that this is somewhat out of scope for the PR, but still… While the code is modified, why not just do it the whole way?
For this, something like: That way, whoever wants to try to merge the Illumos and Linux code (etc) - like I was about to start a couple of months ago, can easily find what is very Linux specific and shouldn't be touched and merged with Illumos. I couldn't even begin, because I didn't know what's Linux unique, what haven't been ported yet and what's "somewhere in between" (?). I bet no-one, not even you guys that fiddle with the core code every day, can say exactly what's Linux unique without missing one (or several). This way, it's absolutly clear to anyone and everyone that this is Linux-unique and not just something we haven't ported yet. As it is now, only by knowing every line of code, going through it AND knowing the history of the code could one find "this stuff". With a comment like that (if you think a "#ifdef LINUX" is to much), it's easy to find and anyone looking through the code will recognize it, even in years to come when Illumos "fiddles" with this... |
|
I think if we're going to start commenting what every taskq is for we should do it in a separate patch and include more than a one line comment. Let's leave that for another time. Keeping it in a separate commit would also make it easier to push upstream which is when we'd ideally like it to be. As for the additional Linux comment we'll have to disagree about the style. Having something you can grep for I don't think is the most important part. Having an explanation for why something is different is what you really want to know. Ideally we want to hide any differences behind a wrapper function to keep the code as readable as possible. I think the updated comment accomplished that. |
|
You've always been over thinking things :). "A journey of a thousand miles starts with a single step". Yes, finding all of them and add comments in one (or several commits) and then push it upstream IS preferable. But starting with this one, here and now is a very good start. NO ONE is going to have neither the time, interest or will to do the "full monty", and you know it… Hence, "it's never going to happen". Think small for once. |
|
Merged as: 1229323 Align thread priority with Linux defaults |
Creating threads with maxclsyspri under Linux results in threads
with lower than the default kernel thread priority. This means
that on an active system they can be starved for CPU cycles which
negatively impacts performance. To prevent this from happening
defclsyspri should be used so they are created with the default
system priority.
Signed-off-by: Brian Behlendorf behlendorf1@llnl.gov
Issue #3607