From: Maxime Coquelin <maxime.coquelin@redhat.com>
To: Yuanhan Liu <yuanhan.liu@linux.intel.com>, dev@dpdk.org
Cc: Tan Jianfeng <jianfeng.tan@intel.com>,
Wang Zhihong <zhihong.wang@intel.com>,
Olivier Matz <olivier.matz@6wind.com>,
"Michael S. Tsirkin" <mst@redhat.com>,
stable@dpdk.org
Subject: Re: [dpdk-stable] [PATCH 1/2] net/virtio: fix performance regression due to TSO enabling
Date: Wed, 11 Jan 2017 08:59:28 +0100 [thread overview]
Message-ID: <e470dbd9-be63-f439-7d2e-dc653f77e514@redhat.com> (raw)
In-Reply-To: <1484108832-19907-2-git-send-email-yuanhan.liu@linux.intel.com>
On 01/11/2017 05:27 AM, Yuanhan Liu wrote:
> TSO is now enabled, but it's not actually being used by default in a
> simple L2 forward mode. In such case, we have to zero the virtio net
> headers, to inform the vhost backend that no offload is being used:
>
> hdr->csum_start = 0;
> hdr->csum_offset = 0;
> hdr->flags = 0;
>
> hdr->gso_type = 0;
> hdr->gso_size = 0;
> hdr->hdr_len = 0;
>
> Such writes could be very costly; it introduces severe cache issues:
> The above operations introduce cache write for each packet, which
> stalls the read operation from the vhost backend.
>
> 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.
Very nice!
>
> Fixes: 58169a9c8153 ("net/virtio: support Tx checksum offload")
> Fixes: 696573046e9e ("net/virtio: support TSO")
>
> Cc: Olivier Matz <olivier.matz@6wind.com>
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: stable@dpdk.org
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> ---
> drivers/net/virtio/virtio_rxtx.c | 18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> index 1e5a6b9..8ec2f1a 100644
> --- a/drivers/net/virtio/virtio_rxtx.c
> +++ b/drivers/net/virtio/virtio_rxtx.c
> @@ -258,6 +258,12 @@
> vtpci_with_feature(hw, VIRTIO_NET_F_HOST_TSO6);
> }
>
> +/* avoid write operation when necessary, to lessen cache issues */
> +#define ASSIGN_UNLESS_EQUAL(var, val) do { \
> + if ((var) != (val)) \
> + (var) = (val); \
> +} while (0)
As it is intended to go in -stable, I think this is fine to have it
only in the driver, but for v17.02, maybe we should have another patch
on top that declares it somewhere so that other libs and drivers can
make use of it?
> +
> static inline void
> virtqueue_enqueue_xmit(struct virtnet_tx *txvq, struct rte_mbuf *cookie,
> uint16_t needed, int use_indirect, int can_push)
> @@ -337,9 +343,9 @@
> break;
>
> default:
> - hdr->csum_start = 0;
> - hdr->csum_offset = 0;
> - hdr->flags = 0;
> + ASSIGN_UNLESS_EQUAL(hdr->csum_start, 0);
> + ASSIGN_UNLESS_EQUAL(hdr->csum_offset, 0);
> + ASSIGN_UNLESS_EQUAL(hdr->flags, 0);
> break;
> }
>
> @@ -355,9 +361,9 @@
> cookie->l3_len +
> cookie->l4_len;
> } else {
> - hdr->gso_type = 0;
> - hdr->gso_size = 0;
> - hdr->hdr_len = 0;
> + ASSIGN_UNLESS_EQUAL(hdr->gso_type, 0);
> + ASSIGN_UNLESS_EQUAL(hdr->gso_size, 0);
> + ASSIGN_UNLESS_EQUAL(hdr->hdr_len, 0);
> }
> }
>
>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Thanks!
Maxime
next prev parent reply other threads:[~2017-01-11 7:59 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <1484108832-19907-1-git-send-email-yuanhan.liu@linux.intel.com>
2017-01-11 4:27 ` Yuanhan Liu
2017-01-11 7:59 ` Maxime Coquelin [this message]
2017-01-11 8:08 ` Yuanhan Liu
2017-01-11 8:22 ` Olivier MATZ
2017-01-11 4:27 ` [dpdk-stable] [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=e470dbd9-be63-f439-7d2e-dc653f77e514@redhat.com \
--to=maxime.coquelin@redhat.com \
--cc=dev@dpdk.org \
--cc=jianfeng.tan@intel.com \
--cc=mst@redhat.com \
--cc=olivier.matz@6wind.com \
--cc=stable@dpdk.org \
--cc=yuanhan.liu@linux.intel.com \
--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).