[API-NEXT,5/5] validation: implement pktio statistics counters

Message ID 1447068113-3733-6-git-send-email-maxim.uvarov@linaro.org
State New
Headers show

Commit Message

Maxim Uvarov Nov. 9, 2015, 11:21 a.m.
Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
---
 test/validation/pktio/pktio.c | 130 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 130 insertions(+)

Comments

Stuart Haslam Nov. 10, 2015, 6:30 p.m. | #1
On Mon, Nov 09, 2015 at 02:21:53PM +0300, Maxim Uvarov wrote:
> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> ---
>  test/validation/pktio/pktio.c | 130 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 130 insertions(+)
> 
> diff --git a/test/validation/pktio/pktio.c b/test/validation/pktio/pktio.c
> index 6320b77..c591c94 100644
> --- a/test/validation/pktio/pktio.c
> +++ b/test/validation/pktio/pktio.c
> @@ -690,6 +690,135 @@ void pktio_test_inq(void)
>  	CU_ASSERT(odp_pktio_close(pktio) == 0);
>  }
>  
> +static void _print_pktio_stats(odp_pktio_stats_t *s, const char *name)
> +{
> +	printf("\n%s:\n"
> +		"  in_octets %"PRIu64"\n"
> +		"  in_ucast_pkts %"PRIu64"\n"
> +		"  in_discards %"PRIu64"\n"
> +		"  in_errors %"PRIu64"\n"
> +		"  in_unknown_protos %"PRIu64"\n"
> +		"  out_octets %"PRIu64"\n"
> +		"  out_ucast_pkts %"PRIu64"\n"
> +		"  out_discards %"PRIu64"\n"
> +		"  out_errors %"PRIu64"\n",
> +		name,
> +		s->in_octets,
> +		s->in_ucast_pkts,
> +		s->in_discards,
> +		s->in_errors,
> +		s->in_unknown_protos,
> +		s->out_octets,
> +		s->out_ucast_pkts,
> +		s->out_discards,
> +		s->out_errors);
> +}
> +
> +static void pktio_test_statistics_counters(void)
> +{
> +	odp_pktio_t pktio[MAX_NUM_IFACES];
> +	odp_packet_t pkt;
> +	odp_event_t tx_ev[1000];
> +	odp_event_t ev;
> +	int i, pkts, ret, alloc = 0;
> +	odp_queue_t outq;
> +	uint64_t wait = odp_schedule_wait_time(ODP_TIME_MSEC);
> +	odp_pktio_stats_t stats[2];
> +
> +	for (i = 0; i < num_ifaces; i++) {
> +		pktio[i] = create_pktio(i, ODP_PKTIN_MODE_SCHED);
> +		CU_ASSERT_FATAL(pktio[i] != ODP_PKTIO_INVALID);
> +		create_inq(pktio[i],  ODP_QUEUE_TYPE_SCHED);
> +	}
> +
> +	outq = odp_pktio_outq_getdef(pktio[0]);
> +
> +	ret = odp_pktio_start(pktio[0]);
> +	CU_ASSERT(ret == 0);
> +	if (num_ifaces > 1) {
> +		ret = odp_pktio_start(pktio[1]);
> +		CU_ASSERT(ret == 0);
> +	}
> +
> +	/* flush packets with magic number in pipes */
> +	for (i = 0; i < 1000; i++) {
> +		ev = odp_schedule(NULL, wait);
> +		if (ev != ODP_EVENT_INVALID)
> +			odp_event_free(ev);
> +	}
> +
> +	/* alloc */
> +	for (alloc = 0; alloc < 1000; alloc++) {
> +		pkt = odp_packet_alloc(default_pkt_pool, packet_len);
> +		if (pkt == ODP_PACKET_INVALID)
> +			break;
> +		pktio_init_packet(pkt);
> +		tx_ev[alloc] = odp_packet_to_event(pkt);
> +	}
> +
> +	ret = odp_pktio_stats_reset(pktio[0]);
> +	CU_ASSERT(ret == 0);
> +	if (num_ifaces > 1) {
> +		ret = odp_pktio_stats_reset(pktio[1]);
> +		CU_ASSERT(ret == 0);
> +	}
> +
> +	/* send */
> +	for (pkts = 0; pkts != alloc; ) {
> +		ret = odp_queue_enq_multi(outq, &tx_ev[pkts], alloc - pkts);
> +		if (ret < 0) {
> +			CU_FAIL("unable to enqueue packet\n");
> +			break;
> +		}
> +		pkts += ret;
> +	}
> +
> +	/* get */
> +	for (i = 0, pkts = 0; i < 1000; i++) {
> +		ev = odp_schedule(NULL, wait);
> +		if (ev != ODP_EVENT_INVALID) {
> +			if (odp_event_type(ev) == ODP_EVENT_PACKET) {
> +				pkt = odp_packet_from_event(ev);
> +				if (pktio_pkt_seq(pkt) != TEST_SEQ_INVALID)
> +					pkts++;
> +			}
> +			odp_event_free(ev);
> +		}
> +	}
> +
> +	ret = odp_pktio_stats(pktio[0], &stats[0]);
> +	CU_ASSERT(ret == 0);
> +	_print_pktio_stats(&stats[0], iface_name[0]);

I don't like tests printing to stdout, it clutters the output and makes
it harder to see what's passed/failed. If all of the asserts pass then
it doesn't matter what the actual values are, that's a detail of the
test, if something fails though it's reasonable to dump to stderr.

> +
> +	if (num_ifaces > 1) {
> +		ret = odp_pktio_stats(pktio[1], &stats[1]);
> +		CU_ASSERT(ret == 0);
> +		_print_pktio_stats(&stats[1], iface_name[1]);
> +
> +		CU_ASSERT(stats[1].in_ucast_pkts >= (uint64_t)pkts);
> +		CU_ASSERT(stats[0].out_ucast_pkts == stats[1].in_ucast_pkts);
> +		CU_ASSERT(stats[0].out_octets == stats[1].in_octets);
> +		CU_ASSERT(stats[0].out_octets >=
> +			  (stats[0].out_ucast_pkts * (uint64_t)pkts));

What's this check for?.. seems to assert that average pkt size >= 2
octets.

> +	} else {

Why not check out_octets and out_ucast_pkts in this case?

The logic would be simpler with something like:

if (num_ifaces == 1)
	pktio[1] = pktio[0];

stats[0] = odp_pktio_stats(pktio[0], &stats[0]);
stats[1] = odp_pktio_stats(pktio[1], &stats[1]);

Then all the asserts would be the same regardless of whether you have 1
or 2 interfaces.

> +		CU_ASSERT(stats[0].in_ucast_pkts == (uint64_t)pkts);
> +		CU_ASSERT(stats[0].in_octets ==
> +			  (PKT_LEN_NORMAL * (uint64_t)pkts));
> +	}
> +
> +	CU_ASSERT(pkts == alloc);
> +	CU_ASSERT((stats[0].in_discards |
> +		stats[0].in_errors |
> +		stats[0].in_unknown_protos |
> +		stats[0].out_discards |
> +		stats[0].out_errors) == 0);

It would be better to have separate asserts for each of these so you
can tell from the logs which failed.

> +
> +	for (i = 0; i < num_ifaces; i++) {
> +		destroy_inq(pktio[i]);
> +		CU_ASSERT(odp_pktio_close(pktio[i]) == 0);
> +	}
> +}
> +
>  static void pktio_test_start_stop(void)
>  {
>  	odp_pktio_t pktio[MAX_NUM_IFACES];
> @@ -1057,6 +1186,7 @@ odp_testinfo_t pktio_suite_unsegmented[] = {
>  	ODP_TEST_INFO(pktio_test_mac),
>  	ODP_TEST_INFO(pktio_test_inq_remdef),
>  	ODP_TEST_INFO(pktio_test_start_stop),
> +	ODP_TEST_INFO(pktio_test_statistics_counters),
>  	ODP_TEST_INFO_NULL
>  };
>  
> -- 
> 1.9.1
>
Maxim Uvarov Nov. 12, 2015, 2:30 p.m. | #2
On 11/10/2015 21:30, Stuart Haslam wrote:
> On Mon, Nov 09, 2015 at 02:21:53PM +0300, Maxim Uvarov wrote:
>> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
>> ---
>>   test/validation/pktio/pktio.c | 130 ++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 130 insertions(+)
>>
>> diff --git a/test/validation/pktio/pktio.c b/test/validation/pktio/pktio.c
>> index 6320b77..c591c94 100644
>> --- a/test/validation/pktio/pktio.c
>> +++ b/test/validation/pktio/pktio.c
>> @@ -690,6 +690,135 @@ void pktio_test_inq(void)
>>   	CU_ASSERT(odp_pktio_close(pktio) == 0);
>>   }
>>   
>> +static void _print_pktio_stats(odp_pktio_stats_t *s, const char *name)
>> +{
>> +	printf("\n%s:\n"
>> +		"  in_octets %"PRIu64"\n"
>> +		"  in_ucast_pkts %"PRIu64"\n"
>> +		"  in_discards %"PRIu64"\n"
>> +		"  in_errors %"PRIu64"\n"
>> +		"  in_unknown_protos %"PRIu64"\n"
>> +		"  out_octets %"PRIu64"\n"
>> +		"  out_ucast_pkts %"PRIu64"\n"
>> +		"  out_discards %"PRIu64"\n"
>> +		"  out_errors %"PRIu64"\n",
>> +		name,
>> +		s->in_octets,
>> +		s->in_ucast_pkts,
>> +		s->in_discards,
>> +		s->in_errors,
>> +		s->in_unknown_protos,
>> +		s->out_octets,
>> +		s->out_ucast_pkts,
>> +		s->out_discards,
>> +		s->out_errors);
>> +}
>> +
>> +static void pktio_test_statistics_counters(void)
>> +{
>> +	odp_pktio_t pktio[MAX_NUM_IFACES];
>> +	odp_packet_t pkt;
>> +	odp_event_t tx_ev[1000];
>> +	odp_event_t ev;
>> +	int i, pkts, ret, alloc = 0;
>> +	odp_queue_t outq;
>> +	uint64_t wait = odp_schedule_wait_time(ODP_TIME_MSEC);
>> +	odp_pktio_stats_t stats[2];
>> +
>> +	for (i = 0; i < num_ifaces; i++) {
>> +		pktio[i] = create_pktio(i, ODP_PKTIN_MODE_SCHED);
>> +		CU_ASSERT_FATAL(pktio[i] != ODP_PKTIO_INVALID);
>> +		create_inq(pktio[i],  ODP_QUEUE_TYPE_SCHED);
>> +	}
>> +
>> +	outq = odp_pktio_outq_getdef(pktio[0]);
>> +
>> +	ret = odp_pktio_start(pktio[0]);
>> +	CU_ASSERT(ret == 0);
>> +	if (num_ifaces > 1) {
>> +		ret = odp_pktio_start(pktio[1]);
>> +		CU_ASSERT(ret == 0);
>> +	}
>> +
>> +	/* flush packets with magic number in pipes */
>> +	for (i = 0; i < 1000; i++) {
>> +		ev = odp_schedule(NULL, wait);
>> +		if (ev != ODP_EVENT_INVALID)
>> +			odp_event_free(ev);
>> +	}
>> +
>> +	/* alloc */
>> +	for (alloc = 0; alloc < 1000; alloc++) {
>> +		pkt = odp_packet_alloc(default_pkt_pool, packet_len);
>> +		if (pkt == ODP_PACKET_INVALID)
>> +			break;
>> +		pktio_init_packet(pkt);
>> +		tx_ev[alloc] = odp_packet_to_event(pkt);
>> +	}
>> +
>> +	ret = odp_pktio_stats_reset(pktio[0]);
>> +	CU_ASSERT(ret == 0);
>> +	if (num_ifaces > 1) {
>> +		ret = odp_pktio_stats_reset(pktio[1]);
>> +		CU_ASSERT(ret == 0);
>> +	}
>> +
>> +	/* send */
>> +	for (pkts = 0; pkts != alloc; ) {
>> +		ret = odp_queue_enq_multi(outq, &tx_ev[pkts], alloc - pkts);
>> +		if (ret < 0) {
>> +			CU_FAIL("unable to enqueue packet\n");
>> +			break;
>> +		}
>> +		pkts += ret;
>> +	}
>> +
>> +	/* get */
>> +	for (i = 0, pkts = 0; i < 1000; i++) {
>> +		ev = odp_schedule(NULL, wait);
>> +		if (ev != ODP_EVENT_INVALID) {
>> +			if (odp_event_type(ev) == ODP_EVENT_PACKET) {
>> +				pkt = odp_packet_from_event(ev);
>> +				if (pktio_pkt_seq(pkt) != TEST_SEQ_INVALID)
>> +					pkts++;
>> +			}
>> +			odp_event_free(ev);
>> +		}
>> +	}
>> +
>> +	ret = odp_pktio_stats(pktio[0], &stats[0]);
>> +	CU_ASSERT(ret == 0);
>> +	_print_pktio_stats(&stats[0], iface_name[0]);
> I don't like tests printing to stdout, it clutters the output and makes
> it harder to see what's passed/failed. If all of the asserts pass then
> it doesn't matter what the actual values are, that's a detail of the
> test, if something fails though it's reasonable to dump to stderr.

ok, I can hide to error case. It was easy for me to debug when I see 
numbers.

>
>> +
>> +	if (num_ifaces > 1) {
>> +		ret = odp_pktio_stats(pktio[1], &stats[1]);
>> +		CU_ASSERT(ret == 0);
>> +		_print_pktio_stats(&stats[1], iface_name[1]);
>> +
>> +		CU_ASSERT(stats[1].in_ucast_pkts >= (uint64_t)pkts);
>> +		CU_ASSERT(stats[0].out_ucast_pkts == stats[1].in_ucast_pkts);
>> +		CU_ASSERT(stats[0].out_octets == stats[1].in_octets);
>> +		CU_ASSERT(stats[0].out_octets >=
>> +			  (stats[0].out_ucast_pkts * (uint64_t)pkts));
> What's this check for?.. seems to assert that average pkt size >= 2
> octets.

Opps. I planned to write:

+		CU_ASSERT(stats[0].out_octets >=
+			  (pkt_size * (uint64_t)pkts));


For loop it's ==. But for other virtual dev some packets may be on 
network (like dhcp),
so it will be >=.


>> +	} else {
> Why not check out_octets and out_ucast_pkts in this case?
>
> The logic would be simpler with something like:
>
> if (num_ifaces == 1)
> 	pktio[1] = pktio[0];
>
> stats[0] = odp_pktio_stats(pktio[0], &stats[0]);
> stats[1] = odp_pktio_stats(pktio[1], &stats[1]);
>
> Then all the asserts would be the same regardless of whether you have 1
> or 2 interfaces.
yes,  it's the same think. Calling 2 times odp_pktio_stats() for the 
same device
but different index in array a little bit confusing (complicates code 
understanding).
At least for me.
>> +		CU_ASSERT(stats[0].in_ucast_pkts == (uint64_t)pkts);
>> +		CU_ASSERT(stats[0].in_octets ==
>> +			  (PKT_LEN_NORMAL * (uint64_t)pkts));
>> +	}
>> +
>> +	CU_ASSERT(pkts == alloc);
>> +	CU_ASSERT((stats[0].in_discards |
>> +		stats[0].in_errors |
>> +		stats[0].in_unknown_protos |
>> +		stats[0].out_discards |
>> +		stats[0].out_errors) == 0);
> It would be better to have separate asserts for each of these so you
> can tell from the logs which failed.
Ok. My plan was that current counters are not well tested. When we will 
add tests
for them then we can exclude from that group. But I can do test case per 
counter,
it's not a problem.

>
>> +
>> +	for (i = 0; i < num_ifaces; i++) {
>> +		destroy_inq(pktio[i]);
>> +		CU_ASSERT(odp_pktio_close(pktio[i]) == 0);
>> +	}
>> +}
>> +
>>   static void pktio_test_start_stop(void)
>>   {
>>   	odp_pktio_t pktio[MAX_NUM_IFACES];
>> @@ -1057,6 +1186,7 @@ odp_testinfo_t pktio_suite_unsegmented[] = {
>>   	ODP_TEST_INFO(pktio_test_mac),
>>   	ODP_TEST_INFO(pktio_test_inq_remdef),
>>   	ODP_TEST_INFO(pktio_test_start_stop),
>> +	ODP_TEST_INFO(pktio_test_statistics_counters),
>>   	ODP_TEST_INFO_NULL
>>   };
>>   
>> -- 
>> 1.9.1
>>
Stuart Haslam Nov. 13, 2015, 1:57 p.m. | #3
On Thu, Nov 12, 2015 at 05:30:47PM +0300, Maxim Uvarov wrote:
> On 11/10/2015 21:30, Stuart Haslam wrote:
> >On Mon, Nov 09, 2015 at 02:21:53PM +0300, Maxim Uvarov wrote:
> >>Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> >>---
> >>  test/validation/pktio/pktio.c | 130 ++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 130 insertions(+)
> >>
> >>diff --git a/test/validation/pktio/pktio.c b/test/validation/pktio/pktio.c
> >>index 6320b77..c591c94 100644
> >>--- a/test/validation/pktio/pktio.c
> >>+++ b/test/validation/pktio/pktio.c
> >>@@ -690,6 +690,135 @@ void pktio_test_inq(void)
> >>  	CU_ASSERT(odp_pktio_close(pktio) == 0);
> >>  }
> >>+static void _print_pktio_stats(odp_pktio_stats_t *s, const char *name)
> >>+{
> >>+	printf("\n%s:\n"
> >>+		"  in_octets %"PRIu64"\n"
> >>+		"  in_ucast_pkts %"PRIu64"\n"
> >>+		"  in_discards %"PRIu64"\n"
> >>+		"  in_errors %"PRIu64"\n"
> >>+		"  in_unknown_protos %"PRIu64"\n"
> >>+		"  out_octets %"PRIu64"\n"
> >>+		"  out_ucast_pkts %"PRIu64"\n"
> >>+		"  out_discards %"PRIu64"\n"
> >>+		"  out_errors %"PRIu64"\n",
> >>+		name,
> >>+		s->in_octets,
> >>+		s->in_ucast_pkts,
> >>+		s->in_discards,
> >>+		s->in_errors,
> >>+		s->in_unknown_protos,
> >>+		s->out_octets,
> >>+		s->out_ucast_pkts,
> >>+		s->out_discards,
> >>+		s->out_errors);
> >>+}
> >>+
> >>+static void pktio_test_statistics_counters(void)
> >>+{
> >>+	odp_pktio_t pktio[MAX_NUM_IFACES];
> >>+	odp_packet_t pkt;
> >>+	odp_event_t tx_ev[1000];
> >>+	odp_event_t ev;
> >>+	int i, pkts, ret, alloc = 0;
> >>+	odp_queue_t outq;
> >>+	uint64_t wait = odp_schedule_wait_time(ODP_TIME_MSEC);
> >>+	odp_pktio_stats_t stats[2];
> >>+
> >>+	for (i = 0; i < num_ifaces; i++) {
> >>+		pktio[i] = create_pktio(i, ODP_PKTIN_MODE_SCHED);
> >>+		CU_ASSERT_FATAL(pktio[i] != ODP_PKTIO_INVALID);
> >>+		create_inq(pktio[i],  ODP_QUEUE_TYPE_SCHED);
> >>+	}
> >>+
> >>+	outq = odp_pktio_outq_getdef(pktio[0]);
> >>+
> >>+	ret = odp_pktio_start(pktio[0]);
> >>+	CU_ASSERT(ret == 0);
> >>+	if (num_ifaces > 1) {
> >>+		ret = odp_pktio_start(pktio[1]);
> >>+		CU_ASSERT(ret == 0);
> >>+	}
> >>+
> >>+	/* flush packets with magic number in pipes */
> >>+	for (i = 0; i < 1000; i++) {
> >>+		ev = odp_schedule(NULL, wait);
> >>+		if (ev != ODP_EVENT_INVALID)
> >>+			odp_event_free(ev);
> >>+	}
> >>+
> >>+	/* alloc */
> >>+	for (alloc = 0; alloc < 1000; alloc++) {
> >>+		pkt = odp_packet_alloc(default_pkt_pool, packet_len);
> >>+		if (pkt == ODP_PACKET_INVALID)
> >>+			break;
> >>+		pktio_init_packet(pkt);
> >>+		tx_ev[alloc] = odp_packet_to_event(pkt);
> >>+	}
> >>+
> >>+	ret = odp_pktio_stats_reset(pktio[0]);
> >>+	CU_ASSERT(ret == 0);
> >>+	if (num_ifaces > 1) {
> >>+		ret = odp_pktio_stats_reset(pktio[1]);
> >>+		CU_ASSERT(ret == 0);
> >>+	}
> >>+
> >>+	/* send */
> >>+	for (pkts = 0; pkts != alloc; ) {
> >>+		ret = odp_queue_enq_multi(outq, &tx_ev[pkts], alloc - pkts);
> >>+		if (ret < 0) {
> >>+			CU_FAIL("unable to enqueue packet\n");
> >>+			break;
> >>+		}
> >>+		pkts += ret;
> >>+	}
> >>+
> >>+	/* get */
> >>+	for (i = 0, pkts = 0; i < 1000; i++) {
> >>+		ev = odp_schedule(NULL, wait);
> >>+		if (ev != ODP_EVENT_INVALID) {
> >>+			if (odp_event_type(ev) == ODP_EVENT_PACKET) {
> >>+				pkt = odp_packet_from_event(ev);
> >>+				if (pktio_pkt_seq(pkt) != TEST_SEQ_INVALID)
> >>+					pkts++;
> >>+			}
> >>+			odp_event_free(ev);
> >>+		}
> >>+	}
> >>+
> >>+	ret = odp_pktio_stats(pktio[0], &stats[0]);
> >>+	CU_ASSERT(ret == 0);
> >>+	_print_pktio_stats(&stats[0], iface_name[0]);
> >I don't like tests printing to stdout, it clutters the output and makes
> >it harder to see what's passed/failed. If all of the asserts pass then
> >it doesn't matter what the actual values are, that's a detail of the
> >test, if something fails though it's reasonable to dump to stderr.
> 
> ok, I can hide to error case. It was easy for me to debug when I see
> numbers.
> 
> >
> >>+
> >>+	if (num_ifaces > 1) {
> >>+		ret = odp_pktio_stats(pktio[1], &stats[1]);
> >>+		CU_ASSERT(ret == 0);
> >>+		_print_pktio_stats(&stats[1], iface_name[1]);
> >>+
> >>+		CU_ASSERT(stats[1].in_ucast_pkts >= (uint64_t)pkts);
> >>+		CU_ASSERT(stats[0].out_ucast_pkts == stats[1].in_ucast_pkts);
> >>+		CU_ASSERT(stats[0].out_octets == stats[1].in_octets);
> >>+		CU_ASSERT(stats[0].out_octets >=
> >>+			  (stats[0].out_ucast_pkts * (uint64_t)pkts));
> >What's this check for?.. seems to assert that average pkt size >= 2
> >octets.
> 
> Opps. I planned to write:
> 
> +		CU_ASSERT(stats[0].out_octets >=
> +			  (pkt_size * (uint64_t)pkts));
> 

OK, that makes sense.

> 
> For loop it's ==. But for other virtual dev some packets may be on
> network (like dhcp),
> so it will be >=.
> 
> 
> >>+	} else {
> >Why not check out_octets and out_ucast_pkts in this case?
> >
> >The logic would be simpler with something like:
> >
> >if (num_ifaces == 1)
> >	pktio[1] = pktio[0];
> >
> >stats[0] = odp_pktio_stats(pktio[0], &stats[0]);
> >stats[1] = odp_pktio_stats(pktio[1], &stats[1]);
> >
> >Then all the asserts would be the same regardless of whether you have 1
> >or 2 interfaces.
> yes,  it's the same think. Calling 2 times odp_pktio_stats() for the
> same device
> but different index in array a little bit confusing (complicates
> code understanding).
> At least for me.

The main point was to avoid having two different sets of CU_ASSERT()s
depending on whether you're using one or two interfaces, the test should
be exactly the same in both cases.

> >>+		CU_ASSERT(stats[0].in_ucast_pkts == (uint64_t)pkts);
> >>+		CU_ASSERT(stats[0].in_octets ==
> >>+			  (PKT_LEN_NORMAL * (uint64_t)pkts));
> >>+	}
> >>+
> >>+	CU_ASSERT(pkts == alloc);
> >>+	CU_ASSERT((stats[0].in_discards |
> >>+		stats[0].in_errors |
> >>+		stats[0].in_unknown_protos |
> >>+		stats[0].out_discards |
> >>+		stats[0].out_errors) == 0);
> >It would be better to have separate asserts for each of these so you
> >can tell from the logs which failed.
> Ok. My plan was that current counters are not well tested. When we
> will add tests
> for them then we can exclude from that group. But I can do test case
> per counter,
> it's not a problem.
> 
> >
> >>+
> >>+	for (i = 0; i < num_ifaces; i++) {
> >>+		destroy_inq(pktio[i]);
> >>+		CU_ASSERT(odp_pktio_close(pktio[i]) == 0);
> >>+	}
> >>+}
> >>+
> >>  static void pktio_test_start_stop(void)
> >>  {
> >>  	odp_pktio_t pktio[MAX_NUM_IFACES];
> >>@@ -1057,6 +1186,7 @@ odp_testinfo_t pktio_suite_unsegmented[] = {
> >>  	ODP_TEST_INFO(pktio_test_mac),
> >>  	ODP_TEST_INFO(pktio_test_inq_remdef),
> >>  	ODP_TEST_INFO(pktio_test_start_stop),
> >>+	ODP_TEST_INFO(pktio_test_statistics_counters),
> >>  	ODP_TEST_INFO_NULL
> >>  };
> >>-- 
> >>1.9.1
> >>
>
Maxim Uvarov Nov. 18, 2015, 9:06 a.m. | #4
On 11/10/2015 21:30, Stuart Haslam wrote:
> On Mon, Nov 09, 2015 at 02:21:53PM +0300, Maxim Uvarov wrote:
>> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
>> ---
>>   test/validation/pktio/pktio.c | 130 ++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 130 insertions(+)
>>
>> diff --git a/test/validation/pktio/pktio.c b/test/validation/pktio/pktio.c
>> index 6320b77..c591c94 100644
>> --- a/test/validation/pktio/pktio.c
>> +++ b/test/validation/pktio/pktio.c
>> @@ -690,6 +690,135 @@ void pktio_test_inq(void)
>>   	CU_ASSERT(odp_pktio_close(pktio) == 0);
>>   }
>>   
>> +static void _print_pktio_stats(odp_pktio_stats_t *s, const char *name)
>> +{
>> +	printf("\n%s:\n"
>> +		"  in_octets %"PRIu64"\n"
>> +		"  in_ucast_pkts %"PRIu64"\n"
>> +		"  in_discards %"PRIu64"\n"
>> +		"  in_errors %"PRIu64"\n"
>> +		"  in_unknown_protos %"PRIu64"\n"
>> +		"  out_octets %"PRIu64"\n"
>> +		"  out_ucast_pkts %"PRIu64"\n"
>> +		"  out_discards %"PRIu64"\n"
>> +		"  out_errors %"PRIu64"\n",
>> +		name,
>> +		s->in_octets,
>> +		s->in_ucast_pkts,
>> +		s->in_discards,
>> +		s->in_errors,
>> +		s->in_unknown_protos,
>> +		s->out_octets,
>> +		s->out_ucast_pkts,
>> +		s->out_discards,
>> +		s->out_errors);
>> +}
>> +
>> +static void pktio_test_statistics_counters(void)
>> +{
>> +	odp_pktio_t pktio[MAX_NUM_IFACES];
>> +	odp_packet_t pkt;
>> +	odp_event_t tx_ev[1000];
>> +	odp_event_t ev;
>> +	int i, pkts, ret, alloc = 0;
>> +	odp_queue_t outq;
>> +	uint64_t wait = odp_schedule_wait_time(ODP_TIME_MSEC);
>> +	odp_pktio_stats_t stats[2];
>> +
>> +	for (i = 0; i < num_ifaces; i++) {
>> +		pktio[i] = create_pktio(i, ODP_PKTIN_MODE_SCHED);
>> +		CU_ASSERT_FATAL(pktio[i] != ODP_PKTIO_INVALID);
>> +		create_inq(pktio[i],  ODP_QUEUE_TYPE_SCHED);
>> +	}
>> +
>> +	outq = odp_pktio_outq_getdef(pktio[0]);
>> +
>> +	ret = odp_pktio_start(pktio[0]);
>> +	CU_ASSERT(ret == 0);
>> +	if (num_ifaces > 1) {
>> +		ret = odp_pktio_start(pktio[1]);
>> +		CU_ASSERT(ret == 0);
>> +	}
>> +
>> +	/* flush packets with magic number in pipes */
>> +	for (i = 0; i < 1000; i++) {
>> +		ev = odp_schedule(NULL, wait);
>> +		if (ev != ODP_EVENT_INVALID)
>> +			odp_event_free(ev);
>> +	}
>> +
>> +	/* alloc */
>> +	for (alloc = 0; alloc < 1000; alloc++) {
>> +		pkt = odp_packet_alloc(default_pkt_pool, packet_len);
>> +		if (pkt == ODP_PACKET_INVALID)
>> +			break;
>> +		pktio_init_packet(pkt);
>> +		tx_ev[alloc] = odp_packet_to_event(pkt);
>> +	}
>> +
>> +	ret = odp_pktio_stats_reset(pktio[0]);
>> +	CU_ASSERT(ret == 0);
>> +	if (num_ifaces > 1) {
>> +		ret = odp_pktio_stats_reset(pktio[1]);
>> +		CU_ASSERT(ret == 0);
>> +	}
>> +
>> +	/* send */
>> +	for (pkts = 0; pkts != alloc; ) {
>> +		ret = odp_queue_enq_multi(outq, &tx_ev[pkts], alloc - pkts);
>> +		if (ret < 0) {
>> +			CU_FAIL("unable to enqueue packet\n");
>> +			break;
>> +		}
>> +		pkts += ret;
>> +	}
>> +
>> +	/* get */
>> +	for (i = 0, pkts = 0; i < 1000; i++) {
>> +		ev = odp_schedule(NULL, wait);
>> +		if (ev != ODP_EVENT_INVALID) {
>> +			if (odp_event_type(ev) == ODP_EVENT_PACKET) {
>> +				pkt = odp_packet_from_event(ev);
>> +				if (pktio_pkt_seq(pkt) != TEST_SEQ_INVALID)
>> +					pkts++;
>> +			}
>> +			odp_event_free(ev);
>> +		}
>> +	}
>> +
>> +	ret = odp_pktio_stats(pktio[0], &stats[0]);
>> +	CU_ASSERT(ret == 0);
>> +	_print_pktio_stats(&stats[0], iface_name[0]);
> I don't like tests printing to stdout, it clutters the output and makes
> it harder to see what's passed/failed. If all of the asserts pass then
> it doesn't matter what the actual values are, that's a detail of the
> test, if something fails though it's reasonable to dump to stderr.
>
>> +
>> +	if (num_ifaces > 1) {
>> +		ret = odp_pktio_stats(pktio[1], &stats[1]);
>> +		CU_ASSERT(ret == 0);
>> +		_print_pktio_stats(&stats[1], iface_name[1]);
>> +
>> +		CU_ASSERT(stats[1].in_ucast_pkts >= (uint64_t)pkts);
>> +		CU_ASSERT(stats[0].out_ucast_pkts == stats[1].in_ucast_pkts);
>> +		CU_ASSERT(stats[0].out_octets == stats[1].in_octets);
>> +		CU_ASSERT(stats[0].out_octets >=
>> +			  (stats[0].out_ucast_pkts * (uint64_t)pkts));
> What's this check for?.. seems to assert that average pkt size >= 2
> octets.
>
>> +	} else {
> Why not check out_octets and out_ucast_pkts in this case?
>
> The logic would be simpler with something like:
>
> if (num_ifaces == 1)
> 	pktio[1] = pktio[0];
>
> stats[0] = odp_pktio_stats(pktio[0], &stats[0]);
> stats[1] = odp_pktio_stats(pktio[1], &stats[1]);
>
> Then all the asserts would be the same regardless of whether you have 1
> or 2 interfaces.

that does not go because pktio_open happens only for num devices:

     for (i = 0; i < num_ifaces; i++) {
         pktio[i] = create_pktio(i, ODP_PKTIN_MODE_SCHED);
         CU_ASSERT_FATAL(pktio[i] != ODP_PKTIO_INVALID);
         create_inq(pktio[i],  ODP_QUEUE_TYPE_SCHED);
     }

Other comments look good.

Maxim.

>> +		CU_ASSERT(stats[0].in_ucast_pkts == (uint64_t)pkts);
>> +		CU_ASSERT(stats[0].in_octets ==
>> +			  (PKT_LEN_NORMAL * (uint64_t)pkts));
>> +	}
>> +
>> +	CU_ASSERT(pkts == alloc);
>> +	CU_ASSERT((stats[0].in_discards |
>> +		stats[0].in_errors |
>> +		stats[0].in_unknown_protos |
>> +		stats[0].out_discards |
>> +		stats[0].out_errors) == 0);
> It would be better to have separate asserts for each of these so you
> can tell from the logs which failed.
>
>> +
>> +	for (i = 0; i < num_ifaces; i++) {
>> +		destroy_inq(pktio[i]);
>> +		CU_ASSERT(odp_pktio_close(pktio[i]) == 0);
>> +	}
>> +}
>> +
>>   static void pktio_test_start_stop(void)
>>   {
>>   	odp_pktio_t pktio[MAX_NUM_IFACES];
>> @@ -1057,6 +1186,7 @@ odp_testinfo_t pktio_suite_unsegmented[] = {
>>   	ODP_TEST_INFO(pktio_test_mac),
>>   	ODP_TEST_INFO(pktio_test_inq_remdef),
>>   	ODP_TEST_INFO(pktio_test_start_stop),
>> +	ODP_TEST_INFO(pktio_test_statistics_counters),
>>   	ODP_TEST_INFO_NULL
>>   };
>>   
>> -- 
>> 1.9.1
>>

Patch

diff --git a/test/validation/pktio/pktio.c b/test/validation/pktio/pktio.c
index 6320b77..c591c94 100644
--- a/test/validation/pktio/pktio.c
+++ b/test/validation/pktio/pktio.c
@@ -690,6 +690,135 @@  void pktio_test_inq(void)
 	CU_ASSERT(odp_pktio_close(pktio) == 0);
 }
 
