On 9/24/21 4:08 AM, Damien Le Moal wrote:
On 2021/09/24 8:27, Bart Van Assche wrote:
+/*
+ * Time after which to dispatch lower priority requests even if higher
+ * priority requests are pending.
+ */
+static const int aging_expire = 10 * HZ;
What about calling this prio_starved, to be consistent with writes_starved ?
Or at least let's call it prio_aging_expire to show that it is for priority
levels, and not for requests within a priority queue.
The name 'prio_starved' probably would cause confusion since this parameter
represents a timeout while 'writes_starved' represents a count. I will rename
it into prio_aging_expire.
dispatch_request:
+ if (started_after(dd, rq, latest_start))
+ return NULL;
Nit: add a blank line here ? (for aesthetic :))
Will do.
+/*
+ * Check whether there are any requests with a deadline that expired more than
+ * aging_expire jiffies ago.
Hmm... requests do not have a "deadline", so this is a little hard to
understand. What about something like this:
* Check whether there are any requests at a low priority level inserted
* more than aging_expire jiffies ago.
Agreed, I will clarify this comment.
+ */
+static struct request *dd_dispatch_aged_requests(struct deadline_data *dd,
+ unsigned long now)
Same remark as for the aging_expire variable: it may be good to have prio in the
name of this function. Something like:
dd_dispatch_prio_starved_requests() ?
The name 'aging' has a well-defined meaning so I would like to keep it. See also
https://en.wikipedia.org/wiki/Aging_(scheduling).
spin_lock(&dd->lock);
Nit: Add a blank line here to have the spin_lock() stand out ?
Easier to notice it that way...
The convention in kernel code is a blank line above statements that grab a lock
but not below these statements.
Thanks,
Bart.
