From: "Mattias Rönnblom" <mattias.ronnblom@ericsson.com>
To: David Marchand <david.marchand@redhat.com>,
Harry <harry.van.haaren@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
"Honnappa Nagarahalli" <Honnappa.Nagarahalli@arm.com>,
"Morten Brørup" <mb@smartsharesystems.com>, nd <nd@arm.com>
Subject: Re: [PATCH 1/6] service: reduce statistics overhead for parallel services
Date: Mon, 3 Oct 2022 11:37:11 +0000 [thread overview]
Message-ID: <e777e82a-28a5-f9a2-bd56-f40ed4a70fc4@ericsson.com> (raw)
In-Reply-To: <CAJFAV8w66W7D_9j=nF_As+SF7=iDjKRqS+m8xTC30WcfOdoN4Q@mail.gmail.com>
On 2022-10-03 11:53, David Marchand wrote:
> On Mon, Oct 3, 2022 at 10:40 AM Mattias Rönnblom
> <mattias.ronnblom@ericsson.com> wrote:
>>
>> On 2022-10-03 10:06, David Marchand wrote:
>>> On Tue, Sep 6, 2022 at 6:17 PM Mattias Rönnblom
>>> <mattias.ronnblom@ericsson.com> wrote:
>>>>
>>>> Move the statistics from the service data structure to the per-lcore
>>>> struct. This eliminates contention for the counter cache lines, which
>>>> decreases the producer-side statistics overhead for services deployed
>>>> across many lcores.
>>>>
>>>> Prior to this patch, enabling statistics for a service with a
>>>> per-service function call latency of 1000 clock cycles deployed across
>>>> 16 cores on a Intel Xeon 6230N @ 2,3 GHz would incur a cost of ~10000
>>>> core clock cycles per service call. After this patch, the statistics
>>>> overhead is reduce to 22 clock cycles per call.
>>>>
>>>> Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
>>>
>>> Re-reading the mail archive, I found that Morten acked the series
>>> (replying on patch 4).
>>> Mattias, such a series deserved a cover letter.
>>>
>>
>> Noted. Do you want a v2 with a cover letter?
>>
>>> How does this series fit with other patches from Harry?
>>> https://protect2.fireeye.com/v1/url?k=31323334-501d5122-313273af-454445555731-9ac8e99afde9bc77&q=1&e=946b2bf5-b58c-4c02-b173-bba36b0b12f9&u=https%3A%2F%2Fpatchwork.dpdk.org%2Fproject%2Fdpdk%2Flist%2F%3Fseries%3D23959%26state%3D%2A
>>>
>>> Thanks.
>>>
>>
>> The patchset I submitted is made against the above-linked patchset.
>> Nothing in the patchset I submitted depends on those patches. I just
>> assumed that Harry's patches would have been merged at the point my
>> patchset was considered.
>
> Harry v1 patch was indeed sent a while ago.
> But this v1 patch from Harry was superseded with a v2 and v3 series.
> There were comments on the v3 series from Harry.
>
>
> Now, looking at this series, without a cover letter, I had to guess.
> I saw it is linked to the v1 patch.
> I "assumed" it was then an alternative, since you had comments on
> Harry v3 patch, or at least Harry would reply and we could conclude.
>
Sorry for failing to mention enough context in the patchset. I assumed
it was Harry and not you that would resolve this issue. Harry's patchset
fixes the statistics bug related to MT safe services, but does not
address the performance issue discussed. So Harry's patchset makes sense
on its own. It also adds a performance test case.
I believe the test case is the only thing left of Harry's improvements
after my patchset is applied.
My patchset was meant as an improvement on what Harry already had done,
not as an alternative.
This is all up the maintainer of course, but it seems to me that Harry's
patchset should go in first, and then mine.
If Harry or you so prefer, I can rework my patchset to apply cleanly
against current main (i.e., w/o Harry's patches).
>
> So what do we do?
> Should I understand that your comments on Harry series can be ignored
> and I proceed with all this?
>
My comments were minor, except those that relates to the issue that my
follow-up patchset addresses.
> I hope it applies cleanly.
>
>
next prev parent reply other threads:[~2022-10-03 11:37 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-08 12:56 [PATCH 1/2] test/service: add perf measurements for with stats mode Harry van Haaren
2022-07-08 12:56 ` [PATCH 2/2] service: fix potential stats race-condition on MT services Harry van Haaren
2022-07-08 13:23 ` Morten Brørup
2022-07-08 13:44 ` Van Haaren, Harry
2022-07-08 14:14 ` Morten Brørup
2022-07-08 13:48 ` Mattias Rönnblom
2022-07-08 15:16 ` Honnappa Nagarahalli
2022-07-08 15:31 ` Van Haaren, Harry
2022-07-08 16:21 ` Bruce Richardson
2022-07-08 16:33 ` Honnappa Nagarahalli
2022-07-08 20:02 ` Mattias Rönnblom
2022-07-08 16:29 ` Morten Brørup
2022-07-08 16:45 ` Honnappa Nagarahalli
2022-07-08 17:22 ` Morten Brørup
2022-07-08 17:39 ` Honnappa Nagarahalli
2022-07-08 18:08 ` Morten Brørup
2022-09-06 16:13 ` [PATCH 1/6] service: reduce statistics overhead for parallel services Mattias Rönnblom
2022-09-06 16:13 ` [PATCH 2/6] service: introduce per-lcore cycles counter Mattias Rönnblom
2022-09-06 16:13 ` [PATCH 3/6] service: reduce average case service core overhead Mattias Rönnblom
2022-10-03 13:33 ` Van Haaren, Harry
2022-10-03 14:32 ` Mattias Rönnblom
2022-09-06 16:13 ` [PATCH 4/6] service: tweak cycle statistics semantics Mattias Rönnblom
2022-09-07 8:41 ` Morten Brørup
2022-10-03 13:45 ` Van Haaren, Harry
2022-09-06 16:13 ` [PATCH 5/6] event/sw: report idle when no work is performed Mattias Rönnblom
2022-09-06 16:13 ` [PATCH 6/6] service: provide links to functions in documentation Mattias Rönnblom
2022-10-03 8:06 ` [PATCH 1/6] service: reduce statistics overhead for parallel services David Marchand
2022-10-03 8:40 ` Mattias Rönnblom
2022-10-03 9:53 ` David Marchand
2022-10-03 11:37 ` Mattias Rönnblom [this message]
2022-10-03 13:03 ` Van Haaren, Harry
2022-10-03 13:33 ` Van Haaren, Harry
2022-10-03 14:37 ` Mattias Rönnblom
2022-10-05 9:16 ` [PATCH v2 0/6] Service cores performance and statistics improvements Mattias Rönnblom
2022-10-05 9:16 ` [PATCH v2 1/6] service: reduce statistics overhead for parallel services Mattias Rönnblom
2022-10-05 9:16 ` [PATCH v2 2/6] service: introduce per-lcore cycles counter Mattias Rönnblom
2022-10-05 9:16 ` [PATCH v2 3/6] service: reduce average case service core overhead Mattias Rönnblom
2022-10-05 9:16 ` [PATCH v2 4/6] service: tweak cycle statistics semantics Mattias Rönnblom
2022-10-05 9:16 ` [PATCH v2 5/6] event/sw: report idle when no work is performed Mattias Rönnblom
2022-10-05 9:16 ` [PATCH v2 6/6] service: provide links to functions in documentation Mattias Rönnblom
2022-10-05 9:49 ` [PATCH v2 0/6] Service cores performance and statistics improvements Morten Brørup
2022-10-05 10:14 ` Mattias Rönnblom
2022-10-05 13:39 ` David Marchand
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=e777e82a-28a5-f9a2-bd56-f40ed4a70fc4@ericsson.com \
--to=mattias.ronnblom@ericsson.com \
--cc=Honnappa.Nagarahalli@arm.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=harry.van.haaren@intel.com \
--cc=mb@smartsharesystems.com \
--cc=nd@arm.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).