* [PATCH 2/2]block cfq: don't use atomic_t for cfq_group
@ 2010-12-23 2:45 Shaohua Li
2010-12-23 15:18 ` Vivek Goyal
2010-12-23 15:26 ` Jeff Moyer
0 siblings, 2 replies; 5+ messages in thread
From: Shaohua Li @ 2010-12-23 2:45 UTC (permalink / raw)
To: lkml; +Cc: Jens Axboe, vgoyal, jmoyer
cfq_group->ref is used with queue_lock hold, the only exception is
cfq_set_request, which looks like a bug to me, so ref doesn't need
to be an atomic and atomic operation is slower.
Signed-off-by: Shaohua Li <shaohua.li@intel.com>
---
block/cfq-iosched.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
Index: linux/block/cfq-iosched.c
===================================================================
--- linux.orig/block/cfq-iosched.c 2010-12-23 10:22:21.000000000 +0800
+++ linux/block/cfq-iosched.c 2010-12-23 10:23:03.000000000 +0800
@@ -209,7 +209,7 @@ struct cfq_group {
struct blkio_group blkg;
#ifdef CONFIG_CFQ_GROUP_IOSCHED
struct hlist_node cfqd_node;
- atomic_t ref;
+ int ref;
#endif
/* number of requests that are on the dispatch list or inside driver */
int dispatched;
@@ -1026,7 +1026,7 @@ cfq_find_alloc_cfqg(struct cfq_data *cfq
* elevator which will be dropped by either elevator exit
* or cgroup deletion path depending on who is exiting first.
*/
- atomic_set(&cfqg->ref, 1);
+ cfqg->ref = 1;
/*
* Add group onto cgroup list. It might happen that bdi->dev is
@@ -1071,7 +1071,7 @@ static struct cfq_group *cfq_get_cfqg(st
static inline struct cfq_group *cfq_ref_get_cfqg(struct cfq_group *cfqg)
{
- atomic_inc(&cfqg->ref);
+ cfqg->ref++;
return cfqg;
}
@@ -1083,7 +1083,7 @@ static void cfq_link_cfqq_cfqg(struct cf
cfqq->cfqg = cfqg;
/* cfqq reference on cfqg */
- atomic_inc(&cfqq->cfqg->ref);
+ cfqq->cfqg->ref++;
}
static void cfq_put_cfqg(struct cfq_group *cfqg)
@@ -1091,8 +1091,9 @@ static void cfq_put_cfqg(struct cfq_grou
struct cfq_rb_root *st;
int i, j;
- BUG_ON(atomic_read(&cfqg->ref) <= 0);
- if (!atomic_dec_and_test(&cfqg->ref))
+ BUG_ON(cfqg->ref <= 0);
+ cfqg->ref--;
+ if (cfqg->ref)
return;
for_each_cfqg_st(cfqg, i, j, st)
BUG_ON(!RB_EMPTY_ROOT(&st->rb) || st->active != NULL);
@@ -1200,7 +1201,7 @@ static void cfq_service_tree_add(struct
cfq_group_service_tree_del(cfqd, cfqq->cfqg);
cfqq->orig_cfqg = cfqq->cfqg;
cfqq->cfqg = &cfqd->root_group;
- atomic_inc(&cfqd->root_group.ref);
+ cfqd->root_group.ref++;
group_changed = 1;
} else if (!cfqd->cfq_group_isolation
&& cfqq_type(cfqq) == SYNC_WORKLOAD && cfqq->orig_cfqg) {
@@ -3645,6 +3646,7 @@ cfq_set_request(struct request_queue *q,
const bool is_sync = rq_is_sync(rq);
struct cfq_queue *cfqq;
unsigned long flags;
+ struct cfq_group *cfqg;
might_sleep_if(gfp_mask & __GFP_WAIT);
@@ -3683,12 +3685,13 @@ new_queue:
cfqq->allocated[rw]++;
cfqq->ref++;
+ cfqg = cfq_ref_get_cfqg(cfqq->cfqg);
spin_unlock_irqrestore(q->queue_lock, flags);
rq->elevator_private = cic;
rq->elevator_private2 = cfqq;
- rq->elevator_private3 = cfq_ref_get_cfqg(cfqq->cfqg);
+ rq->elevator_private3 = cfqg;
return 0;
queue_fail:
@@ -3886,7 +3889,7 @@ static void *cfq_init_queue(struct reque
* Take a reference to root group which we never drop. This is just
* to make sure that cfq_put_cfqg() does not try to kfree root group
*/
- atomic_set(&cfqg->ref, 1);
+ cfqg->ref = 1;
rcu_read_lock();
cfq_blkiocg_add_blkio_group(&blkio_root_cgroup, &cfqg->blkg,
(void *)cfqd, 0);
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 2/2]block cfq: don't use atomic_t for cfq_group 2010-12-23 2:45 [PATCH 2/2]block cfq: don't use atomic_t for cfq_group Shaohua Li @ 2010-12-23 15:18 ` Vivek Goyal 2010-12-24 0:40 ` Shaohua Li 2010-12-23 15:26 ` Jeff Moyer 1 sibling, 1 reply; 5+ messages in thread From: Vivek Goyal @ 2010-12-23 15:18 UTC (permalink / raw) To: Shaohua Li; +Cc: lkml, Jens Axboe, jmoyer On Thu, Dec 23, 2010 at 10:45:35AM +0800, Shaohua Li wrote: > cfq_group->ref is used with queue_lock hold, the only exception is > cfq_set_request, which looks like a bug to me, so ref doesn't need > to be an atomic and atomic operation is slower. > [..] > > @@ -3683,12 +3685,13 @@ new_queue: > > cfqq->allocated[rw]++; > cfqq->ref++; > + cfqg = cfq_ref_get_cfqg(cfqq->cfqg); > > spin_unlock_irqrestore(q->queue_lock, flags); > > rq->elevator_private = cic; > rq->elevator_private2 = cfqq; > - rq->elevator_private3 = cfq_ref_get_cfqg(cfqq->cfqg); > + rq->elevator_private3 = cfqg; I think you can move every thing under spinlock. IOW, first set the rq->elevator_private* fields and delay the release of spinlock. Few days back I was also looking at wondering that why are we releasing the spinlock early. Thanks Vivek ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2]block cfq: don't use atomic_t for cfq_group 2010-12-23 15:18 ` Vivek Goyal @ 2010-12-24 0:40 ` Shaohua Li 2010-12-24 19:38 ` Vivek Goyal 0 siblings, 1 reply; 5+ messages in thread From: Shaohua Li @ 2010-12-24 0:40 UTC (permalink / raw) To: Vivek Goyal; +Cc: lkml, Jens Axboe, jmoyer@redhat.com On Thu, 2010-12-23 at 23:18 +0800, Vivek Goyal wrote: > On Thu, Dec 23, 2010 at 10:45:35AM +0800, Shaohua Li wrote: > > cfq_group->ref is used with queue_lock hold, the only exception is > > cfq_set_request, which looks like a bug to me, so ref doesn't need > > to be an atomic and atomic operation is slower. > > > > [..] > > > > @@ -3683,12 +3685,13 @@ new_queue: > > > > cfqq->allocated[rw]++; > > cfqq->ref++; > > + cfqg = cfq_ref_get_cfqg(cfqq->cfqg); > > > > spin_unlock_irqrestore(q->queue_lock, flags); > > > > rq->elevator_private = cic; > > rq->elevator_private2 = cfqq; > > - rq->elevator_private3 = cfq_ref_get_cfqg(cfqq->cfqg); > > + rq->elevator_private3 = cfqg; > > I think you can move every thing under spinlock. IOW, first set the > rq->elevator_private* fields and delay the release of spinlock. Few > days back I was also looking at wondering that why are we releasing > the spinlock early. sure, it's harmless anyway and more readable. Updated the patch. cfq_group->ref is used with queue_lock hold, the only exception is cfq_set_request, which looks like a bug to me, so ref doesn't need to be an atomic and atomic operation is slower. Signed-off-by: Shaohua Li <shaohua.li@intel.com> Reviewed-by: Jeff Moyer <jmoyer@redhat.com> --- block/cfq-iosched.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) Index: linux/block/cfq-iosched.c =================================================================== --- linux.orig/block/cfq-iosched.c 2010-12-24 08:32:19.000000000 +0800 +++ linux/block/cfq-iosched.c 2010-12-24 08:33:10.000000000 +0800 @@ -209,7 +209,7 @@ struct cfq_group { struct blkio_group blkg; #ifdef CONFIG_CFQ_GROUP_IOSCHED struct hlist_node cfqd_node; - atomic_t ref; + int ref; #endif /* number of requests that are on the dispatch list or inside driver */ int dispatched; @@ -1026,7 +1026,7 @@ cfq_find_alloc_cfqg(struct cfq_data *cfq * elevator which will be dropped by either elevator exit * or cgroup deletion path depending on who is exiting first. */ - atomic_set(&cfqg->ref, 1); + cfqg->ref = 1; /* * Add group onto cgroup list. It might happen that bdi->dev is @@ -1071,7 +1071,7 @@ static struct cfq_group *cfq_get_cfqg(st static inline struct cfq_group *cfq_ref_get_cfqg(struct cfq_group *cfqg) { - atomic_inc(&cfqg->ref); + cfqg->ref++; return cfqg; } @@ -1083,7 +1083,7 @@ static void cfq_link_cfqq_cfqg(struct cf cfqq->cfqg = cfqg; /* cfqq reference on cfqg */ - atomic_inc(&cfqq->cfqg->ref); + cfqq->cfqg->ref++; } static void cfq_put_cfqg(struct cfq_group *cfqg) @@ -1091,8 +1091,9 @@ static void cfq_put_cfqg(struct cfq_grou struct cfq_rb_root *st; int i, j; - BUG_ON(atomic_read(&cfqg->ref) <= 0); - if (!atomic_dec_and_test(&cfqg->ref)) + BUG_ON(cfqg->ref <= 0); + cfqg->ref--; + if (cfqg->ref) return; for_each_cfqg_st(cfqg, i, j, st) BUG_ON(!RB_EMPTY_ROOT(&st->rb) || st->active != NULL); @@ -1200,7 +1201,7 @@ static void cfq_service_tree_add(struct cfq_group_service_tree_del(cfqd, cfqq->cfqg); cfqq->orig_cfqg = cfqq->cfqg; cfqq->cfqg = &cfqd->root_group; - atomic_inc(&cfqd->root_group.ref); + cfqd->root_group.ref++; group_changed = 1; } else if (!cfqd->cfq_group_isolation && cfqq_type(cfqq) == SYNC_WORKLOAD && cfqq->orig_cfqg) { @@ -3683,12 +3684,12 @@ new_queue: cfqq->allocated[rw]++; cfqq->ref++; - - spin_unlock_irqrestore(q->queue_lock, flags); - rq->elevator_private = cic; rq->elevator_private2 = cfqq; rq->elevator_private3 = cfq_ref_get_cfqg(cfqq->cfqg); + + spin_unlock_irqrestore(q->queue_lock, flags); + return 0; queue_fail: @@ -3886,7 +3887,7 @@ static void *cfq_init_queue(struct reque * Take a reference to root group which we never drop. This is just * to make sure that cfq_put_cfqg() does not try to kfree root group */ - atomic_set(&cfqg->ref, 1); + cfqg->ref = 1; rcu_read_lock(); cfq_blkiocg_add_blkio_group(&blkio_root_cgroup, &cfqg->blkg, (void *)cfqd, 0); ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2]block cfq: don't use atomic_t for cfq_group 2010-12-24 0:40 ` Shaohua Li @ 2010-12-24 19:38 ` Vivek Goyal 0 siblings, 0 replies; 5+ messages in thread From: Vivek Goyal @ 2010-12-24 19:38 UTC (permalink / raw) To: Shaohua Li; +Cc: lkml, Jens Axboe, jmoyer@redhat.com On Fri, Dec 24, 2010 at 08:40:59AM +0800, Shaohua Li wrote: > On Thu, 2010-12-23 at 23:18 +0800, Vivek Goyal wrote: > > On Thu, Dec 23, 2010 at 10:45:35AM +0800, Shaohua Li wrote: > > > cfq_group->ref is used with queue_lock hold, the only exception is > > > cfq_set_request, which looks like a bug to me, so ref doesn't need > > > to be an atomic and atomic operation is slower. > > > > > > > [..] > > > > > > @@ -3683,12 +3685,13 @@ new_queue: > > > > > > cfqq->allocated[rw]++; > > > cfqq->ref++; > > > + cfqg = cfq_ref_get_cfqg(cfqq->cfqg); > > > > > > spin_unlock_irqrestore(q->queue_lock, flags); > > > > > > rq->elevator_private = cic; > > > rq->elevator_private2 = cfqq; > > > - rq->elevator_private3 = cfq_ref_get_cfqg(cfqq->cfqg); > > > + rq->elevator_private3 = cfqg; > > > > I think you can move every thing under spinlock. IOW, first set the > > rq->elevator_private* fields and delay the release of spinlock. Few > > days back I was also looking at wondering that why are we releasing > > the spinlock early. > sure, it's harmless anyway and more readable. Updated the patch. > > cfq_group->ref is used with queue_lock hold, the only exception is > cfq_set_request, which looks like a bug to me, so ref doesn't need > to be an atomic and atomic operation is slower. > > Signed-off-by: Shaohua Li <shaohua.li@intel.com> > Reviewed-by: Jeff Moyer <jmoyer@redhat.com> Looks good to me. Acked-by: Vivek Goyal <vgoyal@redhat.com> Vivek > --- > block/cfq-iosched.c | 23 ++++++++++++----------- > 1 file changed, 12 insertions(+), 11 deletions(-) > > Index: linux/block/cfq-iosched.c > =================================================================== > --- linux.orig/block/cfq-iosched.c 2010-12-24 08:32:19.000000000 +0800 > +++ linux/block/cfq-iosched.c 2010-12-24 08:33:10.000000000 +0800 > @@ -209,7 +209,7 @@ struct cfq_group { > struct blkio_group blkg; > #ifdef CONFIG_CFQ_GROUP_IOSCHED > struct hlist_node cfqd_node; > - atomic_t ref; > + int ref; > #endif > /* number of requests that are on the dispatch list or inside driver */ > int dispatched; > @@ -1026,7 +1026,7 @@ cfq_find_alloc_cfqg(struct cfq_data *cfq > * elevator which will be dropped by either elevator exit > * or cgroup deletion path depending on who is exiting first. > */ > - atomic_set(&cfqg->ref, 1); > + cfqg->ref = 1; > > /* > * Add group onto cgroup list. It might happen that bdi->dev is > @@ -1071,7 +1071,7 @@ static struct cfq_group *cfq_get_cfqg(st > > static inline struct cfq_group *cfq_ref_get_cfqg(struct cfq_group *cfqg) > { > - atomic_inc(&cfqg->ref); > + cfqg->ref++; > return cfqg; > } > > @@ -1083,7 +1083,7 @@ static void cfq_link_cfqq_cfqg(struct cf > > cfqq->cfqg = cfqg; > /* cfqq reference on cfqg */ > - atomic_inc(&cfqq->cfqg->ref); > + cfqq->cfqg->ref++; > } > > static void cfq_put_cfqg(struct cfq_group *cfqg) > @@ -1091,8 +1091,9 @@ static void cfq_put_cfqg(struct cfq_grou > struct cfq_rb_root *st; > int i, j; > > - BUG_ON(atomic_read(&cfqg->ref) <= 0); > - if (!atomic_dec_and_test(&cfqg->ref)) > + BUG_ON(cfqg->ref <= 0); > + cfqg->ref--; > + if (cfqg->ref) > return; > for_each_cfqg_st(cfqg, i, j, st) > BUG_ON(!RB_EMPTY_ROOT(&st->rb) || st->active != NULL); > @@ -1200,7 +1201,7 @@ static void cfq_service_tree_add(struct > cfq_group_service_tree_del(cfqd, cfqq->cfqg); > cfqq->orig_cfqg = cfqq->cfqg; > cfqq->cfqg = &cfqd->root_group; > - atomic_inc(&cfqd->root_group.ref); > + cfqd->root_group.ref++; > group_changed = 1; > } else if (!cfqd->cfq_group_isolation > && cfqq_type(cfqq) == SYNC_WORKLOAD && cfqq->orig_cfqg) { > @@ -3683,12 +3684,12 @@ new_queue: > > cfqq->allocated[rw]++; > cfqq->ref++; > - > - spin_unlock_irqrestore(q->queue_lock, flags); > - > rq->elevator_private = cic; > rq->elevator_private2 = cfqq; > rq->elevator_private3 = cfq_ref_get_cfqg(cfqq->cfqg); > + > + spin_unlock_irqrestore(q->queue_lock, flags); > + > return 0; > > queue_fail: > @@ -3886,7 +3887,7 @@ static void *cfq_init_queue(struct reque > * Take a reference to root group which we never drop. This is just > * to make sure that cfq_put_cfqg() does not try to kfree root group > */ > - atomic_set(&cfqg->ref, 1); > + cfqg->ref = 1; > rcu_read_lock(); > cfq_blkiocg_add_blkio_group(&blkio_root_cgroup, &cfqg->blkg, > (void *)cfqd, 0); > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2]block cfq: don't use atomic_t for cfq_group 2010-12-23 2:45 [PATCH 2/2]block cfq: don't use atomic_t for cfq_group Shaohua Li 2010-12-23 15:18 ` Vivek Goyal @ 2010-12-23 15:26 ` Jeff Moyer 1 sibling, 0 replies; 5+ messages in thread From: Jeff Moyer @ 2010-12-23 15:26 UTC (permalink / raw) To: Shaohua Li; +Cc: lkml, Jens Axboe, vgoyal Shaohua Li <shaohua.li@intel.com> writes: > cfq_group->ref is used with queue_lock hold, the only exception is > cfq_set_request, which looks like a bug to me, so ref doesn't need > to be an atomic and atomic operation is slower. > > Signed-off-by: Shaohua Li <shaohua.li@intel.com> Reviewed-by: Jeff Moyer <jmoyer@redhat.com> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-12-24 19:38 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-12-23 2:45 [PATCH 2/2]block cfq: don't use atomic_t for cfq_group Shaohua Li 2010-12-23 15:18 ` Vivek Goyal 2010-12-24 0:40 ` Shaohua Li 2010-12-24 19:38 ` Vivek Goyal 2010-12-23 15:26 ` Jeff Moyer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox