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

[Jerin Jacob <jerinjacobk@gmail.com>]

On Thu, Sep 2, 2021 at 1:36 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> 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.

Agree with the rationale.


>
> 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.


I think 25us will not be enough, e.s.p If is PCI-Dev to PCI-Dev kind
of test cases.
Since it is the functional test case, I think, we can keep it a very
higher range to
support all cases. Maybe 50ms is a good target.


>
> Regards,
> /Bruce