From: "Van Haaren, Harry" <harry.van.haaren@intel.com>
To: "Morten Brørup" <mb@smartsharesystems.com>,
"dev@dpdk.org" <dev@dpdk.org>
Cc: mattias.ronnblom <mattias.ronnblom@ericsson.com>,
Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
Subject: RE: [PATCH 2/2] service: fix potential stats race-condition on MT services
Date: Fri, 8 Jul 2022 13:44:48 +0000 [thread overview]
Message-ID: <BN0PR11MB57126DAF097ECD2DA5EB6D6FD7829@BN0PR11MB5712.namprd11.prod.outlook.com> (raw)
In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D871B5@smartserver.smartshare.dk>
> -----Original Message-----
> From: Morten Brørup <mb@smartsharesystems.com>
> Sent: Friday, July 8, 2022 2:23 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>; dev@dpdk.org
> Cc: mattias.ronnblom <mattias.ronnblom@ericsson.com>; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>
> Subject: RE: [PATCH 2/2] service: fix potential stats race-condition on MT services
<snip commit message, focus on performance data>
> > This patch causes a 1.25x increase in cycle-cost for polling a
> > MT safe service when statistics are enabled. No change was seen
> > for MT unsafe services, or when statistics are disabled.
> >
> > Reported-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> > Suggested-by: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> > Suggested-by: Morten Brørup <mb@smartsharesystems.com>
> > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> >
> > ---
>
> [...]
>
> > + if (service_mt_safe(s)) {
> > + __atomic_fetch_add(&s->cycles_spent, cycles,
> > __ATOMIC_RELAXED);
> > + __atomic_fetch_add(&s->calls, 1, __ATOMIC_RELAXED);
> > + } else {
> > + s->cycles_spent += cycles;
> > + s->calls++;
> > + }
>
> Have you considered the performance cost of the
> __atomic_fetch_add(__ATOMIC_RELAXED) versus the performance cost of the
> branch to compare if the service is MT safe? It might be cheaper to just always use
> the atomic addition. I don't know, just mentioning that the compare-and-branch also
> has a cost.
Great question!
> I'm not familiar with the DPDK services library, so perhaps MT safe and MT unsafe
> services are never mixed, in which case the branch will always take the same path,
> so branch prediction will eliminate the cost of branching.
MT safe & unsafe can be mixed yes, so you're right, there may be mis-predicts. Note that
assuming a service is actually doing something useful, there's likely quite a few branches
between each call.. so unknown how fresh/accurate the branch history will be.
The common case is for many services to be "mt unsafe" (think polling an ethdev queue).
In this case, it is nice to try reduce cost. Given this is likely the highest quantity of services,
we'd like the performance here to be reduced the least. The branch method achieves that.
I did benchmark the "always use atomic" case, and it caused a ~30cycle hit in the "mt unsafe" case,
where the atomic is not required (and hence the performance hit can be avoided by branching).
Given branch-misses are handled between 15-20 cycles (uarch dependent), attempting to avoid the
atomic still makes sense from cycle-cost perspective too I think..
I did spend the morning benchmarking solutions (and hence the patch split,
to allow easy benchmarking before & after), so thanks for asking!
Regards, -Harry
next prev parent reply other threads:[~2022-07-08 13:44 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 [this message]
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
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=BN0PR11MB57126DAF097ECD2DA5EB6D6FD7829@BN0PR11MB5712.namprd11.prod.outlook.com \
--to=harry.van.haaren@intel.com \
--cc=Honnappa.Nagarahalli@arm.com \
--cc=dev@dpdk.org \
--cc=mattias.ronnblom@ericsson.com \
--cc=mb@smartsharesystems.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).