From: Thomas Monjalon <thomas@monjalon.net>
To: David Marchand <david.marchand@redhat.com>,
Bruce Richardson <bruce.richardson@intel.com>
Cc: dev@dpdk.org, Aaron Conole <aconole@redhat.com>,
Ferruh Yigit <ferruh.yigit@amd.com>
Subject: Re: [PATCH 0/1] make file prefix unit test more resilient
Date: Fri, 22 Sep 2023 14:57:32 +0200 [thread overview]
Message-ID: <11510039.CDJkKcVGEf@thomas> (raw)
In-Reply-To: <ZQrEvJkeCumzn0BO@bricha3-MOBL.ger.corp.intel.com>
20/09/2023 12:09, Bruce Richardson:
> On Wed, Sep 20, 2023 at 12:00:08PM +0200, David Marchand wrote:
> > On Thu, Sep 14, 2023 at 12:42 PM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > >
> > > When examining the IOL testing failures for patch series [1], I observed
> > > that the failures reported were in the eal_flags_file_prefix unit test.
> > > I was able to reproduce this on my system by passing an additional
> > > "--on-pci" flag to the test run, since the log to the test has errors
> > > about device availability. Adding the "no-pci" flag to the individual
> >
> > Something is not clear to me.
> >
> > While I understand that passing "no-pci" helps avoiding the issue (as
> > described below), I have some trouble understanding this passage
> > (above) with "--on-pci".
>
> That's a typo for no-pci. When I ran the test on my system with the main
> process using no-pci, I was able to reproduce the issue seen in the IOL
> lab. Otherwise I couldn't reproduce it.
>
> > How did you reproduce the issue?
> >
> >
> > > test commands used by the unit tests fixed the issue thereafter,
> > > allowing the test to pass in all cases for me. Therefore, I am
> > > submitting this patch in the hopes of making the test more robust, since
> > > the observed failures seem unrelated to the original patchset [1] I
> > > submitted.
> > >
> > > [1] http://patches.dpdk.org/project/dpdk/list/?series=29406
> > >
> > > Bruce Richardson (1):
> > > app/test: skip PCI bus scan when testing prefix flags
> > >
> > > app/test/test_eal_flags.c | 20 ++++++++++----------
> > > 1 file changed, 10 insertions(+), 10 deletions(-)
> >
> > Iiuc, the problem is that the file_prefix unit test can fail if any
> > DPDK subsystem forgets to release some memory and some hugepages are
> > left behind at the cleanup step.
> > Passing --no-pci as you suggest hides issues coming from PCI drivers.
> >
> > This is something I tried to fix too, with
> > https://patchwork.dpdk.org/project/dpdk/list/?series=29288 though my
> > fix only handles a part of the issue (here, the ethdev drivers).
> >
> > Another way to make the file prefix more robust would be to remove the
> > check on released memory, or move it to another test.
> >
> I actually think the test is a good one to have. Also, taking in your patch
> to help with the issue is a good idea also.
>
> I'd still suggest that this patch be considered anyway, as there is no need
> to do PCI bus scanning as part of this test. Therefore I'd view it as a
> harmless addition that may help things.
I'm hesitating.
This test is checking if some memory is left, and I think it is sane.
If we add --no-pci, we reduce the coverage of this check.
Now that the root cause is fixed by David in ethdev
(https://patches.dpdk.org/project/dpdk/patch/20230821085806.3062613-4-david.marchand@redhat.com/)
we could continue checking memory freeing with PCI drivers.
So I tend to reject this patch.
Other opinions?
next prev parent reply other threads:[~2023-09-22 12:57 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-14 10:42 Bruce Richardson
2023-09-14 10:42 ` [PATCH 1/1] app/test: skip PCI bus scan when testing prefix flags Bruce Richardson
2023-09-20 10:00 ` [PATCH 0/1] make file prefix unit test more resilient David Marchand
2023-09-20 10:09 ` Bruce Richardson
2023-09-22 12:57 ` Thomas Monjalon [this message]
2023-09-22 13:23 ` Bruce Richardson
2023-09-23 8:21 ` Thomas Monjalon
2023-09-25 8:02 ` Bruce Richardson
2023-09-26 15:08 ` Aaron Conole
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=11510039.CDJkKcVGEf@thomas \
--to=thomas@monjalon.net \
--cc=aconole@redhat.com \
--cc=bruce.richardson@intel.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@amd.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).