I've sent a v2 to address these concerns On Tue, Jun 28, 2016 at 2:15 AM, Savolainen, Petri (Nokia - FI/Espoo) < [email protected]> wrote:
> > > From: Bill Fischofer [mailto:[email protected]] > Sent: Monday, June 27, 2016 5:20 PM > To: Savolainen, Petri (Nokia - FI/Espoo) < > [email protected]> > Cc: [email protected] > 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) < > [email protected]> wrote: > > > > -----Original Message----- > > From: lng-odp [mailto:[email protected]] On Behalf Of > Bill > > Fischofer > > Sent: Monday, June 27, 2016 5:09 AM > > To: [email protected] > > 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 <[email protected]> > > --- > > 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 > > _______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