+static void _print_pktio_stats(odp_pktio_stats_t *s, const char *name)
+{
+	printf("\n%s:\n"
+		"  in_octets %"PRIu64"\n"
+		"  in_ucast_pkts %"PRIu64"\n"
+		"  in_discards %"PRIu64"\n"
+		"  in_errors %"PRIu64"\n"
+		"  in_unknown_protos %"PRIu64"\n"
+		"  out_octets %"PRIu64"\n"
+		"  out_ucast_pkts %"PRIu64"\n"
+		"  out_discards %"PRIu64"\n"
+		"  out_errors %"PRIu64"\n",
+		name,
+		s->in_octets,
+		s->in_ucast_pkts,
+		s->in_discards,
+		s->in_errors,
+		s->in_unknown_protos,
+		s->out_octets,
+		s->out_ucast_pkts,
+		s->out_discards,
+		s->out_errors);
+}
+
+static void pktio_test_statistics_counters(void)
+{
+	odp_pktio_t pktio[MAX_NUM_IFACES];
+	odp_packet_t pkt;
+	odp_event_t tx_ev[1000];
+	odp_event_t ev;
+	int i, pkts, ret, alloc = 0;
+	odp_queue_t outq;
+	uint64_t wait = odp_schedule_wait_time(ODP_TIME_MSEC);
+	odp_pktio_stats_t stats[2];
+
+	for (i = 0; i < num_ifaces; i++) {
+		pktio[i] = create_pktio(i, ODP_PKTIN_MODE_SCHED);
+		CU_ASSERT_FATAL(pktio[i] != ODP_PKTIO_INVALID);
+		create_inq(pktio[i],  ODP_QUEUE_TYPE_SCHED);
+	}
+
+	outq = odp_pktio_outq_getdef(pktio[0]);
+
+	ret = odp_pktio_start(pktio[0]);
+	CU_ASSERT(ret == 0);
+	if (num_ifaces > 1) {
+		ret = odp_pktio_start(pktio[1]);
+		CU_ASSERT(ret == 0);
+	}
+
+	/* flush packets with magic number in pipes */
+	for (i = 0; i < 1000; i++) {
+		ev = odp_schedule(NULL, wait);
+		if (ev != ODP_EVENT_INVALID)
+			odp_event_free(ev);
+	}
+
+	/* alloc */
+	for (alloc = 0; alloc < 1000; alloc++) {
+		pkt = odp_packet_alloc(default_pkt_pool, packet_len);
+		if (pkt == ODP_PACKET_INVALID)
+			break;
+		pktio_init_packet(pkt);
+		tx_ev[alloc] = odp_packet_to_event(pkt);
+	}
+
+	ret = odp_pktio_stats_reset(pktio[0]);
+	CU_ASSERT(ret == 0);
+	if (num_ifaces > 1) {
+		ret = odp_pktio_stats_reset(pktio[1]);
+		CU_ASSERT(ret == 0);
+	}
+
+	/* send */
+	for (pkts = 0; pkts != alloc; ) {
+		ret = odp_queue_enq_multi(outq, &tx_ev[pkts], alloc - pkts);
+		if (ret < 0) {
+			CU_FAIL("unable to enqueue packet\n");
+			break;
+		}
+		pkts += ret;
+	}
+
+	/* get */
+	for (i = 0, pkts = 0; i < 1000; i++) {
+		ev = odp_schedule(NULL, wait);
+		if (ev != ODP_EVENT_INVALID) {
+			if (odp_event_type(ev) == ODP_EVENT_PACKET) {
+				pkt = odp_packet_from_event(ev);
+				if (pktio_pkt_seq(pkt) != TEST_SEQ_INVALID)
+					pkts++;
+			}
+			odp_event_free(ev);
+		}
+	}
+
+	ret = odp_pktio_stats(pktio[0], &stats[0]);
+	CU_ASSERT(ret == 0);
+	_print_pktio_stats(&stats[0], iface_name[0]);
+
+	if (num_ifaces > 1) {
+		ret = odp_pktio_stats(pktio[1], &stats[1]);
+		CU_ASSERT(ret == 0);
+		_print_pktio_stats(&stats[1], iface_name[1]);
+
+		CU_ASSERT(stats[1].in_ucast_pkts >= (uint64_t)pkts);
+		CU_ASSERT(stats[0].out_ucast_pkts == stats[1].in_ucast_pkts);
+		CU_ASSERT(stats[0].out_octets == stats[1].in_octets);
+		CU_ASSERT(stats[0].out_octets >=
+			  (stats[0].out_ucast_pkts * (uint64_t)pkts));
+	} else {
+		CU_ASSERT(stats[0].in_ucast_pkts == (uint64_t)pkts);
+		CU_ASSERT(stats[0].in_octets ==
+			  (PKT_LEN_NORMAL * (uint64_t)pkts));
+	}
+
+	CU_ASSERT(pkts == alloc);
+	CU_ASSERT((stats[0].in_discards |
+		stats[0].in_errors |
+		stats[0].in_unknown_protos |
+		stats[0].out_discards |
+		stats[0].out_errors) == 0);
+
+	for (i = 0; i < num_ifaces; i++) {
+		destroy_inq(pktio[i]);
+		CU_ASSERT(odp_pktio_close(pktio[i]) == 0);
+	}
+}
+
 static void pktio_test_start_stop(void)
 {
 	odp_pktio_t pktio[MAX_NUM_IFACES];
@@ -1057,6 +1186,7 @@  odp_testinfo_t pktio_suite_unsegmented[] = {
 	ODP_TEST_INFO(pktio_test_mac),
 	ODP_TEST_INFO(pktio_test_inq_remdef),
 	ODP_TEST_INFO(pktio_test_start_stop),
+	ODP_TEST_INFO(pktio_test_statistics_counters),
 	ODP_TEST_INFO_NULL
 };