| Message ID | 1426586009-21519-4-git-send-email-ciprian.barbu@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 03/17/15 12:53, Ciprian Barbu wrote: > ODP_QUEUE_TYPE_SCHED queues only get marked as destroyed, they need to be > removed from the pri_queues of the linux-generic scheduler > > Signed-off-by: Ciprian Barbu <ciprian.barbu@linaro.org> > --- > v2: > - removed #if 1 and trailing whitespaces > > platform/linux-generic/odp_schedule.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/platform/linux-generic/odp_schedule.c b/platform/linux-generic/odp_schedule.c > index dd65168..18513da 100644 > --- a/platform/linux-generic/odp_schedule.c > +++ b/platform/linux-generic/odp_schedule.c > @@ -153,6 +153,28 @@ int odp_schedule_term_global(void) > > for (i = 0; i < ODP_CONFIG_SCHED_PRIOS; i++) { > for (j = 0; j < QUEUES_PER_PRIO; j++) { > + odp_queue_t pri_q = sched->pri_queue[i][j]; > + > + for (;;) { > + odp_event_t ev = odp_queue_deq(pri_q); > + odp_buffer_t desc_buf; > + queue_desc_t *desc; > + odp_queue_t queue; > + > + desc_buf = odp_buffer_from_event(ev); > + if (desc_buf == ODP_BUFFER_INVALID) > + break; It looks like we did not check return of odp_buffer_from_event() anywhere. But we check that event is not ODP_EVENT_INVALID. That is not performance critical function so I think it's ok to use odp_queue_deq(), not odp_queue_deq_multi(). > + > + desc = odp_buffer_addr(desc_buf); > + queue = desc->queue; > + /* Let deq_multi_destroy do the job */ > + if (queue_is_destroyed(queue)) { > + odp_queue_deq_multi(queue, > + sched_local.ev, > + MAX_DEQ); > + } > + } > + > if (odp_queue_destroy(sched->pri_queue[i][j])) { if you set above it to pri_q var, please use pri_q here also. Maxim. > ODP_ERR("Sched term: Queue destroy fail.\n"); > rc = -1;
On Wed, Mar 18, 2015 at 11:17 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 03/17/15 12:53, Ciprian Barbu wrote: >> >> ODP_QUEUE_TYPE_SCHED queues only get marked as destroyed, they need to be >> removed from the pri_queues of the linux-generic scheduler >> >> Signed-off-by: Ciprian Barbu <ciprian.barbu@linaro.org> >> --- >> v2: >> - removed #if 1 and trailing whitespaces >> >> platform/linux-generic/odp_schedule.c | 22 ++++++++++++++++++++++ >> 1 file changed, 22 insertions(+) >> >> diff --git a/platform/linux-generic/odp_schedule.c >> b/platform/linux-generic/odp_schedule.c >> index dd65168..18513da 100644 >> --- a/platform/linux-generic/odp_schedule.c >> +++ b/platform/linux-generic/odp_schedule.c >> @@ -153,6 +153,28 @@ int odp_schedule_term_global(void) >> for (i = 0; i < ODP_CONFIG_SCHED_PRIOS; i++) { >> for (j = 0; j < QUEUES_PER_PRIO; j++) { >> + odp_queue_t pri_q = sched->pri_queue[i][j]; >> + >> + for (;;) { >> + odp_event_t ev = odp_queue_deq(pri_q); >> + odp_buffer_t desc_buf; >> + queue_desc_t *desc; >> + odp_queue_t queue; >> + >> + desc_buf = odp_buffer_from_event(ev); >> + if (desc_buf == ODP_BUFFER_INVALID) >> + break; > > > It looks like we did not check return of odp_buffer_from_event() anywhere. > But we check that event is not ODP_EVENT_INVALID. > > That is not performance critical function so I think it's ok to use > odp_queue_deq(), not odp_queue_deq_multi(). It's actually not. The problem here is that odp_queu_destroy does not actually destroy SCHED queues. For a queue to be destroyed it's status must be set to QUEUE_STATUS_FREE. For ODP_QUEUE_TYPE_SCHED queues, odp_queue_destroy sets the status to QUEUE_STATUS_DESTROYED, which is not enough for odp_queue_term_global to finish the job, the status must be QUEUE_STATUS_FREE. This sounds like crazy at first, but it is necessary so that the internal priority queues are drained, there are some desc_bufs for each scheduled queues, simply marking the scheduled queue as free is not enough, the desc_buf must be freed too. For that the scheduler code relies on schedule() to process the priority queues, and that code uses queue_deq_multi: https://git.linaro.org/lng/odp.git/blob/df9cacd8cd46629ae74173461f4af7187cfbb670:/platform/linux-generic/odp_schedule.c#l330 > >> + >> + desc = odp_buffer_addr(desc_buf); >> + queue = desc->queue; >> + /* Let deq_multi_destroy do the job */ >> + if (queue_is_destroyed(queue)) { >> + odp_queue_deq_multi(queue, >> + >> sched_local.ev, >> + MAX_DEQ); >> + } >> + } >> + >> if (odp_queue_destroy(sched->pri_queue[i][j])) { > > if you set above it to pri_q var, please use pri_q here also. Ok, I didn't notice this. > > Maxim. >> >> ODP_ERR("Sched term: Queue destroy >> fail.\n"); >> rc = -1; > > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp
On 18 March 2015 at 11:07, Ciprian Barbu <ciprian.barbu@linaro.org> wrote: > On Wed, Mar 18, 2015 at 11:17 AM, Maxim Uvarov <maxim.uvarov@linaro.org> > wrote: > > On 03/17/15 12:53, Ciprian Barbu wrote: > >> > >> ODP_QUEUE_TYPE_SCHED queues only get marked as destroyed, they need to > be > >> removed from the pri_queues of the linux-generic scheduler > >> > >> Signed-off-by: Ciprian Barbu <ciprian.barbu@linaro.org> > >> --- > >> v2: > >> - removed #if 1 and trailing whitespaces > >> > >> platform/linux-generic/odp_schedule.c | 22 ++++++++++++++++++++++ > >> 1 file changed, 22 insertions(+) > >> > >> diff --git a/platform/linux-generic/odp_schedule.c > >> b/platform/linux-generic/odp_schedule.c > >> index dd65168..18513da 100644 > >> --- a/platform/linux-generic/odp_schedule.c > >> +++ b/platform/linux-generic/odp_schedule.c > >> @@ -153,6 +153,28 @@ int odp_schedule_term_global(void) > >> for (i = 0; i < ODP_CONFIG_SCHED_PRIOS; i++) { > >> for (j = 0; j < QUEUES_PER_PRIO; j++) { > >> + odp_queue_t pri_q = sched->pri_queue[i][j]; > >> + > >> + for (;;) { > >> + odp_event_t ev = odp_queue_deq(pri_q); > >> + odp_buffer_t desc_buf; > >> + queue_desc_t *desc; > >> + odp_queue_t queue; > >> + > >> + desc_buf = odp_buffer_from_event(ev); > >> + if (desc_buf == ODP_BUFFER_INVALID) > >> + break; > > > > > > It looks like we did not check return of odp_buffer_from_event() > anywhere. > > But we check that event is not ODP_EVENT_INVALID. > > > > That is not performance critical function so I think it's ok to use > > odp_queue_deq(), not odp_queue_deq_multi(). > > It's actually not. The problem here is that odp_queu_destroy does not > actually destroy SCHED queues. For a queue to be destroyed it's status > must be set to QUEUE_STATUS_FREE. For ODP_QUEUE_TYPE_SCHED queues, > odp_queue_destroy sets the status to QUEUE_STATUS_DESTROYED, which is > not enough for odp_queue_term_global to finish the job, the status > must be QUEUE_STATUS_FREE. > > This sounds like crazy at first, but it is necessary so that the > internal priority queues are drained, there are some desc_bufs for > each scheduled queues, simply marking the scheduled queue as free is > not enough, the desc_buf must be freed too. For that the scheduler > code relies on schedule() to process the priority queues, and that > code uses queue_deq_multi: > > https://git.linaro.org/lng/odp.git/blob/df9cacd8cd46629ae74173461f4af7187cfbb670:/platform/linux-generic/odp_schedule.c#l330 > > Good description. Why isn't it in the code? Who will read this email in one month/year/decade's time? > > >> + > >> + desc = odp_buffer_addr(desc_buf); > >> + queue = desc->queue; > >> + /* Let deq_multi_destroy do the job */ > >> + if (queue_is_destroyed(queue)) { > >> + odp_queue_deq_multi(queue, > >> + > >> sched_local.ev, > >> + MAX_DEQ); > >> + } > >> + } > >> + > >> if (odp_queue_destroy(sched->pri_queue[i][j])) { > > > > if you set above it to pri_q var, please use pri_q here also. > > Ok, I didn't notice this. > > > > > Maxim. > >> > >> ODP_ERR("Sched term: Queue destroy > >> fail.\n"); > >> rc = -1; > > > > > > > > _______________________________________________ > > lng-odp mailing list > > lng-odp@lists.linaro.org > > http://lists.linaro.org/mailman/listinfo/lng-odp > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
On Wed, Mar 18, 2015 at 6:13 PM, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > On 18 March 2015 at 11:07, Ciprian Barbu <ciprian.barbu@linaro.org> wrote: >> >> On Wed, Mar 18, 2015 at 11:17 AM, Maxim Uvarov <maxim.uvarov@linaro.org> >> wrote: >> > On 03/17/15 12:53, Ciprian Barbu wrote: >> >> >> >> ODP_QUEUE_TYPE_SCHED queues only get marked as destroyed, they need to >> >> be >> >> removed from the pri_queues of the linux-generic scheduler >> >> >> >> Signed-off-by: Ciprian Barbu <ciprian.barbu@linaro.org> >> >> --- >> >> v2: >> >> - removed #if 1 and trailing whitespaces >> >> >> >> platform/linux-generic/odp_schedule.c | 22 ++++++++++++++++++++++ >> >> 1 file changed, 22 insertions(+) >> >> >> >> diff --git a/platform/linux-generic/odp_schedule.c >> >> b/platform/linux-generic/odp_schedule.c >> >> index dd65168..18513da 100644 >> >> --- a/platform/linux-generic/odp_schedule.c >> >> +++ b/platform/linux-generic/odp_schedule.c >> >> @@ -153,6 +153,28 @@ int odp_schedule_term_global(void) >> >> for (i = 0; i < ODP_CONFIG_SCHED_PRIOS; i++) { >> >> for (j = 0; j < QUEUES_PER_PRIO; j++) { >> >> + odp_queue_t pri_q = sched->pri_queue[i][j]; >> >> + >> >> + for (;;) { >> >> + odp_event_t ev = odp_queue_deq(pri_q); >> >> + odp_buffer_t desc_buf; >> >> + queue_desc_t *desc; >> >> + odp_queue_t queue; >> >> + >> >> + desc_buf = odp_buffer_from_event(ev); >> >> + if (desc_buf == ODP_BUFFER_INVALID) >> >> + break; >> > >> > >> > It looks like we did not check return of odp_buffer_from_event() >> > anywhere. >> > But we check that event is not ODP_EVENT_INVALID. >> > >> > That is not performance critical function so I think it's ok to use >> > odp_queue_deq(), not odp_queue_deq_multi(). >> >> It's actually not. The problem here is that odp_queu_destroy does not >> actually destroy SCHED queues. For a queue to be destroyed it's status >> must be set to QUEUE_STATUS_FREE. For ODP_QUEUE_TYPE_SCHED queues, >> odp_queue_destroy sets the status to QUEUE_STATUS_DESTROYED, which is >> not enough for odp_queue_term_global to finish the job, the status >> must be QUEUE_STATUS_FREE. >> >> This sounds like crazy at first, but it is necessary so that the >> internal priority queues are drained, there are some desc_bufs for >> each scheduled queues, simply marking the scheduled queue as free is >> not enough, the desc_buf must be freed too. For that the scheduler >> code relies on schedule() to process the priority queues, and that >> code uses queue_deq_multi: >> >> https://git.linaro.org/lng/odp.git/blob/df9cacd8cd46629ae74173461f4af7187cfbb670:/platform/linux-generic/odp_schedule.c#l330 >> > Good description. Why isn't it in the code? Who will read this email in one > month/year/decade's time? I generally saw reluctance towards having long comments in the code, maybe I just misinterpreted. But it wont matter anyway, Petri wants to rework the scheduler so that tricks like this will not be necessary. This was discussed in yesterday's ODP ARCH meeting. Maxim will only apply the first patch of this series, which implements proper queue termination in the schedule validation test. > >> > >> >> + >> >> + desc = odp_buffer_addr(desc_buf); >> >> + queue = desc->queue; >> >> + /* Let deq_multi_destroy do the job */ >> >> + if (queue_is_destroyed(queue)) { >> >> + odp_queue_deq_multi(queue, >> >> + >> >> sched_local.ev, >> >> + MAX_DEQ); >> >> + } >> >> + } >> >> + >> >> if (odp_queue_destroy(sched->pri_queue[i][j])) >> >> { >> > >> > if you set above it to pri_q var, please use pri_q here also. >> >> Ok, I didn't notice this. >> >> > >> > Maxim. >> >> >> >> ODP_ERR("Sched term: Queue destroy >> >> fail.\n"); >> >> rc = -1; >> > >> > >> > >> > _______________________________________________ >> > lng-odp mailing list >> > lng-odp@lists.linaro.org >> > http://lists.linaro.org/mailman/listinfo/lng-odp >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> http://lists.linaro.org/mailman/listinfo/lng-odp > >
diff --git a/platform/linux-generic/odp_schedule.c b/platform/linux-generic/odp_schedule.c index dd65168..18513da 100644 --- a/platform/linux-generic/odp_schedule.c +++ b/platform/linux-generic/odp_schedule.c @@ -153,6 +153,28 @@ int odp_schedule_term_global(void) for (i = 0; i < ODP_CONFIG_SCHED_PRIOS; i++) { for (j = 0; j < QUEUES_PER_PRIO; j++) { + odp_queue_t pri_q = sched->pri_queue[i][j]; + + for (;;) { + odp_event_t ev = odp_queue_deq(pri_q); + odp_buffer_t desc_buf; + queue_desc_t *desc; + odp_queue_t queue; + + desc_buf = odp_buffer_from_event(ev); + if (desc_buf == ODP_BUFFER_INVALID) + break; + + desc = odp_buffer_addr(desc_buf); + queue = desc->queue; + /* Let deq_multi_destroy do the job */ + if (queue_is_destroyed(queue)) { + odp_queue_deq_multi(queue, + sched_local.ev, + MAX_DEQ); + } + } + if (odp_queue_destroy(sched->pri_queue[i][j])) { ODP_ERR("Sched term: Queue destroy fail.\n"); rc = -1;
ODP_QUEUE_TYPE_SCHED queues only get marked as destroyed, they need to be removed from the pri_queues of the linux-generic scheduler Signed-off-by: Ciprian Barbu <ciprian.barbu@linaro.org> --- v2: - removed #if 1 and trailing whitespaces platform/linux-generic/odp_schedule.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)