From: Yuanhan Liu <yuanhan.liu@linux.intel.com>
To: Jan Viktorin <viktorin@rehivetech.com>
Cc: "Thomas Monjalon" <thomas.monjalon@6wind.com>,
"Jianbo Liu" <jianbo.liu@linaro.org>,
"Jerin Jacob" <jerin.jacob@cavium.com>,
"Chao Zhu" <chaozhu@linux.vnet.ibm.com>,
dev@dpdk.org, "Tan Jianfeng" <jianfeng.tan@intel.com>,
"Wang Zhihong" <zhihong.wang@intel.com>,
"Olivier Matz" <olivier.matz@6wind.com>,
"Maxime Coquelin" <maxime.coquelin@redhat.com>,
"Michael S. Tsirkin" <mst@redhat.com>,
"Orsák Michal" <xorsak01@stud.fit.vutbr.cz>
Subject: Re: [dpdk-dev] [dpdk-stable] [PATCH 1/2] net/virtio: fix performance regression due to TSO enabling
Date: Fri, 13 Jan 2017 14:13:09 +0800 [thread overview]
Message-ID: <20170113061309.GF9770@yliu-dev.sh.intel.com> (raw)
In-Reply-To: <20170112160256.6915ff12.viktorin@rehivetech.com>
On Thu, Jan 12, 2017 at 04:02:56PM +0100, Jan Viktorin wrote:
> On Thu, 12 Jan 2017 10:30:58 +0800
> Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote:
>
> > On Wed, Jan 11, 2017 at 03:51:22PM +0100, Thomas Monjalon wrote:
> > > 2017-01-11 12:27, Yuanhan Liu:
> > > > The fact that virtio net header is initiated to zero in PMD driver
> > > > init stage means that these costly writes are unnecessary and could
> > > > be avoided:
> > > >
> > > > if (hdr->csum_start != 0)
> > > > hdr->csum_start = 0;
> > > >
> > > > And that's what the macro ASSIGN_UNLESS_EQUAL does. With this, the
> > > > performance drop introduced by TSO enabling is recovered: it could
> > > > be up to 20% in micro benchmarking.
> > >
> > > This patch is adding a condition to assignments.
> > > We need a benchmark on other architectures like ARM. Please anyone?
> >
> > I think the cost of condition should be way lower than the cost from the
> > penalty introduced by the cache issue, that I don't see it would perform
> > bad on other platforms.
> >
> > But, of course, testing is always welcome!
> >
> > --yliu
>
> Hello,
>
> we've done a synthetic measurement, principle briefly:
Thanks!
>
> == Without condition check ==
>
> start = gettimeofday();
>
> for (i = 0; i < 1024*1024*128; ++i) {
> hdr->csum_start = 0;
> hdr->csum_offset = 0;
> hdr->flags = 0;
> }
>
> end = gettimeofday();
>
>
> == With condition check ==
>
> start = gettimeofday();
>
> for (i = 0; i < 1024*1024*128; ++i) {
> ASSIGN_UNLESS_EQUAL(hdr->csum_start, 0);
> ASSIGN_UNLESS_EQUAL(hdr->csum_offset, 0);
> ASSIGN_UNLESS_EQUAL(hdr->flags, 0);
> }
>
> end = gettimeofday();
But it's not the test methodology I'd expect. You are purely testing
the instruction cycles. The drop on ARM is something more like "the
if instruction takes more cycles than the simple assignment".
This macro is used in the case that one process is heavily writing
same value (0 here) again and again while another process is heavily
read it also again and again. That means cache violation always
happen. With this macro, however, this cache issue could be avoided,
since no write happens.
For such workload, I don't think it would behaviour worse on ARM.
--yliu
> == Results ==
>
> Computed as total time of all threads:
>
> for i = 1..THREAD_COUNT:
> result += end[i] - start[i]
>
> cpu threads without-check (ms) with-check
> Xeon E5-2670 1 516 529
> Xeon E5-2670 2 1155 953
> Xeon E5-2670 8 8947 5044
> Xeon E5-2670 16 23335 16836
> Zynq-7020 (armv7) 1 6735 7205
> Zynq-7020 (armv7) 2 13753 14418
>
> The advantage for Intel is evident when increasing the number
> of threads.
>
> However, on 32-bit ARMs we might expect some performance drop.
>
> Regards
> Jan
>
> > >
> > >
> > > [...]
> > > > +/* avoid write operation when necessary, to lessen cache issues */
> > > > +#define ASSIGN_UNLESS_EQUAL(var, val) do { \
> > > > + if ((var) != (val)) \
> > > > + (var) = (val); \
> > > > +} while (0)
next prev parent reply other threads:[~2017-01-13 6:10 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-01-11 4:27 [dpdk-dev] [PATCH 0/2] net/virtio: optimize virtio net header reset Yuanhan Liu
2017-01-11 4:27 ` [dpdk-dev] [PATCH 1/2] net/virtio: fix performance regression due to TSO enabling Yuanhan Liu
2017-01-11 7:59 ` Maxime Coquelin
2017-01-11 8:08 ` Yuanhan Liu
2017-01-11 8:22 ` Olivier MATZ
2017-01-11 14:51 ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon
2017-01-12 2:30 ` Yuanhan Liu
2017-01-12 15:02 ` Jan Viktorin
2017-01-13 6:13 ` Yuanhan Liu [this message]
2017-01-16 7:12 ` Yuanhan Liu
[not found] ` <46569522-b2c3-2a33-9111-049b73c79760@stud.fit.vutbr.cz>
[not found] ` <20170116111256.GA11439@yliu-dev.sh.intel.com>
[not found] ` <8e8178c6-caa2-1b6e-10a0-c83820868db5@stud.fit.vutbr.cz>
2017-01-16 11:21 ` Yuanhan Liu
[not found] ` <a12848b4-76ef-29bc-f512-81bd8c1b9b76@stud.fit.vutbr.cz>
2017-01-30 13:30 ` Yuanhan Liu
2017-01-30 13:54 ` Maxime Coquelin
2017-01-30 14:10 ` Yuanhan Liu
2017-01-11 4:27 ` [dpdk-dev] [PATCH 2/2] net/virtio: optimize header reset on any layout Yuanhan Liu
2017-01-11 8:01 ` 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=20170113061309.GF9770@yliu-dev.sh.intel.com \
--to=yuanhan.liu@linux.intel.com \
--cc=chaozhu@linux.vnet.ibm.com \
--cc=dev@dpdk.org \
--cc=jerin.jacob@cavium.com \
--cc=jianbo.liu@linaro.org \
--cc=jianfeng.tan@intel.com \
--cc=maxime.coquelin@redhat.com \
--cc=mst@redhat.com \
--cc=olivier.matz@6wind.com \
--cc=thomas.monjalon@6wind.com \
--cc=viktorin@rehivetech.com \
--cc=xorsak01@stud.fit.vutbr.cz \
--cc=zhihong.wang@intel.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).