From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id F1222A0C41; Fri, 27 Aug 2021 12:41:34 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 76B5C406B4; Fri, 27 Aug 2021 12:41:34 +0200 (CEST) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by mails.dpdk.org (Postfix) with ESMTP id 421524067C for ; Fri, 27 Aug 2021 12:41:32 +0200 (CEST) X-IronPort-AV: E=McAfee;i="6200,9189,10088"; a="303513643" X-IronPort-AV: E=Sophos;i="5.84,356,1620716400"; d="scan'208";a="303513643" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 27 Aug 2021 03:41:31 -0700 X-IronPort-AV: E=Sophos;i="5.84,356,1620716400"; d="scan'208";a="495602543" Received: from bricha3-mobl.ger.corp.intel.com ([10.252.24.30]) by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 27 Aug 2021 03:41:29 -0700 Date: Fri, 27 Aug 2021 11:41:26 +0100 From: Bruce Richardson To: Jerin Jacob Cc: dpdk-dev , "Walsh, Conor" , "Laatz, Kevin" , Chengwen Feng , Jerin Jacob Message-ID: References: <20210826183301.333442-1-bruce.richardson@intel.com> <20210826183301.333442-5-bruce.richardson@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [dpdk-dev] [RFC PATCH 4/7] app/test: add basic dmadev copy tests X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Fri, Aug 27, 2021 at 12:44:17PM +0530, Jerin Jacob wrote: > On Fri, Aug 27, 2021 at 12:03 AM Bruce Richardson > wrote: > > > > For each dmadev instance, perform some basic copy tests to validate that > > functionality. > > > > Signed-off-by: Bruce Richardson > > --- > > app/test/test_dmadev.c | 157 +++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 157 insertions(+) > > > > diff --git a/app/test/test_dmadev.c b/app/test/test_dmadev.c > > index f895556d29..a9f7d34a94 100644 > > --- a/app/test/test_dmadev.c > > +++ b/app/test/test_dmadev.c > > @@ -1,11 +1,14 @@ > > /* SPDX-License-Identifier: BSD-3-Clause > > * Copyright(c) 2021 HiSilicon Limited. > > */ > > +#include > > > > #include > > #include > > #include > > #include > > +#include > > +#include > > > > #include "test.h" > > > > @@ -14,6 +17,11 @@ extern int test_dmadev_api(uint16_t dev_id); > > > > #define PRINT_ERR(...) print_err(__func__, __LINE__, __VA_ARGS__) > > > > +#define COPY_LEN 1024 > > + > > +static struct rte_mempool *pool; > > +static uint16_t id_count; > > + > > static inline int > > __rte_format_printf(3, 4) > > print_err(const char *func, int lineno, const char *format, ...) > > @@ -29,10 +37,123 @@ print_err(const char *func, int lineno, const char *format, ...) > > return ret; > > } > > > > +static int > > +test_enqueue_copies(int dev_id, uint16_t vchan) > > +{ > > + unsigned int i; > > + uint16_t id; > > + > > + /* test doing a single copy */ > > + do { > > + struct rte_mbuf *src, *dst; > > + char *src_data, *dst_data; > > + > > + src = rte_pktmbuf_alloc(pool); > > + dst = rte_pktmbuf_alloc(pool); > > + src_data = rte_pktmbuf_mtod(src, char *); > > + dst_data = rte_pktmbuf_mtod(dst, char *); > > + > > + for (i = 0; i < COPY_LEN; i++) > > + src_data[i] = rte_rand() & 0xFF; > > + > > + id = rte_dmadev_copy(dev_id, vchan, src->buf_iova + src->data_off, > > + dst->buf_iova + dst->data_off, COPY_LEN, RTE_DMA_OP_FLAG_SUBMIT); > > + if (id != id_count) { > > + PRINT_ERR("Error with rte_dmadev_copy, got %u, expected %u\n", > > + id, id_count); > > + return -1; > > + } > > + > > + /* give time for copy to finish, then check it was done */ > > + usleep(10); > > Across series, We have this pattern. IMHO, It is not portable. > Can we have a helper function either common lib code or test code to > busy poll for completion with timeout? and in test code, we have a much bigger > timeout to accommodate all the devices. That way if the driver completes > early it can continue to execute and makes it portable. > It's less that ideal, I admit, but I'm not sure it's not portable. The main concern here for these unit tests is to try and ensure that at all times the state of the device is fully known, so the delays are there to ensure that the device has completely finished all work given to it. The suggestion of having a polling loop to gather completions won't work for all scenarios are it doesn't give us the same degree of control in that we cannot know the exact state of job completion when running each poll - meaning that running tests such as for gathering all completions in one go, or in parts of a fixed size are hard to do. The other alternative is to provide an API in dmadev to await quiescence of a DMA device. This would be a better alternative to having the fixed delays, but means a new API for testing only. I can investigate this for the next version of the patchset but it means more work for driver writers. [This could be perhaps worked around by having dmadev implement a "usleep()" call for any drivers that don't implement the function, which would provide a quicker way for driver writers to test incomplete drivers] Also, if having "large" delays is a concern, I don't think it's a major one. With all delays as usleep(10) - and 10 microseconds should be a long time for a HW device - an test run of an optimized build of DPDK takes ~1.5 seconds for 2 dmadev instances, and a debug build only ~2.5 seconds. Increasing the delay tenfold to 100 usec, brings the optimized test run to <2.5 secs and a debug build run to ~4 seconds i.e. 2 seconds per device. Therefore I don't believe having delays in this code is a real problem other than being a less-elegant solution than a polling-based one. In any case, I'll see about adding an "all-jobs-done" API to dmadev for v2 to remove these delays as much as possible. Regards, /Bruce