diff mbox

[v5] queue: handle return value of odp_queue_enq_multi()

Message ID 1436202717-13883-1-git-send-email-zoltan.kiss@linaro.org
State New
Headers show

Commit Message

Zoltan Kiss July 6, 2015, 5:11 p.m. UTC 
Unsent packet has to be released. If the event type is obvious from the
context, use directly the relevant release functions, otherwise
odp_event(free).
Wider error handling is attempted, but this patch can't fix all the flaws
in the many calling functions of odp_queue_enq()
---
v5: fix array use in last hunk

 test/performance/odp_pktio_perf.c | 15 ++++++++++++++-
 test/performance/odp_scheduling.c | 10 ++++++++--
 test/validation/pktio/pktio.c     |  5 +++++
 test/validation/queue/queue.c     |  3 +++
 4 files changed, 30 insertions(+), 3 deletions(-)

Comments

Zoltan Kiss July 6, 2015, 5:18 p.m. UTC | #1 
On 06/07/15 18:11, Zoltan Kiss wrote:
> diff --git a/test/validation/queue/queue.c b/test/validation/queue/queue.c
> index 5f05e49..217c75e 100644
> --- a/test/validation/queue/queue.c
> +++ b/test/validation/queue/queue.c
> @@ -91,6 +91,9 @@ static void queue_test_sunnydays(void)
>   	 */
>   	ret = odp_queue_enq_multi(queue_id, enev, MAX_BUFFER_QUEUE);
>   	CU_ASSERT(MAX_BUFFER_QUEUE == ret);
> +	for (i = ret; i < MAX_BUFFER_QUEUE; i++)
> +		odp_event_free(enev[i]);
> +
>   	pev_tmp = deev;
>   	do {
>   		deq_ret  = odp_queue_deq_multi(queue_id, pev_tmp,

Thinking further, we still has to make sure ret is not smaller than 0, 
so the "if (ret < 0) ret = 0;" part would be necessary, isn't it?

Zoli
Maxim Uvarov July 6, 2015, 5:50 p.m. UTC | #2 
On 07/06/15 20:18, Zoltan Kiss wrote:
>
>
> On 06/07/15 18:11, Zoltan Kiss wrote:
>> diff --git a/test/validation/queue/queue.c 
>> b/test/validation/queue/queue.c
>> index 5f05e49..217c75e 100644
>> --- a/test/validation/queue/queue.c
>> +++ b/test/validation/queue/queue.c
>> @@ -91,6 +91,9 @@ static void queue_test_sunnydays(void)
>>        */
>>       ret = odp_queue_enq_multi(queue_id, enev, MAX_BUFFER_QUEUE);
>>       CU_ASSERT(MAX_BUFFER_QUEUE == ret);
>> +    for (i = ret; i < MAX_BUFFER_QUEUE; i++)
>> +        odp_event_free(enev[i]);
>> +
>>       pev_tmp = deev;
>>       do {
>>           deq_ret  = odp_queue_deq_multi(queue_id, pev_tmp,
>
> Thinking further, we still has to make sure ret is not smaller than 0, 
> so the "if (ret < 0) ret = 0;" part would be necessary, isn't it?
>
> Zoli
Does CU_ASSERT takes care about that?

Maxim.
Zoltan Kiss July 7, 2015, 1:07 p.m. UTC | #3 
On 06/07/15 18:50, Maxim Uvarov wrote:
> On 07/06/15 20:18, Zoltan Kiss wrote:
>>
>>
>> On 06/07/15 18:11, Zoltan Kiss wrote:
>>> diff --git a/test/validation/queue/queue.c
>>> b/test/validation/queue/queue.c
>>> index 5f05e49..217c75e 100644
>>> --- a/test/validation/queue/queue.c
>>> +++ b/test/validation/queue/queue.c
>>> @@ -91,6 +91,9 @@ static void queue_test_sunnydays(void)
>>>        */
>>>       ret = odp_queue_enq_multi(queue_id, enev, MAX_BUFFER_QUEUE);
>>>       CU_ASSERT(MAX_BUFFER_QUEUE == ret);
>>> +    for (i = ret; i < MAX_BUFFER_QUEUE; i++)
>>> +        odp_event_free(enev[i]);
>>> +
>>>       pev_tmp = deev;
>>>       do {
>>>           deq_ret  = odp_queue_deq_multi(queue_id, pev_tmp,
>>
>> Thinking further, we still has to make sure ret is not smaller than 0,
>> so the "if (ret < 0) ret = 0;" part would be necessary, isn't it?
>>
>> Zoli
> Does CU_ASSERT takes care about that?

I thought so too, but this one says only the *_FATAL versions do that:

http://cunit.sourceforge.net/doc/writing_tests.html

"Each assertion tests a single logical condition, and fails if the 
condition evaluates to FALSE. Upon failure, the test function continues 
unless the user chooses the 'xxx_FATAL' version of an assertion. In that 
case, the test function is aborted and returns immediately. FATAL 
versions of assertions should be used with caution! There is no 
opportunity for the test function to clean up after itself once a FATAL 
assertion fails. The normal suite cleanup function is not affected, 
however."

>
> Maxim.
Maxim Uvarov July 7, 2015, 2:33 p.m. UTC | #4 
On 07/07/15 16:07, Zoltan Kiss wrote:
>
>
> On 06/07/15 18:50, Maxim Uvarov wrote:
>> On 07/06/15 20:18, Zoltan Kiss wrote:
>>>
>>>
>>> On 06/07/15 18:11, Zoltan Kiss wrote:
>>>> diff --git a/test/validation/queue/queue.c
>>>> b/test/validation/queue/queue.c
>>>> index 5f05e49..217c75e 100644
>>>> --- a/test/validation/queue/queue.c
>>>> +++ b/test/validation/queue/queue.c
>>>> @@ -91,6 +91,9 @@ static void queue_test_sunnydays(void)
>>>>        */
>>>>       ret = odp_queue_enq_multi(queue_id, enev, MAX_BUFFER_QUEUE);
>>>>       CU_ASSERT(MAX_BUFFER_QUEUE == ret);
>>>> +    for (i = ret; i < MAX_BUFFER_QUEUE; i++)
>>>> +        odp_event_free(enev[i]);
>>>> +
>>>>       pev_tmp = deev;
>>>>       do {
>>>>           deq_ret  = odp_queue_deq_multi(queue_id, pev_tmp,
>>>
>>> Thinking further, we still has to make sure ret is not smaller than 0,
>>> so the "if (ret < 0) ret = 0;" part would be necessary, isn't it?
>>>
>>> Zoli
>> Does CU_ASSERT takes care about that?
>
> I thought so too, but this one says only the *_FATAL versions do that:
>
> http://cunit.sourceforge.net/doc/writing_tests.html
>
> "Each assertion tests a single logical condition, and fails if the 
> condition evaluates to FALSE. Upon failure, the test function 
> continues unless the user chooses the 'xxx_FATAL' version of an 
> assertion. In that case, the test function is aborted and returns 
> immediately. FATAL versions of assertions should be used with caution! 
> There is no opportunity for the test function to clean up after itself 
> once a FATAL assertion fails. The normal suite cleanup function is not 
> affected, however."

Ok. So that we can change that check to FATAL here, right? Or return 
from that function if ret != max_queue.

Maxim.
>
>>
>> Maxim.
Mike Holmes July 7, 2015, 2:48 p.m. UTC | #5 
When you use the cunit fatal MACROs, coverty does not understand that they
abort, and so this will generate some new false positives until we teach it
about the macros.

On 7 July 2015 at 10:33, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:

> On 07/07/15 16:07, Zoltan Kiss wrote:
>
>>
>>
>> On 06/07/15 18:50, Maxim Uvarov wrote:
>>
>>> On 07/06/15 20:18, Zoltan Kiss wrote:
>>>
>>>>
>>>>
>>>> On 06/07/15 18:11, Zoltan Kiss wrote:
>>>>
>>>>> diff --git a/test/validation/queue/queue.c
>>>>> b/test/validation/queue/queue.c
>>>>> index 5f05e49..217c75e 100644
>>>>> --- a/test/validation/queue/queue.c
>>>>> +++ b/test/validation/queue/queue.c
>>>>> @@ -91,6 +91,9 @@ static void queue_test_sunnydays(void)
>>>>>        */
>>>>>       ret = odp_queue_enq_multi(queue_id, enev, MAX_BUFFER_QUEUE);
>>>>>       CU_ASSERT(MAX_BUFFER_QUEUE == ret);
>>>>> +    for (i = ret; i < MAX_BUFFER_QUEUE; i++)
>>>>> +        odp_event_free(enev[i]);
>>>>> +
>>>>>       pev_tmp = deev;
>>>>>       do {
>>>>>           deq_ret  = odp_queue_deq_multi(queue_id, pev_tmp,
>>>>>
>>>>
>>>> Thinking further, we still has to make sure ret is not smaller than 0,
>>>> so the "if (ret < 0) ret = 0;" part would be necessary, isn't it?
>>>>
>>>> Zoli
>>>>
>>> Does CU_ASSERT takes care about that?
>>>
>>
>> I thought so too, but this one says only the *_FATAL versions do that:
>>
>> http://cunit.sourceforge.net/doc/writing_tests.html
>>
>> "Each assertion tests a single logical condition, and fails if the
>> condition evaluates to FALSE. Upon failure, the test function continues
>> unless the user chooses the 'xxx_FATAL' version of an assertion. In that
>> case, the test function is aborted and returns immediately. FATAL versions
>> of assertions should be used with caution! There is no opportunity for the
>> test function to clean up after itself once a FATAL assertion fails. The
>> normal suite cleanup function is not affected, however."
>>
>
> Ok. So that we can change that check to FATAL here, right? Or return from
> that function if ret != max_queue.
>
> Maxim.
>
>
>>
>>> Maxim.
>>>
>>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/lng-odp
>
Zoltan Kiss July 7, 2015, 5:19 p.m. UTC | #6 
On 07/07/15 15:33, Maxim Uvarov wrote:
> On 07/07/15 16:07, Zoltan Kiss wrote:
>>
>>
>> On 06/07/15 18:50, Maxim Uvarov wrote:
>>> On 07/06/15 20:18, Zoltan Kiss wrote:
>>>>
>>>>
>>>> On 06/07/15 18:11, Zoltan Kiss wrote:
>>>>> diff --git a/test/validation/queue/queue.c
>>>>> b/test/validation/queue/queue.c
>>>>> index 5f05e49..217c75e 100644
>>>>> --- a/test/validation/queue/queue.c
>>>>> +++ b/test/validation/queue/queue.c
>>>>> @@ -91,6 +91,9 @@ static void queue_test_sunnydays(void)
>>>>>        */
>>>>>       ret = odp_queue_enq_multi(queue_id, enev, MAX_BUFFER_QUEUE);
>>>>>       CU_ASSERT(MAX_BUFFER_QUEUE == ret);
>>>>> +    for (i = ret; i < MAX_BUFFER_QUEUE; i++)
>>>>> +        odp_event_free(enev[i]);
>>>>> +
>>>>>       pev_tmp = deev;
>>>>>       do {
>>>>>           deq_ret  = odp_queue_deq_multi(queue_id, pev_tmp,
>>>>
>>>> Thinking further, we still has to make sure ret is not smaller than 0,
>>>> so the "if (ret < 0) ret = 0;" part would be necessary, isn't it?
>>>>
>>>> Zoli
>>> Does CU_ASSERT takes care about that?
>>
>> I thought so too, but this one says only the *_FATAL versions do that:
>>
>> http://cunit.sourceforge.net/doc/writing_tests.html
>>
>> "Each assertion tests a single logical condition, and fails if the
>> condition evaluates to FALSE. Upon failure, the test function
>> continues unless the user chooses the 'xxx_FATAL' version of an
>> assertion. In that case, the test function is aborted and returns
>> immediately. FATAL versions of assertions should be used with caution!
>> There is no opportunity for the test function to clean up after itself
>> once a FATAL assertion fails. The normal suite cleanup function is not
>> affected, however."
>
> Ok. So that we can change that check to FATAL here, right? Or return
> from that function if ret != max_queue.
I would recommend to just check for ret < 0 and clean up locally. The 
FATAL macros doesn't let you clean up. And if you would return 
immediately, cleanup wouldn't happen as well.
>
> Maxim.
>>
>>>
>>> Maxim.
>
diff mbox

Patch

diff --git a/test/performance/odp_pktio_perf.c b/test/performance/odp_pktio_perf.c
index 52bddc4..6e07c9f 100644
--- a/test/performance/odp_pktio_perf.c
+++ b/test/performance/odp_pktio_perf.c
@@ -270,6 +270,9 @@  static int alloc_packets(odp_event_t *event_tbl, int num_pkts)
 static int send_packets(odp_queue_t outq,
 			odp_event_t *event_tbl, unsigned num_pkts)
 {
+	int ret;
+	unsigned cnt;
+
 	if (num_pkts == 0)
 		return 0;
 	else if (num_pkts == 1) {
@@ -281,7 +284,17 @@  static int send_packets(odp_queue_t outq,
 		}
 	}
 
-	return odp_queue_enq_multi(outq, event_tbl, num_pkts);
+	ret = odp_queue_enq_multi(outq, event_tbl, num_pkts);
+	if (ret == (signed)num_pkts)
+		return ret;
+
+	if (ret < 0)
+		ret = 0;
+	cnt = ret;
+	do
+		odp_event_free(event_tbl[cnt]);
+	while (++cnt < num_pkts);
+	return ret;
 }
 
 /*
diff --git a/test/performance/odp_scheduling.c b/test/performance/odp_scheduling.c
index 1283986..8b46eb2 100644
--- a/test/performance/odp_scheduling.c
+++ b/test/performance/odp_scheduling.c
@@ -535,9 +535,15 @@  static int test_schedule_multi(const char *str, int thr,
 		}
 
 		/* Assume we can enqueue all events */
-		if (odp_queue_enq_multi(queue, ev, MULTI_BUFS_MAX) !=
-		    MULTI_BUFS_MAX) {
+		j = odp_queue_enq_multi(queue, ev, MULTI_BUFS_MAX);
+		if (j != MULTI_BUFS_MAX) {
 			LOG_ERR("  [%i] Queue enqueue failed.\n", thr);
+			if (j < 0)
+				j = 0;
+			do
+				odp_event_free(ev[j]);
+			while (++j < MULTI_BUFS_MAX);
+
 			return -1;
 		}
 	}
diff --git a/test/validation/pktio/pktio.c b/test/validation/pktio/pktio.c
index 9d5af22..ac12759 100644
--- a/test/validation/pktio/pktio.c
+++ b/test/validation/pktio/pktio.c
@@ -389,6 +389,11 @@  static void pktio_txrx_multi(pktio_info_t *pktio_a, pktio_info_t *pktio_b,
 		ret = odp_queue_enq_multi(pktio_a->outq, tx_ev, num_pkts);
 		if (ret != num_pkts) {
 			CU_FAIL("failed to enqueue test packets");
+			if (ret < 0)
+				ret = 0;
+			do
+				odp_packet_free(tx_pkt[ret]);
+			while (++ret < num_pkts);
 			return;
 		}
 	}
diff --git a/test/validation/queue/queue.c b/test/validation/queue/queue.c
index 5f05e49..217c75e 100644
--- a/test/validation/queue/queue.c
+++ b/test/validation/queue/queue.c
@@ -91,6 +91,9 @@  static void queue_test_sunnydays(void)
 	 */
 	ret = odp_queue_enq_multi(queue_id, enev, MAX_BUFFER_QUEUE);
 	CU_ASSERT(MAX_BUFFER_QUEUE == ret);
+	for (i = ret; i < MAX_BUFFER_QUEUE; i++)
+		odp_event_free(enev[i]);
+
 	pev_tmp = deev;
 	do {
 		deq_ret  = odp_queue_deq_multi(queue_id, pev_tmp,