lkml.org 
[lkml]   [2008]   [Jul]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH] Priorities in Anticipatory I/O scheduler
Hi Naveen,

I have a few observations after a quick read through your patch.


ngupta@google.com wrote:
> Modifications to the Anticipatory I/O scheduler to add multiple priority
> levels. It makes use of anticipation and batching in current
> anticipatory scheduler to implement priorities.
>
> - Minimizes the latency of highest priority level.
> - Low priority requests wait for high priority requests.
> - Higher priority request break any anticipating low priority request.
> - If single priority level is used the scheduler behaves as an
> anticipatory scheduler. So no change for existing users.
>
> With this change, it is possible for a latency sensitive job to coexist
> with background job.
>
> Other possible use of this patch is in context of I/O subsystem controller.
> It can add another dimension to the parameters controlling a particular cgroup.
> While we can easily divide b/w among existing croups, setting a bound on
> latency is not a feasible solution. Hence in context of storage devices
> bandwidth and priority can be two parameters controlling I/O.
>
> In this patch I have added a new class IOPRIO_CLASS_LATENCY to differentiate
> notion of absolute priority over existing uses of various time-slice based
> priority classes in cfq. Though internally within anticipatory scheduler all
> of them map to best-effort levels. Hence, one can also use various best-effort
> priority levels.

I don't see the point of this new priority class; I think ``latency sensitive''
is a reasonable definition of real-time. Especially since you don't actually
use it.

> @@ -21,6 +21,14 @@ config IOSCHED_AS
> deadline I/O scheduler, it can also be slower in some cases
> especially some database loads.
>
> +config IOPRIO_AS_MAX
> + int "Number of valid i/o priority levels"
> + depends on IOSCHED_AS
> + default "4"
> + help
> + This option controls number of priority levels in anticipatory
> + I/O scheduler.

Does this need to be configurable? There are two ``natural'' choices for this
value; the number of iopriorities (10), or the number of priority classes (3).
Why is any intermediate value useful?

> /*
> * rb tree support functions
> */
> -#define RQ_RB_ROOT(ad, rq) (&(ad)->sort_list[rq_is_sync((rq))])
> +static inline struct rb_root *rq_rb_root(struct as_data *ad,
> + struct request *rq)
> +{
> + return (&(ad)->prio_q[rq_prio_level(rq)].sort_list[rq_is_sync(rq)]);
> +}

This change (and the related ones below) is a separate patch which could
also be applied to deadline.

