From: Haiyang Zhang <haiyangz@microsoft.com>
To: Eric Dumazet <eric.dumazet@gmail.com>
Cc: "davem@davemloft.net" <davem@davemloft.net>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
KY Srinivasan <kys@microsoft.com>,
"olaf@aepfle.de" <olaf@aepfle.de>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"driverdev-devel@linuxdriverproject.org"
<driverdev-devel@linuxdriverproject.org>
Subject: RE: [PATCH net-next] hv_netvsc: Use simple parser for IPv4 and v6 headers
Date: Wed, 16 Dec 2015 19:20:44 +0000 [thread overview]
Message-ID: <BN1PR0301MB0770A1CB185D1AB82D5F9E44CAEF0@BN1PR0301MB0770.namprd03.prod.outlook.com> (raw)
In-Reply-To: <1450285688.8474.86.camel@edumazet-glaptop2.roam.corp.google.com>
[-- 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¥
WARNING: multiple messages have this Message-ID (diff)
From: Haiyang Zhang <haiyangz@microsoft.com>
To: Eric Dumazet <eric.dumazet@gmail.com>
Cc: "olaf@aepfle.de" <olaf@aepfle.de>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"driverdev-devel@linuxdriverproject.org"
<driverdev-devel@linuxdriverproject.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"davem@davemloft.net" <davem@davemloft.net>
Subject: RE: [PATCH net-next] hv_netvsc: Use simple parser for IPv4 and v6 headers
Date: Wed, 16 Dec 2015 19:20:44 +0000 [thread overview]
Message-ID: <BN1PR0301MB0770A1CB185D1AB82D5F9E44CAEF0@BN1PR0301MB0770.namprd03.prod.outlook.com> (raw)
In-Reply-To: <1450285688.8474.86.camel@edumazet-glaptop2.roam.corp.google.com>
> -----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
WARNING: multiple messages have this Message-ID (diff)
From: Haiyang Zhang <haiyangz@microsoft.com>
To: Eric Dumazet <eric.dumazet@gmail.com>
Cc: "olaf@aepfle.de" <olaf@aepfle.de>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"driverdev-devel@linuxdriverproject.org"
<driverdev-devel@linuxdriverproject.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"davem@davemloft.net" <davem@davemloft.net>
Subject: RE: [PATCH net-next] hv_netvsc: Use simple parser for IPv4 and v6 headers
Date: Wed, 16 Dec 2015 19:20:44 +0000 [thread overview]
Message-ID: <BN1PR0301MB0770A1CB185D1AB82D5F9E44CAEF0@BN1PR0301MB0770.namprd03.prod.outlook.com> (raw)
In-Reply-To: <1450285688.8474.86.camel@edumazet-glaptop2.roam.corp.google.com>
> -----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
next prev parent reply other threads:[~2015-12-16 19:54 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
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=BN1PR0301MB0770A1CB185D1AB82D5F9E44CAEF0@BN1PR0301MB0770.namprd03.prod.outlook.com \
--to=haiyangz@microsoft.com \
--cc=davem@davemloft.net \
--cc=driverdev-devel@linuxdriverproject.org \
--cc=eric.dumazet@gmail.com \
--cc=kys@microsoft.com \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=olaf@aepfle.de \
/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: link
Be 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.