Set SCHED_BATCH priority on the loadblk thread. #12618

pull eklitzke wants to merge 1 commits into bitcoin:master from eklitzke:sched_batch changing 3 files +25 −0
  1. eklitzke commented at 6:46 pm on March 6, 2018: contributor

    Today I came across #10271, and while reading the discussion #6358 was linked to. Linux systems have a SCHED_BATCH scheduler priority that is useful for threads like loadblk. You can find the full details at sched(7), but I’ll quote the relevant part of the man page below:

    …this policy will cause the scheduler to always assume that the thread is CPU-intensive. Consequently, the scheduler will apply a small scheduling penalty with respect to wakeup behavior, so that this thread is mildly disfavored in scheduling decisions.

    This policy is useful for workloads that are noninteractive, but do not want to lower their nice value, and for workloads that want a deterministic scheduling policy without interactivity causing extra preemptions (between the workload’s tasks).

    I think this change is useful independently of #10271 and irrespective of whether that change is merged. Under normal operation the loadblk thread will just import mempool.dat. However, if Bitcoin is started with -reindex or -reindex-chainstate this thread will use a great deal of CPU while it rebuilds the chainstate database (and the block database in the case of -reindex). By setting SCHED_BATCH this thread is less likely to interfere with interactive tasks (e.g. the user’s web browser, text editor, etc.).

    I’m leaving the nice value unchanged (which also affects scheduling decisions) because I think that’s better set by the user. Likewise I’m not using ioprio_set(2) because it can cause the thread to become completely I/O starved (and knowledgeable users can use ionice(1) anyway).

  2. fanquake added the label Resource usage on Mar 6, 2018
  3. in src/util.cpp:959 in cb89fcaa83 outdated 
    954@@ -955,3 +955,10 @@ fs::path AbsPathForConfigVal(const fs::path& path, bool net_specific)
