| Message ID | 1466993343-28996-2-git-send-email-bill.fischofer@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Mon, Jun 27, 2016 at 2:39 AM, Savolainen, Petri (Nokia - FI/Espoo) < petri.savolainen@nokia-bell-labs.com> wrote: > > > > -----Original Message----- > > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of > Bill > > Fischofer > > Sent: Monday, June 27, 2016 5:09 AM > > To: lng-odp@lists.linaro.org > > Subject: [lng-odp] [PATCH 2/2] validation: queue: use malloc to avoid > > artificial limits on max_queues > > > > odp_queue_capability() returns max_queues which may be more than 64K. > > Use malloc to allocate an array of queue handles to test the ability to > > create max_queues to avoid limiting the test to 64K queues. > > > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > > --- > > test/validation/queue/queue.c | 12 ++++++------ > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/test/validation/queue/queue.c > b/test/validation/queue/queue.c > > index c21897b..9af8c9c 100644 > > --- a/test/validation/queue/queue.c > > +++ b/test/validation/queue/queue.c > > @@ -11,7 +11,6 @@ > > #define MAX_BUFFER_QUEUE (8) > > #define MSG_POOL_SIZE (4 * 1024 * 1024) > > #define CONFIG_MAX_ITERATION (100) > > -#define MAX_QUEUES (64 * 1024) > > > > static int queue_context = 0xff; > > static odp_pool_t pool; > > @@ -55,7 +54,7 @@ void queue_test_capa(void) > > odp_queue_capability_t capa; > > odp_queue_param_t qparams; > > char name[ODP_QUEUE_NAME_LEN]; > > - odp_queue_t queue[MAX_QUEUES]; > > + odp_queue_t *queue; > > int num_queues, i; > > > > memset(&capa, 0, sizeof(odp_queue_capability_t)); > > @@ -71,10 +70,9 @@ void queue_test_capa(void) > > > > name[ODP_QUEUE_NAME_LEN - 1] = 0; > > > > - if (capa.max_queues > MAX_QUEUES) > > - num_queues = MAX_QUEUES; > > - else > > - num_queues = capa.max_queues; > > + num_queues = capa.max_queues; > > + queue = malloc(num_queues * sizeof(odp_queue_t)); > > Num_queues may be large. E.g. a malloc of 100M * 8 bytes may jam a system > which is light on DRAM but has a hard disk, etc. I think it's better to > limit the test to some number which will run sanely on any system. > If the purpose of the test is to verify that an application can allocate the max_queues number of queues, then this is the way to do that. You're suggesting that a platform would support a number of queues that cannot actually be allocated because is would have insufficient memory to support the maximum? That doesn't sound like a reasonable design. If there is to be a "cap" it should be in the implementation of odp_queue_capability(). > > Isn't there a less intrusive workaround for the coverity issue. > The Coverity issue is legitimate and changing from uint32_t to int is the simplest (and most correct) solution. In what way is that intrusive? > > -Petri > >
I've sent a v2 to address these concerns On Tue, Jun 28, 2016 at 2:15 AM, Savolainen, Petri (Nokia - FI/Espoo) < petri.savolainen@nokia-bell-labs.com> wrote: > > > From: Bill Fischofer [mailto:bill.fischofer@linaro.org] > Sent: Monday, June 27, 2016 5:20 PM > To: Savolainen, Petri (Nokia - FI/Espoo) < > petri.savolainen@nokia-bell-labs.com> > Cc: lng-odp@lists.linaro.org > Subject: Re: [lng-odp] [PATCH 2/2] validation: queue: use malloc to avoid > artificial limits on max_queues > > > > On Mon, Jun 27, 2016 at 2:39 AM, Savolainen, Petri (Nokia - FI/Espoo) < > petri.savolainen@nokia-bell-labs.com> wrote: > > > > -----Original Message----- > > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of > Bill > > Fischofer > > Sent: Monday, June 27, 2016 5:09 AM > > To: lng-odp@lists.linaro.org > > Subject: [lng-odp] [PATCH 2/2] validation: queue: use malloc to avoid > > artificial limits on max_queues > > > > odp_queue_capability() returns max_queues which may be more than 64K. > > Use malloc to allocate an array of queue handles to test the ability to > > create max_queues to avoid limiting the test to 64K queues. > > > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > > --- > > test/validation/queue/queue.c | 12 ++++++------ > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/test/validation/queue/queue.c > b/test/validation/queue/queue.c > > index c21897b..9af8c9c 100644 > > --- a/test/validation/queue/queue.c > > +++ b/test/validation/queue/queue.c > > @@ -11,7 +11,6 @@ > > #define MAX_BUFFER_QUEUE (8) > > #define MSG_POOL_SIZE (4 * 1024 * 1024) > > #define CONFIG_MAX_ITERATION (100) > > -#define MAX_QUEUES (64 * 1024) > > > > static int queue_context = 0xff; > > static odp_pool_t pool; > > @@ -55,7 +54,7 @@ void queue_test_capa(void) > > odp_queue_capability_t capa; > > odp_queue_param_t qparams; > > char name[ODP_QUEUE_NAME_LEN]; > > - odp_queue_t queue[MAX_QUEUES]; > > + odp_queue_t *queue; > > int num_queues, i; > > > > memset(&capa, 0, sizeof(odp_queue_capability_t)); > > @@ -71,10 +70,9 @@ void queue_test_capa(void) > > > > name[ODP_QUEUE_NAME_LEN - 1] = 0; > > > > - if (capa.max_queues > MAX_QUEUES) > > - num_queues = MAX_QUEUES; > > - else > > - num_queues = capa.max_queues; > > + num_queues = capa.max_queues; > > + queue = malloc(num_queues * sizeof(odp_queue_t)); > Num_queues may be large. E.g. a malloc of 100M * 8 bytes may jam a system > which is light on DRAM but has a hard disk, etc. I think it's better to > limit the test to some number which will run sanely on any system. > > If the purpose of the test is to verify that an application can allocate > the max_queues number of queues, then this is the way to do that. You're > suggesting that a platform would support a number of queues that cannot > actually be allocated because is would have insufficient memory to support > the maximum? That doesn't sound like a reasonable design. If there is to > be a "cap" it should be in the implementation of odp_queue_capability(). > > <<< Reply to a HTML mail ... >>> > Purpose of the test is to first of all check that .max_queues field is > defined and filled. Malloc may not be usable always for storing maximum > number of resource handles. May be the platform under test provides 16 GB > shm memory, but only 1GB general system memory backed up by a hard disk. A > malloc of 800MB could stuck the system totally (generally fast path > applications avoid using malloc), but a 800MB shm allocation would work > just fine. > > I think it would be better idea to add another test case which really > tries to create and use the max number of queues, groups, priorities, etc. > So that it's easier contain failure cases between the capability call not > supported vs. failure to use maximum (very large) number of resource X. > v2 falls back to a 64K test if the malloc() fails. > <<< ... reply ends >>> > > Isn't there a less intrusive workaround for the coverity issue. > > The Coverity issue is legitimate and changing from uint32_t to int is the > simplest (and most correct) solution. In what way is that intrusive? > > > <<< Reply to a HTML mail >>> > Did realize later on that patch 1/2 was fixing that already. But also > there int32_t, or uint32_t with check against i == 0, would be more > appropriate fix than using int. C spec guarantees that int is at least 16 > bits (not 32 bits) which in theory could cause problems when int is > actually 16 bits and number of queues is >32k. That's why the > capa.max_queues is uint32_t and not int. > v2 uses int32_t instead of int to ensure that we're dealing with 32-bit values (even though ODP doesn't support 16-bit systems so int is going to be 32 bits in all relevant cases). > > > -Petri > >
diff --git a/test/validation/queue/queue.c b/test/validation/queue/queue.c index c21897b..9af8c9c 100644 --- a/test/validation/queue/queue.c +++ b/test/validation/queue/queue.c @@ -11,7 +11,6 @@ #define MAX_BUFFER_QUEUE (8) #define MSG_POOL_SIZE (4 * 1024 * 1024) #define CONFIG_MAX_ITERATION (100) -#define MAX_QUEUES (64 * 1024) static int queue_context = 0xff; static odp_pool_t pool; @@ -55,7 +54,7 @@ void queue_test_capa(void) odp_queue_capability_t capa; odp_queue_param_t qparams; char name[ODP_QUEUE_NAME_LEN]; - odp_queue_t queue[MAX_QUEUES]; + odp_queue_t *queue; int num_queues, i; memset(&capa, 0, sizeof(odp_queue_capability_t)); @@ -71,10 +70,9 @@ void queue_test_capa(void) name[ODP_QUEUE_NAME_LEN - 1] = 0; - if (capa.max_queues > MAX_QUEUES) - num_queues = MAX_QUEUES; - else - num_queues = capa.max_queues; + num_queues = capa.max_queues; + queue = malloc(num_queues * sizeof(odp_queue_t)); + CU_ASSERT_FATAL(queue != NULL); odp_queue_param_init(&qparams); @@ -93,6 +91,8 @@ void queue_test_capa(void) for (i = 0; i < num_queues; i++) CU_ASSERT(odp_queue_destroy(queue[i]) == 0); + + free(queue); } void queue_test_mode(void)
odp_queue_capability() returns max_queues which may be more than 64K. Use malloc to allocate an array of queue handles to test the ability to create max_queues to avoid limiting the test to 64K queues. Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> --- test/validation/queue/queue.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)