| Message ID | 4A3F3648.7080007@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jun 22, 2009 at 03:44:08PM +0800, Gui Jianfeng wrote: > Preempt the ongoing non-rt ioq if there are rt ioqs waiting for dispatching > in ancestor or sibling groups. It will give other group's rt ioq an chance > to dispatch ASAP. > Hi Gui, Will new preempton logic of traversing up the hiearchy so that both new queue and old queue are at same level to take a preemption decision not take care of above scenario? Please have a look at bfq_find_matching_entity(). At the same time we probably don't want to preempt the non-rt queue with an RT queue in sibling group until and unless sibling group is an RT group. root / \ BEgrpA BEgrpB | | BEioq1 RTioq2 Above we got two BE group A and B and assume ioq in group A is being served and then an RT request in group B comes. Because group B is an BE class group, we should not preempt the queue in group A. Thanks Vivek > Signed-off-by: Gui Jianfeng <guijianfeng@cn.fujitsu.com> > --- > block/elevator-fq.c | 44 +++++++++++++++++++++++++++++++++++++++----- > block/elevator-fq.h | 1 + > 2 files changed, 40 insertions(+), 5 deletions(-) > > diff --git a/block/elevator-fq.c b/block/elevator-fq.c > index 2ad40eb..80526fd 100644 > --- a/block/elevator-fq.c > +++ b/block/elevator-fq.c > @@ -3245,8 +3245,16 @@ void elv_add_ioq_busy(struct elv_fq_data *efqd, struct io_queue *ioq) > elv_mark_ioq_busy(ioq); > efqd->busy_queues++; > if (elv_ioq_class_rt(ioq)) { > + struct io_entity *entity; > struct io_group *iog = ioq_to_io_group(ioq); > + > iog->busy_rt_queues++; > + entity = iog->entity.parent; > + > + for_each_entity(entity) { > + iog = io_entity_to_iog(entity); > + iog->sub_busy_rt_queues++; > + } > } > > #ifdef CONFIG_DEBUG_GROUP_IOSCHED > @@ -3290,9 +3298,18 @@ void elv_del_ioq_busy(struct elevator_queue *e, struct io_queue *ioq, > elv_clear_ioq_busy(ioq); > BUG_ON(efqd->busy_queues == 0); > efqd->busy_queues--; > + > if (elv_ioq_class_rt(ioq)) { > + struct io_entity *entity; > struct io_group *iog = ioq_to_io_group(ioq); > + > iog->busy_rt_queues--; > + entity = iog->entity.parent; > + > + for_each_entity(entity) { > + iog = io_entity_to_iog(entity); > + iog->sub_busy_rt_queues--; > + } > } > > elv_deactivate_ioq(efqd, ioq, requeue); > @@ -3735,12 +3752,32 @@ int elv_iosched_expire_ioq(struct request_queue *q, int slice_expired, > return ret; > } > > +static int check_rt_queue(struct io_queue *ioq) > +{ > + struct io_group *iog; > + struct io_entity *entity; > + > + iog = ioq_to_io_group(ioq); > + > + if (iog->busy_rt_queues) > + return 1; > + > + entity = iog->entity.parent; > + > + for_each_entity(entity) { > + iog = io_entity_to_iog(entity); > + if (iog->sub_busy_rt_queues) > + return 1; > + } > + > + return 0; > +} > + > /* Common layer function to select the next queue to dispatch from */ > void *elv_fq_select_ioq(struct request_queue *q, int force) > { > struct elv_fq_data *efqd = &q->elevator->efqd; > struct io_queue *new_ioq = NULL, *ioq = elv_active_ioq(q->elevator); > - struct io_group *iog; > int slice_expired = 1; > > if (!elv_nr_busy_ioq(q->elevator)) > @@ -3811,12 +3848,9 @@ void *elv_fq_select_ioq(struct request_queue *q, int force) > /* > * If we have a RT cfqq waiting, then we pre-empt the current non-rt > * cfqq. > - * > - * TODO: This does not seem right across the io groups. Fix it. > */ > - iog = ioq_to_io_group(ioq); > > - if (!elv_ioq_class_rt(ioq) && iog->busy_rt_queues) { > + if (!elv_ioq_class_rt(ioq) && check_rt_queue(ioq)) { > /* > * We simulate this as cfqq timed out so that it gets to bank > * the remaining of its time slice. > diff --git a/block/elevator-fq.h b/block/elevator-fq.h > index b3193f8..be6c1af 100644 > --- a/block/elevator-fq.h > +++ b/block/elevator-fq.h > @@ -248,6 +248,7 @@ struct io_group { > * non-RT cfqq in service when this value is non-zero. > */ > unsigned int busy_rt_queues; > + unsigned int sub_busy_rt_queues; > > int deleting; > unsigned short iocg_id; > -- > 1.5.4.rc3 > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Vivek Goyal wrote: > On Mon, Jun 22, 2009 at 03:44:08PM +0800, Gui Jianfeng wrote: >> Preempt the ongoing non-rt ioq if there are rt ioqs waiting for dispatching >> in ancestor or sibling groups. It will give other group's rt ioq an chance >> to dispatch ASAP. >> > > Hi Gui, > > Will new preempton logic of traversing up the hiearchy so that both new > queue and old queue are at same level to take a preemption decision not > take care of above scenario? Hi Vivek, Would you explain a bit what do you mean about "both new queue and old queue are at same level to take a preemption decision". I don't understand it well. > > Please have a look at bfq_find_matching_entity(). > > At the same time we probably don't want to preempt the non-rt queue > with an RT queue in sibling group until and unless sibling group is an > RT group. > > root > / \ > BEgrpA BEgrpB > | | > BEioq1 RTioq2 > > Above we got two BE group A and B and assume ioq in group A is being > served and then an RT request in group B comes. Because group B is an > BE class group, we should not preempt the queue in group A. Yes, i also have this concern. So, it does not allow non-rt group preempts another group. I'll check whether there is a way to address this issue. > > Thanks > Vivek > > >> Signed-off-by: Gui Jianfeng <guijianfeng@cn.fujitsu.com> >> --- >> block/elevator-fq.c | 44 +++++++++++++++++++++++++++++++++++++++----- >> block/elevator-fq.h | 1 + >> 2 files changed, 40 insertions(+), 5 deletions(-) >> >> diff --git a/block/elevator-fq.c b/block/elevator-fq.c >> index 2ad40eb..80526fd 100644 >> --- a/block/elevator-fq.c >> +++ b/block/elevator-fq.c >> @@ -3245,8 +3245,16 @@ void elv_add_ioq_busy(struct elv_fq_data *efqd, struct io_queue *ioq) >> elv_mark_ioq_busy(ioq); >> efqd->busy_queues++; >> if (elv_ioq_class_rt(ioq)) { >> + struct io_entity *entity; >> struct io_group *iog = ioq_to_io_group(ioq); >> + >> iog->busy_rt_queues++; >> + entity = iog->entity.parent; >> + >> + for_each_entity(entity) { >> + iog = io_entity_to_iog(entity); >> + iog->sub_busy_rt_queues++; >> + } >> } >> >> #ifdef CONFIG_DEBUG_GROUP_IOSCHED >> @@ -3290,9 +3298,18 @@ void elv_del_ioq_busy(struct elevator_queue *e, struct io_queue *ioq, >> elv_clear_ioq_busy(ioq); >> BUG_ON(efqd->busy_queues == 0); >> efqd->busy_queues--; >> + >> if (elv_ioq_class_rt(ioq)) { >> + struct io_entity *entity; >> struct io_group *iog = ioq_to_io_group(ioq); >> + >> iog->busy_rt_queues--; >> + entity = iog->entity.parent; >> + >> + for_each_entity(entity) { >> + iog = io_entity_to_iog(entity); >> + iog->sub_busy_rt_queues--; >> + } >> } >> >> elv_deactivate_ioq(efqd, ioq, requeue); >> @@ -3735,12 +3752,32 @@ int elv_iosched_expire_ioq(struct request_queue *q, int slice_expired, >> return ret; >> } >> >> +static int check_rt_queue(struct io_queue *ioq) >> +{ >> + struct io_group *iog; >> + struct io_entity *entity; >> + >> + iog = ioq_to_io_group(ioq); >> + >> + if (iog->busy_rt_queues) >> + return 1; >> + >> + entity = iog->entity.parent; >> + >> + for_each_entity(entity) { >> + iog = io_entity_to_iog(entity); >> + if (iog->sub_busy_rt_queues) >> + return 1; >> + } >> + >> + return 0; >> +} >> + >> /* Common layer function to select the next queue to dispatch from */ >> void *elv_fq_select_ioq(struct request_queue *q, int force) >> { >> struct elv_fq_data *efqd = &q->elevator->efqd; >> struct io_queue *new_ioq = NULL, *ioq = elv_active_ioq(q->elevator); >> - struct io_group *iog; >> int slice_expired = 1; >> >> if (!elv_nr_busy_ioq(q->elevator)) >> @@ -3811,12 +3848,9 @@ void *elv_fq_select_ioq(struct request_queue *q, int force) >> /* >> * If we have a RT cfqq waiting, then we pre-empt the current non-rt >> * cfqq. >> - * >> - * TODO: This does not seem right across the io groups. Fix it. >> */ >> - iog = ioq_to_io_group(ioq); >> >> - if (!elv_ioq_class_rt(ioq) && iog->busy_rt_queues) { >> + if (!elv_ioq_class_rt(ioq) && check_rt_queue(ioq)) { >> /* >> * We simulate this as cfqq timed out so that it gets to bank >> * the remaining of its time slice. >> diff --git a/block/elevator-fq.h b/block/elevator-fq.h >> index b3193f8..be6c1af 100644 >> --- a/block/elevator-fq.h >> +++ b/block/elevator-fq.h >> @@ -248,6 +248,7 @@ struct io_group { >> * non-RT cfqq in service when this value is non-zero. >> */ >> unsigned int busy_rt_queues; >> + unsigned int sub_busy_rt_queues; >> >> int deleting; >> unsigned short iocg_id; >> -- >> 1.5.4.rc3 >> > > >
On Tue, Jun 23, 2009 at 02:44:08PM +0800, Gui Jianfeng wrote: > Vivek Goyal wrote: > > On Mon, Jun 22, 2009 at 03:44:08PM +0800, Gui Jianfeng wrote: > >> Preempt the ongoing non-rt ioq if there are rt ioqs waiting for dispatching > >> in ancestor or sibling groups. It will give other group's rt ioq an chance > >> to dispatch ASAP. > >> > > > > Hi Gui, > > > > Will new preempton logic of traversing up the hiearchy so that both new > > queue and old queue are at same level to take a preemption decision not > > take care of above scenario? > > Hi Vivek, > > Would you explain a bit what do you mean about "both new queue and old queue > are at same level to take a preemption decision". I don't understand it well. > Consider following hierarchy. root / | A 1 | 2 In the above diagram, A is the group and "1" and "2" are two io queues associated with tasks. Now assume that queue "1" is being served and queue "2" gets backlogged. Should queue 2 preempt queue 1 now? To take that decision, we need to do the comparision between type of entity of group A and queue 1 (That is at the same level or IOW, the entities in question have the same parent). If group A is of class RT and queue 1 is of type BE then queue 2 should preempt queue 1 otherwise not. Hence in hierarchical setups to take a preemption decision, comparison should be done at same level. > > > > Please have a look at bfq_find_matching_entity(). > > > > At the same time we probably don't want to preempt the non-rt queue > > with an RT queue in sibling group until and unless sibling group is an > > RT group. > > > > root > > / \ > > BEgrpA BEgrpB > > | | > > BEioq1 RTioq2 > > > > Above we got two BE group A and B and assume ioq in group A is being > > served and then an RT request in group B comes. Because group B is an > > BE class group, we should not preempt the queue in group A. > > Yes, i also have this concern. So, it does not allow non-rt group preempts > another group. I'll check whether there is a way to address this issue. > So here also assume ioq1 is being served and ioq2 gets backlogged. To decide whether ioq2 should preempt ioq1 or not, one needs to go up the hiearchy till two paths share the parent. That means one needs to go up at the BEgrpA and BEgrpB level where they have common parent "root". Now both the groups are of class BE hence ioq2 should not preempt ioq1. Hope it helps. Thanks Vivek -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Vivek Goyal wrote: > On Tue, Jun 23, 2009 at 02:44:08PM +0800, Gui Jianfeng wrote: >> Vivek Goyal wrote: >>> On Mon, Jun 22, 2009 at 03:44:08PM +0800, Gui Jianfeng wrote: >>>> Preempt the ongoing non-rt ioq if there are rt ioqs waiting for dispatching >>>> in ancestor or sibling groups. It will give other group's rt ioq an chance >>>> to dispatch ASAP. >>>> >>> Hi Gui, >>> >>> Will new preempton logic of traversing up the hiearchy so that both new >>> queue and old queue are at same level to take a preemption decision not >>> take care of above scenario? >> Hi Vivek, >> >> Would you explain a bit what do you mean about "both new queue and old queue >> are at same level to take a preemption decision". I don't understand it well. >> > > Consider following hierarchy. > > root > / | > A 1 > | > 2 > In the above diagram, A is the group and "1" and "2" are two io queues > associated with tasks. > > Now assume that queue "1" is being served and queue "2" gets backlogged. > Should queue 2 preempt queue 1 now? > > To take that decision, we need to do the comparision between type of > entity of group A and queue 1 (That is at the same level or IOW, the > entities in question have the same parent). If group A is of class RT and > queue 1 is of type BE then queue 2 should preempt queue 1 otherwise not. > > Hence in hierarchical setups to take a preemption decision, comparison > should be done at same level. So what bfq_find_matching_entity has done is going to figure out the same level entities， in turn, taking the decision. > >>> Please have a look at bfq_find_matching_entity(). >>> >>> At the same time we probably don't want to preempt the non-rt queue >>> with an RT queue in sibling group until and unless sibling group is an >>> RT group. >>> >>> root >>> / \ >>> BEgrpA BEgrpB >>> | | >>> BEioq1 RTioq2 >>> >>> Above we got two BE group A and B and assume ioq in group A is being >>> served and then an RT request in group B comes. Because group B is an >>> BE class group, we should not preempt the queue in group A. >> Yes, i also have this concern. So, it does not allow non-rt group preempts >> another group. I'll check whether there is a way to address this issue. >> > > So here also assume ioq1 is being served and ioq2 gets backlogged. To > decide whether ioq2 should preempt ioq1 or not, one needs to go up the > hiearchy till two paths share the parent. That means one needs to go up > at the BEgrpA and BEgrpB level where they have common parent "root". Now > both the groups are of class BE hence ioq2 should not preempt ioq1. > > Hope it helps. Thanks, it's very helpful. I have a thought now. Whether we can maintain a rt ioq list in efqd, and elv_fq_select_ioq() travels this list to take preemtion decision for each available rt ioqs at the same level(by using bfq_find_matching_entity). I'd like to try it out. > > Thanks > Vivek > > >
diff --git a/block/elevator-fq.c b/block/elevator-fq.c index 2ad40eb..80526fd 100644 --- a/block/elevator-fq.c +++ b/block/elevator-fq.c @@ -3245,8 +3245,16 @@ void elv_add_ioq_busy(struct elv_fq_data *efqd, struct io_queue *ioq) elv_mark_ioq_busy(ioq); efqd->busy_queues++; if (elv_ioq_class_rt(ioq)) { + struct io_entity *entity; struct io_group *iog = ioq_to_io_group(ioq); + iog->busy_rt_queues++; + entity = iog->entity.parent; + + for_each_entity(entity) { + iog = io_entity_to_iog(entity); + iog->sub_busy_rt_queues++; + } } #ifdef CONFIG_DEBUG_GROUP_IOSCHED @@ -3290,9 +3298,18 @@ void elv_del_ioq_busy(struct elevator_queue *e, struct io_queue *ioq, elv_clear_ioq_busy(ioq); BUG_ON(efqd->busy_queues == 0); efqd->busy_queues--; + if (elv_ioq_class_rt(ioq)) { + struct io_entity *entity; struct io_group *iog = ioq_to_io_group(ioq); + iog->busy_rt_queues--; + entity = iog->entity.parent; + + for_each_entity(entity) { + iog = io_entity_to_iog(entity); + iog->sub_busy_rt_queues--; + } } elv_deactivate_ioq(efqd, ioq, requeue); @@ -3735,12 +3752,32 @@ int elv_iosched_expire_ioq(struct request_queue *q, int slice_expired, return ret; } +static int check_rt_queue(struct io_queue *ioq) +{ + struct io_group *iog; + struct io_entity *entity; + + iog = ioq_to_io_group(ioq); + + if (iog->busy_rt_queues) + return 1; + + entity = iog->entity.parent; + + for_each_entity(entity) { + iog = io_entity_to_iog(entity); + if (iog->sub_busy_rt_queues) + return 1; + } + + return 0; +} + /* Common layer function to select the next queue to dispatch from */ void *elv_fq_select_ioq(struct request_queue *q, int force) { struct elv_fq_data *efqd = &q->elevator->efqd; struct io_queue *new_ioq = NULL, *ioq = elv_active_ioq(q->elevator); - struct io_group *iog; int slice_expired = 1; if (!elv_nr_busy_ioq(q->elevator)) @@ -3811,12 +3848,9 @@ void *elv_fq_select_ioq(struct request_queue *q, int force) /* * If we have a RT cfqq waiting, then we pre-empt the current non-rt * cfqq. - * - * TODO: This does not seem right across the io groups. Fix it. */ - iog = ioq_to_io_group(ioq); - if (!elv_ioq_class_rt(ioq) && iog->busy_rt_queues) { + if (!elv_ioq_class_rt(ioq) && check_rt_queue(ioq)) { /* * We simulate this as cfqq timed out so that it gets to bank * the remaining of its time slice. diff --git a/block/elevator-fq.h b/block/elevator-fq.h index b3193f8..be6c1af 100644 --- a/block/elevator-fq.h +++ b/block/elevator-fq.h @@ -248,6 +248,7 @@ struct io_group { * non-RT cfqq in service when this value is non-zero. */ unsigned int busy_rt_queues; + unsigned int sub_busy_rt_queues; int deleting; unsigned short iocg_id;
Preempt the ongoing non-rt ioq if there are rt ioqs waiting for dispatching in ancestor or sibling groups. It will give other group's rt ioq an chance to dispatch ASAP. Signed-off-by: Gui Jianfeng <guijianfeng@cn.fujitsu.com> --- block/elevator-fq.c | 44 +++++++++++++++++++++++++++++++++++++++----- block/elevator-fq.h | 1 + 2 files changed, 40 insertions(+), 5 deletions(-)