From: "Trahe, Fiona" <fiona.trahe@intel.com>
To: Akhil Goyal <akhil.goyal@nxp.com>,
"Dybkowski, AdamX" <adamx.dybkowski@intel.com>,
"dev@dpdk.org" <dev@dpdk.org>
Cc: Shally Verma <shallyv@marvell.com>,
"Trahe, Fiona" <fiona.trahe@intel.com>
Subject: Re: [dpdk-dev] [PATCH v2 2/2] test/compress: im buffer too small - add unit tests
Date: Thu, 16 Apr 2020 11:26:46 +0000 [thread overview]
Message-ID: <SN6PR11MB2880E135298D1A3FCE1C5510E4D80@SN6PR11MB2880.namprd11.prod.outlook.com> (raw)
In-Reply-To: <VI1PR04MB31689FA31FF27803D0BD2A5DE6D80@VI1PR04MB3168.eurprd04.prod.outlook.com>
> -----Original Message-----
> From: Akhil Goyal <akhil.goyal@nxp.com>
> Sent: Thursday, April 16, 2020 11:25 AM
> To: Trahe, Fiona <fiona.trahe@intel.com>; Dybkowski, AdamX <adamx.dybkowski@intel.com>;
> dev@dpdk.org
> Cc: Shally Verma <shallyv@marvell.com>
> Subject: RE: [PATCH v2 2/2] test/compress: im buffer too small - add unit tests
>
> Hi Fiona,
> >
> > Hi Akhil,
> >
> > >
> > > Hi Fiona/Adam,
> > >
> > > > This patch adds a new test suite for verification of the "internal
> > > > QAT IM buffer too small" case handling. These unit tests are
> > > > specific to the QAT PMD only - that's why they are contained in
> > > > a separate test suite.
> > > >
> > > > Signed-off-by: Adam Dybkowski <adamx.dybkowski@intel.com>
> > > > ---
> > >
> > > Why do we need to have separate testsuite for QAT?
> > > Can't we have a single one and based on capability of the driver,
> > > Determine which tests need to be skipped in case they are not supported.
> > > This would create a mess in the longer run just like cryptodev.
> > >
> > > Please fix this, we cannot take this patch as is.
> >
> > [Fiona] Yes, I understand your concern and we considered including in the main
> > suite.
> > However these tests are not based on something that can be
> > checked in capabilities. They are tests to hone in on a specific corner case
> > based on a QAT limitation in its intermediate buffer size. So some of the
> > tests are to validate that the recent changes we made in the PMD correctly
> > work around that limitation, but other tests are negative and expected to fail
> > as provoking a corner-case that still exists. Other devices would probably not fail
> > the same tests.
>
> Does that mean that all PMDs will pass with the newly added testcase which is for
> A corner case in QAT. If that is the case what is the issue in adding that in the main
> Test suite. It will get passed in all PMDs, isn't it? Am I missing something?
>
> I believe we should not have PMD specific test suites, rather it should be based on
> Capabilities to identify the cases which should be run for that particular PMD.
[Fiona] yes, several of the cases should pass on all PMDs.
So we could move those into the main suite.
But what to do about the negative tests?
Example: If a very large data buffer is passed to QAT to compress with dyn compression, it will get
split in the PMD into many smaller requests to the hardware. However if the number
of requests is bigger than can fit on the qp then this will never succeed. The test
validates that the PMD behaves appropriately in this expected error case. That same
case would probably not have an error on another device. Maybe we should just leave out
such negative tests, but I find them useful as they validate the known behaviour.
The buffer size used in the test is based on the known size QAT can handle and the
corner case in which QAT will return an error.
I see 4 options to handle this:
1. Leave out those tests
2. Use a qat-specific test suite only for negative cases which are constructed based on specific qat internal meta-data.
3. Include the negative tests in the main suite, but only run them on QAT (by checking driver type)
4. include the negative tests in the main suite, run them on all, expecting a FAIL from QAT and a PASS from other devices.
My preference is for 2.
But up to you.
next prev parent reply other threads:[~2020-04-16 11:26 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-08 12:50 [dpdk-dev] [PATCH v2 0/2] compress/qat: im buffer too small - split op Adam Dybkowski
2020-04-08 12:51 ` [dpdk-dev] [PATCH v2 1/2] " Adam Dybkowski
2020-04-08 15:43 ` Trahe, Fiona
2020-04-08 12:51 ` [dpdk-dev] [PATCH v2 2/2] test/compress: im buffer too small - add unit tests Adam Dybkowski
2020-04-08 15:44 ` Trahe, Fiona
2020-04-15 18:35 ` Akhil Goyal
2020-04-16 10:02 ` Trahe, Fiona
2020-04-16 10:25 ` Akhil Goyal
2020-04-16 11:26 ` Trahe, Fiona [this message]
2020-04-16 14:31 ` Bruce Richardson
2020-04-16 14:55 ` Trahe, Fiona
2020-04-16 14:37 ` Akhil Goyal
2020-04-16 14:52 ` Trahe, Fiona
2020-04-17 15:39 ` Akhil Goyal
2020-04-17 15:56 ` Trahe, Fiona
2020-04-17 15:44 ` [dpdk-dev] [PATCH v3 0/2] compress/qat: im buffer too small - split op Adam Dybkowski
2020-04-17 15:44 ` [dpdk-dev] [PATCH v3 1/2] " Adam Dybkowski
2020-04-17 15:44 ` [dpdk-dev] [PATCH v3 2/2] test/compress: im buffer too small - add unit tests Adam Dybkowski
2020-04-17 15:58 ` Trahe, Fiona
2020-04-17 21:50 ` Akhil Goyal
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=SN6PR11MB2880E135298D1A3FCE1C5510E4D80@SN6PR11MB2880.namprd11.prod.outlook.com \
--to=fiona.trahe@intel.com \
--cc=adamx.dybkowski@intel.com \
--cc=akhil.goyal@nxp.com \
--cc=dev@dpdk.org \
--cc=shallyv@marvell.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).