| Message ID | 1490960803-6549-2-git-send-email-petri.savolainen@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [API-NEXT,v2,1/2] api: queue: added queue size param | expand |
This patch is incomplete. Since you're adding a max_size, we need to add a test for that. If an implementation says the max size of a queue is n (n > 0) then we have to attempt to allocate a queue of that size and verify that trying to add an n+1st element to it will fail. Otherwise, we shouldn't have a max_size at all (which we had discussed) since applications only care about minimums. If the application's requested minimum exceeds any implementation-defined max_size then the queue_create() call will simply fail. On Fri, Mar 31, 2017 at 6:46 AM, Petri Savolainen <petri.savolainen@linaro.org> wrote: > Updated implementation and test with type specific number of > queues. > > Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> > --- > platform/linux-generic/odp_queue.c | 2 ++ > test/common_plat/validation/api/queue/queue.c | 43 ++++++++++++++++----------- > 2 files changed, 28 insertions(+), 17 deletions(-) > > diff --git a/platform/linux-generic/odp_queue.c b/platform/linux-generic/odp_queue.c > index fcf4bf5..1114c95 100644 > --- a/platform/linux-generic/odp_queue.c > +++ b/platform/linux-generic/odp_queue.c > @@ -175,6 +175,8 @@ int odp_queue_capability(odp_queue_capability_t *capa) > capa->max_ordered_locks = sched_fn->max_ordered_locks(); > capa->max_sched_groups = sched_fn->num_grps(); > capa->sched_prios = odp_schedule_num_prio(); > + capa->plain.max_num = capa->max_queues; > + capa->sched.max_num = capa->max_queues; Shouldn't odp_queue_create() also be checking that queue_params.size > 0? > > return 0; > } > diff --git a/test/common_plat/validation/api/queue/queue.c b/test/common_plat/validation/api/queue/queue.c > index 1f7913a..8a874a4 100644 > --- a/test/common_plat/validation/api/queue/queue.c > +++ b/test/common_plat/validation/api/queue/queue.c > @@ -56,7 +56,7 @@ void queue_test_capa(void) > odp_queue_param_t qparams; > char name[ODP_QUEUE_NAME_LEN]; > odp_queue_t queue[MAX_QUEUES]; > - uint32_t num_queues, i; > + uint32_t num_queues, i, j; > > memset(&capa, 0, sizeof(odp_queue_capability_t)); > CU_ASSERT(odp_queue_capability(&capa) == 0); > @@ -65,34 +65,43 @@ void queue_test_capa(void) > CU_ASSERT(capa.max_ordered_locks != 0); > CU_ASSERT(capa.max_sched_groups != 0); > CU_ASSERT(capa.sched_prios != 0); > + CU_ASSERT(capa.plain.max_num != 0); > + CU_ASSERT(capa.sched.max_num != 0); > > for (i = 0; i < ODP_QUEUE_NAME_LEN; i++) > name[i] = 'A' + (i % 26); > > name[ODP_QUEUE_NAME_LEN - 1] = 0; > > - if (capa.max_queues > MAX_QUEUES) > - num_queues = MAX_QUEUES; > - else > - num_queues = capa.max_queues; > - > odp_queue_param_init(&qparams); > > - for (i = 0; i < num_queues; i++) { > - generate_name(name, i); > - queue[i] = odp_queue_create(name, &qparams); > + for (j = 0; j < 2; j++) { > + if (j == 0) { > + num_queues = capa.plain.max_num; > + } else { > + num_queues = capa.sched.max_num; > + qparams.type = ODP_QUEUE_TYPE_SCHED; > + } > + > + if (num_queues > MAX_QUEUES) > + num_queues = MAX_QUEUES; > + > + for (i = 0; i < num_queues; i++) { This code is incorrect since if either max_num is 0 (indicating unbounded numbers of queues supported) num_queues will be set to zero so this loop will not create any queues at all. > + generate_name(name, i); > + queue[i] = odp_queue_create(name, &qparams); > > - if (queue[i] == ODP_QUEUE_INVALID) { > - CU_FAIL("Queue create failed"); > - num_queues = i; > - break; > + if (queue[i] == ODP_QUEUE_INVALID) { > + CU_FAIL("Queue create failed"); > + num_queues = i; > + break; > + } > + > + CU_ASSERT(odp_queue_lookup(name) != ODP_QUEUE_INVALID); > } > > - CU_ASSERT(odp_queue_lookup(name) != ODP_QUEUE_INVALID); > + for (i = 0; i < num_queues; i++) > + CU_ASSERT(odp_queue_destroy(queue[i]) == 0); > } > - > - for (i = 0; i < num_queues; i++) > - CU_ASSERT(odp_queue_destroy(queue[i]) == 0); > } > > void queue_test_mode(void) > -- > 2.8.1 >
> -----Original Message----- > From: Bill Fischofer [mailto:bill.fischofer@linaro.org] > Sent: Friday, March 31, 2017 5:59 PM > To: Petri Savolainen <petri.savolainen@linaro.org> > Cc: lng-odp-forward <lng-odp@lists.linaro.org> > Subject: Re: [lng-odp] [API-NEXT PATCH v2 2/2] validation: queue: test > queue max_num per type > > This patch is incomplete. Since you're adding a max_size, we need to > add a test for that. If an implementation says the max size of a queue > is n (n > 0) then we have to attempt to allocate a queue of that size > and verify that trying to add an n+1st element to it will fail. > Otherwise, we shouldn't have a max_size at all (which we had > discussed) since applications only care about minimums. If the > application's requested minimum exceeds any implementation-defined > max_size then the queue_create() call will simply fail. This patch does not claim to test max_size, but max_num. May be ARM adds max_size test as their scheduler has max_size != 0. Current queue/scheduler is not interesting to test against since max_size is 0 == size is not even checked in create since all size values are OK. In general, if there's a limit in fast path (queue size) and application breaks the limit, it's undefined what happens. Max_size is capability == the maximum that application can ask as 'size'. It's undefined what happens (crash ?) if application tries to do 'size + 1' enqueues without a dequeue in between. Create fails if, size > max_size. > > diff --git a/platform/linux-generic/odp_queue.c b/platform/linux- > generic/odp_queue.c > > index fcf4bf5..1114c95 100644 > > --- a/platform/linux-generic/odp_queue.c > > +++ b/platform/linux-generic/odp_queue.c > > @@ -175,6 +175,8 @@ int odp_queue_capability(odp_queue_capability_t > *capa) > > capa->max_ordered_locks = sched_fn->max_ordered_locks(); > > capa->max_sched_groups = sched_fn->num_grps(); > > capa->sched_prios = odp_schedule_num_prio(); > > + capa->plain.max_num = capa->max_queues; > > + capa->sched.max_num = capa->max_queues; > > Shouldn't odp_queue_create() also be checking that queue_params.size > 0? Queue is linked list ==> size is unlimited ==> we don't care about size. Size == 0 is the default size, which is also OK. > > > > > return 0; > > } > > diff --git a/test/common_plat/validation/api/queue/queue.c > b/test/common_plat/validation/api/queue/queue.c > > index 1f7913a..8a874a4 100644 > > --- a/test/common_plat/validation/api/queue/queue.c > > +++ b/test/common_plat/validation/api/queue/queue.c > > @@ -56,7 +56,7 @@ void queue_test_capa(void) > > odp_queue_param_t qparams; > > char name[ODP_QUEUE_NAME_LEN]; > > odp_queue_t queue[MAX_QUEUES]; > > - uint32_t num_queues, i; > > + uint32_t num_queues, i, j; > > > > memset(&capa, 0, sizeof(odp_queue_capability_t)); > > CU_ASSERT(odp_queue_capability(&capa) == 0); > > @@ -65,34 +65,43 @@ void queue_test_capa(void) > > CU_ASSERT(capa.max_ordered_locks != 0); > > CU_ASSERT(capa.max_sched_groups != 0); > > CU_ASSERT(capa.sched_prios != 0); > > + CU_ASSERT(capa.plain.max_num != 0); > > + CU_ASSERT(capa.sched.max_num != 0); > > > > for (i = 0; i < ODP_QUEUE_NAME_LEN; i++) > > name[i] = 'A' + (i % 26); > > > > name[ODP_QUEUE_NAME_LEN - 1] = 0; > > > > - if (capa.max_queues > MAX_QUEUES) > > - num_queues = MAX_QUEUES; > > - else > > - num_queues = capa.max_queues; > > - > > odp_queue_param_init(&qparams); > > > > - for (i = 0; i < num_queues; i++) { > > - generate_name(name, i); > > - queue[i] = odp_queue_create(name, &qparams); > > + for (j = 0; j < 2; j++) { > > + if (j == 0) { > > + num_queues = capa.plain.max_num; > > + } else { > > + num_queues = capa.sched.max_num; > > + qparams.type = ODP_QUEUE_TYPE_SCHED; > > + } > > + > > + if (num_queues > MAX_QUEUES) > > + num_queues = MAX_QUEUES; > > + > > + for (i = 0; i < num_queues; i++) { > > This code is incorrect since if either max_num is 0 (indicating > unbounded numbers of queues supported) num_queues will be set to zero > so this loop will not create any queues at all. Zero is *not* specified as unlimited. There are asserts above for zero. -Petri
diff --git a/platform/linux-generic/odp_queue.c b/platform/linux-generic/odp_queue.c index fcf4bf5..1114c95 100644 --- a/platform/linux-generic/odp_queue.c +++ b/platform/linux-generic/odp_queue.c @@ -175,6 +175,8 @@ int odp_queue_capability(odp_queue_capability_t *capa) capa->max_ordered_locks = sched_fn->max_ordered_locks(); capa->max_sched_groups = sched_fn->num_grps(); capa->sched_prios = odp_schedule_num_prio(); + capa->plain.max_num = capa->max_queues; + capa->sched.max_num = capa->max_queues; return 0; } diff --git a/test/common_plat/validation/api/queue/queue.c b/test/common_plat/validation/api/queue/queue.c index 1f7913a..8a874a4 100644 --- a/test/common_plat/validation/api/queue/queue.c +++ b/test/common_plat/validation/api/queue/queue.c @@ -56,7 +56,7 @@ void queue_test_capa(void) odp_queue_param_t qparams; char name[ODP_QUEUE_NAME_LEN]; odp_queue_t queue[MAX_QUEUES]; - uint32_t num_queues, i; + uint32_t num_queues, i, j; memset(&capa, 0, sizeof(odp_queue_capability_t)); CU_ASSERT(odp_queue_capability(&capa) == 0); @@ -65,34 +65,43 @@ void queue_test_capa(void) CU_ASSERT(capa.max_ordered_locks != 0); CU_ASSERT(capa.max_sched_groups != 0); CU_ASSERT(capa.sched_prios != 0); + CU_ASSERT(capa.plain.max_num != 0); + CU_ASSERT(capa.sched.max_num != 0); for (i = 0; i < ODP_QUEUE_NAME_LEN; i++) name[i] = 'A' + (i % 26); name[ODP_QUEUE_NAME_LEN - 1] = 0; - if (capa.max_queues > MAX_QUEUES) - num_queues = MAX_QUEUES; - else - num_queues = capa.max_queues; - odp_queue_param_init(&qparams); - for (i = 0; i < num_queues; i++) { - generate_name(name, i); - queue[i] = odp_queue_create(name, &qparams); + for (j = 0; j < 2; j++) { + if (j == 0) { + num_queues = capa.plain.max_num; + } else { + num_queues = capa.sched.max_num; + qparams.type = ODP_QUEUE_TYPE_SCHED; + } + + if (num_queues > MAX_QUEUES) + num_queues = MAX_QUEUES; + + for (i = 0; i < num_queues; i++) { + generate_name(name, i); + queue[i] = odp_queue_create(name, &qparams); - if (queue[i] == ODP_QUEUE_INVALID) { - CU_FAIL("Queue create failed"); - num_queues = i; - break; + if (queue[i] == ODP_QUEUE_INVALID) { + CU_FAIL("Queue create failed"); + num_queues = i; + break; + } + + CU_ASSERT(odp_queue_lookup(name) != ODP_QUEUE_INVALID); } - CU_ASSERT(odp_queue_lookup(name) != ODP_QUEUE_INVALID); + for (i = 0; i < num_queues; i++) + CU_ASSERT(odp_queue_destroy(queue[i]) == 0); } - - for (i = 0; i < num_queues; i++) - CU_ASSERT(odp_queue_destroy(queue[i]) == 0); } void queue_test_mode(void)
Updated implementation and test with type specific number of queues. Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> --- platform/linux-generic/odp_queue.c | 2 ++ test/common_plat/validation/api/queue/queue.c | 43 ++++++++++++++++----------- 2 files changed, 28 insertions(+), 17 deletions(-) -- 2.8.1