From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 84E1169FC; Wed, 11 Jan 2017 08:59:33 +0100 (CET) Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B4629129206; Wed, 11 Jan 2017 07:59:33 +0000 (UTC) Received: from [10.36.116.159] (ovpn-116-159.ams2.redhat.com [10.36.116.159]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v0B7xTCj030816 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 11 Jan 2017 02:59:31 -0500 To: Yuanhan Liu , dev@dpdk.org References: <1484108832-19907-1-git-send-email-yuanhan.liu@linux.intel.com> <1484108832-19907-2-git-send-email-yuanhan.liu@linux.intel.com> Cc: Tan Jianfeng , Wang Zhihong , Olivier Matz , "Michael S. Tsirkin" , stable@dpdk.org From: Maxime Coquelin Message-ID: Date: Wed, 11 Jan 2017 08:59:28 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <1484108832-19907-2-git-send-email-yuanhan.liu@linux.intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Wed, 11 Jan 2017 07:59:33 +0000 (UTC) Subject: Re: [dpdk-dev] [PATCH 1/2] net/virtio: fix performance regression due to TSO enabling X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 11 Jan 2017 07:59:33 -0000 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 > Cc: Maxime Coquelin > Cc: Michael S. Tsirkin > Cc: stable@dpdk.org > Signed-off-by: Yuanhan Liu > --- > 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 Thanks! Maxime