From: Vitaly Kuznetsov <vkuznets@redhat.com> To: netdev@vger.kernel.org Cc: "K. Y. Srinivasan" <kys@microsoft.com>, Haiyang Zhang <haiyangz@microsoft.com>, devel@linuxdriverproject.org, linux-kernel@vger.kernel.org, Eric Dumazet <eric.dumazet@gmail.com>, David Miller <davem@davemloft.net> Subject: [PATCH net-next] hv_netvsc: don't make assumptions on struct flow_keys layout Date: Thu, 7 Jan 2016 10:33:09 +0100 [thread overview] Message-ID: <1452159189-11473-1-git-send-email-vkuznets@redhat.com> (raw) Recent changes to 'struct flow_keys' (e.g commit d34af823ff40 ("net: Add VLAN ID to flow_keys")) introduced a performance regression in netvsc driver. Is problem is, however, not the above mentioned commit but the fact that netvsc_set_hash() function did some assumptions on the struct flow_keys data layout and this is wrong. We need to extract the data we need (src/dst addresses and ports) after the dissect. The issue could also be solved in a completely different way: as suggested by Eric instead of our own homegrown netvsc_set_hash() we could use skb_get_hash() which does more or less the same. Unfortunately, the testing done by Simon showed that Hyper-V hosts are not happy with our Jenkins hash, selecting the output queue with the current algorithm based on Toeplitz hash works significantly better. Tested-by: Simon Xiao <sixiao@microsoft.com> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- This patch is an alternative to Haiyang's "hv_netvsc: Use simple parser for IPv4 and v6 headers". --- drivers/net/hyperv/netvsc_drv.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c index 409b48e..4dea44e 100644 --- a/drivers/net/hyperv/netvsc_drv.c +++ b/drivers/net/hyperv/netvsc_drv.c @@ -238,18 +238,26 @@ static bool netvsc_set_hash(u32 *hash, struct sk_buff *skb) { struct flow_keys flow; int data_len; + u8 data[sizeof(flow.addrs) + sizeof(flow.ports)]; - 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))) + if (!skb_flow_dissect_flow_keys(skb, &flow, 0)) return false; - if (flow.basic.ip_proto == IPPROTO_TCP) - data_len = 12; + if (flow.basic.n_proto == htons(ETH_P_IP)) + data_len = sizeof(flow.addrs.v4addrs); + else if (flow.basic.n_proto == htons(ETH_P_IPV6)) + data_len = sizeof(flow.addrs.v6addrs); else - data_len = 8; + return false; + + memcpy(data, &flow.addrs, data_len); + + if (flow.basic.ip_proto == IPPROTO_TCP) { + memcpy(data + data_len, &flow.ports, sizeof(flow.ports)); + data_len += sizeof(flow.ports); + } - *hash = comp_hash(netvsc_hash_key, HASH_KEYLEN, &flow, data_len); + *hash = comp_hash(netvsc_hash_key, HASH_KEYLEN, data, data_len); return true; } -- 2.4.3
WARNING: multiple messages have this Message-ID (diff)
From: Vitaly Kuznetsov <vkuznets@redhat.com> To: netdev@vger.kernel.org Cc: Eric Dumazet <eric.dumazet@gmail.com>, Haiyang Zhang <haiyangz@microsoft.com>, linux-kernel@vger.kernel.org, devel@linuxdriverproject.org, David Miller <davem@davemloft.net> Subject: [PATCH net-next] hv_netvsc: don't make assumptions on struct flow_keys layout Date: Thu, 7 Jan 2016 10:33:09 +0100 [thread overview] Message-ID: <1452159189-11473-1-git-send-email-vkuznets@redhat.com> (raw) Recent changes to 'struct flow_keys' (e.g commit d34af823ff40 ("net: Add VLAN ID to flow_keys")) introduced a performance regression in netvsc driver. Is problem is, however, not the above mentioned commit but the fact that netvsc_set_hash() function did some assumptions on the struct flow_keys data layout and this is wrong. We need to extract the data we need (src/dst addresses and ports) after the dissect. The issue could also be solved in a completely different way: as suggested by Eric instead of our own homegrown netvsc_set_hash() we could use skb_get_hash() which does more or less the same. Unfortunately, the testing done by Simon showed that Hyper-V hosts are not happy with our Jenkins hash, selecting the output queue with the current algorithm based on Toeplitz hash works significantly better. Tested-by: Simon Xiao <sixiao@microsoft.com> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- This patch is an alternative to Haiyang's "hv_netvsc: Use simple parser for IPv4 and v6 headers". --- drivers/net/hyperv/netvsc_drv.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c index 409b48e..4dea44e 100644 --- a/drivers/net/hyperv/netvsc_drv.c +++ b/drivers/net/hyperv/netvsc_drv.c @@ -238,18 +238,26 @@ static bool netvsc_set_hash(u32 *hash, struct sk_buff *skb) { struct flow_keys flow; int data_len; + u8 data[sizeof(flow.addrs) + sizeof(flow.ports)]; - 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))) + if (!skb_flow_dissect_flow_keys(skb, &flow, 0)) return false; - if (flow.basic.ip_proto == IPPROTO_TCP) - data_len = 12; + if (flow.basic.n_proto == htons(ETH_P_IP)) + data_len = sizeof(flow.addrs.v4addrs); + else if (flow.basic.n_proto == htons(ETH_P_IPV6)) + data_len = sizeof(flow.addrs.v6addrs); else - data_len = 8; + return false; + + memcpy(data, &flow.addrs, data_len); + + if (flow.basic.ip_proto == IPPROTO_TCP) { + memcpy(data + data_len, &flow.ports, sizeof(flow.ports)); + data_len += sizeof(flow.ports); + } - *hash = comp_hash(netvsc_hash_key, HASH_KEYLEN, &flow, data_len); + *hash = comp_hash(netvsc_hash_key, HASH_KEYLEN, data, data_len); return true; } -- 2.4.3
next reply other threads:[~2016-01-07 9:33 UTC|newest] Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-01-07 9:33 Vitaly Kuznetsov [this message] 2016-01-07 9:33 ` [PATCH net-next] hv_netvsc: don't make assumptions on struct flow_keys layout Vitaly Kuznetsov 2016-01-07 12:52 ` Eric Dumazet 2016-01-07 13:28 ` Vitaly Kuznetsov 2016-01-07 13:28 ` Vitaly Kuznetsov 2016-01-08 1:02 ` John Fastabend 2016-01-08 3:49 ` KY Srinivasan 2016-01-08 3:49 ` KY Srinivasan 2016-01-08 6:16 ` John Fastabend 2016-01-08 6:16 ` John Fastabend 2016-01-08 18:01 ` KY Srinivasan 2016-01-08 21:07 ` Haiyang Zhang 2016-01-08 21:07 ` Haiyang Zhang 2016-01-09 0:17 ` Tom Herbert 2016-01-09 0:17 ` Tom Herbert 2016-01-10 22:25 ` David Miller 2016-01-10 22:25 ` David Miller 2016-01-13 23:10 ` Haiyang Zhang 2016-01-13 23:10 ` Haiyang Zhang 2016-01-14 4:56 ` David Miller 2016-01-14 4:56 ` David Miller 2016-01-14 17:14 ` Tom Herbert 2016-01-14 17:14 ` Tom Herbert 2016-01-14 17:53 ` One Thousand Gnomes 2016-01-14 17:53 ` One Thousand Gnomes 2016-01-14 18:24 ` Eric Dumazet 2016-01-14 18:24 ` Eric Dumazet 2016-01-14 18:35 ` Haiyang Zhang 2016-01-14 18:35 ` Haiyang Zhang 2016-01-14 18:48 ` Tom Herbert 2016-01-14 19:15 ` Haiyang Zhang 2016-01-14 19:15 ` Haiyang Zhang 2016-01-14 19:41 ` Tom Herbert 2016-01-14 20:23 ` Haiyang Zhang 2016-01-14 20:23 ` Haiyang Zhang 2016-01-14 21:44 ` Tom Herbert 2016-01-14 21:44 ` Tom Herbert 2016-01-14 22:06 ` David Miller 2016-01-14 22:08 ` Eric Dumazet 2016-01-14 22:08 ` Eric Dumazet 2016-01-14 22:29 ` Haiyang Zhang 2016-01-14 22:29 ` Haiyang Zhang 2016-01-14 17:53 ` Eric Dumazet
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=1452159189-11473-1-git-send-email-vkuznets@redhat.com \ --to=vkuznets@redhat.com \ --cc=davem@davemloft.net \ --cc=devel@linuxdriverproject.org \ --cc=eric.dumazet@gmail.com \ --cc=haiyangz@microsoft.com \ --cc=kys@microsoft.com \ --cc=linux-kernel@vger.kernel.org \ --cc=netdev@vger.kernel.org \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.