> @@ -996,6 +1074,31 @@ static void as_move_to_dispatch(struct a
> ad->nr_dispatched++;
> }
>
> +static unsigned int select_priority_level(struct as_data *ad)
> +{
> + unsigned int i, best_ioprio = 0, ioprio, found_alt = 0;
> +
> + for (ioprio = 0; ioprio < IOPRIO_AS_MAX; ioprio++) {
> + if (!as_has_request_at_priority(ad, ioprio)) {
> + continue;
> + }

Unnecessary braces.

> @@ -1022,21 +1126,32 @@ static int as_dispatch_request(struct re
> ad->changed_batch = 0;
> ad->new_batch = 0;
>
> - while (ad->next_rq[REQ_SYNC]) {
> - as_move_to_dispatch(ad, ad->next_rq[REQ_SYNC]);
> - dispatched++;
> - }
> - ad->last_check_fifo[REQ_SYNC] = jiffies;
> -
> - while (ad->next_rq[REQ_ASYNC]) {
> - as_move_to_dispatch(ad, ad->next_rq[REQ_ASYNC]);
> - dispatched++;
> + for (ioprio = 0; ioprio < IOPRIO_AS_MAX; ioprio++) {
> + while (ad->prio_q[ioprio].next_rq[REQ_SYNC]) {
> + as_move_to_dispatch(ad,
> + ad->prio_q[ioprio].next_rq[REQ_SYNC]);
> + dispatched++;
> + }
> + ad->last_check_fifo[REQ_SYNC] = jiffies;
> +
> + while (ad->prio_q[ioprio].next_rq[REQ_ASYNC]) {
> + as_move_to_dispatch(ad,
> + ad->prio_q[ioprio].next_rq[REQ_ASYNC]);
> + dispatched++;
> + }
> + ad->last_check_fifo[REQ_ASYNC] = jiffies;
> }
> - ad->last_check_fifo[REQ_ASYNC] = jiffies;
>
> return dispatched;
> }
>
> + ioprio = select_priority_level(ad);
> + if (ioprio >= IOPRIO_AS_MAX)
> + return 0;

Why should this ever happen?

> @@ -1049,14 +1164,16 @@ static int as_dispatch_request(struct re
> || ad->changed_batch)
> return 0;
>
> + changed_ioprio = (ad->batch_ioprio != ioprio)?1:0;

Redundant conditional.

> @@ -1216,9 +1341,39 @@ static void as_deactivate_request(struct
> static int as_queue_empty(struct request_queue *q)
> {
> struct as_data *ad = q->elevator->elevator_data;
> + unsigned short ioprio;
>
> - return list_empty(&ad->fifo_list[REQ_ASYNC])
> - && list_empty(&ad->fifo_list[REQ_SYNC]);
> + for (ioprio = 0; ioprio < IOPRIO_AS_MAX; ioprio++) {
> + if (as_has_request_at_priority(ad, ioprio))
> + return 0;
> + }
> + return 1;
> +}
> +
> +static unsigned short as_mapped_priority(unsigned short ioprio)
> +{
> + unsigned short class = IOPRIO_PRIO_CLASS(ioprio);
> + unsigned short data = IOPRIO_PRIO_DATA(ioprio);
> +
> + if (class == IOPRIO_CLASS_BE)
> + return ((data < IOPRIO_AS_MAX)? ioprio:

Doesn't this mean that requests in the BE class with prio level 0 will map to
the same queues as RT requests?

> + IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE,
> + (IOPRIO_AS_MAX - 1)));
> + else if (class == IOPRIO_CLASS_LATENCY)
> + return ((data < IOPRIO_AS_MAX)?
> + IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, data):

Likewise.

> + IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE,
> + (IOPRIO_AS_MAX - 1)));
> + else if (class == IOPRIO_CLASS_RT)
> + return IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, 0);
> + else if (class == IOPRIO_CLASS_IDLE)
> + return IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, (IOPRIO_AS_MAX - 1));
> + else if (class == IOPRIO_CLASS_NONE) {
> + return IOPRIO_AS_DEFAULT;
> + } else {
> + WARN_ON(1);
> + return IOPRIO_AS_DEFAULT;
> + }

It looks like you're mapping all ioprios to a prio level in the BE class, and using
that internally. It would be simpler to use integers in [0, IOPRIO_AS_MAX) internally,
and convert back to ``real'' ioprios where necessary; you do a lot of conversions...
This would also be better expressed as a switch statement.

> @@ -1351,10 +1512,20 @@ static void *as_init_queue(struct reques
> init_timer(&ad->antic_timer);
> INIT_WORK(&ad->antic_work, as_work_handler);
>
> - INIT_LIST_HEAD(&ad->fifo_list[REQ_SYNC]);
> - INIT_LIST_HEAD(&ad->fifo_list[REQ_ASYNC]);
> - ad->sort_list[REQ_SYNC] = RB_ROOT;
> - ad->sort_list[REQ_ASYNC] = RB_ROOT;
> + for (i = IOPRIO_AS_MAX - 1; i >= 0; i--) {
> + INIT_LIST_HEAD(&ad->prio_q[i].fifo_list[REQ_SYNC]);
> + INIT_LIST_HEAD(&ad->prio_q[i].fifo_list[REQ_ASYNC]);
> + ad->prio_q[i].sort_list[REQ_SYNC] = RB_ROOT;
> + ad->prio_q[i].sort_list[REQ_ASYNC] = RB_ROOT;
> + ad->prio_q[i].serviced = 0;
> + if (i == 0)
> + ad->prio_q[i].ioprio_wt = 100;
> + else if (i == 1)
> + ad->prio_q[i].ioprio_wt = 5;
> + else
> + ad->prio_q[i].ioprio_wt = 1;

This seems a bit arbitrary, and means IOPRIO_AS_MAX > 3 is useless unless the weights are
changed manually.


Thanks,
-- Aaron



\
 
 \ /
  Last update: 2008-07-07 06:01    [from the cache]
©2003-2011 Jasper Spaans