Re: [PATCH v8 4/5] app/testpmd: report lcore usage

From: David Marchand <david.marchand@redhat.com>
To: Robin Jarry <rjarry@redhat.com>,
	Aman Singh <aman.deep.singh@intel.com>,
	 Yuying Zhang <yuying.zhang@intel.com>
Cc: dev@dpdk.org, "Morten Brørup" <mb@smartsharesystems.com>,
	"Konstantin Ananyev" <konstantin.v.ananyev@yandex.ru>,
	"Kevin Laatz" <kevin.laatz@intel.com>
Subject: Re: [PATCH v8 4/5] app/testpmd: report lcore usage
Date: Mon, 6 Feb 2023 16:06:25 +0100	[thread overview]
Message-ID: <CAJFAV8zT+Za2Asr1AP7UePRLrh-yUbWGWM9VHNY80gjPiqXa4g@mail.gmail.com> (raw)
In-Reply-To: <CQBD1740I7MF.27AX8WCBNT5OF@ringo>

On Mon, Feb 6, 2023 at 10:08 AM Robin Jarry <rjarry@redhat.com> wrote:
>
> David Marchand, Feb 06, 2023 at 09:58:
> > I have been playing a bit with this series with two lcores, each one
> > polling a net/null port.
> > At first it looked good, but then I started to have one idle lcore, by
> > asking net/null not to receive anything.
> >
> > $ build-clang/app/dpdk-testpmd -c 7 --no-huge -m 40 -a 0:0.0 --vdev
> > net_null1,no-rx=1 --vdev net_null2 -- --no-mlockall
> > --total-num-mbufs=2048 -ia --record-core-cycles --nb-cores=2
> >
> > One thing that struck me is that an idle lcore was always showing less
> > "total_cycles" than a busy one.
> > The more time testpmd was running, the bigger the divergence between
> > lcores would be.
> >
> > Re-reading the API, it is unclear to me (which is the reason for my
> > comments on patch 2).
> > Let's first sort out my patch 2 comments and we may revisit this patch
> > 4 implementation afterwards (as I think we are not accounting some
> > mainloop cycles with current implementation).
>
> Indeed, we are not accounting for all cycles. Only the cycles spent in
> the packet_fwd_t functions. This was already the case before my series
> I only added the busy cycles accounting.

"busy" cycles is what was already present in testpmd under the
core_cycles report existing feature: get_end_cycles was only called
with nb_rx + nb_tx > 0.
The only change with this patch is its internal name, there is no
addition on this topic.

But this patch adds "total_cycles" for testpmd...

>
>
> However, I agree that this should be updated to take all cycles into
> account (as much as it is possible with the current code base). Maybe
> this could be done as a separate patch or do you want to include it in
> this series?

... and its implementation seems non compliant with the lcore_usage
API as discussed in patch 2.

As for how much cycles are counted as busy (meaning, should we count
cycles spent in the mainloop too), I think it is better but that would
be a change in the core_cycles report existing feature.

I'd really like to hear from testpmd maintainers.

-- 
David Marchand