From: "Chautru, Nicolas" <nicolas.chautru@intel.com>
To: "Coquelin, Maxime" <maxime.coquelin@redhat.com>,
"Vargas, Hernan" <hernan.vargas@intel.com>,
"dev@dpdk.org" <dev@dpdk.org>,
"gakhil@marvell.com" <gakhil@marvell.com>,
"Rix, Tom" <trix@redhat.com>,
Hemant Agrawal <hemant.agrawal@nxp.com>
Cc: "Zhang, Qi Z" <qi.z.zhang@intel.com>
Subject: RE: [PATCH v1 02/11] test/bbdev: update python script parameters
Date: Thu, 2 Nov 2023 18:18:30 +0000 [thread overview]
Message-ID: <BY5PR11MB44517B03FEAA13602E14E34AF8A6A@BY5PR11MB4451.namprd11.prod.outlook.com> (raw)
In-Reply-To: <25566cf0-2c91-43b3-b7cd-7a93b935c4b2@redhat.com>
Hi Maxime,
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Thursday, November 2, 2023 10:00 AM
> To: Chautru, Nicolas <nicolas.chautru@intel.com>; Vargas, Hernan
> <hernan.vargas@intel.com>; dev@dpdk.org; gakhil@marvell.com; Rix, Tom
> <trix@redhat.com>; Hemant Agrawal <hemant.agrawal@nxp.com>
> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>
> Subject: Re: [PATCH v1 02/11] test/bbdev: update python script parameters
>
> Hi Nicolas,
>
> On 10/27/23 22:02, Chautru, Nicolas wrote:
> > Hi Maxime,
> >
> > Back on this discussion and there was some misunderstanding. This is really
> a bug for the python script helper only.
> > The -t option is already reserved for bbdev-test application (and in doc) for
> setting the iter_max (see main.c).
> > The problem is that the python script introduced -t by mistake for an
> additional time out when calling the binary to be handled in the script only,
> now resolving this by using -T to avoid clash with existing -t option.
> > No one is genuinely using -t for timeout.
> > Ping me if still unclear
>
> I actually understood it from the beginning, -t was used as timeout option in
> the test-bbdev.py script, this patch changes -t to now represent the maximum
> number of iterations.
>
> I'm sure you can see the problem if someone was using -t for timeout in some
> CI (Neither me or you can guarantee this is not used)?
>
> If you think we really should change the meaning of this option, we should
> have a deprecation notice, have both -t and -T to represent timeout during
> the deprecation period and emit a warning when -t is used. Once deprecated,
> you can assign -t to max iterations.
>
> Sounds good?
The problem with your option is that during that period of time the script is still broken since you cannot set the number of iterations which is what -t is meant for in the binary application.
This is really a fix, and until that fix is applied the python script should not be used, instead the binary should be called directly.
If you want to do it your suggested way and then do the formal fix in 24.03 that is possible even if not ideal to me, but it would still mean the script should arguably not be used during that period of time from my point of view.
Let me know
>
> Regards,
> Maxime
>
> > Nic
> >
> >> -----Original Message-----
> >> From: Chautru, Nicolas
> >> Sent: Thursday, October 19, 2023 8:10 AM
> >> To: Maxime Coquelin <maxime.coquelin@redhat.com>; Vargas, Hernan
> >> <Hernan.Vargas@intel.com>; dev@dpdk.org; gakhil@marvell.com; Rix,
> Tom
> >> <trix@redhat.com>; Hemant Agrawal <hemant.agrawal@nxp.com>
> >> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>
> >> Subject: RE: [PATCH v1 02/11] test/bbdev: update python script
> >> parameters
> >>
> >> Hi Maxime,
> >> In practice anyone using that API is already using the one defined in
> >> the patch below and not using -t for time out. So not a concern to do
> >> it properly through that patch.
> >> Heman, any concern on your side with this change?
> >>
> >>> -----Original Message-----
> >>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>> Sent: Thursday, October 19, 2023 11:19 AM
> >>> To: Chautru, Nicolas <nicolas.chautru@intel.com>; Vargas, Hernan
> >>> <hernan.vargas@intel.com>; dev@dpdk.org; gakhil@marvell.com; Rix,
> >> Tom
> >>> <trix@redhat.com>
> >>> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>
> >>> Subject: Re: [PATCH v1 02/11] test/bbdev: update python script
> >>> parameters
> >>>
> >>>
> >>>
> >>> On 10/19/23 11:01, Chautru, Nicolas wrote:
> >>>> Hi Maxime,
> >>>>
> >>>> I believe there was some historical discrepancy, even in doc both
> >>>> appeared
> >>> but none of the 2 -t options with the cap.
> >>>> https://doc.dpdk.org/guides/tools/testbbdev.html
> >>>> Resolving this historical issue here.
> >>>
> >>> Ok, then we should fix the doc, not the code.
> >>>
> >>> Thanks,
> >>> Maxime
> >>>
> >>>> Thanks
> >>>> Nic
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>>> Sent: Tuesday, October 17, 2023 9:08 PM
> >>>>> To: Vargas, Hernan <hernan.vargas@intel.com>; dev@dpdk.org;
> >>>>> gakhil@marvell.com; Rix, Tom <trix@redhat.com>
> >>>>> Cc: Chautru, Nicolas <nicolas.chautru@intel.com>; Zhang, Qi Z
> >>>>> <qi.z.zhang@intel.com>
> >>>>> Subject: Re: [PATCH v1 02/11] test/bbdev: update python script
> >>>>> parameters
> >>>>>
> >>>>>
> >>>>>
> >>>>> On 9/29/23 20:13, Hernan Vargas wrote:
> >>>>>> Update the timeout argument and default values.
> >>>>>> Update EAL help message and default value.
> >>>>>> Add iter_max and snr arguments.
> >>>>>>
> >>>>>> Signed-off-by: Hernan Vargas <hernan.vargas@intel.com>
> >>>>>> ---
> >>>>>> app/test-bbdev/test-bbdev.py | 22 ++++++++++++++++++----
> >>>>>> app/test-bbdev/test_bbdev_perf.c | 2 +-
> >>>>>> 2 files changed, 19 insertions(+), 5 deletions(-)
> >>>>>>
> >>>>>> diff --git a/app/test-bbdev/test-bbdev.py
> >>>>>> b/app/test-bbdev/test-bbdev.py index
> 9cdb4659724d..8d0145076e4d
> >>>>>> 100755
> >>>>>> --- a/app/test-bbdev/test-bbdev.py
> >>>>>> +++ b/app/test-bbdev/test-bbdev.py
> >>>>>> @@ -25,12 +25,12 @@ def kill(process):
> >>>>>> help="specifies path to the bbdev test app",
> >>>>>> default=dpdk_path + "/" + dpdk_target +
> >>>>>> "/app/dpdk-test-
> >>> bbdev")
> >>>>>> parser.add_argument("-e", "--eal-params",
> >>>>>> - help="EAL arguments which are passed to the test app",
> >>>>>> - default="--vdev=baseband_null0")
> >>>>>> -parser.add_argument("-t", "--timeout",
> >>>>>> + help="EAL arguments which must be passed to
> >>>>>> + the test
> >> app",
> >>>>>> + default="--vdev=baseband_null0 -a00:00.0")
> >>>>>> +parser.add_argument("-T", "--timeout",
> >>>>>> type=int,
> >>>>>> help="Timeout in seconds",
> >>>>>> - default=300)
> >>>>>> + default=600)
> >>>>>> parser.add_argument("-c", "--test-cases",
> >>>>>> nargs="+",
> >>>>>> help="Defines test cases to run. Run all
> >>>>>> if not
> >>>>>> specified") @@ -48,6 +48,14 @@ def kill(process):
> >>>>>> type=int,
> >>>>>> help="Operations enqueue/dequeue burst size.",
> >>>>>> default=[32])
> >>>>>> +parser.add_argument("-s", "--snr",
> >>>>>> + type=int,
> >>>>>> + help="SNR in dB for BLER tests",
> >>>>>> + default=0)
> >>>>>> +parser.add_argument("-t", "--iter-max",
> >>>>>
> >>>>> We shouldn't change parameters meaning, it will silently break
> >>>>> existing scripts making use of it.
> >>>>>
> >>>>>> + type=int,
> >>>>>> + help="Max iterations",
> >>>>>> + default=6)
> >>>>>> parser.add_argument("-l", "--num-lcores",
> >>>>>> type=int,
> >>>>>> help="Number of lcores to run.", @@ -68,6
> >>>>>> +76,12 @@ def kill(process):
> >>>>>>
> >>>>>> params.extend(["--"])
> >>>>>>
> >>>>>> +if args.snr:
> >>>>>> + params.extend(["-s", str(args.snr)])
> >>>>>> +
> >>>>>> +if args.iter_max:
> >>>>>> + params.extend(["-t", str(args.iter_max)])
> >>>>>> +
> >>>>>> if args.num_ops:
> >>>>>> params.extend(["-n", str(args.num_ops)])
> >>>>>>
> >>>>>> diff --git a/app/test-bbdev/test_bbdev_perf.c
> >>>>>> b/app/test-bbdev/test_bbdev_perf.c
> >>>>>> index 276bbf0a2e6d..faea26c10eed 100644
> >>>>>> --- a/app/test-bbdev/test_bbdev_perf.c
> >>>>>> +++ b/app/test-bbdev/test_bbdev_perf.c
> >>>>>> @@ -26,7 +26,7 @@
> >>>>>>
> >>>>>> #define MAX_QUEUES RTE_MAX_LCORE
> >>>>>> #define TEST_REPETITIONS 100
> >>>>>> -#define TIME_OUT_POLL 1e8
> >>>>>> +#define TIME_OUT_POLL 1e9
> >>>>>> #define WAIT_OFFLOAD_US 1000
> >>>>>>
> >>>>>> #ifdef RTE_BASEBAND_FPGA_LTE_FEC
> >>>>
> >
next prev parent reply other threads:[~2023-11-02 18:18 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-29 18:13 [PATCH v1 00/11] test-bbdev changes for 23.11 Hernan Vargas
2023-09-29 18:13 ` [PATCH v1 01/11] test/bbdev: fix python script subprocess Hernan Vargas
2023-10-17 19:05 ` Maxime Coquelin
2023-09-29 18:13 ` [PATCH v1 02/11] test/bbdev: update python script parameters Hernan Vargas
2023-10-17 19:07 ` Maxime Coquelin
2023-10-19 9:01 ` Chautru, Nicolas
2023-10-19 9:19 ` Maxime Coquelin
2023-10-19 12:09 ` Chautru, Nicolas
2023-10-19 12:20 ` Maxime Coquelin
2023-10-27 20:02 ` Chautru, Nicolas
2023-11-02 17:00 ` Maxime Coquelin
2023-11-02 18:18 ` Chautru, Nicolas [this message]
2023-11-03 9:48 ` Maxime Coquelin
2023-09-29 18:13 ` [PATCH v1 03/11] test/bbdev: rename macros from acc200 to vrb Hernan Vargas
2023-10-17 19:30 ` Maxime Coquelin
2023-09-29 18:13 ` [PATCH v1 04/11] test/bbdev: handle exception for LLR generation Hernan Vargas
2023-10-17 19:30 ` Maxime Coquelin
2023-09-29 18:13 ` [PATCH v1 05/11] test/bbdev: improve test log messages Hernan Vargas
2023-10-17 19:40 ` Maxime Coquelin
2023-09-29 18:13 ` [PATCH v1 06/11] test/bbdev: assert failed test for queue configure Hernan Vargas
2023-10-17 19:43 ` Maxime Coquelin
2023-10-19 8:41 ` Chautru, Nicolas
2023-10-19 8:47 ` Maxime Coquelin
2023-10-19 12:12 ` Chautru, Nicolas
2023-10-23 9:05 ` Maxime Coquelin
2023-10-23 9:07 ` Maxime Coquelin
2023-10-23 15:10 ` Kevin Traynor
2023-09-29 18:13 ` [PATCH v1 07/11] test/bbdev: ldpc encoder concatenation vector Hernan Vargas
2023-10-17 19:44 ` Maxime Coquelin
2023-09-29 18:13 ` [PATCH v1 08/11] test/bbdev: increase max burst size Hernan Vargas
2023-10-17 19:45 ` Maxime Coquelin
2023-09-29 18:13 ` [PATCH v1 09/11] test/bbdev: add MLD support Hernan Vargas
2023-10-17 20:03 ` Maxime Coquelin
2023-09-29 18:13 ` [PATCH v1 10/11] test/bbdev: support new FFT capabilities Hernan Vargas
2023-10-17 20:06 ` Maxime Coquelin
2023-09-29 18:13 ` [PATCH v1 11/11] test/bbdev: support 4 bit LLR compression Hernan Vargas
2023-10-17 20:08 ` Maxime Coquelin
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=BY5PR11MB44517B03FEAA13602E14E34AF8A6A@BY5PR11MB4451.namprd11.prod.outlook.com \
--to=nicolas.chautru@intel.com \
--cc=dev@dpdk.org \
--cc=gakhil@marvell.com \
--cc=hemant.agrawal@nxp.com \
--cc=hernan.vargas@intel.com \
--cc=maxime.coquelin@redhat.com \
--cc=qi.z.zhang@intel.com \
--cc=trix@redhat.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).