From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Burakov, Anatoly" <anatoly.burakov@intel.com>
Subject: Re: [PATCH v5 1/4] test: add ring pmd based packet rx/tx
 for UT
Date: Tue, 24 Jul 2018 12:21:18 +0100
Message-ID: <5bb45e92-504d-bab3-bbba-55a785c9fe5b@intel.com>
References: <1531821650-5506-1-git-send-email-jananeex.m.parthasarathy@intel.com>
 <1532429671-1606-1-git-send-email-naga.sureshx.somarowthu@intel.com>
 <1532429671-1606-2-git-send-email-naga.sureshx.somarowthu@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Cc: remy.horton@intel.com, reshma.pattan@intel.com
To: Naga Suresh Somarowthu <naga.sureshx.somarowthu@intel.com>, dev@dpdk.org
Return-path: <dev-bounces@dpdk.org>
Received: from mga05.intel.com (mga05.intel.com [192.55.52.43])
 by dpdk.org (Postfix) with ESMTP id 929DE1DA4
 for <dev@dpdk.org>; Tue, 24 Jul 2018 13:21:28 +0200 (CEST)
In-Reply-To: <1532429671-1606-2-git-send-email-naga.sureshx.somarowthu@intel.com>
Content-Language: en-US
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

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@intel.com>
> Reviewed-by: Reshma Pattan <reshma.pattan@intel.com>
> Reviewed-by: Anatoly Burakov <anatoly.burakov@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