[dpdk-dev] [PATCH v5 1/4] test: add ring pmd based packet rx/tx for UT

[Burakov, Anatoly]

On 24-Jul-18 11:54 AM, Naga Suresh Somarowthu wrote:
> Added ring pmd based packet rx/tx helper functions
> for verifying Latency, Bitrate and pdump lib UTs.
> 
> Signed-off-by: Naga Suresh Somarowthu <naga.sureshx.somarowthu at intel.com>
> Reviewed-by: Reshma Pattan <reshma.pattan at intel.com>
> Reviewed-by: Anatoly Burakov <anatoly.burakov at intel.com>

Why is it carrying my Reviewed-by: ? I do not recall sending it...

> ---
>   test/test/Makefile                |   1 +
>   test/test/sample_packet_forward.c | 115 ++++++++++++++++++++++++++++++++++++++
>   test/test/sample_packet_forward.h |  41 ++++++++++++++
>   3 files changed, 157 insertions(+)
>   create mode 100644 test/test/sample_packet_forward.c
>   create mode 100644 test/test/sample_packet_forward.h
> 
> diff --git a/test/test/Makefile b/test/test/Makefile
> index e6967bab6..9f7d398e4 100644

<snip>

> +/* Sample test to create virtual rings and tx,rx portid from rings */
> +int
> +test_ring_setup(struct rte_ring *ring[], uint16_t *portid)
> +{
> +	ring[0] = rte_ring_create("R0", RING_SIZE, rte_socket_id(),
> +				  RING_F_SP_ENQ | RING_F_SC_DEQ);
> +	if (ring[0] == NULL) {
> +		printf("%s() line %u: rte_ring_create R0 failed",
> +		       __func__, __LINE__);
> +		return TEST_FAILED;
> +	}
> +	*portid = rte_eth_from_rings("net_ringa", ring, NUM_QUEUES,
> +			ring, NUM_QUEUES, rte_socket_id());
> +
> +	return TEST_SUCCESS;
> +}

Previous comments have not been addressed.

You are always creating one ring, yet you create an array of rings (with 
a size of one). Why?

Even assuming you need to create multiple rings (which, as far as i can 
tell from other patches, you don't), why not just accept a 
pointer-to-pointer, like this:

int test_ring_setup(struct rte_ring **ring) {
    *ring = rte_ring_create();
}

// calling code
struct rte_ring *ring;
test_ring_setup(&ring);

Surely this would be more understandable?

(in fact, you do this with mempools - why not rings?)

Also, here and in lots of other places: why is this function returning 
TEST_SUCCESS? Is this an autotest? If not, then it shouldn't return 
these values.

> +
> +/* Sample test to free the mempool */
> +void
> +test_mp_free(struct rte_mempool *mp)
> +{
> +	rte_mempool_free(mp);
> +}
> +
> +/* Sample test to free the virtual rings */
> +void
> +test_ring_free(struct rte_ring *rxtx)
> +{
> +	rte_ring_free(rxtx);
> +}

<snip>

> +}
> diff --git a/test/test/sample_packet_forward.h b/test/test/sample_packet_forward.h
> new file mode 100644
> index 000000000..1293ee0a8
> --- /dev/null
> +++ b/test/test/sample_packet_forward.h
> @@ -0,0 +1,41 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2018 Intel Corporation
> + */
> +
> +#ifndef _SAMPLE_PACKET_FORWARD_H_
> +#define _SAMPLE_PACKET_FORWARD_H_
> +
> +/* MACROS to support virtual ring creation */
> +#define RING_SIZE 256
> +#define NUM_RINGS 1
> +#define NUM_QUEUES 1
> +#define NB_MBUF 512
> +
> +#define NUM_PACKETS 10

Some of the above #define's are not used in this patch. They probably 
should be added in later patches?

> +
> +/* Sample test to create virtual rings and tx,rx portid from rings */
> +int test_ring_setup(struct rte_ring *ring_server[], uint16_t *portid);
> +
> +/* Sample test to free the virtual rings */
> +void test_ring_free(struct rte_ring *rxtx);
> +
> +/* Sample test to forward packet using virtual port id */
> +int test_packet_forward(struct rte_mbuf **pbuf, uint16_t portid,
> +		uint16_t queue_id);
> +
> +/* sample test to allocate buffer for pkts */
> +int test_get_mbuf_from_pool(struct rte_mempool **mp, struct rte_mbuf **pbuf,
> +		char *poolname);
> +
> +/* Sample test to create the mempool */
> +int test_get_mempool(struct rte_mempool **mp, char *poolname);
> +
> +/* sample test to deallocate the allocated buffers */
> +void test_put_mbuf_to_pool(struct rte_mempool *mp, struct rte_mbuf **pbuf);
> +
> +/* Sample test to free the mempool */
> +void test_mp_free(struct rte_mempool *mp);
> +
> +/* Sample test to release the vdev */
> +void test_vdev_uninit(const char *vdev);
> +#endif				/* _SAMPLE_PACKET_FORWARD_H_ */
> 


-- 
Thanks,
Anatoly