* [dpdk-stable] [PATCH 1/2] net/virtio: fix performance regression due to TSO enabling
[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
2017-01-11 4:27 ` [dpdk-stable] [PATCH 2/2] net/virtio: optimize header reset on any layout Yuanhan Liu
1 sibling, 1 reply; 6+ messages in thread
From: Yuanhan Liu @ 2017-01-11 4:27 UTC (permalink / raw)
To: dev
Cc: Tan Jianfeng, Wang Zhihong, Yuanhan Liu, Olivier Matz,
Maxime Coquelin, Michael S. Tsirkin, stable
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.
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)
+
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);
}
}
--
1.9.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [dpdk-stable] [PATCH 2/2] net/virtio: optimize header reset on any layout
[not found] <1484108832-19907-1-git-send-email-yuanhan.liu@linux.intel.com>
2017-01-11 4:27 ` [dpdk-stable] [PATCH 1/2] net/virtio: fix performance regression due to TSO enabling Yuanhan Liu
@ 2017-01-11 4:27 ` Yuanhan Liu
2017-01-11 8:01 ` Maxime Coquelin
1 sibling, 1 reply; 6+ messages in thread
From: Yuanhan Liu @ 2017-01-11 4:27 UTC (permalink / raw)
To: dev
Cc: Tan Jianfeng, Wang Zhihong, Yuanhan Liu, Maxime Coquelin,
Michael S. Tsirkin, stable
When any layout is used, the header is stored in the head room of mbuf.
mbuf is allocated and filled by user, means there is no gurateen the
header is all zero for non TSO case. Therefore, we have to do the reset
by ourself:
memest(hdr, 0, head_size);
The memset has two impacts on performance:
- memset could not be inlined, which is a bit costly.
- more importantly, it touches the mbuf, which could introduce severe
cache issues as described by former patch.
Similiary, we could do the same trick: reset just when necessary, when
the corresponding field is already 0, which is likely true for a simple
l2 forward case. It could boost the performance up to 20+% in micro
benchmarking.
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 | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 8ec2f1a..5ca3a88 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -292,8 +292,14 @@
hdr = (struct virtio_net_hdr *)
rte_pktmbuf_prepend(cookie, head_size);
/* if offload disabled, it is not zeroed below, do it now */
- if (offload == 0)
- memset(hdr, 0, head_size);
+ if (offload == 0) {
+ ASSIGN_UNLESS_EQUAL(hdr->csum_start, 0);
+ ASSIGN_UNLESS_EQUAL(hdr->csum_offset, 0);
+ ASSIGN_UNLESS_EQUAL(hdr->flags, 0);
+ ASSIGN_UNLESS_EQUAL(hdr->gso_type, 0);
+ ASSIGN_UNLESS_EQUAL(hdr->gso_size, 0);
+ ASSIGN_UNLESS_EQUAL(hdr->hdr_len, 0);
+ }
} else if (use_indirect) {
/* setup tx ring slot to point to indirect
* descriptor list stored in reserved region.
--
1.9.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-stable] [PATCH 1/2] net/virtio: fix performance regression due to TSO enabling
2017-01-11 4:27 ` [dpdk-stable] [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
0 siblings, 2 replies; 6+ messages in thread
From: Maxime Coquelin @ 2017-01-11 7:59 UTC (permalink / raw)
To: Yuanhan Liu, dev
Cc: Tan Jianfeng, Wang Zhihong, Olivier Matz, Michael S. Tsirkin, stable
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-stable] [PATCH 2/2] net/virtio: optimize header reset on any layout
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
0 siblings, 0 replies; 6+ messages in thread
From: Maxime Coquelin @ 2017-01-11 8:01 UTC (permalink / raw)
To: Yuanhan Liu, dev; +Cc: Tan Jianfeng, Wang Zhihong, Michael S. Tsirkin, stable
On 01/11/2017 05:27 AM, Yuanhan Liu wrote:
> When any layout is used, the header is stored in the head room of mbuf.
> mbuf is allocated and filled by user, means there is no gurateen the
> header is all zero for non TSO case. Therefore, we have to do the reset
> by ourself:
>
> memest(hdr, 0, head_size);
>
> The memset has two impacts on performance:
>
> - memset could not be inlined, which is a bit costly.
> - more importantly, it touches the mbuf, which could introduce severe
> cache issues as described by former patch.
>
> Similiary, we could do the same trick: reset just when necessary, when
> the corresponding field is already 0, which is likely true for a simple
> l2 forward case. It could boost the performance up to 20+% in micro
> benchmarking.
>
> 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 | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> index 8ec2f1a..5ca3a88 100644
> --- a/drivers/net/virtio/virtio_rxtx.c
> +++ b/drivers/net/virtio/virtio_rxtx.c
> @@ -292,8 +292,14 @@
> hdr = (struct virtio_net_hdr *)
> rte_pktmbuf_prepend(cookie, head_size);
> /* if offload disabled, it is not zeroed below, do it now */
> - if (offload == 0)
> - memset(hdr, 0, head_size);
> + if (offload == 0) {
> + ASSIGN_UNLESS_EQUAL(hdr->csum_start, 0);
> + ASSIGN_UNLESS_EQUAL(hdr->csum_offset, 0);
> + ASSIGN_UNLESS_EQUAL(hdr->flags, 0);
> + ASSIGN_UNLESS_EQUAL(hdr->gso_type, 0);
> + ASSIGN_UNLESS_EQUAL(hdr->gso_size, 0);
> + ASSIGN_UNLESS_EQUAL(hdr->hdr_len, 0);
> + }
> } else if (use_indirect) {
> /* setup tx ring slot to point to indirect
> * descriptor list stored in reserved region.
>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Thanks!
Maxime
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-stable] [PATCH 1/2] net/virtio: fix performance regression due to TSO enabling
2017-01-11 7:59 ` Maxime Coquelin
@ 2017-01-11 8:08 ` Yuanhan Liu
2017-01-11 8:22 ` Olivier MATZ
1 sibling, 0 replies; 6+ messages in thread
From: Yuanhan Liu @ 2017-01-11 8:08 UTC (permalink / raw)
To: Maxime Coquelin
Cc: dev, Tan Jianfeng, Wang Zhihong, Olivier Matz,
Michael S. Tsirkin, stable, Thomas Monjalon
On Wed, Jan 11, 2017 at 08:59:28AM +0100, Maxime Coquelin wrote:
> >+/* 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?
Yes, if it has any other usages :)
--yliu
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-stable] [PATCH 1/2] net/virtio: fix performance regression due to TSO enabling
2017-01-11 7:59 ` Maxime Coquelin
2017-01-11 8:08 ` Yuanhan Liu
@ 2017-01-11 8:22 ` Olivier MATZ
1 sibling, 0 replies; 6+ messages in thread
From: Olivier MATZ @ 2017-01-11 8:22 UTC (permalink / raw)
To: Maxime Coquelin
Cc: Yuanhan Liu, dev, Tan Jianfeng, Wang Zhihong, Olivier Matz,
Michael S. Tsirkin, stable
Hi Yuanhan,
Thanks for the fix.
On Wed, 11 Jan 2017 08:59:28 +0100, Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
> 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!
Yep, I'm also impressed by the result.
I would have think this is something already done by the hardware and
transparent to the software.
>
> >
> > 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>
>
> [...]
>
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Reviewed-by: Olivier Matz <olivier.matz@6wind.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-01-11 8:23 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1484108832-19907-1-git-send-email-yuanhan.liu@linux.intel.com>
2017-01-11 4:27 ` [dpdk-stable] [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 4:27 ` [dpdk-stable] [PATCH 2/2] net/virtio: optimize header reset on any layout Yuanhan Liu
2017-01-11 8:01 ` Maxime Coquelin
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).