Re: [dpdk-dev] [PATCH v2 3/6] app/test: add basic dmadev copy tests

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

On Thu, Sep 02, 2021 at 01:14:38PM +0530, Jerin Jacob wrote:
> On Wed, Sep 1, 2021 at 10:02 PM 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 | 174 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 174 insertions(+)
> >
> > diff --git a/app/test/test_dmadev.c b/app/test/test_dmadev.c
> > index 12f7c69629..261f45db71 100644
> > --- a/app/test/test_dmadev.c
> > +++ b/app/test/test_dmadev.c
> > @@ -2,12 +2,15 @@
> >   * Copyright(c) 2021 HiSilicon Limited.
> >   * Copyright(c) 2021 Intel Corporation.
> >   */
> > +#include <unistd.h>
> >  #include <inttypes.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"
> >
> > @@ -16,6 +19,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, ...)
> > @@ -31,6 +39,134 @@ print_err(const char *func, int lineno, const char *format, ...)
> >         return ret;
> >  }
> >
> > +static inline void
> > +await_hw(int dev_id, uint16_t vchan)
> > +{
> > +       int idle = rte_dmadev_vchan_idle(dev_id, vchan);
> > +       if (idle < 0) {
> > +               /* for drivers that don't support this op, just sleep for 25 microseconds */
> > +               usleep(25);
> > +               return;
> > +       }
> 
> Can following model eliminate the need for rte_dmadev_vchan_idle() API. Right?
> 
> static inline bool
> await_hw(int dev_id, uint16_t vchan, uint16_t  nb_req, uint16_t *last_idx)
> {
>              const uint64_t tmo =   rte_get_timer_hz();
>              bool has_error  = false;
> 
>              const uint64_t end_cycles = rte_get_timer_cycles() + tmo;
>               while (rte_get_timer_cycles() < end_cycles && nb_req > 0
> && has_error  == false) {
>                            rte_pause();
>                            nb_req -= rte_dmadev_completed(dev_id,
> nb_req, last_idx, &has_error);
>               }
> 
>               return has_error ;
> }
>
It would, but unfortunately it also removes the possibility of doing a
number of the tests in the set, particularly around failure handling. We
used runtime coverage tools to ensure we were hitting as many legs of code
as possible in drivers, and to cover these possibilities we need to do
various different types of completion gathering, e.g. gather multiple
bursts in one go, gathering a single burst in two halves, gathering a burst
using completion_status rather than completion, gathering completions
one-at-a-time with a call for each individually, and for error handling
gathering just the failing element alone, or gathering completions for all
remaining elements not just the failing one, etc. etc. 

These tests are useful both for finding bugs (and they did find ones in our
drivers), but also to ensure similar behaviour across different drivers
using the API. However, they really only can be done in a consistent way if
we are able to ensure that at certain points the hardware has finished
processing before we begin gathering completions. Therefore, having a way
to poll for idle is useful. As you see, I've also left in the delay as a
fallback in case drivers choose not to implement it, thereby making it an
optional API.

Beyond testing, I can see the API to poll for idleness being useful for the
device shutdown case. I was considering whether the "stop" API should also
use it to ensure that the hardware is idle before stopping. I decided
against it for now, but you could see applications making use of this -
waiting for the hardware to finish its work before stopping it.

Regards,
/Bruce