* Re: [PATCH net-next] hv_netvsc: Use simple parser for IPv4 and v6 headers
2015-12-16 18:03 ` Haiyang Zhang
(?)
@ 2015-12-16 17:08 ` Eric Dumazet
-1 siblings, 0 replies; 19+ messages in thread
From: Eric Dumazet @ 2015-12-16 17:08 UTC (permalink / raw)
To: Haiyang Zhang; +Cc: davem, netdev, kys, olaf, linux-kernel, driverdev-devel
On Wed, 2015-12-16 at 10:03 -0800, Haiyang Zhang wrote:
> To avoid performance overhead when using skb_flow_dissect_flow_keys(),
> we switch to the simple parsers to get the IP and port numbers.
>
> Performance comparison: throughput (Gbps):
> Number of connections, before patch, after patch
> 1 8.56 10.18
> 4 11.17 14.07
> 16 12.21 21.78
> 64 18.71 32.08
> 256 15.92 26.32
> 1024 8.41 15.49
> 3000 7.82 11.58
>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> Tested-by: Simon Xiao <sixiao@microsoft.com>
> Reviewed-by: K. Y. Srinivasan <kys@microsoft.com>
> ---
> drivers/net/hyperv/netvsc_drv.c | 38 +++++++++++++++++++++++++++++---------
> 1 files changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index 1c8db9a..e28951f 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -237,20 +237,40 @@ static u32 comp_hash(u8 *key, int klen, void *data, int dlen)
>
> static bool netvsc_set_hash(u32 *hash, struct sk_buff *skb)
> {
> - struct flow_keys flow;
> + struct iphdr *iphdr;
> + struct ipv6hdr *ipv6hdr;
> + __be32 dbuf[9];
> 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)))
> + if (eth_hdr(skb)->h_proto != htons(ETH_P_IP) &&
> + eth_hdr(skb)->h_proto != htons(ETH_P_IPV6))
> return false;
>
> - if (flow.basic.ip_proto == IPPROTO_TCP)
> - data_len = 12;
> - else
> - data_len = 8;
> + iphdr = ip_hdr(skb);
> + ipv6hdr = ipv6_hdr(skb);
> +
> + if (iphdr->version == 4) {
> + dbuf[0] = iphdr->saddr;
> + dbuf[1] = iphdr->daddr;
> + if (iphdr->protocol == IPPROTO_TCP) {
> + dbuf[2] = *(__be32 *)&tcp_hdr(skb)->source;
> + data_len = 12;
> + } else {
> + data_len = 8;
> + }
> + } else if (ipv6hdr->version == 6) {
> + memcpy(dbuf, &ipv6hdr->saddr, 32);
> + if (ipv6hdr->nexthdr == IPPROTO_TCP) {
> + dbuf[8] = *(__be32 *)&tcp_hdr(skb)->source;
> + data_len = 36;
> + } else {
> + data_len = 32;
> + }
> + } else {
> + return false;
> + }
>
> - *hash = comp_hash(netvsc_hash_key, HASH_KEYLEN, &flow, data_len);
> + *hash = comp_hash(netvsc_hash_key, HASH_KEYLEN, dbuf, data_len);
>
> return true;
> }
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next] hv_netvsc: Use simple parser for IPv4 and v6 headers
@ 2015-12-16 17:08 ` Eric Dumazet
0 siblings, 0 replies; 19+ messages in thread
From: Eric Dumazet @ 2015-12-16 17:08 UTC (permalink / raw)
To: Haiyang Zhang; +Cc: olaf, netdev, driverdev-devel, linux-kernel, davem
On Wed, 2015-12-16 at 10:03 -0800, Haiyang Zhang wrote:
> To avoid performance overhead when using skb_flow_dissect_flow_keys(),
> we switch to the simple parsers to get the IP and port numbers.
>
> Performance comparison: throughput (Gbps):
> Number of connections, before patch, after patch
> 1 8.56 10.18
> 4 11.17 14.07
> 16 12.21 21.78
> 64 18.71 32.08
> 256 15.92 26.32
> 1024 8.41 15.49
> 3000 7.82 11.58
>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> Tested-by: Simon Xiao <sixiao@microsoft.com>
> Reviewed-by: K. Y. Srinivasan <kys@microsoft.com>
> ---
> drivers/net/hyperv/netvsc_drv.c | 38 +++++++++++++++++++++++++++++---------
> 1 files changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index 1c8db9a..e28951f 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -237,20 +237,40 @@ static u32 comp_hash(u8 *key, int klen, void *data, int dlen)
>
> static bool netvsc_set_hash(u32 *hash, struct sk_buff *skb)
> {
> - struct flow_keys flow;
> + struct iphdr *iphdr;
> + struct ipv6hdr *ipv6hdr;
> + __be32 dbuf[9];
> 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)))
> + if (eth_hdr(skb)->h_proto != htons(ETH_P_IP) &&
> + eth_hdr(skb)->h_proto != htons(ETH_P_IPV6))
> return false;
>
> - if (flow.basic.ip_proto == IPPROTO_TCP)
> - data_len = 12;
> - else
> - data_len = 8;
> + iphdr = ip_hdr(skb);
> + ipv6hdr = ipv6_hdr(skb);
> +
> + if (iphdr->version == 4) {
> + dbuf[0] = iphdr->saddr;
> + dbuf[1] = iphdr->daddr;
> + if (iphdr->protocol == IPPROTO_TCP) {
> + dbuf[2] = *(__be32 *)&tcp_hdr(skb)->source;
> + data_len = 12;
> + } else {
> + data_len = 8;
> + }
> + } else if (ipv6hdr->version == 6) {
> + memcpy(dbuf, &ipv6hdr->saddr, 32);
> + if (ipv6hdr->nexthdr == IPPROTO_TCP) {
> + dbuf[8] = *(__be32 *)&tcp_hdr(skb)->source;
> + data_len = 36;
> + } else {
> + data_len = 32;
> + }
> + } else {
> + return false;
> + }
>
> - *hash = comp_hash(netvsc_hash_key, HASH_KEYLEN, &flow, data_len);
> + *hash = comp_hash(netvsc_hash_key, HASH_KEYLEN, dbuf, data_len);
>
> return true;
> }
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next] hv_netvsc: Use simple parser for IPv4 and v6 headers
@ 2015-12-16 17:08 ` Eric Dumazet
0 siblings, 0 replies; 19+ messages in thread
From: Eric Dumazet @ 2015-12-16 17:08 UTC (permalink / raw)
To: Haiyang Zhang; +Cc: olaf, netdev, driverdev-devel, linux-kernel, davem
On Wed, 2015-12-16 at 10:03 -0800, Haiyang Zhang wrote:
> To avoid performance overhead when using skb_flow_dissect_flow_keys(),
> we switch to the simple parsers to get the IP and port numbers.
>
> Performance comparison: throughput (Gbps):
> Number of connections, before patch, after patch
> 1 8.56 10.18
> 4 11.17 14.07
> 16 12.21 21.78
> 64 18.71 32.08
> 256 15.92 26.32
> 1024 8.41 15.49
> 3000 7.82 11.58
>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> Tested-by: Simon Xiao <sixiao@microsoft.com>
> Reviewed-by: K. Y. Srinivasan <kys@microsoft.com>
> ---
> drivers/net/hyperv/netvsc_drv.c | 38 +++++++++++++++++++++++++++++---------
> 1 files changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index 1c8db9a..e28951f 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -237,20 +237,40 @@ static u32 comp_hash(u8 *key, int klen, void *data, int dlen)
>
> static bool netvsc_set_hash(u32 *hash, struct sk_buff *skb)
> {
> - struct flow_keys flow;
> + struct iphdr *iphdr;
> + struct ipv6hdr *ipv6hdr;
> + __be32 dbuf[9];
> 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)))
> + if (eth_hdr(skb)->h_proto != htons(ETH_P_IP) &&
> + eth_hdr(skb)->h_proto != htons(ETH_P_IPV6))
> return false;
>
> - if (flow.basic.ip_proto == IPPROTO_TCP)
> - data_len = 12;
> - else
> - data_len = 8;
> + iphdr = ip_hdr(skb);
> + ipv6hdr = ipv6_hdr(skb);
> +
> + if (iphdr->version == 4) {
> + dbuf[0] = iphdr->saddr;
> + dbuf[1] = iphdr->daddr;
> + if (iphdr->protocol == IPPROTO_TCP) {
> + dbuf[2] = *(__be32 *)&tcp_hdr(skb)->source;
> + data_len = 12;
> + } else {
> + data_len = 8;
> + }
> + } else if (ipv6hdr->version == 6) {
> + memcpy(dbuf, &ipv6hdr->saddr, 32);
> + if (ipv6hdr->nexthdr == IPPROTO_TCP) {
> + dbuf[8] = *(__be32 *)&tcp_hdr(skb)->source;
> + data_len = 36;
> + } else {
> + data_len = 32;
> + }
> + } else {
> + return false;
> + }
>
> - *hash = comp_hash(netvsc_hash_key, HASH_KEYLEN, &flow, data_len);
> + *hash = comp_hash(netvsc_hash_key, HASH_KEYLEN, dbuf, data_len);
>
> return true;
> }
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.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH net-next] hv_netvsc: Use simple parser for IPv4 and v6 headers
@ 2015-12-16 18:03 ` Haiyang Zhang
0 siblings, 0 replies; 19+ messages in thread
From: Haiyang Zhang @ 2015-12-16 18:03 UTC (permalink / raw)
To: davem, netdev; +Cc: haiyangz, kys, olaf, linux-kernel, driverdev-devel
To avoid performance overhead when using skb_flow_dissect_flow_keys(),
we switch to the simple parsers to get the IP and port numbers.
Performance comparison: throughput (Gbps):
Number of connections, before patch, after patch
1 8.56 10.18
4 11.17 14.07
16 12.21 21.78
64 18.71 32.08
256 15.92 26.32
1024 8.41 15.49
3000 7.82 11.58
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Tested-by: Simon Xiao <sixiao@microsoft.com>
Reviewed-by: K. Y. Srinivasan <kys@microsoft.com>
---
drivers/net/hyperv/netvsc_drv.c | 38 +++++++++++++++++++++++++++++---------
1 files changed, 29 insertions(+), 9 deletions(-)
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 1c8db9a..e28951f 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -237,20 +237,40 @@ static u32 comp_hash(u8 *key, int klen, void *data, int dlen)
static bool netvsc_set_hash(u32 *hash, struct sk_buff *skb)
{
- struct flow_keys flow;
+ struct iphdr *iphdr;
+ struct ipv6hdr *ipv6hdr;
+ __be32 dbuf[9];
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)))
+ if (eth_hdr(skb)->h_proto != htons(ETH_P_IP) &&
+ eth_hdr(skb)->h_proto != htons(ETH_P_IPV6))
return false;
- if (flow.basic.ip_proto == IPPROTO_TCP)
- data_len = 12;
- else
- data_len = 8;
+ iphdr = ip_hdr(skb);
+ ipv6hdr = ipv6_hdr(skb);
+
+ if (iphdr->version == 4) {
+ dbuf[0] = iphdr->saddr;
+ dbuf[1] = iphdr->daddr;
+ if (iphdr->protocol == IPPROTO_TCP) {
+ dbuf[2] = *(__be32 *)&tcp_hdr(skb)->source;
+ data_len = 12;
+ } else {
+ data_len = 8;
+ }
+ } else if (ipv6hdr->version == 6) {
+ memcpy(dbuf, &ipv6hdr->saddr, 32);
+ if (ipv6hdr->nexthdr == IPPROTO_TCP) {
+ dbuf[8] = *(__be32 *)&tcp_hdr(skb)->source;
+ data_len = 36;
+ } else {
+ data_len = 32;
+ }
+ } else {
+ return false;
+ }
- *hash = comp_hash(netvsc_hash_key, HASH_KEYLEN, &flow, data_len);
+ *hash = comp_hash(netvsc_hash_key, HASH_KEYLEN, dbuf, data_len);
return true;
}
--
1.7.4.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH net-next] hv_netvsc: Use simple parser for IPv4 and v6 headers
@ 2015-12-16 18:03 ` Haiyang Zhang
0 siblings, 0 replies; 19+ messages in thread
From: Haiyang Zhang @ 2015-12-16 18:03 UTC (permalink / raw)
To: davem, netdev; +Cc: driverdev-devel, haiyangz, olaf, linux-kernel
To avoid performance overhead when using skb_flow_dissect_flow_keys(),
we switch to the simple parsers to get the IP and port numbers.
Performance comparison: throughput (Gbps):
Number of connections, before patch, after patch
1 8.56 10.18
4 11.17 14.07
16 12.21 21.78
64 18.71 32.08
256 15.92 26.32
1024 8.41 15.49
3000 7.82 11.58
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Tested-by: Simon Xiao <sixiao@microsoft.com>
Reviewed-by: K. Y. Srinivasan <kys@microsoft.com>
---
drivers/net/hyperv/netvsc_drv.c | 38 +++++++++++++++++++++++++++++---------
1 files changed, 29 insertions(+), 9 deletions(-)
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 1c8db9a..e28951f 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -237,20 +237,40 @@ static u32 comp_hash(u8 *key, int klen, void *data, int dlen)
static bool netvsc_set_hash(u32 *hash, struct sk_buff *skb)
{
- struct flow_keys flow;
+ struct iphdr *iphdr;
+ struct ipv6hdr *ipv6hdr;
+ __be32 dbuf[9];
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)))
+ if (eth_hdr(skb)->h_proto != htons(ETH_P_IP) &&
+ eth_hdr(skb)->h_proto != htons(ETH_P_IPV6))
return false;
- if (flow.basic.ip_proto == IPPROTO_TCP)
- data_len = 12;
- else
- data_len = 8;
+ iphdr = ip_hdr(skb);
+ ipv6hdr = ipv6_hdr(skb);
+
+ if (iphdr->version == 4) {
+ dbuf[0] = iphdr->saddr;
+ dbuf[1] = iphdr->daddr;
+ if (iphdr->protocol == IPPROTO_TCP) {
+ dbuf[2] = *(__be32 *)&tcp_hdr(skb)->source;
+ data_len = 12;
+ } else {
+ data_len = 8;
+ }
+ } else if (ipv6hdr->version == 6) {
+ memcpy(dbuf, &ipv6hdr->saddr, 32);
+ if (ipv6hdr->nexthdr == IPPROTO_TCP) {
+ dbuf[8] = *(__be32 *)&tcp_hdr(skb)->source;
+ data_len = 36;
+ } else {
+ data_len = 32;
+ }
+ } else {
+ return false;
+ }
- *hash = comp_hash(netvsc_hash_key, HASH_KEYLEN, &flow, data_len);
+ *hash = comp_hash(netvsc_hash_key, HASH_KEYLEN, dbuf, data_len);
return true;
}
--
1.7.4.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH net-next] hv_netvsc: Use simple parser for IPv4 and v6 headers
@ 2015-12-16 18:03 ` Haiyang Zhang
0 siblings, 0 replies; 19+ messages in thread
From: Haiyang Zhang @ 2015-12-16 18:03 UTC (permalink / raw)
To: davem, netdev; +Cc: driverdev-devel, haiyangz, olaf, linux-kernel
To avoid performance overhead when using skb_flow_dissect_flow_keys(),
we switch to the simple parsers to get the IP and port numbers.
Performance comparison: throughput (Gbps):
Number of connections, before patch, after patch
1 8.56 10.18
4 11.17 14.07
16 12.21 21.78
64 18.71 32.08
256 15.92 26.32
1024 8.41 15.49
3000 7.82 11.58
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Tested-by: Simon Xiao <sixiao@microsoft.com>
Reviewed-by: K. Y. Srinivasan <kys@microsoft.com>
---
drivers/net/hyperv/netvsc_drv.c | 38 +++++++++++++++++++++++++++++---------
1 files changed, 29 insertions(+), 9 deletions(-)
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 1c8db9a..e28951f 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -237,20 +237,40 @@ static u32 comp_hash(u8 *key, int klen, void *data, int dlen)
static bool netvsc_set_hash(u32 *hash, struct sk_buff *skb)
{
- struct flow_keys flow;
+ struct iphdr *iphdr;
+ struct ipv6hdr *ipv6hdr;
+ __be32 dbuf[9];
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)))
+ if (eth_hdr(skb)->h_proto != htons(ETH_P_IP) &&
+ eth_hdr(skb)->h_proto != htons(ETH_P_IPV6))
return false;
- if (flow.basic.ip_proto == IPPROTO_TCP)
- data_len = 12;
- else
- data_len = 8;
+ iphdr = ip_hdr(skb);
+ ipv6hdr = ipv6_hdr(skb);
+
+ if (iphdr->version == 4) {
+ dbuf[0] = iphdr->saddr;
+ dbuf[1] = iphdr->daddr;
+ if (iphdr->protocol == IPPROTO_TCP) {
+ dbuf[2] = *(__be32 *)&tcp_hdr(skb)->source;
+ data_len = 12;
+ } else {
+ data_len = 8;
+ }
+ } else if (ipv6hdr->version == 6) {
+ memcpy(dbuf, &ipv6hdr->saddr, 32);
+ if (ipv6hdr->nexthdr == IPPROTO_TCP) {
+ dbuf[8] = *(__be32 *)&tcp_hdr(skb)->source;
+ data_len = 36;
+ } else {
+ data_len = 32;
+ }
+ } else {
+ return false;
+ }
- *hash = comp_hash(netvsc_hash_key, HASH_KEYLEN, &flow, data_len);
+ *hash = comp_hash(netvsc_hash_key, HASH_KEYLEN, dbuf, data_len);
return true;
}
--
1.7.4.1
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH net-next] hv_netvsc: Use simple parser for IPv4 and v6 headers
2015-12-16 18:03 ` Haiyang Zhang
@ 2015-12-16 18:34 ` Sergei Shtylyov
-1 siblings, 0 replies; 19+ messages in thread
From: Sergei Shtylyov @ 2015-12-16 18:34 UTC (permalink / raw)
To: Haiyang Zhang, davem, netdev; +Cc: kys, olaf, linux-kernel, driverdev-devel
Hello.
On 12/16/2015 09:03 PM, Haiyang Zhang wrote:
> To avoid performance overhead when using skb_flow_dissect_flow_keys(),
> we switch to the simple parsers to get the IP and port numbers.
>
> Performance comparison: throughput (Gbps):
> Number of connections, before patch, after patch
> 1 8.56 10.18
> 4 11.17 14.07
> 16 12.21 21.78
> 64 18.71 32.08
> 256 15.92 26.32
> 1024 8.41 15.49
> 3000 7.82 11.58
>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> Tested-by: Simon Xiao <sixiao@microsoft.com>
> Reviewed-by: K. Y. Srinivasan <kys@microsoft.com>
> ---
> drivers/net/hyperv/netvsc_drv.c | 38 +++++++++++++++++++++++++++++---------
> 1 files changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index 1c8db9a..e28951f 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -237,20 +237,40 @@ static u32 comp_hash(u8 *key, int klen, void *data, int dlen)
[...]
> + if (iphdr->version == 4) {
> + dbuf[0] = iphdr->saddr;
> + dbuf[1] = iphdr->daddr;
> + if (iphdr->protocol == IPPROTO_TCP) {
> + dbuf[2] = *(__be32 *)&tcp_hdr(skb)->source;
> + data_len = 12;
> + } else {
> + data_len = 8;
> + }
> + } else if (ipv6hdr->version == 6) {
> + memcpy(dbuf, &ipv6hdr->saddr, 32);
> + if (ipv6hdr->nexthdr == IPPROTO_TCP) {
> + dbuf[8] = *(__be32 *)&tcp_hdr(skb)->source;
> + data_len = 36;
> + } else {
> + data_len = 32;
> + }
> + } else {
> + return false;
> + }
This is asking to be a *switch* statement.
[...]
MBR, Sergei
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next] hv_netvsc: Use simple parser for IPv4 and v6 headers
@ 2015-12-16 18:34 ` Sergei Shtylyov
0 siblings, 0 replies; 19+ messages in thread
From: Sergei Shtylyov @ 2015-12-16 18:34 UTC (permalink / raw)
To: Haiyang Zhang, davem, netdev; +Cc: driverdev-devel, olaf, linux-kernel
Hello.
On 12/16/2015 09:03 PM, Haiyang Zhang wrote:
> To avoid performance overhead when using skb_flow_dissect_flow_keys(),
> we switch to the simple parsers to get the IP and port numbers.
>
> Performance comparison: throughput (Gbps):
> Number of connections, before patch, after patch
> 1 8.56 10.18
> 4 11.17 14.07
> 16 12.21 21.78
> 64 18.71 32.08
> 256 15.92 26.32
> 1024 8.41 15.49
> 3000 7.82 11.58
>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> Tested-by: Simon Xiao <sixiao@microsoft.com>
> Reviewed-by: K. Y. Srinivasan <kys@microsoft.com>
> ---
> drivers/net/hyperv/netvsc_drv.c | 38 +++++++++++++++++++++++++++++---------
> 1 files changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index 1c8db9a..e28951f 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -237,20 +237,40 @@ static u32 comp_hash(u8 *key, int klen, void *data, int dlen)
[...]
> + if (iphdr->version == 4) {
> + dbuf[0] = iphdr->saddr;
> + dbuf[1] = iphdr->daddr;
> + if (iphdr->protocol == IPPROTO_TCP) {
> + dbuf[2] = *(__be32 *)&tcp_hdr(skb)->source;
> + data_len = 12;
> + } else {
> + data_len = 8;
> + }
> + } else if (ipv6hdr->version == 6) {
> + memcpy(dbuf, &ipv6hdr->saddr, 32);
> + if (ipv6hdr->nexthdr == IPPROTO_TCP) {
> + dbuf[8] = *(__be32 *)&tcp_hdr(skb)->source;
> + data_len = 36;
> + } else {
> + data_len = 32;
> + }
> + } else {
> + return false;
> + }
This is asking to be a *switch* statement.
[...]
MBR, Sergei
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next] hv_netvsc: Use simple parser for IPv4 and v6 headers
2015-12-16 18:34 ` Sergei Shtylyov
@ 2015-12-16 18:45 ` Sergei Shtylyov
-1 siblings, 0 replies; 19+ messages in thread
From: Sergei Shtylyov @ 2015-12-16 18:45 UTC (permalink / raw)
To: Haiyang Zhang, davem, netdev; +Cc: kys, olaf, linux-kernel, driverdev-devel
On 12/16/2015 09:34 PM, Sergei Shtylyov wrote:
>> To avoid performance overhead when using skb_flow_dissect_flow_keys(),
>> we switch to the simple parsers to get the IP and port numbers.
>>
>> Performance comparison: throughput (Gbps):
>> Number of connections, before patch, after patch
>> 1 8.56 10.18
>> 4 11.17 14.07
>> 16 12.21 21.78
>> 64 18.71 32.08
>> 256 15.92 26.32
>> 1024 8.41 15.49
>> 3000 7.82 11.58
>>
>> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
>> Tested-by: Simon Xiao <sixiao@microsoft.com>
>> Reviewed-by: K. Y. Srinivasan <kys@microsoft.com>
>> ---
>> drivers/net/hyperv/netvsc_drv.c | 38 +++++++++++++++++++++++++++++---------
>> 1 files changed, 29 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
>> index 1c8db9a..e28951f 100644
>> --- a/drivers/net/hyperv/netvsc_drv.c
>> +++ b/drivers/net/hyperv/netvsc_drv.c
>> @@ -237,20 +237,40 @@ static u32 comp_hash(u8 *key, int klen, void *data,
>> int dlen)
> [...]
>> + if (iphdr->version == 4) {
>> + dbuf[0] = iphdr->saddr;
>> + dbuf[1] = iphdr->daddr;
>> + if (iphdr->protocol == IPPROTO_TCP) {
>> + dbuf[2] = *(__be32 *)&tcp_hdr(skb)->source;
>> + data_len = 12;
>> + } else {
>> + data_len = 8;
>> + }
>> + } else if (ipv6hdr->version == 6) {
>> + memcpy(dbuf, &ipv6hdr->saddr, 32);
>> + if (ipv6hdr->nexthdr == IPPROTO_TCP) {
>> + dbuf[8] = *(__be32 *)&tcp_hdr(skb)->source;
>> + data_len = 36;
>> + } else {
>> + data_len = 32;
>> + }
>> + } else {
>> + return false;
>> + }
>
> This is asking to be a *switch* statement.
Oops, nevermind. I'd misread the code.
> [...]
MBR, Sergei
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next] hv_netvsc: Use simple parser for IPv4 and v6 headers
@ 2015-12-16 18:45 ` Sergei Shtylyov
0 siblings, 0 replies; 19+ messages in thread
From: Sergei Shtylyov @ 2015-12-16 18:45 UTC (permalink / raw)
To: Haiyang Zhang, davem, netdev; +Cc: driverdev-devel, olaf, linux-kernel
On 12/16/2015 09:34 PM, Sergei Shtylyov wrote:
>> To avoid performance overhead when using skb_flow_dissect_flow_keys(),
>> we switch to the simple parsers to get the IP and port numbers.
>>
>> Performance comparison: throughput (Gbps):
>> Number of connections, before patch, after patch
>> 1 8.56 10.18
>> 4 11.17 14.07
>> 16 12.21 21.78
>> 64 18.71 32.08
>> 256 15.92 26.32
>> 1024 8.41 15.49
>> 3000 7.82 11.58
>>
>> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
>> Tested-by: Simon Xiao <sixiao@microsoft.com>
>> Reviewed-by: K. Y. Srinivasan <kys@microsoft.com>
>> ---
>> drivers/net/hyperv/netvsc_drv.c | 38 +++++++++++++++++++++++++++++---------
>> 1 files changed, 29 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
>> index 1c8db9a..e28951f 100644
>> --- a/drivers/net/hyperv/netvsc_drv.c
>> +++ b/drivers/net/hyperv/netvsc_drv.c
>> @@ -237,20 +237,40 @@ static u32 comp_hash(u8 *key, int klen, void *data,
>> int dlen)
> [...]
>> + if (iphdr->version == 4) {
>> + dbuf[0] = iphdr->saddr;
>> + dbuf[1] = iphdr->daddr;
>> + if (iphdr->protocol == IPPROTO_TCP) {
>> + dbuf[2] = *(__be32 *)&tcp_hdr(skb)->source;
>> + data_len = 12;
>> + } else {
>> + data_len = 8;
>> + }
>> + } else if (ipv6hdr->version == 6) {
>> + memcpy(dbuf, &ipv6hdr->saddr, 32);
>> + if (ipv6hdr->nexthdr == IPPROTO_TCP) {
>> + dbuf[8] = *(__be32 *)&tcp_hdr(skb)->source;
>> + data_len = 36;
>> + } else {
>> + data_len = 32;
>> + }
>> + } else {
>> + return false;
>> + }
>
> This is asking to be a *switch* statement.
Oops, nevermind. I'd misread the code.
> [...]
MBR, Sergei
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH net-next] hv_netvsc: Use simple parser for IPv4 and v6 headers
2015-12-16 17:08 ` Eric Dumazet
(?)
@ 2015-12-16 19:20 ` Haiyang Zhang
-1 siblings, 0 replies; 19+ messages in thread
From: Haiyang Zhang @ 2015-12-16 19:20 UTC (permalink / raw)
To: Eric Dumazet
Cc: davem, netdev, KY Srinivasan, olaf, linux-kernel, driverdev-devel
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2186 bytes --]
> -----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
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.
static inline __u32 skb_get_hash(struct sk_buff *skb)
{
if (!skb->l4_hash && !skb->sw_hash)
__skb_get_hash(skb);
return skb->hash;
}
void __skb_get_hash(struct sk_buff *skb)
{
struct flow_keys keys;
__flow_hash_secret_init();
__skb_set_sw_hash(skb, ___skb_get_hash(skb, &keys, hashrnd),
flow_keys_have_l4(&keys));
}
static inline u32 ___skb_get_hash(const struct sk_buff *skb,
struct flow_keys *keys, u32 keyval)
{
skb_flow_dissect_flow_keys(skb, keys,
FLOW_DISSECTOR_F_STOP_AT_FLOW_LABEL);
return __flow_hash_from_keys(keys, keyval);
}
Thanks,
- Haiyang
ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥
^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH net-next] hv_netvsc: Use simple parser for IPv4 and v6 headers
@ 2015-12-16 19:20 ` Haiyang Zhang
0 siblings, 0 replies; 19+ messages in thread
From: Haiyang Zhang @ 2015-12-16 19:20 UTC (permalink / raw)
To: Eric Dumazet; +Cc: olaf, netdev, driverdev-devel, linux-kernel, davem
> -----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
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.
static inline __u32 skb_get_hash(struct sk_buff *skb)
{
if (!skb->l4_hash && !skb->sw_hash)
__skb_get_hash(skb);
return skb->hash;
}
void __skb_get_hash(struct sk_buff *skb)
{
struct flow_keys keys;
__flow_hash_secret_init();
__skb_set_sw_hash(skb, ___skb_get_hash(skb, &keys, hashrnd),
flow_keys_have_l4(&keys));
}
static inline u32 ___skb_get_hash(const struct sk_buff *skb,
struct flow_keys *keys, u32 keyval)
{
skb_flow_dissect_flow_keys(skb, keys,
FLOW_DISSECTOR_F_STOP_AT_FLOW_LABEL);
return __flow_hash_from_keys(keys, keyval);
}
Thanks,
- Haiyang
^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH net-next] hv_netvsc: Use simple parser for IPv4 and v6 headers
@ 2015-12-16 19:20 ` Haiyang Zhang
0 siblings, 0 replies; 19+ messages in thread
From: Haiyang Zhang @ 2015-12-16 19:20 UTC (permalink / raw)
To: Eric Dumazet; +Cc: olaf, netdev, driverdev-devel, linux-kernel, davem
> -----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
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.
static inline __u32 skb_get_hash(struct sk_buff *skb)
{
if (!skb->l4_hash && !skb->sw_hash)
__skb_get_hash(skb);
return skb->hash;
}
void __skb_get_hash(struct sk_buff *skb)
{
struct flow_keys keys;
__flow_hash_secret_init();
__skb_set_sw_hash(skb, ___skb_get_hash(skb, &keys, hashrnd),
flow_keys_have_l4(&keys));
}
static inline u32 ___skb_get_hash(const struct sk_buff *skb,
struct flow_keys *keys, u32 keyval)
{
skb_flow_dissect_flow_keys(skb, keys,
FLOW_DISSECTOR_F_STOP_AT_FLOW_LABEL);
return __flow_hash_from_keys(keys, keyval);
}
Thanks,
- Haiyang
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next] hv_netvsc: Use simple parser for IPv4 and v6 headers
2015-12-16 19:20 ` Haiyang Zhang
(?)
@ 2015-12-16 21:19 ` Eric Dumazet
-1 siblings, 0 replies; 19+ messages in thread
From: Eric Dumazet @ 2015-12-16 21:19 UTC (permalink / raw)
To: Haiyang Zhang, Tom Herbert
Cc: davem, netdev, KY Srinivasan, olaf, linux-kernel, driverdev-devel
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 <tom@herbertland.com>
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next] hv_netvsc: Use simple parser for IPv4 and v6 headers
@ 2015-12-16 21:19 ` Eric Dumazet
0 siblings, 0 replies; 19+ messages in thread
From: Eric Dumazet @ 2015-12-16 21:19 UTC (permalink / raw)
To: Haiyang Zhang, Tom Herbert
Cc: olaf, netdev, driverdev-devel, linux-kernel, davem
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 <tom@herbertland.com>
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next] hv_netvsc: Use simple parser for IPv4 and v6 headers
@ 2015-12-16 21:19 ` Eric Dumazet
0 siblings, 0 replies; 19+ messages in thread
From: Eric Dumazet @ 2015-12-16 21:19 UTC (permalink / raw)
To: Haiyang Zhang, Tom Herbert
Cc: olaf, netdev, driverdev-devel, linux-kernel, davem
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 <tom@herbertland.com>
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next] hv_netvsc: Use simple parser for IPv4 and v6 headers
2015-12-16 19:20 ` Haiyang Zhang
(?)
@ 2015-12-16 23:23 ` David Miller
-1 siblings, 0 replies; 19+ messages in thread
From: David Miller @ 2015-12-16 23:23 UTC (permalink / raw)
To: haiyangz; +Cc: eric.dumazet, netdev, kys, olaf, linux-kernel, driverdev-devel
From: Haiyang Zhang <haiyangz@microsoft.com>
Date: Wed, 16 Dec 2015 19:20:44 +0000
> 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
NEVER _EVER_ work around this kind of problem by bypassing the code in
question in your driver.
ALWAYS work to fix the actual problem.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next] hv_netvsc: Use simple parser for IPv4 and v6 headers
@ 2015-12-16 23:23 ` David Miller
0 siblings, 0 replies; 19+ messages in thread
From: David Miller @ 2015-12-16 23:23 UTC (permalink / raw)
To: haiyangz; +Cc: olaf, eric.dumazet, netdev, driverdev-devel, linux-kernel
From: Haiyang Zhang <haiyangz@microsoft.com>
Date: Wed, 16 Dec 2015 19:20:44 +0000
> 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
NEVER _EVER_ work around this kind of problem by bypassing the code in
question in your driver.
ALWAYS work to fix the actual problem.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next] hv_netvsc: Use simple parser for IPv4 and v6 headers
@ 2015-12-16 23:23 ` David Miller
0 siblings, 0 replies; 19+ messages in thread
From: David Miller @ 2015-12-16 23:23 UTC (permalink / raw)
To: haiyangz; +Cc: olaf, eric.dumazet, netdev, driverdev-devel, linux-kernel
From: Haiyang Zhang <haiyangz@microsoft.com>
Date: Wed, 16 Dec 2015 19:20:44 +0000
> 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
NEVER _EVER_ work around this kind of problem by bypassing the code in
question in your driver.
ALWAYS work to fix the actual problem.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2015-12-16 23:23 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-16 18:03 [PATCH net-next] hv_netvsc: Use simple parser for IPv4 and v6 headers Haiyang Zhang
2015-12-16 18:03 ` Haiyang Zhang
2015-12-16 18:03 ` Haiyang Zhang
2015-12-16 17:08 ` Eric Dumazet
2015-12-16 17:08 ` Eric Dumazet
2015-12-16 17:08 ` Eric Dumazet
2015-12-16 19:20 ` Haiyang Zhang
2015-12-16 19:20 ` Haiyang Zhang
2015-12-16 19:20 ` Haiyang Zhang
2015-12-16 21:19 ` Eric Dumazet
2015-12-16 21:19 ` Eric Dumazet
2015-12-16 21:19 ` Eric Dumazet
2015-12-16 23:23 ` David Miller
2015-12-16 23:23 ` David Miller
2015-12-16 23:23 ` David Miller
2015-12-16 18:34 ` Sergei Shtylyov
2015-12-16 18:34 ` Sergei Shtylyov
2015-12-16 18:45 ` Sergei Shtylyov
2015-12-16 18:45 ` Sergei Shtylyov
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.