955 {
956     return fs::absolute(path, GetDataDir(net_specific));
957 }
958+
959+void ScheduleBatchPriority(pid_t tid) {
    laanwj commented at 7:03 pm on March 6, 2018:
    pid_t is not available on windows, I think it would be better to remove the parameter as it’s always passed as 0.
    eklitzke commented at 7:09 pm on March 6, 2018:
    I think you’re right.
  4. laanwj commented at 7:03 pm on March 6, 2018: member
    Is loadblk thread the only thread this is relevant for? what about the script verification threads?
  5. promag commented at 7:48 pm on March 6, 2018: member

    Is this the windows equivalent?

    0SetThreadPriority(GetCurrentThread(), THREAD_MODE_BACKGROUND_BEGIN)
  6. eklitzke commented at 8:34 pm on March 6, 2018: contributor

    On Linux SCHED_BATCH just prevents batch tasks from pre-empting normal tasks (i.e. normal tasks will run until their timeslice expires, or another normal tasks pre-empts them). When picking the next task to run the scheduler considers SCHED_OTHER (i.e. normal) and SCHED_BATCH tasks equally. This is why setting SCHED_BATCH tasks are only “mildly disfavored”, because they’ll still be considered to run using the normal metrics whenever the scheduler ticks (usually 100Hz or 1000Hz).

    It sounds like Windows THREAD_MODE_BACKGROUND_BEGIN is much more extreme. It looks like maybe the Windows equivalent is THREAD_PRIORITY_BELOW_NORMAL but I do not know a lot about Windows.

    I don’t know as much about the script verification threads. If we are going to set SCHED_BATCH for loadblk then it makes sense to me to set the script verification threads to SCHED_BATCH as well, at least until the loadblk thread completes. You can freely switch back from SCHED_BATCH to SCHED_OTHER so it would be possible to only run them in the batch mode while reindexing is happening, or we could just always have them set at batch priority.

  7. JeremyRubin commented at 11:00 pm on March 6, 2018: contributor

    Concept ACK for this:

    one note: you would want something realtime for scriptcheck threads while they are validating a block incoming from network – we want these to take priority over other threads aggressively.

  8. eklitzke commented at 6:42 pm on March 9, 2018: contributor

    Re: Jeremy’s comment, there are a few ways to increase the priority of the script verification thread to ensure low-latency block relay. On Linux there exists SCHED_FIFO and SCHED_RR threads which give threads semi-realtime scheduling guarantees, and can ensure that such threads are always prioritized above other threads. Linux also supports per-thread nice values (this is a Linux extension, POSIX specifies that nice values are per-process).

    Using the other scheduling modes is better for realtime behavior, but somewhat dangerous because such threads can pre-empt all other non-realtime processes, meaning that this could be used as a DOS vector. Changing the nice value is a bit safer. I will probably look into this as part of another PR because I’d like to benchmark that change.

  9. MarcoFalke added this to the milestone 0.17.0 on Mar 18, 2018
  10. MarcoFalke commented at 11:14 pm on March 18, 2018: member
    I think the commits can be squashed?
  11. eklitzke force-pushed on Mar 19, 2018
  12. eklitzke commented at 6:58 am on March 19, 2018: contributor
    Thanks @MarcoFalke , squashed to one commit. I’m interested into looking at the RT change Jeremy suggested as well, but that’s more impactful so I’d rather do that separately unless people want to see that change done here.
  13. in src/init.cpp:628 in a2ed276b52 outdated 
    626@@ -627,6 +627,7 @@ void ThreadImport(std::vector<fs::path> vImportFiles)
627 {
628     const CChainParams& chainparams = Params();
629     RenameThread("bitcoin-loadblk");
630+    ScheduleBatchPriority();
    ryanofsky commented at 2:20 pm on March 22, 2018:
    Would be nice to log the error message if sched_batch returns -1. EINVAL / EPERM cases would be good to know about.
  14. in src/util.cpp:964 in a2ed276b52 outdated 
    956@@ -953,3 +957,12 @@ fs::path AbsPathForConfigVal(const fs::path& path, bool net_specific)
957 {
958     return fs::absolute(path, GetDataDir(net_specific));
959 }
960+
961+int ScheduleBatchPriority(void) {
962+#if HAVE_SCHED_H && defined(SCHED_BATCH)
963+    sched_param param{.sched_priority = 0};
964+    return sched_setscheduler(0, SCHED_BATCH, &param);
    ryanofsky commented at 2:26 pm on March 22, 2018:
    Would calling pthread_setschedparam instead of sched_setscheduler be a little more portable? It seems like you might not need the configure check using this.
  15. ryanofsky commented at 2:29 pm on March 22, 2018: member
    utACK a2ed276b52aec97d37257b7aad73f7f332a926d3
  16. eklitzke force-pushed on Mar 26, 2018
  17. eklitzke force-pushed on Mar 26, 2018
  18. eklitzke force-pushed on Mar 26, 2018
  19. eklitzke commented at 10:58 pm on March 26, 2018: contributor
    Updated to use pthread_setschedparam(). It seems that <sched.h> is standard on Posix systems but SCHED_BATCH is not, so I updated the #ifdef logic a little to reflect that. I also put the error logging in SchedBatchPriority() itself as this return 1 on Windows but errno won’t be set. Could also do it the other way (return 0 on non-Linux hosts and then move the log statement to the call site).
  20. Set SCHED_BATCH priority on the loadblk thread. 
    While reading another PR I saw a mention of #6358. The use case for
SCHED_BATCH is to hint to the kernel that the thread is running a
non-interactive workload that consumes a lot of CPU time. This is
helpful on desktop machines where the loadblk thread can interfere with
interactive applications. More details can be found in the sched(7) man
page.
    d54874d795
  21. eklitzke force-pushed on Mar 26, 2018
  23. laanwj merged this on Apr 7, 2018
  24. laanwj closed this on Apr 7, 2018
  25. laanwj referenced this in commit becd8dd2ec on Apr 7, 2018
  26. Fabcien referenced this in commit 5127de5857 on Aug 30, 2019
  27. jonspock referenced this in commit 3686149ae1 on Dec 8, 2019
  28. jonspock referenced this in commit b8504cca02 on Dec 8, 2019
  29. jonspock referenced this in commit dae092aa76 on Dec 8, 2019
  30. proteanx referenced this in commit c68b742e42 on Dec 12, 2019
  31. PastaPastaPasta referenced this in commit 1f46de1973 on Dec 16, 2020
  32. PastaPastaPasta referenced this in commit 02edd7066e on Dec 18, 2020
  33. DrahtBot locked this on Sep 8, 2021


eklitzke laanwj promag JeremyRubin MarcoFalke ryanofsky

Labels
Resource usage

Milestone
0.17.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-23 06:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me