From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id 929DE1DA4 for ; Tue, 24 Jul 2018 13:21:28 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 24 Jul 2018 04:21:27 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,397,1526367600"; d="scan'208";a="247886712" Received: from aburakov-mobl.ger.corp.intel.com (HELO [10.237.220.86]) ([10.237.220.86]) by fmsmga005.fm.intel.com with ESMTP; 24 Jul 2018 04:21:19 -0700 To: Naga Suresh Somarowthu , dev@dpdk.org Cc: remy.horton@intel.com, reshma.pattan@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> From: "Burakov, Anatoly" Message-ID: <5bb45e92-504d-bab3-bbba-55a785c9fe5b@intel.com> Date: Tue, 24 Jul 2018 12:21:18 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <1532429671-1606-2-git-send-email-naga.sureshx.somarowthu@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v5 1/4] test: add ring pmd based packet rx/tx for UT X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 24 Jul 2018 11:21:29 -0000 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 > Reviewed-by: Reshma Pattan > Reviewed-by: Anatoly Burakov 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 > +/* 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); > +} > +} > 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