From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966859AbbLPVTg (ORCPT ); Wed, 16 Dec 2015 16:19:36 -0500 Received: from mail-pf0-f173.google.com ([209.85.192.173]:33351 "EHLO mail-pf0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965885AbbLPVTe (ORCPT ); Wed, 16 Dec 2015 16:19:34 -0500 Message-ID: <1450300772.8474.96.camel@edumazet-glaptop2.roam.corp.google.com> Subject: Re: [PATCH net-next] hv_netvsc: Use simple parser for IPv4 and v6 headers From: Eric Dumazet To: Haiyang Zhang , Tom Herbert Cc: "davem@davemloft.net" , "netdev@vger.kernel.org" , KY Srinivasan , "olaf@aepfle.de" , "linux-kernel@vger.kernel.org" , "driverdev-devel@linuxdriverproject.org" Date: Wed, 16 Dec 2015 13:19:32 -0800 In-Reply-To: References: <1450289022-9958-1-git-send-email-haiyangz@microsoft.com> <1450285688.8474.86.camel@edumazet-glaptop2.roam.corp.google.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.10.4-0ubuntu2 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 2015-12-16 at 19:20 +0000, Haiyang Zhang wrote: > > -----Original Message----- > > From: Eric Dumazet [mailto:eric.dumazet@gmail.com] > > Sent: Wednesday, December 16, 2015 12:08 PM > > > > This looks very very wrong to me. > > > > How many times this is called per second, for the 'one flow' case ? > > > > Don't you use TSO in this driver ? > > > > What about encapsulation ? > > > > I suspect you have a quite different issue here. > > > > You simply could use skb_get_hash() since local TCP flows will provide a > > l4 skb->hash and you have no further flow dissection to do. > > In our test, we have bisected and found the following patch introduced big > overhead into skb_flow_dissect_flow_keys(), and caused performance > regression: > commit: d34af823 > net: Add VLAN ID to flow_keys Adding Tom Herbert Your driver was assuming things about "struct flow_keys" layout. This is not permitted. Magic numbers like 12 and 8 are really bad... static bool netvsc_set_hash(u32 *hash, struct sk_buff *skb) { struct flow_keys flow; int data_len; if (!skb_flow_dissect_flow_keys(skb, &flow, 0) || !(flow.basic.n_proto == htons(ETH_P_IP) || flow.basic.n_proto == htons(ETH_P_IPV6))) return false; if (flow.basic.ip_proto == IPPROTO_TCP) data_len = 12; else data_len = 8; *hash = comp_hash(netvsc_hash_key, HASH_KEYLEN, &flow, data_len); return true; } > This patch didn't add too many instructions, but we think the change to > the size of struct flow_keys may cause different cache missing rate... > > To avoid affecting other drivers using this function, our patch limits the > change inside our driver to fix this performance regression. > > Regarding your suggestion on skb_get_hash(), I looked at the code and ran > some tests, and found the skb->l4_hash and skb->sw_hash bits are not set, > so it calls __skb_get_hash() which eventually calls > skb_flow_dissect_flow_keys(). So it still includes the performance > overhead mentioned above. Okay, but have you tried this instead of just guessing ? Are you forwarding traffic, or is the traffic locally generated ? TCP stack does set skb->l4_hash for sure in current kernels. Your 'basic flow dissection' is very buggy and a step backward. Just call skb_get_hash() : Not only your perf problem will vanish, but your driver will correctly work with all possible malformed packets (like pretending to be TCP packets but too small to even contain one byte of TCP header) and well formed ones, with all encapsulations. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Dumazet Subject: Re: [PATCH net-next] hv_netvsc: Use simple parser for IPv4 and v6 headers Date: Wed, 16 Dec 2015 13:19:32 -0800 Message-ID: <1450300772.8474.96.camel@edumazet-glaptop2.roam.corp.google.com> References: <1450289022-9958-1-git-send-email-haiyangz@microsoft.com> <1450285688.8474.86.camel@edumazet-glaptop2.roam.corp.google.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: "olaf@aepfle.de" , "netdev@vger.kernel.org" , "driverdev-devel@linuxdriverproject.org" , "linux-kernel@vger.kernel.org" , "davem@davemloft.net" To: Haiyang Zhang , Tom Herbert Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: driverdev-devel-bounces@linuxdriverproject.org Sender: "devel" List-Id: netdev.vger.kernel.org On Wed, 2015-12-16 at 19:20 +0000, Haiyang Zhang wrote: > > -----Original Message----- > > From: Eric Dumazet [mailto:eric.dumazet@gmail.com] > > Sent: Wednesday, December 16, 2015 12:08 PM > > > > This looks very very wrong to me. > > > > How many times this is called per second, for the 'one flow' case ? > > > > Don't you use TSO in this driver ? > > > > What about encapsulation ? > > > > I suspect you have a quite different issue here. > > > > You simply could use skb_get_hash() since local TCP flows will provide a > > l4 skb->hash and you have no further flow dissection to do. > > In our test, we have bisected and found the following patch introduced big > overhead into skb_flow_dissect_flow_keys(), and caused performance > regression: > commit: d34af823 > net: Add VLAN ID to flow_keys Adding Tom Herbert Your driver was assuming things about "struct flow_keys" layout. This is not permitted. Magic numbers like 12 and 8 are really bad... static bool netvsc_set_hash(u32 *hash, struct sk_buff *skb) { struct flow_keys flow; int data_len; if (!skb_flow_dissect_flow_keys(skb, &flow, 0) || !(flow.basic.n_proto == htons(ETH_P_IP) || flow.basic.n_proto == htons(ETH_P_IPV6))) return false; if (flow.basic.ip_proto == IPPROTO_TCP) data_len = 12; else data_len = 8; *hash = comp_hash(netvsc_hash_key, HASH_KEYLEN, &flow, data_len); return true; } > This patch didn't add too many instructions, but we think the change to > the size of struct flow_keys may cause different cache missing rate... > > To avoid affecting other drivers using this function, our patch limits the > change inside our driver to fix this performance regression. > > Regarding your suggestion on skb_get_hash(), I looked at the code and ran > some tests, and found the skb->l4_hash and skb->sw_hash bits are not set, > so it calls __skb_get_hash() which eventually calls > skb_flow_dissect_flow_keys(). So it still includes the performance > overhead mentioned above. Okay, but have you tried this instead of just guessing ? Are you forwarding traffic, or is the traffic locally generated ? TCP stack does set skb->l4_hash for sure in current kernels. Your 'basic flow dissection' is very buggy and a step backward. Just call skb_get_hash() : Not only your perf problem will vanish, but your driver will correctly work with all possible malformed packets (like pretending to be TCP packets but too small to even contain one byte of TCP header) and well formed ones, with all encapsulations. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from hemlock.osuosl.org (smtp2.osuosl.org [140.211.166.133]) by ash.osuosl.org (Postfix) with ESMTP id 4A6041C102A for ; Wed, 16 Dec 2015 21:19:35 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by hemlock.osuosl.org (Postfix) with ESMTP id 47D1695CC6 for ; Wed, 16 Dec 2015 21:19:35 +0000 (UTC) Received: from hemlock.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id XiodhpQqonkY for ; Wed, 16 Dec 2015 21:19:34 +0000 (UTC) Received: from mail-pf0-f172.google.com (mail-pf0-f172.google.com [209.85.192.172]) by hemlock.osuosl.org (Postfix) with ESMTPS id 81ABB95C98 for ; Wed, 16 Dec 2015 21:19:34 +0000 (UTC) Received: by mail-pf0-f172.google.com with SMTP id e66so18832918pfe.0 for ; Wed, 16 Dec 2015 13:19:34 -0800 (PST) Message-ID: <1450300772.8474.96.camel@edumazet-glaptop2.roam.corp.google.com> Subject: Re: [PATCH net-next] hv_netvsc: Use simple parser for IPv4 and v6 headers From: Eric Dumazet Date: Wed, 16 Dec 2015 13:19:32 -0800 In-Reply-To: References: <1450289022-9958-1-git-send-email-haiyangz@microsoft.com> <1450285688.8474.86.camel@edumazet-glaptop2.roam.corp.google.com> Mime-Version: 1.0 List-Id: Linux Driver Project Developer List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: driverdev-devel-bounces@linuxdriverproject.org Sender: "devel" To: Haiyang Zhang , Tom Herbert Cc: "olaf@aepfle.de" , "netdev@vger.kernel.org" , "driverdev-devel@linuxdriverproject.org" , "linux-kernel@vger.kernel.org" , "davem@davemloft.net" On Wed, 2015-12-16 at 19:20 +0000, Haiyang Zhang wrote: > > -----Original Message----- > > From: Eric Dumazet [mailto:eric.dumazet@gmail.com] > > Sent: Wednesday, December 16, 2015 12:08 PM > > > > This looks very very wrong to me. > > > > How many times this is called per second, for the 'one flow' case ? > > > > Don't you use TSO in this driver ? > > > > What about encapsulation ? > > > > I suspect you have a quite different issue here. > > > > You simply could use skb_get_hash() since local TCP flows will provide a > > l4 skb->hash and you have no further flow dissection to do. > > In our test, we have bisected and found the following patch introduced big > overhead into skb_flow_dissect_flow_keys(), and caused performance > regression: > commit: d34af823 > net: Add VLAN ID to flow_keys Adding Tom Herbert Your driver was assuming things about "struct flow_keys" layout. This is not permitted. Magic numbers like 12 and 8 are really bad... static bool netvsc_set_hash(u32 *hash, struct sk_buff *skb) { struct flow_keys flow; int data_len; if (!skb_flow_dissect_flow_keys(skb, &flow, 0) || !(flow.basic.n_proto == htons(ETH_P_IP) || flow.basic.n_proto == htons(ETH_P_IPV6))) return false; if (flow.basic.ip_proto == IPPROTO_TCP) data_len = 12; else data_len = 8; *hash = comp_hash(netvsc_hash_key, HASH_KEYLEN, &flow, data_len); return true; } > This patch didn't add too many instructions, but we think the change to > the size of struct flow_keys may cause different cache missing rate... > > To avoid affecting other drivers using this function, our patch limits the > change inside our driver to fix this performance regression. > > Regarding your suggestion on skb_get_hash(), I looked at the code and ran > some tests, and found the skb->l4_hash and skb->sw_hash bits are not set, > so it calls __skb_get_hash() which eventually calls > skb_flow_dissect_flow_keys(). So it still includes the performance > overhead mentioned above. Okay, but have you tried this instead of just guessing ? Are you forwarding traffic, or is the traffic locally generated ? TCP stack does set skb->l4_hash for sure in current kernels. Your 'basic flow dissection' is very buggy and a step backward. Just call skb_get_hash() : Not only your perf problem will vanish, but your driver will correctly work with all possible malformed packets (like pretending to be TCP packets but too small to even contain one byte of TCP header) and well formed ones, with all encapsulations. _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel