cfq-iosched: algebraic simplification in cfq_prio_to_maxrq()
PATCH 1/4 cfq-iosched: algebraic simplification in cfq_prio_to_maxrq()
Simplify the calculation in cfq_prio_to_maxrq(), plus replace CFQ_PRIO_LISTS to IOPRIO_BE_NR since they are the same and IOPRIO_BE_NR looks more reasonable in this context IMHO.
--- block/cfq-iosched.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
block/cfq-iosched.c +1 -1
@@ -2029,7 +2029,7 @@ cfq_prio_to_maxrq(struct cfq_data *cfqd, struct cfq_queue *cfqq)WARN_ON (cfqq->ioprio >=IOPRIO_BE_NR ); -return 2 * (base_rq + base_rq * (CFQ_PRIO_LISTS -1 - cfqq->ioprio)); +return 2 * base_rq * (IOPRIO_BE_NR - cfqq->ioprio); }/*
Signature · 1 line
1.7.5.2
PATCH 2/4 cfq-iosched: reduce bit operations in cfq_choose_req()
Reduce the number of bit operations in cfq_choose_req() on average (and worst) cases.
--- block/cfq-iosched.c | 14 +++++--------- 1 files changed, 5 insertions(+), 9 deletions(-)
block/cfq-iosched.c +5 -9
@@ -665,15 +665,11 @@ cfq_choose_req(struct cfq_data *cfqd, struct request *rq1, struct request *rq2,if (rq2 ==NULL )return rq1; -if (rq_is_sync (rq1) && !rq_is_sync (rq2)) -return rq1; -else if (rq_is_sync (rq2) && !rq_is_sync (rq1)) -return rq2; -if ((rq1->cmd_flags &REQ_META ) && !(rq2->cmd_flags &REQ_META )) -return rq1; -else if ((rq2->cmd_flags &REQ_META ) && - !(rq1->cmd_flags &REQ_META )) -return rq2; +if (rq_is_sync (rq1) !=rq_is_sync (rq2)) +return rq_is_sync (rq1) ? rq1 : rq2; + +if ((rq1->cmd_flags ^ rq2->cmd_flags) &REQ_META ) +return rq1->cmd_flags &REQ_META ? rq1 : rq2; s1 =blk_rq_pos (rq1); s2 =blk_rq_pos (rq2);
Signature · 1 line
1.7.5.2
Re: [PATCH 2/4] cfq-iosched: reduce bit operations in cfq_choose_req()
Why make this change? Are we that sensitive to the number of operations?
It makes the code a bit harder to read, I think.
Quoted 42 lines · Namhyung Kim
Reduce the number of bit operations in
cfq_choose_req()on average (and worst) cases.Signed-off-by Namhyung Kim <[email protected]>---
block/cfq-iosched.c | 14 +++++---------1 files changed, 5 insertions(+), 9 deletions(-)block/cfq-iosched.c +5
-10@@ -665,15 +665,11 @@ cfq_choose_req(struct cfq_data *cfqd, struct request *rq1, struct request *rq2, if (rq2 == NULL) return rq1; - if (rq_is_sync(rq1) && !rq_is_sync(rq2)) - return rq1; - else if (rq_is_sync(rq2) && !rq_is_sync(rq1)) - return rq2; - if ((rq1->cmd_flags & REQ_META) && !(rq2->cmd_flags & REQ_META)) - return rq1; - else if ((rq2->cmd_flags & REQ_META) && - !(rq1->cmd_flags & REQ_META)) - return rq2; + if (rq_is_sync(rq1) != rq_is_sync(rq2)) + return rq_is_sync(rq1) ? rq1 : rq2; + + if ((rq1->cmd_flags ^ rq2->cmd_flags) & REQ_META) + return rq1->cmd_flags & REQ_META ? rq1 : rq2; s1 = blk_rq_pos(rq1); s2 = blk_rq_pos(rq2); --1.7.5.2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] cfq-iosched: reduce bit operations in cfq_choose_req()
2011-05-24 (화), 10:34 -0700, Justin TerAvest:
Quoted 4 lines
Why make this change? Are we that sensitive to the number of operations?
It makes the code a bit harder to read, I think.
Hi Justin,
I thought that the function is a sort of hot path operation so it would be better make it even a bit faster, no? And if you guys think this patch makes the code harder we can add a comment, although I don't think it's hard enough. :)
Thanks.
Signature · 2 lines
Regards, Namhyung Kim
PATCH 3/4 cfq-iosched: remove unused 'group_changed' in cfq_service_tree_add()
In reply to Namhyung KimThe 'group_changed' variable is initialized to 0 and never changed, so checking the variable is meaningless.
It is a leftover from 0bbfeb832042 ("cfq-iosched: Always provide group iosolation."). Let's get rid of it.
--- block/cfq-iosched.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)
block/cfq-iosched.c +1 -2
@@ -1206,7 +1206,6 @@ static void cfq_service_tree_add(struct cfq_data *cfqd, struct cfq_queue *cfqq,struct cfq_rb_root *service_tree;int left;int new_cfqq =1 ; -int group_changed =0 ; service_tree =service_tree_for (cfqq->cfqg,cfqq_prio (cfqq),cfqq_type (cfqq));
@@ -1277,7 +1276,7 @@ static void cfq_service_tree_add(struct cfq_data *cfqd, struct cfq_queue *cfqq,rb_link_node (&cfqq->rb_node, parent, p);rb_insert_color (&cfqq->rb_node, &service_tree->rb); service_tree->count++; -if ((add_front || !new_cfqq) && !group_changed) +if (add_front || !new_cfqq)return ;cfq_group_notify_queue_add (cfqd, cfqq->cfqg); }
Signature · 1 line
1.7.5.2
Re: [PATCH 3/4] cfq-iosched: remove unused 'group_changed' in cfq_service_tree_add()
Looks good to me.
Quoted 37 lines · Namhyung Kim
The '
group_changed' variable is initialized to 0 and never changed, so checking the variable is meaningless.It is a leftover from
0bbfeb832042("cfq-iosched: Always provide group iosolation."). Let's get rid of it.Signed-off-by Namhyung Kim <[email protected]>Cc Justin TerAvest <[email protected]>---
block/cfq-iosched.c | 3 +--1 files changed, 1 insertions(+), 2 deletions(-)block/cfq-iosched.c +1
-3@@ -1206,7 +1206,6 @@ static void cfq_service_tree_add(struct cfq_data *cfqd, struct cfq_queue *cfqq, struct cfq_rb_root *service_tree; int left; int new_cfqq = 1; - int group_changed = 0; service_tree = service_tree_for(cfqq->cfqg, cfqq_prio(cfqq), cfqq_type(cfqq));@@ -1277,7 +1276,7 @@ static void cfq_service_tree_add(struct cfq_data *cfqd, struct cfq_queue *cfqq, rb_link_node(&cfqq->rb_node, parent, p); rb_insert_color(&cfqq->rb_node, &service_tree->rb); service_tree->count++; - if ((add_front || !new_cfqq) && !group_changed) + if (add_front || !new_cfqq) return; cfq_group_notify_queue_add(cfqd, cfqq->cfqg); } --1.7.5.2
PATCH 4/4 cfq-iosched: free cic_index if cfqd allocation fails
In reply to Namhyung KimWhen struct cfq_data allocation fails, cic_index need to be freed.
--- block/cfq-iosched.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)
block/cfq-iosched.c +5 -1
@@ -3881,8 +3881,12 @@ static void *cfq_init_queue(struct request_queue *q)return NULL ; cfqd =kmalloc_node (sizeof (*cfqd),GFP_KERNEL | __GFP_ZERO, q->node); -if (!cfqd) +if (!cfqd) { +spin_lock (&cic_index_lock); +ida_remove (&cic_index_ida, i); +spin_unlock (&cic_index_lock);return NULL ; + }/* * Don't need take queue_lock in the routine, since we are
Signature · 1 line
1.7.5.2
Jens Axboe
maintainerQuoted 3 lines · Namhyung Kim
Simplify the calculation in
cfq_prio_to_maxrq(), plus replaceCFQ_PRIO_LISTStoIOPRIO_BE_NRsince they are the same andIOPRIO_BE_NRlooks more reasonable in this context IMHO.
Double checked your math, it is indeed identical to 2*base_rq*IOPRIO_BE_NR - 2*base_rq*cfqq->ioprio. Looks a lot cleaner, thanks.
Signature · 1 line
Jens Axboe