Re: [dpdk-dev] [RFC PATCH 4/7] app/test: add basic dmadev copy tests

[Bruce Richardson <bruce.richardson@intel.com>]

On Fri, Aug 27, 2021 at 12:44:17PM +0530, Jerin Jacob wrote:
> On Fri, Aug 27, 2021 at 12:03 AM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > For each dmadev instance, perform some basic copy tests to validate that
> > functionality.
> >
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> >  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 <unistd.h>
> >
> >  #include <rte_common.h>
> >  #include <rte_dev.h>
> >  #include <rte_dmadev.h>
> >  #include <rte_bus_vdev.h>
> > +#include <rte_mbuf.h>
> > +#include <rte_random.h>
> >
> >  #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