Bug 221956 - cam iosched: Schedule cam_iosched_ticker() quanta times per second
Summary: cam iosched: Schedule cam_iosched_ticker() quanta times per second
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Warner Losh
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2017-08-31 10:21 UTC by Fabian Keil
Modified: 2018-05-27 12:09 UTC (History)
5 users (show)

See Also:
koobs: mfc-stable11+


Attachments
cam iosched: Schedule cam_iosched_ticker() quanta times per second (2.24 KB, patch)
2017-08-31 10:21 UTC, Fabian Keil
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fabian Keil 2017-08-31 10:21:10 UTC
Created attachment 185947 [details]
cam iosched: Schedule cam_iosched_ticker() quanta times per second

The attached patch lets cam iosched schedule cam_iosched_ticker()
quanta times per second

According to callout_reset(9), "[w]hen ticks is used, the callout is
scheduled to execute after ticks/hz seconds".

It follows that to get quanta calls per second one should specify
a tick value of hz / isc->quanta.

Previously the value was additionally reduced by one.

As a result cam_iosched_ticker() was called more often than
requested by the user and expected by some of the limiters.

On a system with hz=1000 a quanta value of 200 resulted in 250
calls and a value of 100 in 111 calls.

Without the unexplained tick reduction the number of calls matches
the quanta value reasonably well.

Obtained from: ElectroBSD
Comment 1 Warner Losh freebsd_committer freebsd_triage 2017-09-01 01:01:36 UTC
It used to be you needed to reduce the number of ticks by one to get the desired behavior from callout. The exact value of quanta doesn't matter too much since the result of this bug is systematic (and we take it into account based on elapsed time). However, this makes the behavior more predictable.
Comment 2 Fabian Keil 2017-09-01 10:42:02 UTC
It matters for the iops limiter.

I previously thought it would matter for the queue_depth limiter as
well but as it doesn't have a "tick function" it probably doesn't.
Comment 3 commit-hook freebsd_committer freebsd_triage 2017-09-20 21:26:53 UTC
A commit references this bug:

Author: imp
Date: Wed Sep 20 21:25:56 UTC 2017
New revision: 323831
URL: https://svnweb.freebsd.org/changeset/base/323831

Log:
  cam iosched: Schedule cam_iosched_ticker() quanta times per second

  Previously callout_reset() was called with a "ticks" value that was
  off by one.  As a result cam_iosched_ticker() was called a bit too
  frequently: On systems with hz=1000 a quanta value of 200 resulted in
  ~250 calls and a value of 100 in ~111 calls.

  For the "queue_depth" and "bandwidth" limiters the difference doesn't
  matter but the "iops" limiter depends on the scheduling to enforce the
  correct maximum.

  PR: 221956
  Obtained from: ElectroBSD
  Submitted by: Fabian Keil
  Differential Revision: https://reviews.freebsd.org/D12350

Changes:
  head/sys/cam/cam_iosched.c
Comment 4 commit-hook freebsd_committer freebsd_triage 2018-05-25 23:18:32 UTC
A commit references this bug:

Author: sbruno
Date: Fri May 25 23:18:06 UTC 2018
New revision: 334229
URL: https://svnweb.freebsd.org/changeset/base/334229

Log:
  MFC r323829
    cam iosched: Add a handler for the quanta sysctl to enforce valid
                 values

  MFC r323831
      cam iosched: Schedule cam_iosched_ticker() quanta times per second

  PR:		221956 221957
  Submitted by:	imp
  Approved by:	re (marius)

Changes:
_U  stable/11/
  stable/11/sys/cam/cam_iosched.c
Comment 5 Kubilay Kocak freebsd_committer freebsd_triage 2018-05-27 12:06:46 UTC
Re-open
Comment 6 Kubilay Kocak freebsd_committer freebsd_triage 2018-05-27 12:09:16 UTC
Apologies, was going to reopen due to https://lists.freebsd.org/pipermail/freebsd-stable/2018-May/089005.html Let the mailing list thread continue for now.

These (this and bug 221957) should be re-opened if necessary, particularly if re@ required for any subsequent commit/merge approvals