From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56182) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cA5SC-0003e4-An for qemu-devel@nongnu.org; Thu, 24 Nov 2016 20:37:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cA5S9-0003SR-8D for qemu-devel@nongnu.org; Thu, 24 Nov 2016 20:37:28 -0500 Received: from mx1.redhat.com ([209.132.183.28]:54568) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1cA5S8-0003Qn-Vw for qemu-devel@nongnu.org; Thu, 24 Nov 2016 20:37:25 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (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 7C67685542 for ; Fri, 25 Nov 2016 01:37:22 +0000 (UTC) References: <1477935706-4250-1-git-send-email-wexu@redhat.com> From: Wei Xu Message-ID: <583795CE.2060700@redhat.com> Date: Fri, 25 Nov 2016 09:37:18 +0800 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [ RFC Patch v7 0/2] Support Receive-Segment-Offload(RSC) for WHQL List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jason Wang , qemu-devel@nongnu.org Cc: yvugenfi@redhat.com, dfleytma@redhat.com, mst@redhat.com On 2016=E5=B9=B411=E6=9C=8824=E6=97=A5 12:28, Jason Wang wrote: > > > On 2016=E5=B9=B411=E6=9C=8801=E6=97=A5 01:41, wexu@redhat.com wrote: >> From: Wei Xu >> >> This patch is to support WHQL test for Windows guest, while this >> feature also benifits other guest works as a kernel 'gro' like >> feature with userspace implementation. >> >> Feature information: >> http://msdn.microsoft.com/en-us/library/windows/hardware/jj853324 >> >> v6->v7 >> - Change the drain timer from 'virtual' to 'host' since it invisible >> to guest. >> - Move the buffer list empty check to virtio_net_rsc_do_coalesc(). >> - The header comparision is a bit odd for ipv4 in this patch, it >> should be simpler with equal check, but this is also a helper for i= pv6 >> in next patch, and ipv6 used a different size address fields, so i >> used >> an 'address + size' byte comparision for address, and change compar= ing >> the tcp port with 'int' equal check. >> - Add count for packets whose size less than a normal tcp packet in >> sanity check. >> - Move constant value comparison to the right side of the equal symbol= . >> - Use host header length in stead of guest header length to verify a >> packet in virtio_net_rsc_receive(), in case of the different header >> length for guest and host. >> - Check whether the packet size is enough to hold a legal packet befor= e >> extract ip unit. >> - Bypass ip/tcp ECN packets. >> - Expand the feature bit definition from 32 to 64 bits. >> >> Other notes: >> - About tcp windows scale, we don't have connection tracking about all >> tcp connections, so we don't know what the exact window size is usi= ng, >> thus this feature may get negative influence to it, have to turn th= is >> feature off for such a user case currently. >> - There are 2 new fields in the virtio net header, it's not in either >> kernel tree or maintainer's tree right now, I just put it directly >> here. >> - The statistics is kept in this version since it's helpful for >> troubleshooting. > > Please do not adding more and more stuffs in the same patch. Instead, > you can add them by using new patches on top. This can greatly simplify > the reviewers' work. E.g in this version, it looks like the parts of > virtio extension brings lots of troubles. So I suggest to split the > patch into several parts: > > - helpers (e.g macro for ECN bit) > - core coalescing logic which has been reviewed for several version, > please do not add more functions to this part. This part could be even > disabled in the code until virtio part is introduced. > - virtio extension (e.g virtio-net header extension and feature bits) > - stats OK, get split it in next version. > > Thanks