* [PATCH 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash
@ 2016-07-28 7:14 fgao
2016-08-02 1:36 ` Philp Prindeville
2016-08-02 20:35 ` Tom Herbert
0 siblings, 2 replies; 18+ messages in thread
From: fgao @ 2016-07-28 7:14 UTC (permalink / raw)
To: davem, philipp, stephen, pshelar, tom, aduyck, netdev
Cc: gfree.wind, Gao Feng
From: Gao Feng <fgao@ikuai8.com>
The PPTP is encapsulated by GRE header with that GRE_VERSION bits
must contain one. But current GRE RPS needs the GRE_VERSION must be
zero. So RPS does not work for PPTP traffic.
In my test environment, there are four MIPS cores, and all traffic
are passed through by PPTP. As a result, only one core is 100% busy
while other three cores are very idle. After this patch, the usage
of four cores are balanced well.
Signed-off-by: Gao Feng <fgao@ikuai8.com>
---
v2: Update according to Tom and Philp's advice.
1) Consolidate the codes with GRE version 0 path;
2) Use PPP_PROTOCOL to get ppp protol;
3) Set the FLOW_DIS_ENCAPSULATION flag;
v1: Initial patch
include/uapi/linux/if_tunnel.h | 5 +-
net/core/flow_dissector.c | 146 ++++++++++++++++++++++++++---------------
2 files changed, 97 insertions(+), 54 deletions(-)
diff --git a/include/uapi/linux/if_tunnel.h b/include/uapi/linux/if_tunnel.h
index 1046f55..dda4e4b 100644
--- a/include/uapi/linux/if_tunnel.h
+++ b/include/uapi/linux/if_tunnel.h
@@ -24,9 +24,12 @@
#define GRE_SEQ __cpu_to_be16(0x1000)
#define GRE_STRICT __cpu_to_be16(0x0800)
#define GRE_REC __cpu_to_be16(0x0700)
-#define GRE_FLAGS __cpu_to_be16(0x00F8)
+#define GRE_ACK __cpu_to_be16(0x0080)
+#define GRE_FLAGS __cpu_to_be16(0x0078)
#define GRE_VERSION __cpu_to_be16(0x0007)
+#define GRE_PROTO_PPP __cpu_to_be16(0x880b)
+
struct ip_tunnel_parm {
char name[IFNAMSIZ];
int link;
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 61ad43f..33e957b 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -346,63 +346,103 @@ ip_proto_again:
hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data, hlen, &_hdr);
if (!hdr)
goto out_bad;
- /*
- * Only look inside GRE if version zero and no
- * routing
- */
- if (hdr->flags & (GRE_VERSION | GRE_ROUTING))
- break;
-
- proto = hdr->proto;
- nhoff += 4;
- if (hdr->flags & GRE_CSUM)
- nhoff += 4;
- if (hdr->flags & GRE_KEY) {
- const __be32 *keyid;
- __be32 _keyid;
-
- keyid = __skb_header_pointer(skb, nhoff, sizeof(_keyid),
- data, hlen, &_keyid);
- if (!keyid)
- goto out_bad;
+ /* Only look inside GRE without routing */
+ if (!(hdr->flags & GRE_ROUTING)) {
+ proto = hdr->proto;
+
+ if (hdr->flags & GRE_VERSION) {
+ /* It should be the PPTP in GRE */
+ u8 _ppp_hdr[PPP_HDRLEN];
+ u8 *ppp_hdr;
+ int offset = 0;
+
+ /* Check the flags according to RFC 2637*/
+ if (!(proto == GRE_PROTO_PPP && (hdr->flags & GRE_KEY) &&
+ !(hdr->flags & (GRE_CSUM | GRE_STRICT | GRE_REC | GRE_FLAGS)))) {
+ break;
+ }
+
+ /* Skip GRE header */
+ offset += 4;
+ /* Skip payload length and call id */
+ offset += 4;
+
+ if (hdr->flags & GRE_SEQ)
+ offset += 4;
+
+ if (hdr->flags & GRE_ACK)
+ offset += 4;
+
+ ppp_hdr = skb_header_pointer(skb, nhoff + offset, sizeof(_ppp_hdr), _ppp_hdr);
+ if (!ppp_hdr)
+ goto out_bad;
+ proto = PPP_PROTOCOL(ppp_hdr);
+ if (proto == PPP_IP) {
+ nhoff += (PPP_HDRLEN + offset);
+ proto = htons(ETH_P_IP);
+ key_control->flags |= FLOW_DIS_ENCAPSULATION;
+ goto again;
+ } else if (proto == PPP_IPV6) {
+ nhoff += (PPP_HDRLEN + offset);
+ proto = htons(ETH_P_IPV6);
+ key_control->flags |= FLOW_DIS_ENCAPSULATION;
+ goto again;
+ }
+ } else {
+ /* Original GRE */
+ nhoff += 4;
+ if (hdr->flags & GRE_CSUM)
+ nhoff += 4;
+ if (hdr->flags & GRE_KEY) {
+ const __be32 *keyid;
+ __be32 _keyid;
+
+ keyid = __skb_header_pointer(skb, nhoff, sizeof(_keyid),
+ data, hlen, &_keyid);
+
+ if (!keyid)
+ goto out_bad;
+
+ if (dissector_uses_key(flow_dissector,
+ FLOW_DISSECTOR_KEY_GRE_KEYID)) {
+ key_keyid = skb_flow_dissector_target(flow_dissector,
+ FLOW_DISSECTOR_KEY_GRE_KEYID,
+ target_container);
+ key_keyid->keyid = *keyid;
+ }
+ nhoff += 4;
+ }
+ if (hdr->flags & GRE_SEQ)
+ nhoff += 4;
+ if (proto == htons(ETH_P_TEB)) {
+ const struct ethhdr *eth;
+ struct ethhdr _eth;
+
+ eth = __skb_header_pointer(skb, nhoff,
+ sizeof(_eth),
+ data, hlen, &_eth);
+ if (!eth)
+ goto out_bad;
+ proto = eth->h_proto;
+ nhoff += sizeof(*eth);
+
+ /* Cap headers that we access via pointers at the
+ * end of the Ethernet header as our maximum alignment
+ * at that point is only 2 bytes.
+ */
+ if (NET_IP_ALIGN)
+ hlen = nhoff;
+ }
+
+ key_control->flags |= FLOW_DIS_ENCAPSULATION;
+ if (flags & FLOW_DISSECTOR_F_STOP_AT_ENCAP)
+ goto out_good;
- if (dissector_uses_key(flow_dissector,
- FLOW_DISSECTOR_KEY_GRE_KEYID)) {
- key_keyid = skb_flow_dissector_target(flow_dissector,
- FLOW_DISSECTOR_KEY_GRE_KEYID,
- target_container);
- key_keyid->keyid = *keyid;
+ goto again;
}
- nhoff += 4;
- }
- if (hdr->flags & GRE_SEQ)
- nhoff += 4;
- if (proto == htons(ETH_P_TEB)) {
- const struct ethhdr *eth;
- struct ethhdr _eth;
-
- eth = __skb_header_pointer(skb, nhoff,
- sizeof(_eth),
- data, hlen, &_eth);
- if (!eth)
- goto out_bad;
- proto = eth->h_proto;
- nhoff += sizeof(*eth);
-
- /* Cap headers that we access via pointers at the
- * end of the Ethernet header as our maximum alignment
- * at that point is only 2 bytes.
- */
- if (NET_IP_ALIGN)
- hlen = nhoff;
}
-
- key_control->flags |= FLOW_DIS_ENCAPSULATION;
- if (flags & FLOW_DISSECTOR_F_STOP_AT_ENCAP)
- goto out_good;
-
- goto again;
+ break;
}
case NEXTHDR_HOP:
case NEXTHDR_ROUTING:
--
1.9.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash
2016-07-28 7:14 [PATCH 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash fgao
@ 2016-08-02 1:36 ` Philp Prindeville
2016-08-02 6:10 ` Feng Gao
2016-08-02 20:35 ` Tom Herbert
1 sibling, 1 reply; 18+ messages in thread
From: Philp Prindeville @ 2016-08-02 1:36 UTC (permalink / raw)
To: fgao, davem, stephen, pshelar, tom, aduyck, netdev; +Cc: gfree.wind
Inline...
Main issue in a nutshell is I don't like using "4" instead of
"sizeof(my_32bit_header_field_here)".
On 07/28/2016 01:14 AM, fgao@48lvckh6395k16k5.yundunddos.com wrote:
> From: Gao Feng <fgao@ikuai8.com>
>
> The PPTP is encapsulated by GRE header with that GRE_VERSION bits
> must contain one. But current GRE RPS needs the GRE_VERSION must be
> zero. So RPS does not work for PPTP traffic.
>
> In my test environment, there are four MIPS cores, and all traffic
> are passed through by PPTP. As a result, only one core is 100% busy
> while other three cores are very idle. After this patch, the usage
> of four cores are balanced well.
>
> Signed-off-by: Gao Feng <fgao@ikuai8.com>
> ---
> v2: Update according to Tom and Philp's advice.
> 1) Consolidate the codes with GRE version 0 path;
> 2) Use PPP_PROTOCOL to get ppp protol;
> 3) Set the FLOW_DIS_ENCAPSULATION flag;
> v1: Initial patch
>
> include/uapi/linux/if_tunnel.h | 5 +-
> net/core/flow_dissector.c | 146 ++++++++++++++++++++++++++---------------
> 2 files changed, 97 insertions(+), 54 deletions(-)
>
> diff --git a/include/uapi/linux/if_tunnel.h b/include/uapi/linux/if_tunnel.h
> index 1046f55..dda4e4b 100644
> --- a/include/uapi/linux/if_tunnel.h
> +++ b/include/uapi/linux/if_tunnel.h
> @@ -24,9 +24,12 @@
> #define GRE_SEQ __cpu_to_be16(0x1000)
> #define GRE_STRICT __cpu_to_be16(0x0800)
> #define GRE_REC __cpu_to_be16(0x0700)
> -#define GRE_FLAGS __cpu_to_be16(0x00F8)
> +#define GRE_ACK __cpu_to_be16(0x0080)
> +#define GRE_FLAGS __cpu_to_be16(0x0078)
> #define GRE_VERSION __cpu_to_be16(0x0007)
>
> +#define GRE_PROTO_PPP __cpu_to_be16(0x880b)
> +
> struct ip_tunnel_parm {
> char name[IFNAMSIZ];
> int link;
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 61ad43f..33e957b 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -346,63 +346,103 @@ ip_proto_again:
> hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data, hlen, &_hdr);
> if (!hdr)
> goto out_bad;
> - /*
> - * Only look inside GRE if version zero and no
> - * routing
> - */
> - if (hdr->flags & (GRE_VERSION | GRE_ROUTING))
> - break;
> -
> - proto = hdr->proto;
> - nhoff += 4;
> - if (hdr->flags & GRE_CSUM)
> - nhoff += 4;
> - if (hdr->flags & GRE_KEY) {
> - const __be32 *keyid;
> - __be32 _keyid;
> -
> - keyid = __skb_header_pointer(skb, nhoff, sizeof(_keyid),
> - data, hlen, &_keyid);
>
> - if (!keyid)
> - goto out_bad;
> + /* Only look inside GRE without routing */
> + if (!(hdr->flags & GRE_ROUTING)) {
> + proto = hdr->proto;
> +
> + if (hdr->flags & GRE_VERSION) {
> + /* It should be the PPTP in GRE */
> + u8 _ppp_hdr[PPP_HDRLEN];
> + u8 *ppp_hdr;
> + int offset = 0;
> +
> + /* Check the flags according to RFC 2637*/
> + if (!(proto == GRE_PROTO_PPP && (hdr->flags & GRE_KEY) &&
> + !(hdr->flags & (GRE_CSUM | GRE_STRICT | GRE_REC | GRE_FLAGS)))) {
> + break;
> + }
> +
> + /* Skip GRE header */
> + offset += 4;
Please don't use literal values. Instead use the size of the field(s):
sizeof(struct gre_base_hdr) or offsetof(pptp_gre_header.payload_len)
> + /* Skip payload length and call id */
> + offset += 4;
sizeof(pptp_gre_header.payload_len) + sizeof(pptp_gre_header.call_id)
> +
> + if (hdr->flags & GRE_SEQ)
> + offset += 4;
sizeof(pptp_gre_header.seq)
> +
> + if (hdr->flags & GRE_ACK)
> + offset += 4;
sizeof(pptp_gre_header.ack)
> +
> + ppp_hdr = skb_header_pointer(skb, nhoff + offset, sizeof(_ppp_hdr), _ppp_hdr);
> + if (!ppp_hdr)
> + goto out_bad;
> + proto = PPP_PROTOCOL(ppp_hdr);
> + if (proto == PPP_IP) {
> + nhoff += (PPP_HDRLEN + offset);
> + proto = htons(ETH_P_IP);
> + key_control->flags |= FLOW_DIS_ENCAPSULATION;
> + goto again;
> + } else if (proto == PPP_IPV6) {
> + nhoff += (PPP_HDRLEN + offset);
> + proto = htons(ETH_P_IPV6);
> + key_control->flags |= FLOW_DIS_ENCAPSULATION;
> + goto again;
> + }
> + } else {
> + /* Original GRE */
> + nhoff += 4;
> + if (hdr->flags & GRE_CSUM)
> + nhoff += 4;
Again, use sizeof(gre_base_hdr) ...
> + if (hdr->flags & GRE_KEY) {
> + const __be32 *keyid;
> + __be32 _keyid;
> +
> + keyid = __skb_header_pointer(skb, nhoff, sizeof(_keyid),
> + data, hlen, &_keyid);
> +
> + if (!keyid)
> + goto out_bad;
> +
> + if (dissector_uses_key(flow_dissector,
> + FLOW_DISSECTOR_KEY_GRE_KEYID)) {
> + key_keyid = skb_flow_dissector_target(flow_dissector,
> + FLOW_DISSECTOR_KEY_GRE_KEYID,
> + target_container);
> + key_keyid->keyid = *keyid;
> + }
> + nhoff += 4;
Again, use the appropriate sizeof or sizeof's.
> + }
> + if (hdr->flags & GRE_SEQ)
> + nhoff += 4;
sizeof(pptp_gre_header.seq)
> + if (proto == htons(ETH_P_TEB)) {
> + const struct ethhdr *eth;
> + struct ethhdr _eth;
> +
> + eth = __skb_header_pointer(skb, nhoff,
> + sizeof(_eth),
> + data, hlen, &_eth);
> + if (!eth)
> + goto out_bad;
> + proto = eth->h_proto;
> + nhoff += sizeof(*eth);
> +
> + /* Cap headers that we access via pointers at the
> + * end of the Ethernet header as our maximum alignment
> + * at that point is only 2 bytes.
> + */
> + if (NET_IP_ALIGN)
> + hlen = nhoff;
> + }
> +
> + key_control->flags |= FLOW_DIS_ENCAPSULATION;
> + if (flags & FLOW_DISSECTOR_F_STOP_AT_ENCAP)
> + goto out_good;
>
> - if (dissector_uses_key(flow_dissector,
> - FLOW_DISSECTOR_KEY_GRE_KEYID)) {
> - key_keyid = skb_flow_dissector_target(flow_dissector,
> - FLOW_DISSECTOR_KEY_GRE_KEYID,
> - target_container);
> - key_keyid->keyid = *keyid;
> + goto again;
> }
> - nhoff += 4;
> - }
> - if (hdr->flags & GRE_SEQ)
> - nhoff += 4;
> - if (proto == htons(ETH_P_TEB)) {
> - const struct ethhdr *eth;
> - struct ethhdr _eth;
> -
> - eth = __skb_header_pointer(skb, nhoff,
> - sizeof(_eth),
> - data, hlen, &_eth);
> - if (!eth)
> - goto out_bad;
> - proto = eth->h_proto;
> - nhoff += sizeof(*eth);
> -
> - /* Cap headers that we access via pointers at the
> - * end of the Ethernet header as our maximum alignment
> - * at that point is only 2 bytes.
> - */
> - if (NET_IP_ALIGN)
> - hlen = nhoff;
> }
> -
> - key_control->flags |= FLOW_DIS_ENCAPSULATION;
> - if (flags & FLOW_DISSECTOR_F_STOP_AT_ENCAP)
> - goto out_good;
> -
> - goto again;
> + break;
> }
> case NEXTHDR_HOP:
> case NEXTHDR_ROUTING:
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash
2016-08-02 1:36 ` Philp Prindeville
@ 2016-08-02 6:10 ` Feng Gao
2016-08-02 17:33 ` Philp Prindeville
0 siblings, 1 reply; 18+ messages in thread
From: Feng Gao @ 2016-08-02 6:10 UTC (permalink / raw)
To: Philp Prindeville
Cc: fgao, David S. Miller, Stephen Hemminger, Pravin B Shelar,
Tom Herbert, Alex Duyck, Linux Kernel Network Developers
Thanks.
Because the original GRE uses the literal number directly, so I follow
this style.
Then I have two questions.
1. pptp_gre_header is defined in "drivers/net/ppp/pptp.c"; If we want
to use it, need to create one new header file, is it ok ?
2. The GRE version 0 patch could use the sizeof pptp_gre_header.seq ?
Best Regards
Feng
On Tue, Aug 2, 2016 at 9:36 AM, Philp Prindeville
<philipp@redfish-solutions.com> wrote:
> Inline...
>
> Main issue in a nutshell is I don't like using "4" instead of
> "sizeof(my_32bit_header_field_here)".
>
>
>
> On 07/28/2016 01:14 AM, fgao@48lvckh6395k16k5.yundunddos.com wrote:
>>
>> From: Gao Feng <fgao@ikuai8.com>
>>
>> The PPTP is encapsulated by GRE header with that GRE_VERSION bits
>> must contain one. But current GRE RPS needs the GRE_VERSION must be
>> zero. So RPS does not work for PPTP traffic.
>>
>> In my test environment, there are four MIPS cores, and all traffic
>> are passed through by PPTP. As a result, only one core is 100% busy
>> while other three cores are very idle. After this patch, the usage
>> of four cores are balanced well.
>>
>> Signed-off-by: Gao Feng <fgao@ikuai8.com>
>> ---
>> v2: Update according to Tom and Philp's advice.
>> 1) Consolidate the codes with GRE version 0 path;
>> 2) Use PPP_PROTOCOL to get ppp protol;
>> 3) Set the FLOW_DIS_ENCAPSULATION flag;
>> v1: Initial patch
>>
>> include/uapi/linux/if_tunnel.h | 5 +-
>> net/core/flow_dissector.c | 146
>> ++++++++++++++++++++++++++---------------
>> 2 files changed, 97 insertions(+), 54 deletions(-)
>>
>> diff --git a/include/uapi/linux/if_tunnel.h
>> b/include/uapi/linux/if_tunnel.h
>> index 1046f55..dda4e4b 100644
>> --- a/include/uapi/linux/if_tunnel.h
>> +++ b/include/uapi/linux/if_tunnel.h
>> @@ -24,9 +24,12 @@
>> #define GRE_SEQ __cpu_to_be16(0x1000)
>> #define GRE_STRICT __cpu_to_be16(0x0800)
>> #define GRE_REC __cpu_to_be16(0x0700)
>> -#define GRE_FLAGS __cpu_to_be16(0x00F8)
>> +#define GRE_ACK __cpu_to_be16(0x0080)
>> +#define GRE_FLAGS __cpu_to_be16(0x0078)
>> #define GRE_VERSION __cpu_to_be16(0x0007)
>> +#define GRE_PROTO_PPP __cpu_to_be16(0x880b)
>> +
>> struct ip_tunnel_parm {
>> char name[IFNAMSIZ];
>> int link;
>> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>> index 61ad43f..33e957b 100644
>> --- a/net/core/flow_dissector.c
>> +++ b/net/core/flow_dissector.c
>> @@ -346,63 +346,103 @@ ip_proto_again:
>> hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data,
>> hlen, &_hdr);
>> if (!hdr)
>> goto out_bad;
>> - /*
>> - * Only look inside GRE if version zero and no
>> - * routing
>> - */
>> - if (hdr->flags & (GRE_VERSION | GRE_ROUTING))
>> - break;
>> -
>> - proto = hdr->proto;
>> - nhoff += 4;
>> - if (hdr->flags & GRE_CSUM)
>> - nhoff += 4;
>> - if (hdr->flags & GRE_KEY) {
>> - const __be32 *keyid;
>> - __be32 _keyid;
>> -
>> - keyid = __skb_header_pointer(skb, nhoff,
>> sizeof(_keyid),
>> - data, hlen, &_keyid);
>> - if (!keyid)
>> - goto out_bad;
>> + /* Only look inside GRE without routing */
>> + if (!(hdr->flags & GRE_ROUTING)) {
>> + proto = hdr->proto;
>> +
>> + if (hdr->flags & GRE_VERSION) {
>> + /* It should be the PPTP in GRE */
>> + u8 _ppp_hdr[PPP_HDRLEN];
>> + u8 *ppp_hdr;
>> + int offset = 0;
>> +
>> + /* Check the flags according to RFC 2637*/
>> + if (!(proto == GRE_PROTO_PPP &&
>> (hdr->flags & GRE_KEY) &&
>> + !(hdr->flags & (GRE_CSUM |
>> GRE_STRICT | GRE_REC | GRE_FLAGS)))) {
>> + break;
>> + }
>> +
>> + /* Skip GRE header */
>> + offset += 4;
>
>
> Please don't use literal values. Instead use the size of the field(s):
>
> sizeof(struct gre_base_hdr) or offsetof(pptp_gre_header.payload_len)
>
>> + /* Skip payload length and call id */
>> + offset += 4;
>
>
> sizeof(pptp_gre_header.payload_len) + sizeof(pptp_gre_header.call_id)
>
>> +
>> + if (hdr->flags & GRE_SEQ)
>> + offset += 4;
>
>
> sizeof(pptp_gre_header.seq)
>
>> +
>> + if (hdr->flags & GRE_ACK)
>> + offset += 4;
>
>
> sizeof(pptp_gre_header.ack)
>
>
>> +
>> + ppp_hdr = skb_header_pointer(skb, nhoff +
>> offset, sizeof(_ppp_hdr), _ppp_hdr);
>> + if (!ppp_hdr)
>> + goto out_bad;
>> + proto = PPP_PROTOCOL(ppp_hdr);
>> + if (proto == PPP_IP) {
>> + nhoff += (PPP_HDRLEN + offset);
>> + proto = htons(ETH_P_IP);
>> + key_control->flags |=
>> FLOW_DIS_ENCAPSULATION;
>> + goto again;
>> + } else if (proto == PPP_IPV6) {
>> + nhoff += (PPP_HDRLEN + offset);
>> + proto = htons(ETH_P_IPV6);
>> + key_control->flags |=
>> FLOW_DIS_ENCAPSULATION;
>> + goto again;
>> + }
>> + } else {
>> + /* Original GRE */
>> + nhoff += 4;
>> + if (hdr->flags & GRE_CSUM)
>> + nhoff += 4;
>
>
> Again, use sizeof(gre_base_hdr) ...
>
>> + if (hdr->flags & GRE_KEY) {
>> + const __be32 *keyid;
>> + __be32 _keyid;
>> +
>> + keyid = __skb_header_pointer(skb,
>> nhoff, sizeof(_keyid),
>> + data,
>> hlen, &_keyid);
>> +
>> + if (!keyid)
>> + goto out_bad;
>> +
>> + if
>> (dissector_uses_key(flow_dissector,
>> +
>> FLOW_DISSECTOR_KEY_GRE_KEYID)) {
>> + key_keyid =
>> skb_flow_dissector_target(flow_dissector,
>> +
>> FLOW_DISSECTOR_KEY_GRE_KEYID,
>> +
>> target_container);
>> + key_keyid->keyid = *keyid;
>> + }
>> + nhoff += 4;
>
>
> Again, use the appropriate sizeof or sizeof's.
>
>> + }
>> + if (hdr->flags & GRE_SEQ)
>> + nhoff += 4;
>
>
> sizeof(pptp_gre_header.seq)
>
>
>> + if (proto == htons(ETH_P_TEB)) {
>> + const struct ethhdr *eth;
>> + struct ethhdr _eth;
>> +
>> + eth = __skb_header_pointer(skb,
>> nhoff,
>> +
>> sizeof(_eth),
>> + data,
>> hlen, &_eth);
>> + if (!eth)
>> + goto out_bad;
>> + proto = eth->h_proto;
>> + nhoff += sizeof(*eth);
>> +
>> + /* Cap headers that we access via
>> pointers at the
>> + * end of the Ethernet header as
>> our maximum alignment
>> + * at that point is only 2 bytes.
>> + */
>> + if (NET_IP_ALIGN)
>> + hlen = nhoff;
>> + }
>> +
>> + key_control->flags |=
>> FLOW_DIS_ENCAPSULATION;
>> + if (flags &
>> FLOW_DISSECTOR_F_STOP_AT_ENCAP)
>> + goto out_good;
>> - if (dissector_uses_key(flow_dissector,
>> -
>> FLOW_DISSECTOR_KEY_GRE_KEYID)) {
>> - key_keyid =
>> skb_flow_dissector_target(flow_dissector,
>> -
>> FLOW_DISSECTOR_KEY_GRE_KEYID,
>> -
>> target_container);
>> - key_keyid->keyid = *keyid;
>> + goto again;
>> }
>> - nhoff += 4;
>> - }
>> - if (hdr->flags & GRE_SEQ)
>> - nhoff += 4;
>> - if (proto == htons(ETH_P_TEB)) {
>> - const struct ethhdr *eth;
>> - struct ethhdr _eth;
>> -
>> - eth = __skb_header_pointer(skb, nhoff,
>> - sizeof(_eth),
>> - data, hlen, &_eth);
>> - if (!eth)
>> - goto out_bad;
>> - proto = eth->h_proto;
>> - nhoff += sizeof(*eth);
>> -
>> - /* Cap headers that we access via pointers at the
>> - * end of the Ethernet header as our maximum
>> alignment
>> - * at that point is only 2 bytes.
>> - */
>> - if (NET_IP_ALIGN)
>> - hlen = nhoff;
>> }
>> -
>> - key_control->flags |= FLOW_DIS_ENCAPSULATION;
>> - if (flags & FLOW_DISSECTOR_F_STOP_AT_ENCAP)
>> - goto out_good;
>> -
>> - goto again;
>> + break;
>> }
>> case NEXTHDR_HOP:
>> case NEXTHDR_ROUTING:
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash
2016-08-02 6:10 ` Feng Gao
@ 2016-08-02 17:33 ` Philp Prindeville
2016-08-03 0:22 ` Feng Gao
0 siblings, 1 reply; 18+ messages in thread
From: Philp Prindeville @ 2016-08-02 17:33 UTC (permalink / raw)
To: Feng Gao
Cc: fgao, David S. Miller, Stephen Hemminger, Pravin B Shelar,
Tom Herbert, Alex Duyck, Linux Kernel Network Developers
Inline...
On 08/02/2016 12:10 AM, Feng Gao wrote:
> Thanks.
> Because the original GRE uses the literal number directly, so I follow
> this style.
>
> Then I have two questions.
> 1. pptp_gre_header is defined in "drivers/net/ppp/pptp.c"; If we want
> to use it, need to create one new header file, is it ok ?
Yes, I would move the relevant constants and packet structures into
include/net/pptp.h ...
While you're at it, there are places like:
islcp = ((data[0] << 8) + data[1]) == PPP_LCP && 1 <= data[2] &&
data[2] <= 7;
that should be written as:
islcp = PPP_PROTOCOL(data-2) == PPP_LCP && CP_CONF_REQ <= data[2]
&& data[2] <= CP_CODE_REJ;
Similarly for:
data[0] = PPP_ALLSTATIONS;
data[1] = PPP_UI;
that should be using:
PPP_ADDRESS(data) = PPP_ALLSTATIONS;
PPP_CONTROL(data) = PPP_UI;
etc. but this cleanup can go in a 2nd patch. The definitions for
CONFREQ and CONFACK and CP_CONF_REQ and CP_CONF_ACK and PPP_LCP_ECHOREQ
and PPP_LCP_ECHOREP are redundant and probably should all go into
ppp_defs.h as well (which would require refactoring
drivers/net/ppp/ppp_async.c).
> 2. The GRE version 0 patch could use the sizeof pptp_gre_header.seq ?
Actually, since you're going to be doing the pptp.h header anyway, I'd
add a v0 as well as a v1 struct just to be more clear/concise.
-Philip
> Best Regards
> Feng
>
>
[snip]
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash
2016-08-02 17:33 ` Philp Prindeville
@ 2016-08-03 0:22 ` Feng Gao
0 siblings, 0 replies; 18+ messages in thread
From: Feng Gao @ 2016-08-03 0:22 UTC (permalink / raw)
To: Philp Prindeville
Cc: fgao, David S. Miller, Stephen Hemminger, Pravin B Shelar,
Tom Herbert, Alex Duyck, Linux Kernel Network Developers
Thanks Philp.
I get it, then i will send another update v3 patch soon.
On Wed, Aug 3, 2016 at 1:33 AM, Philp Prindeville
<philipp@redfish-solutions.com> wrote:
> Inline...
>
>
> On 08/02/2016 12:10 AM, Feng Gao wrote:
>>
>> Thanks.
>> Because the original GRE uses the literal number directly, so I follow
>> this style.
>>
>> Then I have two questions.
>> 1. pptp_gre_header is defined in "drivers/net/ppp/pptp.c"; If we want
>> to use it, need to create one new header file, is it ok ?
>
>
> Yes, I would move the relevant constants and packet structures into
> include/net/pptp.h ...
>
> While you're at it, there are places like:
>
> islcp = ((data[0] << 8) + data[1]) == PPP_LCP && 1 <= data[2] && data[2]
> <= 7;
>
> that should be written as:
>
> islcp = PPP_PROTOCOL(data-2) == PPP_LCP && CP_CONF_REQ <= data[2] &&
> data[2] <= CP_CODE_REJ;
>
> Similarly for:
>
> data[0] = PPP_ALLSTATIONS;
> data[1] = PPP_UI;
>
> that should be using:
>
> PPP_ADDRESS(data) = PPP_ALLSTATIONS;
> PPP_CONTROL(data) = PPP_UI;
>
> etc. but this cleanup can go in a 2nd patch. The definitions for CONFREQ
> and CONFACK and CP_CONF_REQ and CP_CONF_ACK and PPP_LCP_ECHOREQ and
> PPP_LCP_ECHOREP are redundant and probably should all go into ppp_defs.h as
> well (which would require refactoring drivers/net/ppp/ppp_async.c).
>
>
>> 2. The GRE version 0 patch could use the sizeof pptp_gre_header.seq ?
>
>
> Actually, since you're going to be doing the pptp.h header anyway, I'd add a
> v0 as well as a v1 struct just to be more clear/concise.
>
> -Philip
>
>> Best Regards
>> Feng
>>
>>
> [snip]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash
2016-07-28 7:14 [PATCH 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash fgao
2016-08-02 1:36 ` Philp Prindeville
@ 2016-08-02 20:35 ` Tom Herbert
2016-08-03 0:28 ` Feng Gao
1 sibling, 1 reply; 18+ messages in thread
From: Tom Herbert @ 2016-08-02 20:35 UTC (permalink / raw)
To: fgao
Cc: David S. Miller, philipp, Stephen Hemminger, Pravin B Shelar,
Alex Duyck, Linux Kernel Network Developers, Feng Gao
On Thu, Jul 28, 2016 at 12:14 AM, <fgao@ikuai8.com> wrote:
> From: Gao Feng <fgao@ikuai8.com>
>
> The PPTP is encapsulated by GRE header with that GRE_VERSION bits
> must contain one. But current GRE RPS needs the GRE_VERSION must be
> zero. So RPS does not work for PPTP traffic.
>
> In my test environment, there are four MIPS cores, and all traffic
> are passed through by PPTP. As a result, only one core is 100% busy
> while other three cores are very idle. After this patch, the usage
> of four cores are balanced well.
>
> Signed-off-by: Gao Feng <fgao@ikuai8.com>
> ---
> v2: Update according to Tom and Philp's advice.
> 1) Consolidate the codes with GRE version 0 path;
> 2) Use PPP_PROTOCOL to get ppp protol;
> 3) Set the FLOW_DIS_ENCAPSULATION flag;
> v1: Initial patch
>
> include/uapi/linux/if_tunnel.h | 5 +-
> net/core/flow_dissector.c | 146 ++++++++++++++++++++++++++---------------
> 2 files changed, 97 insertions(+), 54 deletions(-)
>
> diff --git a/include/uapi/linux/if_tunnel.h b/include/uapi/linux/if_tunnel.h
> index 1046f55..dda4e4b 100644
> --- a/include/uapi/linux/if_tunnel.h
> +++ b/include/uapi/linux/if_tunnel.h
> @@ -24,9 +24,12 @@
> #define GRE_SEQ __cpu_to_be16(0x1000)
> #define GRE_STRICT __cpu_to_be16(0x0800)
> #define GRE_REC __cpu_to_be16(0x0700)
> -#define GRE_FLAGS __cpu_to_be16(0x00F8)
> +#define GRE_ACK __cpu_to_be16(0x0080)
> +#define GRE_FLAGS __cpu_to_be16(0x0078)
> #define GRE_VERSION __cpu_to_be16(0x0007)
>
> +#define GRE_PROTO_PPP __cpu_to_be16(0x880b)
> +
> struct ip_tunnel_parm {
> char name[IFNAMSIZ];
> int link;
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 61ad43f..33e957b 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -346,63 +346,103 @@ ip_proto_again:
> hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data, hlen, &_hdr);
> if (!hdr)
> goto out_bad;
> - /*
> - * Only look inside GRE if version zero and no
> - * routing
> - */
> - if (hdr->flags & (GRE_VERSION | GRE_ROUTING))
> - break;
> -
> - proto = hdr->proto;
> - nhoff += 4;
> - if (hdr->flags & GRE_CSUM)
> - nhoff += 4;
> - if (hdr->flags & GRE_KEY) {
> - const __be32 *keyid;
> - __be32 _keyid;
> -
> - keyid = __skb_header_pointer(skb, nhoff, sizeof(_keyid),
> - data, hlen, &_keyid);
>
> - if (!keyid)
> - goto out_bad;
> + /* Only look inside GRE without routing */
> + if (!(hdr->flags & GRE_ROUTING)) {
> + proto = hdr->proto;
> +
> + if (hdr->flags & GRE_VERSION) {
This matches any non-zero GRE version (1-7). Is that really what you want?
> + /* It should be the PPTP in GRE */
> + u8 _ppp_hdr[PPP_HDRLEN];
> + u8 *ppp_hdr;
> + int offset = 0;
> +
> + /* Check the flags according to RFC 2637*/
> + if (!(proto == GRE_PROTO_PPP && (hdr->flags & GRE_KEY) &&
> + !(hdr->flags & (GRE_CSUM | GRE_STRICT | GRE_REC | GRE_FLAGS)))) {
> + break;
> + }
> +
I might be missing something, but I only see two material differences
between v0 and v1 of GRE. First is that keyid in v1 has specific use
that is not appropriate to use as entropy. Second is how the payload
is parsed. All the flag processing is common so it stills seems like
this should be consolidated.
> + /* Skip GRE header */
> + offset += 4;
> + /* Skip payload length and call id */
> + offset += 4;
> +
> + if (hdr->flags & GRE_SEQ)
> + offset += 4;
> +
> + if (hdr->flags & GRE_ACK)
> + offset += 4;
> +
> + ppp_hdr = skb_header_pointer(skb, nhoff + offset, sizeof(_ppp_hdr), _ppp_hdr);
> + if (!ppp_hdr)
> + goto out_bad;
> + proto = PPP_PROTOCOL(ppp_hdr);
> + if (proto == PPP_IP) {
> + nhoff += (PPP_HDRLEN + offset);
> + proto = htons(ETH_P_IP);
> + key_control->flags |= FLOW_DIS_ENCAPSULATION;
> + goto again;
> + } else if (proto == PPP_IPV6) {
> + nhoff += (PPP_HDRLEN + offset);
> + proto = htons(ETH_P_IPV6);
> + key_control->flags |= FLOW_DIS_ENCAPSULATION;
> + goto again;
> + }
> + } else {
> + /* Original GRE */
> + nhoff += 4;
> + if (hdr->flags & GRE_CSUM)
> + nhoff += 4;
> + if (hdr->flags & GRE_KEY) {
> + const __be32 *keyid;
> + __be32 _keyid;
> +
> + keyid = __skb_header_pointer(skb, nhoff, sizeof(_keyid),
> + data, hlen, &_keyid);
> +
> + if (!keyid)
> + goto out_bad;
> +
> + if (dissector_uses_key(flow_dissector,
> + FLOW_DISSECTOR_KEY_GRE_KEYID)) {
> + key_keyid = skb_flow_dissector_target(flow_dissector,
> + FLOW_DISSECTOR_KEY_GRE_KEYID,
> + target_container);
> + key_keyid->keyid = *keyid;
> + }
> + nhoff += 4;
> + }
> + if (hdr->flags & GRE_SEQ)
> + nhoff += 4;
> + if (proto == htons(ETH_P_TEB)) {
> + const struct ethhdr *eth;
> + struct ethhdr _eth;
> +
> + eth = __skb_header_pointer(skb, nhoff,
> + sizeof(_eth),
> + data, hlen, &_eth);
> + if (!eth)
> + goto out_bad;
> + proto = eth->h_proto;
> + nhoff += sizeof(*eth);
> +
> + /* Cap headers that we access via pointers at the
> + * end of the Ethernet header as our maximum alignment
> + * at that point is only 2 bytes.
> + */
> + if (NET_IP_ALIGN)
> + hlen = nhoff;
> + }
> +
> + key_control->flags |= FLOW_DIS_ENCAPSULATION;
> + if (flags & FLOW_DISSECTOR_F_STOP_AT_ENCAP)
> + goto out_good;
>
> - if (dissector_uses_key(flow_dissector,
> - FLOW_DISSECTOR_KEY_GRE_KEYID)) {
> - key_keyid = skb_flow_dissector_target(flow_dissector,
> - FLOW_DISSECTOR_KEY_GRE_KEYID,
> - target_container);
> - key_keyid->keyid = *keyid;
> + goto again;
> }
> - nhoff += 4;
> - }
> - if (hdr->flags & GRE_SEQ)
> - nhoff += 4;
> - if (proto == htons(ETH_P_TEB)) {
> - const struct ethhdr *eth;
> - struct ethhdr _eth;
> -
> - eth = __skb_header_pointer(skb, nhoff,
> - sizeof(_eth),
> - data, hlen, &_eth);
> - if (!eth)
> - goto out_bad;
> - proto = eth->h_proto;
> - nhoff += sizeof(*eth);
> -
> - /* Cap headers that we access via pointers at the
> - * end of the Ethernet header as our maximum alignment
> - * at that point is only 2 bytes.
> - */
> - if (NET_IP_ALIGN)
> - hlen = nhoff;
> }
> -
> - key_control->flags |= FLOW_DIS_ENCAPSULATION;
> - if (flags & FLOW_DISSECTOR_F_STOP_AT_ENCAP)
> - goto out_good;
> -
> - goto again;
> + break;
> }
> case NEXTHDR_HOP:
> case NEXTHDR_ROUTING:
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash
2016-08-02 20:35 ` Tom Herbert
@ 2016-08-03 0:28 ` Feng Gao
2016-08-03 1:11 ` Philip Prindeville
2016-08-03 1:59 ` Tom Herbert
0 siblings, 2 replies; 18+ messages in thread
From: Feng Gao @ 2016-08-03 0:28 UTC (permalink / raw)
To: Tom Herbert
Cc: Gao Feng, David S. Miller, Philp Prindeville, Stephen Hemminger,
Pravin B Shelar, Alex Duyck, Linux Kernel Network Developers
Hi Tom.
I quote some description in RFC 2637.
/*********************************************************************/
C
(Bit 0) Checksum Present. Set to zero (0).
R
(Bit 1) Routing Present. Set to zero (0).
K
(Bit 2) Key Present. Set to one (1).
S
(Bit 3) Sequence Number Present. Set to one (1) if a payload
(data) packet is present. Set to zero (0) if payload is not
present (GRE packet is an Acknowledgment only).
s
(Bit 4) Strict source route present. Set to zero (0).
Recur
(Bits 5-7) Recursion control. Set to zero (0).
A
(Bit 8) Acknowledgment sequence number present. Set to one (1) if
packet contains Acknowledgment Number to be used for acknowledging
previously transmitted data.
Flags
(Bits 9-12) Must be set to zero (0).
Ver
(Bits 13-15) Must contain 1 (enhanced GRE).
Protocol Type
Set to hex 880B [8].
Key
Use of the Key field is up to the implementation. PPTP uses it as
follows:
Payload Length
(High 2 octets of Key) Size of the payload, not including
the GRE header
Call ID
(Low 2 octets) Contains the Peer's Call ID for the session
to which this packet belongs.
/*********************************************************************/
Just a reminder, the version description is "contain 1", not "set 1".
Best Regards
Feng
On Wed, Aug 3, 2016 at 4:35 AM, Tom Herbert <tom@herbertland.com> wrote:
> On Thu, Jul 28, 2016 at 12:14 AM, <fgao@ikuai8.com> wrote:
>> From: Gao Feng <fgao@ikuai8.com>
>>
>> The PPTP is encapsulated by GRE header with that GRE_VERSION bits
>> must contain one. But current GRE RPS needs the GRE_VERSION must be
>> zero. So RPS does not work for PPTP traffic.
>>
>> In my test environment, there are four MIPS cores, and all traffic
>> are passed through by PPTP. As a result, only one core is 100% busy
>> while other three cores are very idle. After this patch, the usage
>> of four cores are balanced well.
>>
>> Signed-off-by: Gao Feng <fgao@ikuai8.com>
>> ---
>> v2: Update according to Tom and Philp's advice.
>> 1) Consolidate the codes with GRE version 0 path;
>> 2) Use PPP_PROTOCOL to get ppp protol;
>> 3) Set the FLOW_DIS_ENCAPSULATION flag;
>> v1: Initial patch
>>
>> include/uapi/linux/if_tunnel.h | 5 +-
>> net/core/flow_dissector.c | 146 ++++++++++++++++++++++++++---------------
>> 2 files changed, 97 insertions(+), 54 deletions(-)
>>
>> diff --git a/include/uapi/linux/if_tunnel.h b/include/uapi/linux/if_tunnel.h
>> index 1046f55..dda4e4b 100644
>> --- a/include/uapi/linux/if_tunnel.h
>> +++ b/include/uapi/linux/if_tunnel.h
>> @@ -24,9 +24,12 @@
>> #define GRE_SEQ __cpu_to_be16(0x1000)
>> #define GRE_STRICT __cpu_to_be16(0x0800)
>> #define GRE_REC __cpu_to_be16(0x0700)
>> -#define GRE_FLAGS __cpu_to_be16(0x00F8)
>> +#define GRE_ACK __cpu_to_be16(0x0080)
>> +#define GRE_FLAGS __cpu_to_be16(0x0078)
>> #define GRE_VERSION __cpu_to_be16(0x0007)
>>
>> +#define GRE_PROTO_PPP __cpu_to_be16(0x880b)
>> +
>> struct ip_tunnel_parm {
>> char name[IFNAMSIZ];
>> int link;
>> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>> index 61ad43f..33e957b 100644
>> --- a/net/core/flow_dissector.c
>> +++ b/net/core/flow_dissector.c
>> @@ -346,63 +346,103 @@ ip_proto_again:
>> hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data, hlen, &_hdr);
>> if (!hdr)
>> goto out_bad;
>> - /*
>> - * Only look inside GRE if version zero and no
>> - * routing
>> - */
>> - if (hdr->flags & (GRE_VERSION | GRE_ROUTING))
>> - break;
>> -
>> - proto = hdr->proto;
>> - nhoff += 4;
>> - if (hdr->flags & GRE_CSUM)
>> - nhoff += 4;
>> - if (hdr->flags & GRE_KEY) {
>> - const __be32 *keyid;
>> - __be32 _keyid;
>> -
>> - keyid = __skb_header_pointer(skb, nhoff, sizeof(_keyid),
>> - data, hlen, &_keyid);
>>
>> - if (!keyid)
>> - goto out_bad;
>> + /* Only look inside GRE without routing */
>> + if (!(hdr->flags & GRE_ROUTING)) {
>> + proto = hdr->proto;
>> +
>> + if (hdr->flags & GRE_VERSION) {
>
> This matches any non-zero GRE version (1-7). Is that really what you want?
>
>> + /* It should be the PPTP in GRE */
>> + u8 _ppp_hdr[PPP_HDRLEN];
>> + u8 *ppp_hdr;
>> + int offset = 0;
>> +
>> + /* Check the flags according to RFC 2637*/
>> + if (!(proto == GRE_PROTO_PPP && (hdr->flags & GRE_KEY) &&
>> + !(hdr->flags & (GRE_CSUM | GRE_STRICT | GRE_REC | GRE_FLAGS)))) {
>> + break;
>> + }
>> +
> I might be missing something, but I only see two material differences
> between v0 and v1 of GRE. First is that keyid in v1 has specific use
> that is not appropriate to use as entropy. Second is how the payload
> is parsed. All the flag processing is common so it stills seems like
> this should be consolidated.
>
>> + /* Skip GRE header */
>> + offset += 4;
>> + /* Skip payload length and call id */
>> + offset += 4;
>> +
>> + if (hdr->flags & GRE_SEQ)
>> + offset += 4;
>> +
>> + if (hdr->flags & GRE_ACK)
>> + offset += 4;
>> +
>> + ppp_hdr = skb_header_pointer(skb, nhoff + offset, sizeof(_ppp_hdr), _ppp_hdr);
>> + if (!ppp_hdr)
>> + goto out_bad;
>> + proto = PPP_PROTOCOL(ppp_hdr);
>> + if (proto == PPP_IP) {
>> + nhoff += (PPP_HDRLEN + offset);
>> + proto = htons(ETH_P_IP);
>> + key_control->flags |= FLOW_DIS_ENCAPSULATION;
>> + goto again;
>> + } else if (proto == PPP_IPV6) {
>> + nhoff += (PPP_HDRLEN + offset);
>> + proto = htons(ETH_P_IPV6);
>> + key_control->flags |= FLOW_DIS_ENCAPSULATION;
>> + goto again;
>> + }
>> + } else {
>> + /* Original GRE */
>> + nhoff += 4;
>> + if (hdr->flags & GRE_CSUM)
>> + nhoff += 4;
>> + if (hdr->flags & GRE_KEY) {
>> + const __be32 *keyid;
>> + __be32 _keyid;
>> +
>> + keyid = __skb_header_pointer(skb, nhoff, sizeof(_keyid),
>> + data, hlen, &_keyid);
>> +
>> + if (!keyid)
>> + goto out_bad;
>> +
>> + if (dissector_uses_key(flow_dissector,
>> + FLOW_DISSECTOR_KEY_GRE_KEYID)) {
>> + key_keyid = skb_flow_dissector_target(flow_dissector,
>> + FLOW_DISSECTOR_KEY_GRE_KEYID,
>> + target_container);
>> + key_keyid->keyid = *keyid;
>> + }
>> + nhoff += 4;
>> + }
>> + if (hdr->flags & GRE_SEQ)
>> + nhoff += 4;
>> + if (proto == htons(ETH_P_TEB)) {
>> + const struct ethhdr *eth;
>> + struct ethhdr _eth;
>> +
>> + eth = __skb_header_pointer(skb, nhoff,
>> + sizeof(_eth),
>> + data, hlen, &_eth);
>> + if (!eth)
>> + goto out_bad;
>> + proto = eth->h_proto;
>> + nhoff += sizeof(*eth);
>> +
>> + /* Cap headers that we access via pointers at the
>> + * end of the Ethernet header as our maximum alignment
>> + * at that point is only 2 bytes.
>> + */
>> + if (NET_IP_ALIGN)
>> + hlen = nhoff;
>> + }
>> +
>> + key_control->flags |= FLOW_DIS_ENCAPSULATION;
>> + if (flags & FLOW_DISSECTOR_F_STOP_AT_ENCAP)
>> + goto out_good;
>>
>> - if (dissector_uses_key(flow_dissector,
>> - FLOW_DISSECTOR_KEY_GRE_KEYID)) {
>> - key_keyid = skb_flow_dissector_target(flow_dissector,
>> - FLOW_DISSECTOR_KEY_GRE_KEYID,
>> - target_container);
>> - key_keyid->keyid = *keyid;
>> + goto again;
>> }
>> - nhoff += 4;
>> - }
>> - if (hdr->flags & GRE_SEQ)
>> - nhoff += 4;
>> - if (proto == htons(ETH_P_TEB)) {
>> - const struct ethhdr *eth;
>> - struct ethhdr _eth;
>> -
>> - eth = __skb_header_pointer(skb, nhoff,
>> - sizeof(_eth),
>> - data, hlen, &_eth);
>> - if (!eth)
>> - goto out_bad;
>> - proto = eth->h_proto;
>> - nhoff += sizeof(*eth);
>> -
>> - /* Cap headers that we access via pointers at the
>> - * end of the Ethernet header as our maximum alignment
>> - * at that point is only 2 bytes.
>> - */
>> - if (NET_IP_ALIGN)
>> - hlen = nhoff;
>> }
>> -
>> - key_control->flags |= FLOW_DIS_ENCAPSULATION;
>> - if (flags & FLOW_DISSECTOR_F_STOP_AT_ENCAP)
>> - goto out_good;
>> -
>> - goto again;
>> + break;
>> }
>> case NEXTHDR_HOP:
>> case NEXTHDR_ROUTING:
>> --
>> 1.9.1
>>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash
2016-08-03 0:28 ` Feng Gao
@ 2016-08-03 1:11 ` Philip Prindeville
2016-08-03 1:59 ` Tom Herbert
1 sibling, 0 replies; 18+ messages in thread
From: Philip Prindeville @ 2016-08-03 1:11 UTC (permalink / raw)
To: Feng Gao
Cc: Tom Herbert, Gao Feng, David S. Miller, Stephen Hemminger,
Pravin B Shelar, Alex Duyck, Linux Kernel Network Developers
[-- Attachment #1: Type: text/plain, Size: 216 bytes --]
In this case they both mean the same thing.
> On Aug 2, 2016, at 6:28 PM, Feng Gao <gfree.wind@gmail.com> wrote:
>
> Just a reminder, the version description is "contain 1", not "set 1".
>
> Best Regards
> Feng
[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 496 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash
2016-08-03 0:28 ` Feng Gao
2016-08-03 1:11 ` Philip Prindeville
@ 2016-08-03 1:59 ` Tom Herbert
2016-08-03 2:17 ` Feng Gao
1 sibling, 1 reply; 18+ messages in thread
From: Tom Herbert @ 2016-08-03 1:59 UTC (permalink / raw)
To: Feng Gao
Cc: Gao Feng, David S. Miller, Philp Prindeville, Stephen Hemminger,
Pravin B Shelar, Alex Duyck, Linux Kernel Network Developers
On Tue, Aug 2, 2016 at 5:28 PM, Feng Gao <gfree.wind@gmail.com> wrote:
> Hi Tom.
>
> I quote some description in RFC 2637.
>
> /*********************************************************************/
> C
> (Bit 0) Checksum Present. Set to zero (0).
>
Note that the requirement is "*Set* to zero"; it is not must check to
be zero on reception. Per the robustness principle (part about be
liberal in what you receive) it is allowable to process the flag if
the bit is set set. In practice this is nice to have because we know
that overloading fields in GRE (seq, csum in particular) with private
data has been done before. Consolidating makes for cleaner code, and
if someone is really violating the protocol the worst thing that would
happen it that they would just get suboptimal RPS for those packets.
> R
> (Bit 1) Routing Present. Set to zero (0).
>
> K
> (Bit 2) Key Present. Set to one (1).
> S
> (Bit 3) Sequence Number Present. Set to one (1) if a payload
> (data) packet is present. Set to zero (0) if payload is not
> present (GRE packet is an Acknowledgment only).
>
> s
> (Bit 4) Strict source route present. Set to zero (0).
>
> Recur
> (Bits 5-7) Recursion control. Set to zero (0).
>
> A
> (Bit 8) Acknowledgment sequence number present. Set to one (1) if
> packet contains Acknowledgment Number to be used for acknowledging
> previously transmitted data.
>
> Flags
> (Bits 9-12) Must be set to zero (0).
>
> Ver
> (Bits 13-15) Must contain 1 (enhanced GRE).
>
> Protocol Type
> Set to hex 880B [8].
>
> Key
> Use of the Key field is up to the implementation. PPTP uses it as
> follows:
> Payload Length
> (High 2 octets of Key) Size of the payload, not including
> the GRE header
>
> Call ID
> (Low 2 octets) Contains the Peer's Call ID for the session
> to which this packet belongs.
> /*********************************************************************/
>
> Just a reminder, the version description is "contain 1", not "set 1".
>
I think your misinterpreting that (at least I hope :-) ). "Contains 1"
should be that the contents of the Ver field is the value of "1".
Otherwise, the RFC has appropriated half of the eight possible
versions to be enhanced GRE.
Tom
> Best Regards
> Feng
>
> On Wed, Aug 3, 2016 at 4:35 AM, Tom Herbert <tom@herbertland.com> wrote:
>> On Thu, Jul 28, 2016 at 12:14 AM, <fgao@ikuai8.com> wrote:
>>> From: Gao Feng <fgao@ikuai8.com>
>>>
>>> The PPTP is encapsulated by GRE header with that GRE_VERSION bits
>>> must contain one. But current GRE RPS needs the GRE_VERSION must be
>>> zero. So RPS does not work for PPTP traffic.
>>>
>>> In my test environment, there are four MIPS cores, and all traffic
>>> are passed through by PPTP. As a result, only one core is 100% busy
>>> while other three cores are very idle. After this patch, the usage
>>> of four cores are balanced well.
>>>
>>> Signed-off-by: Gao Feng <fgao@ikuai8.com>
>>> ---
>>> v2: Update according to Tom and Philp's advice.
>>> 1) Consolidate the codes with GRE version 0 path;
>>> 2) Use PPP_PROTOCOL to get ppp protol;
>>> 3) Set the FLOW_DIS_ENCAPSULATION flag;
>>> v1: Initial patch
>>>
>>> include/uapi/linux/if_tunnel.h | 5 +-
>>> net/core/flow_dissector.c | 146 ++++++++++++++++++++++++++---------------
>>> 2 files changed, 97 insertions(+), 54 deletions(-)
>>>
>>> diff --git a/include/uapi/linux/if_tunnel.h b/include/uapi/linux/if_tunnel.h
>>> index 1046f55..dda4e4b 100644
>>> --- a/include/uapi/linux/if_tunnel.h
>>> +++ b/include/uapi/linux/if_tunnel.h
>>> @@ -24,9 +24,12 @@
>>> #define GRE_SEQ __cpu_to_be16(0x1000)
>>> #define GRE_STRICT __cpu_to_be16(0x0800)
>>> #define GRE_REC __cpu_to_be16(0x0700)
>>> -#define GRE_FLAGS __cpu_to_be16(0x00F8)
>>> +#define GRE_ACK __cpu_to_be16(0x0080)
>>> +#define GRE_FLAGS __cpu_to_be16(0x0078)
>>> #define GRE_VERSION __cpu_to_be16(0x0007)
>>>
>>> +#define GRE_PROTO_PPP __cpu_to_be16(0x880b)
>>> +
>>> struct ip_tunnel_parm {
>>> char name[IFNAMSIZ];
>>> int link;
>>> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>>> index 61ad43f..33e957b 100644
>>> --- a/net/core/flow_dissector.c
>>> +++ b/net/core/flow_dissector.c
>>> @@ -346,63 +346,103 @@ ip_proto_again:
>>> hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data, hlen, &_hdr);
>>> if (!hdr)
>>> goto out_bad;
>>> - /*
>>> - * Only look inside GRE if version zero and no
>>> - * routing
>>> - */
>>> - if (hdr->flags & (GRE_VERSION | GRE_ROUTING))
>>> - break;
>>> -
>>> - proto = hdr->proto;
>>> - nhoff += 4;
>>> - if (hdr->flags & GRE_CSUM)
>>> - nhoff += 4;
>>> - if (hdr->flags & GRE_KEY) {
>>> - const __be32 *keyid;
>>> - __be32 _keyid;
>>> -
>>> - keyid = __skb_header_pointer(skb, nhoff, sizeof(_keyid),
>>> - data, hlen, &_keyid);
>>>
>>> - if (!keyid)
>>> - goto out_bad;
>>> + /* Only look inside GRE without routing */
>>> + if (!(hdr->flags & GRE_ROUTING)) {
>>> + proto = hdr->proto;
>>> +
>>> + if (hdr->flags & GRE_VERSION) {
>>
>> This matches any non-zero GRE version (1-7). Is that really what you want?
>>
>>> + /* It should be the PPTP in GRE */
>>> + u8 _ppp_hdr[PPP_HDRLEN];
>>> + u8 *ppp_hdr;
>>> + int offset = 0;
>>> +
>>> + /* Check the flags according to RFC 2637*/
>>> + if (!(proto == GRE_PROTO_PPP && (hdr->flags & GRE_KEY) &&
>>> + !(hdr->flags & (GRE_CSUM | GRE_STRICT | GRE_REC | GRE_FLAGS)))) {
>>> + break;
>>> + }
>>> +
>> I might be missing something, but I only see two material differences
>> between v0 and v1 of GRE. First is that keyid in v1 has specific use
>> that is not appropriate to use as entropy. Second is how the payload
>> is parsed. All the flag processing is common so it stills seems like
>> this should be consolidated.
>>
>>> + /* Skip GRE header */
>>> + offset += 4;
>>> + /* Skip payload length and call id */
>>> + offset += 4;
>>> +
>>> + if (hdr->flags & GRE_SEQ)
>>> + offset += 4;
>>> +
>>> + if (hdr->flags & GRE_ACK)
>>> + offset += 4;
>>> +
>>> + ppp_hdr = skb_header_pointer(skb, nhoff + offset, sizeof(_ppp_hdr), _ppp_hdr);
>>> + if (!ppp_hdr)
>>> + goto out_bad;
>>> + proto = PPP_PROTOCOL(ppp_hdr);
>>> + if (proto == PPP_IP) {
>>> + nhoff += (PPP_HDRLEN + offset);
>>> + proto = htons(ETH_P_IP);
>>> + key_control->flags |= FLOW_DIS_ENCAPSULATION;
>>> + goto again;
>>> + } else if (proto == PPP_IPV6) {
>>> + nhoff += (PPP_HDRLEN + offset);
>>> + proto = htons(ETH_P_IPV6);
>>> + key_control->flags |= FLOW_DIS_ENCAPSULATION;
>>> + goto again;
>>> + }
>>> + } else {
>>> + /* Original GRE */
>>> + nhoff += 4;
>>> + if (hdr->flags & GRE_CSUM)
>>> + nhoff += 4;
>>> + if (hdr->flags & GRE_KEY) {
>>> + const __be32 *keyid;
>>> + __be32 _keyid;
>>> +
>>> + keyid = __skb_header_pointer(skb, nhoff, sizeof(_keyid),
>>> + data, hlen, &_keyid);
>>> +
>>> + if (!keyid)
>>> + goto out_bad;
>>> +
>>> + if (dissector_uses_key(flow_dissector,
>>> + FLOW_DISSECTOR_KEY_GRE_KEYID)) {
>>> + key_keyid = skb_flow_dissector_target(flow_dissector,
>>> + FLOW_DISSECTOR_KEY_GRE_KEYID,
>>> + target_container);
>>> + key_keyid->keyid = *keyid;
>>> + }
>>> + nhoff += 4;
>>> + }
>>> + if (hdr->flags & GRE_SEQ)
>>> + nhoff += 4;
>>> + if (proto == htons(ETH_P_TEB)) {
>>> + const struct ethhdr *eth;
>>> + struct ethhdr _eth;
>>> +
>>> + eth = __skb_header_pointer(skb, nhoff,
>>> + sizeof(_eth),
>>> + data, hlen, &_eth);
>>> + if (!eth)
>>> + goto out_bad;
>>> + proto = eth->h_proto;
>>> + nhoff += sizeof(*eth);
>>> +
>>> + /* Cap headers that we access via pointers at the
>>> + * end of the Ethernet header as our maximum alignment
>>> + * at that point is only 2 bytes.
>>> + */
>>> + if (NET_IP_ALIGN)
>>> + hlen = nhoff;
>>> + }
>>> +
>>> + key_control->flags |= FLOW_DIS_ENCAPSULATION;
>>> + if (flags & FLOW_DISSECTOR_F_STOP_AT_ENCAP)
>>> + goto out_good;
>>>
>>> - if (dissector_uses_key(flow_dissector,
>>> - FLOW_DISSECTOR_KEY_GRE_KEYID)) {
>>> - key_keyid = skb_flow_dissector_target(flow_dissector,
>>> - FLOW_DISSECTOR_KEY_GRE_KEYID,
>>> - target_container);
>>> - key_keyid->keyid = *keyid;
>>> + goto again;
>>> }
>>> - nhoff += 4;
>>> - }
>>> - if (hdr->flags & GRE_SEQ)
>>> - nhoff += 4;
>>> - if (proto == htons(ETH_P_TEB)) {
>>> - const struct ethhdr *eth;
>>> - struct ethhdr _eth;
>>> -
>>> - eth = __skb_header_pointer(skb, nhoff,
>>> - sizeof(_eth),
>>> - data, hlen, &_eth);
>>> - if (!eth)
>>> - goto out_bad;
>>> - proto = eth->h_proto;
>>> - nhoff += sizeof(*eth);
>>> -
>>> - /* Cap headers that we access via pointers at the
>>> - * end of the Ethernet header as our maximum alignment
>>> - * at that point is only 2 bytes.
>>> - */
>>> - if (NET_IP_ALIGN)
>>> - hlen = nhoff;
>>> }
>>> -
>>> - key_control->flags |= FLOW_DIS_ENCAPSULATION;
>>> - if (flags & FLOW_DISSECTOR_F_STOP_AT_ENCAP)
>>> - goto out_good;
>>> -
>>> - goto again;
>>> + break;
>>> }
>>> case NEXTHDR_HOP:
>>> case NEXTHDR_ROUTING:
>>> --
>>> 1.9.1
>>>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash
2016-08-03 1:59 ` Tom Herbert
@ 2016-08-03 2:17 ` Feng Gao
2016-08-03 4:01 ` Tom Herbert
0 siblings, 1 reply; 18+ messages in thread
From: Feng Gao @ 2016-08-03 2:17 UTC (permalink / raw)
To: Tom Herbert
Cc: Gao Feng, David S. Miller, Philp Prindeville, Stephen Hemminger,
Pravin B Shelar, Alex Duyck, Linux Kernel Network Developers
Thanks Tom's detail explanation.
It is my misinterpret because my mother language is not English.
Thanks again:))
On Wed, Aug 3, 2016 at 9:59 AM, Tom Herbert <tom@herbertland.com> wrote:
> On Tue, Aug 2, 2016 at 5:28 PM, Feng Gao <gfree.wind@gmail.com> wrote:
>> Hi Tom.
>>
>> I quote some description in RFC 2637.
>>
>> /*********************************************************************/
>> C
>> (Bit 0) Checksum Present. Set to zero (0).
>>
> Note that the requirement is "*Set* to zero"; it is not must check to
> be zero on reception. Per the robustness principle (part about be
> liberal in what you receive) it is allowable to process the flag if
> the bit is set set. In practice this is nice to have because we know
> that overloading fields in GRE (seq, csum in particular) with private
> data has been done before. Consolidating makes for cleaner code, and
> if someone is really violating the protocol the worst thing that would
> happen it that they would just get suboptimal RPS for those packets.
>
>> R
>> (Bit 1) Routing Present. Set to zero (0).
>>
>> K
>> (Bit 2) Key Present. Set to one (1).
>> S
>> (Bit 3) Sequence Number Present. Set to one (1) if a payload
>> (data) packet is present. Set to zero (0) if payload is not
>> present (GRE packet is an Acknowledgment only).
>>
>> s
>> (Bit 4) Strict source route present. Set to zero (0).
>>
>> Recur
>> (Bits 5-7) Recursion control. Set to zero (0).
>>
>> A
>> (Bit 8) Acknowledgment sequence number present. Set to one (1) if
>> packet contains Acknowledgment Number to be used for acknowledging
>> previously transmitted data.
>>
>> Flags
>> (Bits 9-12) Must be set to zero (0).
>>
>> Ver
>> (Bits 13-15) Must contain 1 (enhanced GRE).
>>
>> Protocol Type
>> Set to hex 880B [8].
>>
>> Key
>> Use of the Key field is up to the implementation. PPTP uses it as
>> follows:
>> Payload Length
>> (High 2 octets of Key) Size of the payload, not including
>> the GRE header
>>
>> Call ID
>> (Low 2 octets) Contains the Peer's Call ID for the session
>> to which this packet belongs.
>> /*********************************************************************/
>>
>> Just a reminder, the version description is "contain 1", not "set 1".
>>
> I think your misinterpreting that (at least I hope :-) ). "Contains 1"
> should be that the contents of the Ver field is the value of "1".
> Otherwise, the RFC has appropriated half of the eight possible
> versions to be enhanced GRE.
>
> Tom
>
>> Best Regards
>> Feng
>>
>> On Wed, Aug 3, 2016 at 4:35 AM, Tom Herbert <tom@herbertland.com> wrote:
>>> On Thu, Jul 28, 2016 at 12:14 AM, <fgao@ikuai8.com> wrote:
>>>> From: Gao Feng <fgao@ikuai8.com>
>>>>
>>>> The PPTP is encapsulated by GRE header with that GRE_VERSION bits
>>>> must contain one. But current GRE RPS needs the GRE_VERSION must be
>>>> zero. So RPS does not work for PPTP traffic.
>>>>
>>>> In my test environment, there are four MIPS cores, and all traffic
>>>> are passed through by PPTP. As a result, only one core is 100% busy
>>>> while other three cores are very idle. After this patch, the usage
>>>> of four cores are balanced well.
>>>>
>>>> Signed-off-by: Gao Feng <fgao@ikuai8.com>
>>>> ---
>>>> v2: Update according to Tom and Philp's advice.
>>>> 1) Consolidate the codes with GRE version 0 path;
>>>> 2) Use PPP_PROTOCOL to get ppp protol;
>>>> 3) Set the FLOW_DIS_ENCAPSULATION flag;
>>>> v1: Initial patch
>>>>
>>>> include/uapi/linux/if_tunnel.h | 5 +-
>>>> net/core/flow_dissector.c | 146 ++++++++++++++++++++++++++---------------
>>>> 2 files changed, 97 insertions(+), 54 deletions(-)
>>>>
>>>> diff --git a/include/uapi/linux/if_tunnel.h b/include/uapi/linux/if_tunnel.h
>>>> index 1046f55..dda4e4b 100644
>>>> --- a/include/uapi/linux/if_tunnel.h
>>>> +++ b/include/uapi/linux/if_tunnel.h
>>>> @@ -24,9 +24,12 @@
>>>> #define GRE_SEQ __cpu_to_be16(0x1000)
>>>> #define GRE_STRICT __cpu_to_be16(0x0800)
>>>> #define GRE_REC __cpu_to_be16(0x0700)
>>>> -#define GRE_FLAGS __cpu_to_be16(0x00F8)
>>>> +#define GRE_ACK __cpu_to_be16(0x0080)
>>>> +#define GRE_FLAGS __cpu_to_be16(0x0078)
>>>> #define GRE_VERSION __cpu_to_be16(0x0007)
>>>>
>>>> +#define GRE_PROTO_PPP __cpu_to_be16(0x880b)
>>>> +
>>>> struct ip_tunnel_parm {
>>>> char name[IFNAMSIZ];
>>>> int link;
>>>> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>>>> index 61ad43f..33e957b 100644
>>>> --- a/net/core/flow_dissector.c
>>>> +++ b/net/core/flow_dissector.c
>>>> @@ -346,63 +346,103 @@ ip_proto_again:
>>>> hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data, hlen, &_hdr);
>>>> if (!hdr)
>>>> goto out_bad;
>>>> - /*
>>>> - * Only look inside GRE if version zero and no
>>>> - * routing
>>>> - */
>>>> - if (hdr->flags & (GRE_VERSION | GRE_ROUTING))
>>>> - break;
>>>> -
>>>> - proto = hdr->proto;
>>>> - nhoff += 4;
>>>> - if (hdr->flags & GRE_CSUM)
>>>> - nhoff += 4;
>>>> - if (hdr->flags & GRE_KEY) {
>>>> - const __be32 *keyid;
>>>> - __be32 _keyid;
>>>> -
>>>> - keyid = __skb_header_pointer(skb, nhoff, sizeof(_keyid),
>>>> - data, hlen, &_keyid);
>>>>
>>>> - if (!keyid)
>>>> - goto out_bad;
>>>> + /* Only look inside GRE without routing */
>>>> + if (!(hdr->flags & GRE_ROUTING)) {
>>>> + proto = hdr->proto;
>>>> +
>>>> + if (hdr->flags & GRE_VERSION) {
>>>
>>> This matches any non-zero GRE version (1-7). Is that really what you want?
>>>
>>>> + /* It should be the PPTP in GRE */
>>>> + u8 _ppp_hdr[PPP_HDRLEN];
>>>> + u8 *ppp_hdr;
>>>> + int offset = 0;
>>>> +
>>>> + /* Check the flags according to RFC 2637*/
>>>> + if (!(proto == GRE_PROTO_PPP && (hdr->flags & GRE_KEY) &&
>>>> + !(hdr->flags & (GRE_CSUM | GRE_STRICT | GRE_REC | GRE_FLAGS)))) {
>>>> + break;
>>>> + }
>>>> +
>>> I might be missing something, but I only see two material differences
>>> between v0 and v1 of GRE. First is that keyid in v1 has specific use
>>> that is not appropriate to use as entropy. Second is how the payload
>>> is parsed. All the flag processing is common so it stills seems like
>>> this should be consolidated.
>>>
>>>> + /* Skip GRE header */
>>>> + offset += 4;
>>>> + /* Skip payload length and call id */
>>>> + offset += 4;
>>>> +
>>>> + if (hdr->flags & GRE_SEQ)
>>>> + offset += 4;
>>>> +
>>>> + if (hdr->flags & GRE_ACK)
>>>> + offset += 4;
>>>> +
>>>> + ppp_hdr = skb_header_pointer(skb, nhoff + offset, sizeof(_ppp_hdr), _ppp_hdr);
>>>> + if (!ppp_hdr)
>>>> + goto out_bad;
>>>> + proto = PPP_PROTOCOL(ppp_hdr);
>>>> + if (proto == PPP_IP) {
>>>> + nhoff += (PPP_HDRLEN + offset);
>>>> + proto = htons(ETH_P_IP);
>>>> + key_control->flags |= FLOW_DIS_ENCAPSULATION;
>>>> + goto again;
>>>> + } else if (proto == PPP_IPV6) {
>>>> + nhoff += (PPP_HDRLEN + offset);
>>>> + proto = htons(ETH_P_IPV6);
>>>> + key_control->flags |= FLOW_DIS_ENCAPSULATION;
>>>> + goto again;
>>>> + }
>>>> + } else {
>>>> + /* Original GRE */
>>>> + nhoff += 4;
>>>> + if (hdr->flags & GRE_CSUM)
>>>> + nhoff += 4;
>>>> + if (hdr->flags & GRE_KEY) {
>>>> + const __be32 *keyid;
>>>> + __be32 _keyid;
>>>> +
>>>> + keyid = __skb_header_pointer(skb, nhoff, sizeof(_keyid),
>>>> + data, hlen, &_keyid);
>>>> +
>>>> + if (!keyid)
>>>> + goto out_bad;
>>>> +
>>>> + if (dissector_uses_key(flow_dissector,
>>>> + FLOW_DISSECTOR_KEY_GRE_KEYID)) {
>>>> + key_keyid = skb_flow_dissector_target(flow_dissector,
>>>> + FLOW_DISSECTOR_KEY_GRE_KEYID,
>>>> + target_container);
>>>> + key_keyid->keyid = *keyid;
>>>> + }
>>>> + nhoff += 4;
>>>> + }
>>>> + if (hdr->flags & GRE_SEQ)
>>>> + nhoff += 4;
>>>> + if (proto == htons(ETH_P_TEB)) {
>>>> + const struct ethhdr *eth;
>>>> + struct ethhdr _eth;
>>>> +
>>>> + eth = __skb_header_pointer(skb, nhoff,
>>>> + sizeof(_eth),
>>>> + data, hlen, &_eth);
>>>> + if (!eth)
>>>> + goto out_bad;
>>>> + proto = eth->h_proto;
>>>> + nhoff += sizeof(*eth);
>>>> +
>>>> + /* Cap headers that we access via pointers at the
>>>> + * end of the Ethernet header as our maximum alignment
>>>> + * at that point is only 2 bytes.
>>>> + */
>>>> + if (NET_IP_ALIGN)
>>>> + hlen = nhoff;
>>>> + }
>>>> +
>>>> + key_control->flags |= FLOW_DIS_ENCAPSULATION;
>>>> + if (flags & FLOW_DISSECTOR_F_STOP_AT_ENCAP)
>>>> + goto out_good;
>>>>
>>>> - if (dissector_uses_key(flow_dissector,
>>>> - FLOW_DISSECTOR_KEY_GRE_KEYID)) {
>>>> - key_keyid = skb_flow_dissector_target(flow_dissector,
>>>> - FLOW_DISSECTOR_KEY_GRE_KEYID,
>>>> - target_container);
>>>> - key_keyid->keyid = *keyid;
>>>> + goto again;
>>>> }
>>>> - nhoff += 4;
>>>> - }
>>>> - if (hdr->flags & GRE_SEQ)
>>>> - nhoff += 4;
>>>> - if (proto == htons(ETH_P_TEB)) {
>>>> - const struct ethhdr *eth;
>>>> - struct ethhdr _eth;
>>>> -
>>>> - eth = __skb_header_pointer(skb, nhoff,
>>>> - sizeof(_eth),
>>>> - data, hlen, &_eth);
>>>> - if (!eth)
>>>> - goto out_bad;
>>>> - proto = eth->h_proto;
>>>> - nhoff += sizeof(*eth);
>>>> -
>>>> - /* Cap headers that we access via pointers at the
>>>> - * end of the Ethernet header as our maximum alignment
>>>> - * at that point is only 2 bytes.
>>>> - */
>>>> - if (NET_IP_ALIGN)
>>>> - hlen = nhoff;
>>>> }
>>>> -
>>>> - key_control->flags |= FLOW_DIS_ENCAPSULATION;
>>>> - if (flags & FLOW_DISSECTOR_F_STOP_AT_ENCAP)
>>>> - goto out_good;
>>>> -
>>>> - goto again;
>>>> + break;
>>>> }
>>>> case NEXTHDR_HOP:
>>>> case NEXTHDR_ROUTING:
>>>> --
>>>> 1.9.1
>>>>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash
2016-08-03 2:17 ` Feng Gao
@ 2016-08-03 4:01 ` Tom Herbert
2016-08-03 15:02 ` Feng Gao
0 siblings, 1 reply; 18+ messages in thread
From: Tom Herbert @ 2016-08-03 4:01 UTC (permalink / raw)
To: Feng Gao
Cc: Gao Feng, David S. Miller, Philp Prindeville, Stephen Hemminger,
Pravin B Shelar, Alex Duyck, Linux Kernel Network Developers
On Tue, Aug 2, 2016 at 7:17 PM, Feng Gao <gfree.wind@gmail.com> wrote:
> Thanks Tom's detail explanation.
> It is my misinterpret because my mother language is not English.
>
No, the RFC is somewhat poorly worded with respect to setting the
version number. The fact that flag bit must be set to zero on
transmit, but can be processed on received is permitted by
"legalistic" interpretation of an IETF RFC :-)
Tom
> Thanks again:))
>
> On Wed, Aug 3, 2016 at 9:59 AM, Tom Herbert <tom@herbertland.com> wrote:
>> On Tue, Aug 2, 2016 at 5:28 PM, Feng Gao <gfree.wind@gmail.com> wrote:
>>> Hi Tom.
>>>
>>> I quote some description in RFC 2637.
>>>
>>> /*********************************************************************/
>>> C
>>> (Bit 0) Checksum Present. Set to zero (0).
>>>
>> Note that the requirement is "*Set* to zero"; it is not must check to
>> be zero on reception. Per the robustness principle (part about be
>> liberal in what you receive) it is allowable to process the flag if
>> the bit is set set. In practice this is nice to have because we know
>> that overloading fields in GRE (seq, csum in particular) with private
>> data has been done before. Consolidating makes for cleaner code, and
>> if someone is really violating the protocol the worst thing that would
>> happen it that they would just get suboptimal RPS for those packets.
>>
>>> R
>>> (Bit 1) Routing Present. Set to zero (0).
>>>
>>> K
>>> (Bit 2) Key Present. Set to one (1).
>>> S
>>> (Bit 3) Sequence Number Present. Set to one (1) if a payload
>>> (data) packet is present. Set to zero (0) if payload is not
>>> present (GRE packet is an Acknowledgment only).
>>>
>>> s
>>> (Bit 4) Strict source route present. Set to zero (0).
>>>
>>> Recur
>>> (Bits 5-7) Recursion control. Set to zero (0).
>>>
>>> A
>>> (Bit 8) Acknowledgment sequence number present. Set to one (1) if
>>> packet contains Acknowledgment Number to be used for acknowledging
>>> previously transmitted data.
>>>
>>> Flags
>>> (Bits 9-12) Must be set to zero (0).
>>>
>>> Ver
>>> (Bits 13-15) Must contain 1 (enhanced GRE).
>>>
>>> Protocol Type
>>> Set to hex 880B [8].
>>>
>>> Key
>>> Use of the Key field is up to the implementation. PPTP uses it as
>>> follows:
>>> Payload Length
>>> (High 2 octets of Key) Size of the payload, not including
>>> the GRE header
>>>
>>> Call ID
>>> (Low 2 octets) Contains the Peer's Call ID for the session
>>> to which this packet belongs.
>>> /*********************************************************************/
>>>
>>> Just a reminder, the version description is "contain 1", not "set 1".
>>>
>> I think your misinterpreting that (at least I hope :-) ). "Contains 1"
>> should be that the contents of the Ver field is the value of "1".
>> Otherwise, the RFC has appropriated half of the eight possible
>> versions to be enhanced GRE.
>>
>> Tom
>>
>>> Best Regards
>>> Feng
>>>
>>> On Wed, Aug 3, 2016 at 4:35 AM, Tom Herbert <tom@herbertland.com> wrote:
>>>> On Thu, Jul 28, 2016 at 12:14 AM, <fgao@ikuai8.com> wrote:
>>>>> From: Gao Feng <fgao@ikuai8.com>
>>>>>
>>>>> The PPTP is encapsulated by GRE header with that GRE_VERSION bits
>>>>> must contain one. But current GRE RPS needs the GRE_VERSION must be
>>>>> zero. So RPS does not work for PPTP traffic.
>>>>>
>>>>> In my test environment, there are four MIPS cores, and all traffic
>>>>> are passed through by PPTP. As a result, only one core is 100% busy
>>>>> while other three cores are very idle. After this patch, the usage
>>>>> of four cores are balanced well.
>>>>>
>>>>> Signed-off-by: Gao Feng <fgao@ikuai8.com>
>>>>> ---
>>>>> v2: Update according to Tom and Philp's advice.
>>>>> 1) Consolidate the codes with GRE version 0 path;
>>>>> 2) Use PPP_PROTOCOL to get ppp protol;
>>>>> 3) Set the FLOW_DIS_ENCAPSULATION flag;
>>>>> v1: Initial patch
>>>>>
>>>>> include/uapi/linux/if_tunnel.h | 5 +-
>>>>> net/core/flow_dissector.c | 146 ++++++++++++++++++++++++++---------------
>>>>> 2 files changed, 97 insertions(+), 54 deletions(-)
>>>>>
>>>>> diff --git a/include/uapi/linux/if_tunnel.h b/include/uapi/linux/if_tunnel.h
>>>>> index 1046f55..dda4e4b 100644
>>>>> --- a/include/uapi/linux/if_tunnel.h
>>>>> +++ b/include/uapi/linux/if_tunnel.h
>>>>> @@ -24,9 +24,12 @@
>>>>> #define GRE_SEQ __cpu_to_be16(0x1000)
>>>>> #define GRE_STRICT __cpu_to_be16(0x0800)
>>>>> #define GRE_REC __cpu_to_be16(0x0700)
>>>>> -#define GRE_FLAGS __cpu_to_be16(0x00F8)
>>>>> +#define GRE_ACK __cpu_to_be16(0x0080)
>>>>> +#define GRE_FLAGS __cpu_to_be16(0x0078)
>>>>> #define GRE_VERSION __cpu_to_be16(0x0007)
>>>>>
>>>>> +#define GRE_PROTO_PPP __cpu_to_be16(0x880b)
>>>>> +
>>>>> struct ip_tunnel_parm {
>>>>> char name[IFNAMSIZ];
>>>>> int link;
>>>>> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>>>>> index 61ad43f..33e957b 100644
>>>>> --- a/net/core/flow_dissector.c
>>>>> +++ b/net/core/flow_dissector.c
>>>>> @@ -346,63 +346,103 @@ ip_proto_again:
>>>>> hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data, hlen, &_hdr);
>>>>> if (!hdr)
>>>>> goto out_bad;
>>>>> - /*
>>>>> - * Only look inside GRE if version zero and no
>>>>> - * routing
>>>>> - */
>>>>> - if (hdr->flags & (GRE_VERSION | GRE_ROUTING))
>>>>> - break;
>>>>> -
>>>>> - proto = hdr->proto;
>>>>> - nhoff += 4;
>>>>> - if (hdr->flags & GRE_CSUM)
>>>>> - nhoff += 4;
>>>>> - if (hdr->flags & GRE_KEY) {
>>>>> - const __be32 *keyid;
>>>>> - __be32 _keyid;
>>>>> -
>>>>> - keyid = __skb_header_pointer(skb, nhoff, sizeof(_keyid),
>>>>> - data, hlen, &_keyid);
>>>>>
>>>>> - if (!keyid)
>>>>> - goto out_bad;
>>>>> + /* Only look inside GRE without routing */
>>>>> + if (!(hdr->flags & GRE_ROUTING)) {
>>>>> + proto = hdr->proto;
>>>>> +
>>>>> + if (hdr->flags & GRE_VERSION) {
>>>>
>>>> This matches any non-zero GRE version (1-7). Is that really what you want?
>>>>
>>>>> + /* It should be the PPTP in GRE */
>>>>> + u8 _ppp_hdr[PPP_HDRLEN];
>>>>> + u8 *ppp_hdr;
>>>>> + int offset = 0;
>>>>> +
>>>>> + /* Check the flags according to RFC 2637*/
>>>>> + if (!(proto == GRE_PROTO_PPP && (hdr->flags & GRE_KEY) &&
>>>>> + !(hdr->flags & (GRE_CSUM | GRE_STRICT | GRE_REC | GRE_FLAGS)))) {
>>>>> + break;
>>>>> + }
>>>>> +
>>>> I might be missing something, but I only see two material differences
>>>> between v0 and v1 of GRE. First is that keyid in v1 has specific use
>>>> that is not appropriate to use as entropy. Second is how the payload
>>>> is parsed. All the flag processing is common so it stills seems like
>>>> this should be consolidated.
>>>>
>>>>> + /* Skip GRE header */
>>>>> + offset += 4;
>>>>> + /* Skip payload length and call id */
>>>>> + offset += 4;
>>>>> +
>>>>> + if (hdr->flags & GRE_SEQ)
>>>>> + offset += 4;
>>>>> +
>>>>> + if (hdr->flags & GRE_ACK)
>>>>> + offset += 4;
>>>>> +
>>>>> + ppp_hdr = skb_header_pointer(skb, nhoff + offset, sizeof(_ppp_hdr), _ppp_hdr);
>>>>> + if (!ppp_hdr)
>>>>> + goto out_bad;
>>>>> + proto = PPP_PROTOCOL(ppp_hdr);
>>>>> + if (proto == PPP_IP) {
>>>>> + nhoff += (PPP_HDRLEN + offset);
>>>>> + proto = htons(ETH_P_IP);
>>>>> + key_control->flags |= FLOW_DIS_ENCAPSULATION;
>>>>> + goto again;
>>>>> + } else if (proto == PPP_IPV6) {
>>>>> + nhoff += (PPP_HDRLEN + offset);
>>>>> + proto = htons(ETH_P_IPV6);
>>>>> + key_control->flags |= FLOW_DIS_ENCAPSULATION;
>>>>> + goto again;
>>>>> + }
>>>>> + } else {
>>>>> + /* Original GRE */
>>>>> + nhoff += 4;
>>>>> + if (hdr->flags & GRE_CSUM)
>>>>> + nhoff += 4;
>>>>> + if (hdr->flags & GRE_KEY) {
>>>>> + const __be32 *keyid;
>>>>> + __be32 _keyid;
>>>>> +
>>>>> + keyid = __skb_header_pointer(skb, nhoff, sizeof(_keyid),
>>>>> + data, hlen, &_keyid);
>>>>> +
>>>>> + if (!keyid)
>>>>> + goto out_bad;
>>>>> +
>>>>> + if (dissector_uses_key(flow_dissector,
>>>>> + FLOW_DISSECTOR_KEY_GRE_KEYID)) {
>>>>> + key_keyid = skb_flow_dissector_target(flow_dissector,
>>>>> + FLOW_DISSECTOR_KEY_GRE_KEYID,
>>>>> + target_container);
>>>>> + key_keyid->keyid = *keyid;
>>>>> + }
>>>>> + nhoff += 4;
>>>>> + }
>>>>> + if (hdr->flags & GRE_SEQ)
>>>>> + nhoff += 4;
>>>>> + if (proto == htons(ETH_P_TEB)) {
>>>>> + const struct ethhdr *eth;
>>>>> + struct ethhdr _eth;
>>>>> +
>>>>> + eth = __skb_header_pointer(skb, nhoff,
>>>>> + sizeof(_eth),
>>>>> + data, hlen, &_eth);
>>>>> + if (!eth)
>>>>> + goto out_bad;
>>>>> + proto = eth->h_proto;
>>>>> + nhoff += sizeof(*eth);
>>>>> +
>>>>> + /* Cap headers that we access via pointers at the
>>>>> + * end of the Ethernet header as our maximum alignment
>>>>> + * at that point is only 2 bytes.
>>>>> + */
>>>>> + if (NET_IP_ALIGN)
>>>>> + hlen = nhoff;
>>>>> + }
>>>>> +
>>>>> + key_control->flags |= FLOW_DIS_ENCAPSULATION;
>>>>> + if (flags & FLOW_DISSECTOR_F_STOP_AT_ENCAP)
>>>>> + goto out_good;
>>>>>
>>>>> - if (dissector_uses_key(flow_dissector,
>>>>> - FLOW_DISSECTOR_KEY_GRE_KEYID)) {
>>>>> - key_keyid = skb_flow_dissector_target(flow_dissector,
>>>>> - FLOW_DISSECTOR_KEY_GRE_KEYID,
>>>>> - target_container);
>>>>> - key_keyid->keyid = *keyid;
>>>>> + goto again;
>>>>> }
>>>>> - nhoff += 4;
>>>>> - }
>>>>> - if (hdr->flags & GRE_SEQ)
>>>>> - nhoff += 4;
>>>>> - if (proto == htons(ETH_P_TEB)) {
>>>>> - const struct ethhdr *eth;
>>>>> - struct ethhdr _eth;
>>>>> -
>>>>> - eth = __skb_header_pointer(skb, nhoff,
>>>>> - sizeof(_eth),
>>>>> - data, hlen, &_eth);
>>>>> - if (!eth)
>>>>> - goto out_bad;
>>>>> - proto = eth->h_proto;
>>>>> - nhoff += sizeof(*eth);
>>>>> -
>>>>> - /* Cap headers that we access via pointers at the
>>>>> - * end of the Ethernet header as our maximum alignment
>>>>> - * at that point is only 2 bytes.
>>>>> - */
>>>>> - if (NET_IP_ALIGN)
>>>>> - hlen = nhoff;
>>>>> }
>>>>> -
>>>>> - key_control->flags |= FLOW_DIS_ENCAPSULATION;
>>>>> - if (flags & FLOW_DISSECTOR_F_STOP_AT_ENCAP)
>>>>> - goto out_good;
>>>>> -
>>>>> - goto again;
>>>>> + break;
>>>>> }
>>>>> case NEXTHDR_HOP:
>>>>> case NEXTHDR_ROUTING:
>>>>> --
>>>>> 1.9.1
>>>>>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash
2016-08-03 4:01 ` Tom Herbert
@ 2016-08-03 15:02 ` Feng Gao
0 siblings, 0 replies; 18+ messages in thread
From: Feng Gao @ 2016-08-03 15:02 UTC (permalink / raw)
To: Tom Herbert
Cc: Gao Feng, David S. Miller, Philp Prindeville, Stephen Hemminger,
Pravin B Shelar, Alex Duyck, Linux Kernel Network Developers
Hi Philip & Tom,
I have sent the v3 patch now.
Philip,
I just move the definition of pptp_gre_header into new file
include/net/pptp.h without refactoring the pptp codes now.
I think the refactor should be done in another patch.
Tom,
Now I have consolidate the PPTP codes with GRE codes together.
Thanks both of you :-))
On Wed, Aug 3, 2016 at 12:01 PM, Tom Herbert <tom@herbertland.com> wrote:
> On Tue, Aug 2, 2016 at 7:17 PM, Feng Gao <gfree.wind@gmail.com> wrote:
>> Thanks Tom's detail explanation.
>> It is my misinterpret because my mother language is not English.
>>
> No, the RFC is somewhat poorly worded with respect to setting the
> version number. The fact that flag bit must be set to zero on
> transmit, but can be processed on received is permitted by
> "legalistic" interpretation of an IETF RFC :-)
>
> Tom
>
>> Thanks again:))
>>
>> On Wed, Aug 3, 2016 at 9:59 AM, Tom Herbert <tom@herbertland.com> wrote:
>>> On Tue, Aug 2, 2016 at 5:28 PM, Feng Gao <gfree.wind@gmail.com> wrote:
>>>> Hi Tom.
>>>>
>>>> I quote some description in RFC 2637.
>>>>
>>>> /*********************************************************************/
>>>> C
>>>> (Bit 0) Checksum Present. Set to zero (0).
>>>>
>>> Note that the requirement is "*Set* to zero"; it is not must check to
>>> be zero on reception. Per the robustness principle (part about be
>>> liberal in what you receive) it is allowable to process the flag if
>>> the bit is set set. In practice this is nice to have because we know
>>> that overloading fields in GRE (seq, csum in particular) with private
>>> data has been done before. Consolidating makes for cleaner code, and
>>> if someone is really violating the protocol the worst thing that would
>>> happen it that they would just get suboptimal RPS for those packets.
>>>
>>>> R
>>>> (Bit 1) Routing Present. Set to zero (0).
>>>>
>>>> K
>>>> (Bit 2) Key Present. Set to one (1).
>>>> S
>>>> (Bit 3) Sequence Number Present. Set to one (1) if a payload
>>>> (data) packet is present. Set to zero (0) if payload is not
>>>> present (GRE packet is an Acknowledgment only).
>>>>
>>>> s
>>>> (Bit 4) Strict source route present. Set to zero (0).
>>>>
>>>> Recur
>>>> (Bits 5-7) Recursion control. Set to zero (0).
>>>>
>>>> A
>>>> (Bit 8) Acknowledgment sequence number present. Set to one (1) if
>>>> packet contains Acknowledgment Number to be used for acknowledging
>>>> previously transmitted data.
>>>>
>>>> Flags
>>>> (Bits 9-12) Must be set to zero (0).
>>>>
>>>> Ver
>>>> (Bits 13-15) Must contain 1 (enhanced GRE).
>>>>
>>>> Protocol Type
>>>> Set to hex 880B [8].
>>>>
>>>> Key
>>>> Use of the Key field is up to the implementation. PPTP uses it as
>>>> follows:
>>>> Payload Length
>>>> (High 2 octets of Key) Size of the payload, not including
>>>> the GRE header
>>>>
>>>> Call ID
>>>> (Low 2 octets) Contains the Peer's Call ID for the session
>>>> to which this packet belongs.
>>>> /*********************************************************************/
>>>>
>>>> Just a reminder, the version description is "contain 1", not "set 1".
>>>>
>>> I think your misinterpreting that (at least I hope :-) ). "Contains 1"
>>> should be that the contents of the Ver field is the value of "1".
>>> Otherwise, the RFC has appropriated half of the eight possible
>>> versions to be enhanced GRE.
>>>
>>> Tom
>>>
>>>> Best Regards
>>>> Feng
>>>>
>>>> On Wed, Aug 3, 2016 at 4:35 AM, Tom Herbert <tom@herbertland.com> wrote:
>>>>> On Thu, Jul 28, 2016 at 12:14 AM, <fgao@ikuai8.com> wrote:
>>>>>> From: Gao Feng <fgao@ikuai8.com>
>>>>>>
>>>>>> The PPTP is encapsulated by GRE header with that GRE_VERSION bits
>>>>>> must contain one. But current GRE RPS needs the GRE_VERSION must be
>>>>>> zero. So RPS does not work for PPTP traffic.
>>>>>>
>>>>>> In my test environment, there are four MIPS cores, and all traffic
>>>>>> are passed through by PPTP. As a result, only one core is 100% busy
>>>>>> while other three cores are very idle. After this patch, the usage
>>>>>> of four cores are balanced well.
>>>>>>
>>>>>> Signed-off-by: Gao Feng <fgao@ikuai8.com>
>>>>>> ---
>>>>>> v2: Update according to Tom and Philp's advice.
>>>>>> 1) Consolidate the codes with GRE version 0 path;
>>>>>> 2) Use PPP_PROTOCOL to get ppp protol;
>>>>>> 3) Set the FLOW_DIS_ENCAPSULATION flag;
>>>>>> v1: Initial patch
>>>>>>
>>>>>> include/uapi/linux/if_tunnel.h | 5 +-
>>>>>> net/core/flow_dissector.c | 146 ++++++++++++++++++++++++++---------------
>>>>>> 2 files changed, 97 insertions(+), 54 deletions(-)
>>>>>>
>>>>>> diff --git a/include/uapi/linux/if_tunnel.h b/include/uapi/linux/if_tunnel.h
>>>>>> index 1046f55..dda4e4b 100644
>>>>>> --- a/include/uapi/linux/if_tunnel.h
>>>>>> +++ b/include/uapi/linux/if_tunnel.h
>>>>>> @@ -24,9 +24,12 @@
>>>>>> #define GRE_SEQ __cpu_to_be16(0x1000)
>>>>>> #define GRE_STRICT __cpu_to_be16(0x0800)
>>>>>> #define GRE_REC __cpu_to_be16(0x0700)
>>>>>> -#define GRE_FLAGS __cpu_to_be16(0x00F8)
>>>>>> +#define GRE_ACK __cpu_to_be16(0x0080)
>>>>>> +#define GRE_FLAGS __cpu_to_be16(0x0078)
>>>>>> #define GRE_VERSION __cpu_to_be16(0x0007)
>>>>>>
>>>>>> +#define GRE_PROTO_PPP __cpu_to_be16(0x880b)
>>>>>> +
>>>>>> struct ip_tunnel_parm {
>>>>>> char name[IFNAMSIZ];
>>>>>> int link;
>>>>>> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>>>>>> index 61ad43f..33e957b 100644
>>>>>> --- a/net/core/flow_dissector.c
>>>>>> +++ b/net/core/flow_dissector.c
>>>>>> @@ -346,63 +346,103 @@ ip_proto_again:
>>>>>> hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data, hlen, &_hdr);
>>>>>> if (!hdr)
>>>>>> goto out_bad;
>>>>>> - /*
>>>>>> - * Only look inside GRE if version zero and no
>>>>>> - * routing
>>>>>> - */
>>>>>> - if (hdr->flags & (GRE_VERSION | GRE_ROUTING))
>>>>>> - break;
>>>>>> -
>>>>>> - proto = hdr->proto;
>>>>>> - nhoff += 4;
>>>>>> - if (hdr->flags & GRE_CSUM)
>>>>>> - nhoff += 4;
>>>>>> - if (hdr->flags & GRE_KEY) {
>>>>>> - const __be32 *keyid;
>>>>>> - __be32 _keyid;
>>>>>> -
>>>>>> - keyid = __skb_header_pointer(skb, nhoff, sizeof(_keyid),
>>>>>> - data, hlen, &_keyid);
>>>>>>
>>>>>> - if (!keyid)
>>>>>> - goto out_bad;
>>>>>> + /* Only look inside GRE without routing */
>>>>>> + if (!(hdr->flags & GRE_ROUTING)) {
>>>>>> + proto = hdr->proto;
>>>>>> +
>>>>>> + if (hdr->flags & GRE_VERSION) {
>>>>>
>>>>> This matches any non-zero GRE version (1-7). Is that really what you want?
>>>>>
>>>>>> + /* It should be the PPTP in GRE */
>>>>>> + u8 _ppp_hdr[PPP_HDRLEN];
>>>>>> + u8 *ppp_hdr;
>>>>>> + int offset = 0;
>>>>>> +
>>>>>> + /* Check the flags according to RFC 2637*/
>>>>>> + if (!(proto == GRE_PROTO_PPP && (hdr->flags & GRE_KEY) &&
>>>>>> + !(hdr->flags & (GRE_CSUM | GRE_STRICT | GRE_REC | GRE_FLAGS)))) {
>>>>>> + break;
>>>>>> + }
>>>>>> +
>>>>> I might be missing something, but I only see two material differences
>>>>> between v0 and v1 of GRE. First is that keyid in v1 has specific use
>>>>> that is not appropriate to use as entropy. Second is how the payload
>>>>> is parsed. All the flag processing is common so it stills seems like
>>>>> this should be consolidated.
>>>>>
>>>>>> + /* Skip GRE header */
>>>>>> + offset += 4;
>>>>>> + /* Skip payload length and call id */
>>>>>> + offset += 4;
>>>>>> +
>>>>>> + if (hdr->flags & GRE_SEQ)
>>>>>> + offset += 4;
>>>>>> +
>>>>>> + if (hdr->flags & GRE_ACK)
>>>>>> + offset += 4;
>>>>>> +
>>>>>> + ppp_hdr = skb_header_pointer(skb, nhoff + offset, sizeof(_ppp_hdr), _ppp_hdr);
>>>>>> + if (!ppp_hdr)
>>>>>> + goto out_bad;
>>>>>> + proto = PPP_PROTOCOL(ppp_hdr);
>>>>>> + if (proto == PPP_IP) {
>>>>>> + nhoff += (PPP_HDRLEN + offset);
>>>>>> + proto = htons(ETH_P_IP);
>>>>>> + key_control->flags |= FLOW_DIS_ENCAPSULATION;
>>>>>> + goto again;
>>>>>> + } else if (proto == PPP_IPV6) {
>>>>>> + nhoff += (PPP_HDRLEN + offset);
>>>>>> + proto = htons(ETH_P_IPV6);
>>>>>> + key_control->flags |= FLOW_DIS_ENCAPSULATION;
>>>>>> + goto again;
>>>>>> + }
>>>>>> + } else {
>>>>>> + /* Original GRE */
>>>>>> + nhoff += 4;
>>>>>> + if (hdr->flags & GRE_CSUM)
>>>>>> + nhoff += 4;
>>>>>> + if (hdr->flags & GRE_KEY) {
>>>>>> + const __be32 *keyid;
>>>>>> + __be32 _keyid;
>>>>>> +
>>>>>> + keyid = __skb_header_pointer(skb, nhoff, sizeof(_keyid),
>>>>>> + data, hlen, &_keyid);
>>>>>> +
>>>>>> + if (!keyid)
>>>>>> + goto out_bad;
>>>>>> +
>>>>>> + if (dissector_uses_key(flow_dissector,
>>>>>> + FLOW_DISSECTOR_KEY_GRE_KEYID)) {
>>>>>> + key_keyid = skb_flow_dissector_target(flow_dissector,
>>>>>> + FLOW_DISSECTOR_KEY_GRE_KEYID,
>>>>>> + target_container);
>>>>>> + key_keyid->keyid = *keyid;
>>>>>> + }
>>>>>> + nhoff += 4;
>>>>>> + }
>>>>>> + if (hdr->flags & GRE_SEQ)
>>>>>> + nhoff += 4;
>>>>>> + if (proto == htons(ETH_P_TEB)) {
>>>>>> + const struct ethhdr *eth;
>>>>>> + struct ethhdr _eth;
>>>>>> +
>>>>>> + eth = __skb_header_pointer(skb, nhoff,
>>>>>> + sizeof(_eth),
>>>>>> + data, hlen, &_eth);
>>>>>> + if (!eth)
>>>>>> + goto out_bad;
>>>>>> + proto = eth->h_proto;
>>>>>> + nhoff += sizeof(*eth);
>>>>>> +
>>>>>> + /* Cap headers that we access via pointers at the
>>>>>> + * end of the Ethernet header as our maximum alignment
>>>>>> + * at that point is only 2 bytes.
>>>>>> + */
>>>>>> + if (NET_IP_ALIGN)
>>>>>> + hlen = nhoff;
>>>>>> + }
>>>>>> +
>>>>>> + key_control->flags |= FLOW_DIS_ENCAPSULATION;
>>>>>> + if (flags & FLOW_DISSECTOR_F_STOP_AT_ENCAP)
>>>>>> + goto out_good;
>>>>>>
>>>>>> - if (dissector_uses_key(flow_dissector,
>>>>>> - FLOW_DISSECTOR_KEY_GRE_KEYID)) {
>>>>>> - key_keyid = skb_flow_dissector_target(flow_dissector,
>>>>>> - FLOW_DISSECTOR_KEY_GRE_KEYID,
>>>>>> - target_container);
>>>>>> - key_keyid->keyid = *keyid;
>>>>>> + goto again;
>>>>>> }
>>>>>> - nhoff += 4;
>>>>>> - }
>>>>>> - if (hdr->flags & GRE_SEQ)
>>>>>> - nhoff += 4;
>>>>>> - if (proto == htons(ETH_P_TEB)) {
>>>>>> - const struct ethhdr *eth;
>>>>>> - struct ethhdr _eth;
>>>>>> -
>>>>>> - eth = __skb_header_pointer(skb, nhoff,
>>>>>> - sizeof(_eth),
>>>>>> - data, hlen, &_eth);
>>>>>> - if (!eth)
>>>>>> - goto out_bad;
>>>>>> - proto = eth->h_proto;
>>>>>> - nhoff += sizeof(*eth);
>>>>>> -
>>>>>> - /* Cap headers that we access via pointers at the
>>>>>> - * end of the Ethernet header as our maximum alignment
>>>>>> - * at that point is only 2 bytes.
>>>>>> - */
>>>>>> - if (NET_IP_ALIGN)
>>>>>> - hlen = nhoff;
>>>>>> }
>>>>>> -
>>>>>> - key_control->flags |= FLOW_DIS_ENCAPSULATION;
>>>>>> - if (flags & FLOW_DISSECTOR_F_STOP_AT_ENCAP)
>>>>>> - goto out_good;
>>>>>> -
>>>>>> - goto again;
>>>>>> + break;
>>>>>> }
>>>>>> case NEXTHDR_HOP:
>>>>>> case NEXTHDR_ROUTING:
>>>>>> --
>>>>>> 1.9.1
>>>>>>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash
@ 2016-07-27 23:42 fgao
2016-07-28 0:04 ` Tom Herbert
0 siblings, 1 reply; 18+ messages in thread
From: fgao @ 2016-07-27 23:42 UTC (permalink / raw)
To: davem, philipp, stephen, pshelar, tom, aduyck, netdev
Cc: gfree.wind, Gao Feng
From: Gao Feng <fgao@ikuai8.com>
The PPTP is encapsulated by GRE header with that GRE_VERSION bits
must contain one. But current GRE RPS needs the GRE_VERSION must be
zero. So RPS does not work for PPTP traffic.
In my test environment, there are four MIPS cores, and all traffic
are passed through by PPTP. As a result, only one core is 100% busy
while other three cores are very idle. After this patch, the usage
of four cores are balanced well.
Signed-off-by: Gao Feng <fgao@ikuai8.com>
---
v1: Initial patch
include/uapi/linux/if_tunnel.h | 5 +-
net/core/flow_dissector.c | 138 ++++++++++++++++++++++++++---------------
2 files changed, 92 insertions(+), 51 deletions(-)
diff --git a/include/uapi/linux/if_tunnel.h b/include/uapi/linux/if_tunnel.h
index 1046f55..dda4e4b 100644
--- a/include/uapi/linux/if_tunnel.h
+++ b/include/uapi/linux/if_tunnel.h
@@ -24,9 +24,12 @@
#define GRE_SEQ __cpu_to_be16(0x1000)
#define GRE_STRICT __cpu_to_be16(0x0800)
#define GRE_REC __cpu_to_be16(0x0700)
-#define GRE_FLAGS __cpu_to_be16(0x00F8)
+#define GRE_ACK __cpu_to_be16(0x0080)
+#define GRE_FLAGS __cpu_to_be16(0x0078)
#define GRE_VERSION __cpu_to_be16(0x0007)
+#define GRE_PROTO_PPP __cpu_to_be16(0x880b)
+
struct ip_tunnel_parm {
char name[IFNAMSIZ];
int link;
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 61ad43f..d95e060 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -346,63 +346,101 @@ ip_proto_again:
hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data, hlen, &_hdr);
if (!hdr)
goto out_bad;
- /*
- * Only look inside GRE if version zero and no
- * routing
- */
- if (hdr->flags & (GRE_VERSION | GRE_ROUTING))
- break;
- proto = hdr->proto;
- nhoff += 4;
- if (hdr->flags & GRE_CSUM)
+ /*
+ * Only look inside GRE if version zero and no
+ * routing
+ */
+ if (!(hdr->flags & (GRE_VERSION | GRE_ROUTING))) {
+ proto = hdr->proto;
nhoff += 4;
- if (hdr->flags & GRE_KEY) {
- const __be32 *keyid;
- __be32 _keyid;
+ if (hdr->flags & GRE_CSUM)
+ nhoff += 4;
+ if (hdr->flags & GRE_KEY) {
+ const __be32 *keyid;
+ __be32 _keyid;
+
+ keyid = __skb_header_pointer(skb, nhoff, sizeof(_keyid),
+ data, hlen, &_keyid);
+
+ if (!keyid)
+ goto out_bad;
+
+ if (dissector_uses_key(flow_dissector,
+ FLOW_DISSECTOR_KEY_GRE_KEYID)) {
+ key_keyid = skb_flow_dissector_target(flow_dissector,
+ FLOW_DISSECTOR_KEY_GRE_KEYID,
+ target_container);
+ key_keyid->keyid = *keyid;
+ }
+ nhoff += 4;
+ }
+ if (hdr->flags & GRE_SEQ)
+ nhoff += 4;
+ if (proto == htons(ETH_P_TEB)) {
+ const struct ethhdr *eth;
+ struct ethhdr _eth;
+
+ eth = __skb_header_pointer(skb, nhoff,
+ sizeof(_eth),
+ data, hlen, &_eth);
+ if (!eth)
+ goto out_bad;
+ proto = eth->h_proto;
+ nhoff += sizeof(*eth);
+
+ /* Cap headers that we access via pointers at the
+ * end of the Ethernet header as our maximum alignment
+ * at that point is only 2 bytes.
+ */
+ if (NET_IP_ALIGN)
+ hlen = nhoff;
+ }
- keyid = __skb_header_pointer(skb, nhoff, sizeof(_keyid),
- data, hlen, &_keyid);
+ key_control->flags |= FLOW_DIS_ENCAPSULATION;
+ if (flags & FLOW_DISSECTOR_F_STOP_AT_ENCAP)
+ goto out_good;
- if (!keyid)
+ goto again;
+ } else if (hdr->proto == GRE_PROTO_PPP && (hdr->flags & GRE_VERSION) &&
+ (hdr->flags & GRE_KEY) &&
+ !(hdr->flags & (GRE_CSUM | GRE_ROUTING | GRE_STRICT | GRE_REC | GRE_FLAGS))) {
+ /* It should be the PPTP in GRE
+ * We have checked the flags according to the
+ * RFC 2637
+ */
+ struct ppp_frame {
+ /* address & control, just ignore it */
+ __be16 ignore;
+ __be16 proto;
+ } *frame, _frame;
+ int offset = 0;
+
+ /* Skip GRE header */
+ offset += 4;
+ /* Skip payload length and call id */
+ offset += 4;
+
+ if (hdr->flags & GRE_SEQ)
+ offset += 4;
+
+ if (hdr->flags & GRE_ACK)
+ offset += 4;
+
+ frame = skb_header_pointer(skb, nhoff + offset, sizeof(_frame), &_frame);
+ if (!frame)
goto out_bad;
-
- if (dissector_uses_key(flow_dissector,
- FLOW_DISSECTOR_KEY_GRE_KEYID)) {
- key_keyid = skb_flow_dissector_target(flow_dissector,
- FLOW_DISSECTOR_KEY_GRE_KEYID,
- target_container);
- key_keyid->keyid = *keyid;
+ if (frame->proto == __constant_htons(PPP_IP)) {
+ nhoff += (sizeof(*frame) + offset);
+ proto = __constant_htons(ETH_P_IP);
+ goto again;
+ } else if (frame->proto == __constant_htons(PPP_IPV6)) {
+ nhoff += (sizeof(*frame) + offset);
+ proto = __constant_htons(ETH_P_IPV6);
+ goto again;
}
- nhoff += 4;
}
- if (hdr->flags & GRE_SEQ)
- nhoff += 4;
- if (proto == htons(ETH_P_TEB)) {
- const struct ethhdr *eth;
- struct ethhdr _eth;
-
- eth = __skb_header_pointer(skb, nhoff,
- sizeof(_eth),
- data, hlen, &_eth);
- if (!eth)
- goto out_bad;
- proto = eth->h_proto;
- nhoff += sizeof(*eth);
-
- /* Cap headers that we access via pointers at the
- * end of the Ethernet header as our maximum alignment
- * at that point is only 2 bytes.
- */
- if (NET_IP_ALIGN)
- hlen = nhoff;
- }
-
- key_control->flags |= FLOW_DIS_ENCAPSULATION;
- if (flags & FLOW_DISSECTOR_F_STOP_AT_ENCAP)
- goto out_good;
-
- goto again;
+ break;
}
case NEXTHDR_HOP:
case NEXTHDR_ROUTING:
--
1.9.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash
2016-07-27 23:42 fgao
@ 2016-07-28 0:04 ` Tom Herbert
2016-07-28 0:24 ` Feng Gao
2016-07-28 0:46 ` Philp Prindeville
0 siblings, 2 replies; 18+ messages in thread
From: Tom Herbert @ 2016-07-28 0:04 UTC (permalink / raw)
To: fgao
Cc: David S. Miller, philipp, Stephen Hemminger, Pravin B Shelar,
Alex Duyck, Linux Kernel Network Developers, gfree.wind
On Wed, Jul 27, 2016 at 4:42 PM, <fgao@ikuai8.com> wrote:
> From: Gao Feng <fgao@ikuai8.com>
>
> The PPTP is encapsulated by GRE header with that GRE_VERSION bits
> must contain one. But current GRE RPS needs the GRE_VERSION must be
> zero. So RPS does not work for PPTP traffic.
>
> In my test environment, there are four MIPS cores, and all traffic
> are passed through by PPTP. As a result, only one core is 100% busy
> while other three cores are very idle. After this patch, the usage
> of four cores are balanced well.
>
> Signed-off-by: Gao Feng <fgao@ikuai8.com>
> ---
> v1: Initial patch
>
> include/uapi/linux/if_tunnel.h | 5 +-
> net/core/flow_dissector.c | 138 ++++++++++++++++++++++++++---------------
> 2 files changed, 92 insertions(+), 51 deletions(-)
>
> diff --git a/include/uapi/linux/if_tunnel.h b/include/uapi/linux/if_tunnel.h
> index 1046f55..dda4e4b 100644
> --- a/include/uapi/linux/if_tunnel.h
> +++ b/include/uapi/linux/if_tunnel.h
> @@ -24,9 +24,12 @@
> #define GRE_SEQ __cpu_to_be16(0x1000)
> #define GRE_STRICT __cpu_to_be16(0x0800)
> #define GRE_REC __cpu_to_be16(0x0700)
> -#define GRE_FLAGS __cpu_to_be16(0x00F8)
> +#define GRE_ACK __cpu_to_be16(0x0080)
> +#define GRE_FLAGS __cpu_to_be16(0x0078)
> #define GRE_VERSION __cpu_to_be16(0x0007)
>
> +#define GRE_PROTO_PPP __cpu_to_be16(0x880b)
> +
> struct ip_tunnel_parm {
> char name[IFNAMSIZ];
> int link;
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 61ad43f..d95e060 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -346,63 +346,101 @@ ip_proto_again:
> hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data, hlen, &_hdr);
> if (!hdr)
> goto out_bad;
> - /*
> - * Only look inside GRE if version zero and no
> - * routing
> - */
> - if (hdr->flags & (GRE_VERSION | GRE_ROUTING))
> - break;
>
> - proto = hdr->proto;
> - nhoff += 4;
> - if (hdr->flags & GRE_CSUM)
> + /*
> + * Only look inside GRE if version zero and no
> + * routing
This comment is no longer true, GRE version 1 is being handled.
> + */
> + if (!(hdr->flags & (GRE_VERSION | GRE_ROUTING))) {
> + proto = hdr->proto;
> nhoff += 4;
> - if (hdr->flags & GRE_KEY) {
> - const __be32 *keyid;
> - __be32 _keyid;
> + if (hdr->flags & GRE_CSUM)
> + nhoff += 4;
> + if (hdr->flags & GRE_KEY) {
> + const __be32 *keyid;
> + __be32 _keyid;
> +
> + keyid = __skb_header_pointer(skb, nhoff, sizeof(_keyid),
> + data, hlen, &_keyid);
> +
> + if (!keyid)
> + goto out_bad;
> +
> + if (dissector_uses_key(flow_dissector,
> + FLOW_DISSECTOR_KEY_GRE_KEYID)) {
> + key_keyid = skb_flow_dissector_target(flow_dissector,
> + FLOW_DISSECTOR_KEY_GRE_KEYID,
> + target_container);
> + key_keyid->keyid = *keyid;
> + }
> + nhoff += 4;
> + }
> + if (hdr->flags & GRE_SEQ)
> + nhoff += 4;
> + if (proto == htons(ETH_P_TEB)) {
> + const struct ethhdr *eth;
> + struct ethhdr _eth;
> +
> + eth = __skb_header_pointer(skb, nhoff,
> + sizeof(_eth),
> + data, hlen, &_eth);
> + if (!eth)
> + goto out_bad;
> + proto = eth->h_proto;
> + nhoff += sizeof(*eth);
> +
> + /* Cap headers that we access via pointers at the
> + * end of the Ethernet header as our maximum alignment
> + * at that point is only 2 bytes.
> + */
> + if (NET_IP_ALIGN)
> + hlen = nhoff;
> + }
>
> - keyid = __skb_header_pointer(skb, nhoff, sizeof(_keyid),
> - data, hlen, &_keyid);
> + key_control->flags |= FLOW_DIS_ENCAPSULATION;
> + if (flags & FLOW_DISSECTOR_F_STOP_AT_ENCAP)
> + goto out_good;
>
> - if (!keyid)
> + goto again;
> + } else if (hdr->proto == GRE_PROTO_PPP && (hdr->flags & GRE_VERSION) &&
> + (hdr->flags & GRE_KEY) &&
> + !(hdr->flags & (GRE_CSUM | GRE_ROUTING | GRE_STRICT | GRE_REC | GRE_FLAGS))) {
> + /* It should be the PPTP in GRE
> + * We have checked the flags according to the
> + * RFC 2637
> + */
> + struct ppp_frame {
> + /* address & control, just ignore it */
> + __be16 ignore;
> + __be16 proto;
> + } *frame, _frame;
Isn't there a common definition of a PPP frame?
> + int offset = 0;
> +
> + /* Skip GRE header */
> + offset += 4;
> + /* Skip payload length and call id */
> + offset += 4;
> +
> + if (hdr->flags & GRE_SEQ)
> + offset += 4;
> +
> + if (hdr->flags & GRE_ACK)
> + offset += 4;
> +
> + frame = skb_header_pointer(skb, nhoff + offset, sizeof(_frame), &_frame);
> + if (!frame)
> goto out_bad;
> -
> - if (dissector_uses_key(flow_dissector,
> - FLOW_DISSECTOR_KEY_GRE_KEYID)) {
> - key_keyid = skb_flow_dissector_target(flow_dissector,
> - FLOW_DISSECTOR_KEY_GRE_KEYID,
> - target_container);
> - key_keyid->keyid = *keyid;
> + if (frame->proto == __constant_htons(PPP_IP)) {
> + nhoff += (sizeof(*frame) + offset);
> + proto = __constant_htons(ETH_P_IP);
> + goto again;
> + } else if (frame->proto == __constant_htons(PPP_IPV6)) {
> + nhoff += (sizeof(*frame) + offset);
> + proto = __constant_htons(ETH_P_IPV6);
> + goto again;
There's a lot of code replication here with the version 0 path. Please
consolidate these. Also, this looks an awful lot like ETH_P_PPP_SES,
can this code leverage that code?
> }
> - nhoff += 4;
> }
> - if (hdr->flags & GRE_SEQ)
> - nhoff += 4;
> - if (proto == htons(ETH_P_TEB)) {
> - const struct ethhdr *eth;
> - struct ethhdr _eth;
> -
> - eth = __skb_header_pointer(skb, nhoff,
> - sizeof(_eth),
> - data, hlen, &_eth);
> - if (!eth)
> - goto out_bad;
> - proto = eth->h_proto;
> - nhoff += sizeof(*eth);
> -
> - /* Cap headers that we access via pointers at the
> - * end of the Ethernet header as our maximum alignment
> - * at that point is only 2 bytes.
> - */
> - if (NET_IP_ALIGN)
> - hlen = nhoff;
> - }
> -
> - key_control->flags |= FLOW_DIS_ENCAPSULATION;
> - if (flags & FLOW_DISSECTOR_F_STOP_AT_ENCAP)
> - goto out_good;
> -
> - goto again;
> + break;
> }
> case NEXTHDR_HOP:
> case NEXTHDR_ROUTING:
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash
2016-07-28 0:04 ` Tom Herbert
@ 2016-07-28 0:24 ` Feng Gao
2016-07-28 0:35 ` Feng Gao
2016-07-28 0:46 ` Philp Prindeville
1 sibling, 1 reply; 18+ messages in thread
From: Feng Gao @ 2016-07-28 0:24 UTC (permalink / raw)
To: Tom Herbert
Cc: Gao Feng, David S. Miller, philipp, Stephen Hemminger,
Pravin B Shelar, Alex Duyck, Linux Kernel Network Developers
Thanks Tom, I append some inline comments.
BTW, forgive me reply the email by gmail because the original mail
server doesn't support kernel email format.
I will send the update later.
Best Regards
Feng
On Thu, Jul 28, 2016 at 8:04 AM, Tom Herbert <tom@herbertland.com> wrote:
>
> On Wed, Jul 27, 2016 at 4:42 PM, <fgao@ikuai8.com> wrote:
> > From: Gao Feng <fgao@ikuai8.com>
> >
> > The PPTP is encapsulated by GRE header with that GRE_VERSION bits
> > must contain one. But current GRE RPS needs the GRE_VERSION must be
> > zero. So RPS does not work for PPTP traffic.
> >
> > In my test environment, there are four MIPS cores, and all traffic
> > are passed through by PPTP. As a result, only one core is 100% busy
> > while other three cores are very idle. After this patch, the usage
> > of four cores are balanced well.
> >
> > Signed-off-by: Gao Feng <fgao@ikuai8.com>
> > ---
> > v1: Initial patch
> >
> > include/uapi/linux/if_tunnel.h | 5 +-
> > net/core/flow_dissector.c | 138 ++++++++++++++++++++++++++---------------
> > 2 files changed, 92 insertions(+), 51 deletions(-)
> >
> > diff --git a/include/uapi/linux/if_tunnel.h b/include/uapi/linux/if_tunnel.h
> > index 1046f55..dda4e4b 100644
> > --- a/include/uapi/linux/if_tunnel.h
> > +++ b/include/uapi/linux/if_tunnel.h
> > @@ -24,9 +24,12 @@
> > #define GRE_SEQ __cpu_to_be16(0x1000)
> > #define GRE_STRICT __cpu_to_be16(0x0800)
> > #define GRE_REC __cpu_to_be16(0x0700)
> > -#define GRE_FLAGS __cpu_to_be16(0x00F8)
> > +#define GRE_ACK __cpu_to_be16(0x0080)
> > +#define GRE_FLAGS __cpu_to_be16(0x0078)
> > #define GRE_VERSION __cpu_to_be16(0x0007)
> >
> > +#define GRE_PROTO_PPP __cpu_to_be16(0x880b)
> > +
> > struct ip_tunnel_parm {
> > char name[IFNAMSIZ];
> > int link;
> > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> > index 61ad43f..d95e060 100644
> > --- a/net/core/flow_dissector.c
> > +++ b/net/core/flow_dissector.c
> > @@ -346,63 +346,101 @@ ip_proto_again:
> > hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data, hlen, &_hdr);
> > if (!hdr)
> > goto out_bad;
> > - /*
> > - * Only look inside GRE if version zero and no
> > - * routing
> > - */
> > - if (hdr->flags & (GRE_VERSION | GRE_ROUTING))
> > - break;
> >
> > - proto = hdr->proto;
> > - nhoff += 4;
> > - if (hdr->flags & GRE_CSUM)
> > + /*
> > + * Only look inside GRE if version zero and no
> > + * routing
>
> This comment is no longer true, GRE version 1 is being handled.
Ok, I get it. Thanks.
>
> > + */
> > + if (!(hdr->flags & (GRE_VERSION | GRE_ROUTING))) {
> > + proto = hdr->proto;
> > nhoff += 4;
> > - if (hdr->flags & GRE_KEY) {
> > - const __be32 *keyid;
> > - __be32 _keyid;
> > + if (hdr->flags & GRE_CSUM)
> > + nhoff += 4;
> > + if (hdr->flags & GRE_KEY) {
> > + const __be32 *keyid;
> > + __be32 _keyid;
> > +
> > + keyid = __skb_header_pointer(skb, nhoff, sizeof(_keyid),
> > + data, hlen, &_keyid);
> > +
> > + if (!keyid)
> > + goto out_bad;
> > +
> > + if (dissector_uses_key(flow_dissector,
> > + FLOW_DISSECTOR_KEY_GRE_KEYID)) {
> > + key_keyid = skb_flow_dissector_target(flow_dissector,
> > + FLOW_DISSECTOR_KEY_GRE_KEYID,
> > + target_container);
> > + key_keyid->keyid = *keyid;
> > + }
> > + nhoff += 4;
> > + }
> > + if (hdr->flags & GRE_SEQ)
> > + nhoff += 4;
> > + if (proto == htons(ETH_P_TEB)) {
> > + const struct ethhdr *eth;
> > + struct ethhdr _eth;
> > +
> > + eth = __skb_header_pointer(skb, nhoff,
> > + sizeof(_eth),
> > + data, hlen, &_eth);
> > + if (!eth)
> > + goto out_bad;
> > + proto = eth->h_proto;
> > + nhoff += sizeof(*eth);
> > +
> > + /* Cap headers that we access via pointers at the
> > + * end of the Ethernet header as our maximum alignment
> > + * at that point is only 2 bytes.
> > + */
> > + if (NET_IP_ALIGN)
> > + hlen = nhoff;
> > + }
> >
> > - keyid = __skb_header_pointer(skb, nhoff, sizeof(_keyid),
> > - data, hlen, &_keyid);
> > + key_control->flags |= FLOW_DIS_ENCAPSULATION;
> > + if (flags & FLOW_DISSECTOR_F_STOP_AT_ENCAP)
> > + goto out_good;
> >
> > - if (!keyid)
> > + goto again;
> > + } else if (hdr->proto == GRE_PROTO_PPP && (hdr->flags & GRE_VERSION) &&
> > + (hdr->flags & GRE_KEY) &&
> > + !(hdr->flags & (GRE_CSUM | GRE_ROUTING | GRE_STRICT | GRE_REC | GRE_FLAGS))) {
> > + /* It should be the PPTP in GRE
> > + * We have checked the flags according to the
> > + * RFC 2637
> > + */
> > + struct ppp_frame {
> > + /* address & control, just ignore it */
> > + __be16 ignore;
> > + __be16 proto;
> > + } *frame, _frame;
>
> Isn't there a common definition of a PPP frame?
Sorry, I fail to find the common definition of PPP frame indeed by
cscope search.
>
> > + int offset = 0;
> > +
> > + /* Skip GRE header */
> > + offset += 4;
> > + /* Skip payload length and call id */
> > + offset += 4;
> > +
> > + if (hdr->flags & GRE_SEQ)
> > + offset += 4;
> > +
> > + if (hdr->flags & GRE_ACK)
> > + offset += 4;
> > +
> > + frame = skb_header_pointer(skb, nhoff + offset, sizeof(_frame), &_frame);
> > + if (!frame)
> > goto out_bad;
> > -
> > - if (dissector_uses_key(flow_dissector,
> > - FLOW_DISSECTOR_KEY_GRE_KEYID)) {
> > - key_keyid = skb_flow_dissector_target(flow_dissector,
> > - FLOW_DISSECTOR_KEY_GRE_KEYID,
> > - target_container);
> > - key_keyid->keyid = *keyid;
> > + if (frame->proto == __constant_htons(PPP_IP)) {
> > + nhoff += (sizeof(*frame) + offset);
> > + proto = __constant_htons(ETH_P_IP);
> > + goto again;
> > + } else if (frame->proto == __constant_htons(PPP_IPV6)) {
> > + nhoff += (sizeof(*frame) + offset);
> > + proto = __constant_htons(ETH_P_IPV6);
> > + goto again;
>
> There's a lot of code replication here with the version 0 path. Please
> consolidate these. Also, this looks an awful lot like ETH_P_PPP_SES,
> can this code leverage that code?
Ok, I could try to consolidate these codes with the version 0 path.
About the ETH_P_PPP_SES, there are some differences between them.
So I have no idea to leverage that code.
>
> > }
> > - nhoff += 4;
> > }
> > - if (hdr->flags & GRE_SEQ)
> > - nhoff += 4;
> > - if (proto == htons(ETH_P_TEB)) {
> > - const struct ethhdr *eth;
> > - struct ethhdr _eth;
> > -
> > - eth = __skb_header_pointer(skb, nhoff,
> > - sizeof(_eth),
> > - data, hlen, &_eth);
> > - if (!eth)
> > - goto out_bad;
> > - proto = eth->h_proto;
> > - nhoff += sizeof(*eth);
> > -
> > - /* Cap headers that we access via pointers at the
> > - * end of the Ethernet header as our maximum alignment
> > - * at that point is only 2 bytes.
> > - */
> > - if (NET_IP_ALIGN)
> > - hlen = nhoff;
> > - }
> > -
> > - key_control->flags |= FLOW_DIS_ENCAPSULATION;
> > - if (flags & FLOW_DISSECTOR_F_STOP_AT_ENCAP)
> > - goto out_good;
> > -
> > - goto again;
> > + break;
> > }
> > case NEXTHDR_HOP:
> > case NEXTHDR_ROUTING:
> > --
> > 1.9.1
> >
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash
2016-07-28 0:24 ` Feng Gao
@ 2016-07-28 0:35 ` Feng Gao
0 siblings, 0 replies; 18+ messages in thread
From: Feng Gao @ 2016-07-28 0:35 UTC (permalink / raw)
To: Tom Herbert
Cc: Gao Feng, David S. Miller, philipp, Stephen Hemminger,
Pravin B Shelar, Alex Duyck, Linux Kernel Network Developers
Hi Tom,
I think maybe it is not good idea to consolidate these codes with the
version 0 path.
Because I need to check the flags strictly like this "!(hdr->flags &
(GRE_CSUM | GRE_ROUTING | GRE_STRICT | GRE_REC | GRE_FLAGS))".
These flag must be zero with PPTP GRE according to the RFC, but
original GRE only ask the "GRE_VERSION|GRE_ROUTING" must be zero.
And the inner proto structures are different between with the original
GRE and PPTP GRE.
So I think it would be more awkful if consolidate these codes, unless
we don't check the flags strictly.
On Thu, Jul 28, 2016 at 8:24 AM, Feng Gao <gfree.wind@gmail.com> wrote:
> Thanks Tom, I append some inline comments.
>
> BTW, forgive me reply the email by gmail because the original mail
> server doesn't support kernel email format.
> I will send the update later.
>
> Best Regards
> Feng
>
> On Thu, Jul 28, 2016 at 8:04 AM, Tom Herbert <tom@herbertland.com> wrote:
>>
>> On Wed, Jul 27, 2016 at 4:42 PM, <fgao@ikuai8.com> wrote:
>> > From: Gao Feng <fgao@ikuai8.com>
>> >
>> > The PPTP is encapsulated by GRE header with that GRE_VERSION bits
>> > must contain one. But current GRE RPS needs the GRE_VERSION must be
>> > zero. So RPS does not work for PPTP traffic.
>> >
>> > In my test environment, there are four MIPS cores, and all traffic
>> > are passed through by PPTP. As a result, only one core is 100% busy
>> > while other three cores are very idle. After this patch, the usage
>> > of four cores are balanced well.
>> >
>> > Signed-off-by: Gao Feng <fgao@ikuai8.com>
>> > ---
>> > v1: Initial patch
>> >
>> > include/uapi/linux/if_tunnel.h | 5 +-
>> > net/core/flow_dissector.c | 138 ++++++++++++++++++++++++++---------------
>> > 2 files changed, 92 insertions(+), 51 deletions(-)
>> >
>> > diff --git a/include/uapi/linux/if_tunnel.h b/include/uapi/linux/if_tunnel.h
>> > index 1046f55..dda4e4b 100644
>> > --- a/include/uapi/linux/if_tunnel.h
>> > +++ b/include/uapi/linux/if_tunnel.h
>> > @@ -24,9 +24,12 @@
>> > #define GRE_SEQ __cpu_to_be16(0x1000)
>> > #define GRE_STRICT __cpu_to_be16(0x0800)
>> > #define GRE_REC __cpu_to_be16(0x0700)
>> > -#define GRE_FLAGS __cpu_to_be16(0x00F8)
>> > +#define GRE_ACK __cpu_to_be16(0x0080)
>> > +#define GRE_FLAGS __cpu_to_be16(0x0078)
>> > #define GRE_VERSION __cpu_to_be16(0x0007)
>> >
>> > +#define GRE_PROTO_PPP __cpu_to_be16(0x880b)
>> > +
>> > struct ip_tunnel_parm {
>> > char name[IFNAMSIZ];
>> > int link;
>> > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>> > index 61ad43f..d95e060 100644
>> > --- a/net/core/flow_dissector.c
>> > +++ b/net/core/flow_dissector.c
>> > @@ -346,63 +346,101 @@ ip_proto_again:
>> > hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data, hlen, &_hdr);
>> > if (!hdr)
>> > goto out_bad;
>> > - /*
>> > - * Only look inside GRE if version zero and no
>> > - * routing
>> > - */
>> > - if (hdr->flags & (GRE_VERSION | GRE_ROUTING))
>> > - break;
>> >
>> > - proto = hdr->proto;
>> > - nhoff += 4;
>> > - if (hdr->flags & GRE_CSUM)
>> > + /*
>> > + * Only look inside GRE if version zero and no
>> > + * routing
>>
>> This comment is no longer true, GRE version 1 is being handled.
>
> Ok, I get it. Thanks.
>
>>
>> > + */
>> > + if (!(hdr->flags & (GRE_VERSION | GRE_ROUTING))) {
>> > + proto = hdr->proto;
>> > nhoff += 4;
>> > - if (hdr->flags & GRE_KEY) {
>> > - const __be32 *keyid;
>> > - __be32 _keyid;
>> > + if (hdr->flags & GRE_CSUM)
>> > + nhoff += 4;
>> > + if (hdr->flags & GRE_KEY) {
>> > + const __be32 *keyid;
>> > + __be32 _keyid;
>> > +
>> > + keyid = __skb_header_pointer(skb, nhoff, sizeof(_keyid),
>> > + data, hlen, &_keyid);
>> > +
>> > + if (!keyid)
>> > + goto out_bad;
>> > +
>> > + if (dissector_uses_key(flow_dissector,
>> > + FLOW_DISSECTOR_KEY_GRE_KEYID)) {
>> > + key_keyid = skb_flow_dissector_target(flow_dissector,
>> > + FLOW_DISSECTOR_KEY_GRE_KEYID,
>> > + target_container);
>> > + key_keyid->keyid = *keyid;
>> > + }
>> > + nhoff += 4;
>> > + }
>> > + if (hdr->flags & GRE_SEQ)
>> > + nhoff += 4;
>> > + if (proto == htons(ETH_P_TEB)) {
>> > + const struct ethhdr *eth;
>> > + struct ethhdr _eth;
>> > +
>> > + eth = __skb_header_pointer(skb, nhoff,
>> > + sizeof(_eth),
>> > + data, hlen, &_eth);
>> > + if (!eth)
>> > + goto out_bad;
>> > + proto = eth->h_proto;
>> > + nhoff += sizeof(*eth);
>> > +
>> > + /* Cap headers that we access via pointers at the
>> > + * end of the Ethernet header as our maximum alignment
>> > + * at that point is only 2 bytes.
>> > + */
>> > + if (NET_IP_ALIGN)
>> > + hlen = nhoff;
>> > + }
>> >
>> > - keyid = __skb_header_pointer(skb, nhoff, sizeof(_keyid),
>> > - data, hlen, &_keyid);
>> > + key_control->flags |= FLOW_DIS_ENCAPSULATION;
>> > + if (flags & FLOW_DISSECTOR_F_STOP_AT_ENCAP)
>> > + goto out_good;
>> >
>> > - if (!keyid)
>> > + goto again;
>> > + } else if (hdr->proto == GRE_PROTO_PPP && (hdr->flags & GRE_VERSION) &&
>> > + (hdr->flags & GRE_KEY) &&
>> > + !(hdr->flags & (GRE_CSUM | GRE_ROUTING | GRE_STRICT | GRE_REC | GRE_FLAGS))) {
>> > + /* It should be the PPTP in GRE
>> > + * We have checked the flags according to the
>> > + * RFC 2637
>> > + */
>> > + struct ppp_frame {
>> > + /* address & control, just ignore it */
>> > + __be16 ignore;
>> > + __be16 proto;
>> > + } *frame, _frame;
>>
>> Isn't there a common definition of a PPP frame?
>
> Sorry, I fail to find the common definition of PPP frame indeed by
> cscope search.
>
>>
>> > + int offset = 0;
>> > +
>> > + /* Skip GRE header */
>> > + offset += 4;
>> > + /* Skip payload length and call id */
>> > + offset += 4;
>> > +
>> > + if (hdr->flags & GRE_SEQ)
>> > + offset += 4;
>> > +
>> > + if (hdr->flags & GRE_ACK)
>> > + offset += 4;
>> > +
>> > + frame = skb_header_pointer(skb, nhoff + offset, sizeof(_frame), &_frame);
>> > + if (!frame)
>> > goto out_bad;
>> > -
>> > - if (dissector_uses_key(flow_dissector,
>> > - FLOW_DISSECTOR_KEY_GRE_KEYID)) {
>> > - key_keyid = skb_flow_dissector_target(flow_dissector,
>> > - FLOW_DISSECTOR_KEY_GRE_KEYID,
>> > - target_container);
>> > - key_keyid->keyid = *keyid;
>> > + if (frame->proto == __constant_htons(PPP_IP)) {
>> > + nhoff += (sizeof(*frame) + offset);
>> > + proto = __constant_htons(ETH_P_IP);
>> > + goto again;
>> > + } else if (frame->proto == __constant_htons(PPP_IPV6)) {
>> > + nhoff += (sizeof(*frame) + offset);
>> > + proto = __constant_htons(ETH_P_IPV6);
>> > + goto again;
>>
>> There's a lot of code replication here with the version 0 path. Please
>> consolidate these. Also, this looks an awful lot like ETH_P_PPP_SES,
>> can this code leverage that code?
>
> Ok, I could try to consolidate these codes with the version 0 path.
> About the ETH_P_PPP_SES, there are some differences between them.
> So I have no idea to leverage that code.
>
>>
>> > }
>> > - nhoff += 4;
>> > }
>> > - if (hdr->flags & GRE_SEQ)
>> > - nhoff += 4;
>> > - if (proto == htons(ETH_P_TEB)) {
>> > - const struct ethhdr *eth;
>> > - struct ethhdr _eth;
>> > -
>> > - eth = __skb_header_pointer(skb, nhoff,
>> > - sizeof(_eth),
>> > - data, hlen, &_eth);
>> > - if (!eth)
>> > - goto out_bad;
>> > - proto = eth->h_proto;
>> > - nhoff += sizeof(*eth);
>> > -
>> > - /* Cap headers that we access via pointers at the
>> > - * end of the Ethernet header as our maximum alignment
>> > - * at that point is only 2 bytes.
>> > - */
>> > - if (NET_IP_ALIGN)
>> > - hlen = nhoff;
>> > - }
>> > -
>> > - key_control->flags |= FLOW_DIS_ENCAPSULATION;
>> > - if (flags & FLOW_DISSECTOR_F_STOP_AT_ENCAP)
>> > - goto out_good;
>> > -
>> > - goto again;
>> > + break;
>> > }
>> > case NEXTHDR_HOP:
>> > case NEXTHDR_ROUTING:
>> > --
>> > 1.9.1
>> >
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash
2016-07-28 0:04 ` Tom Herbert
2016-07-28 0:24 ` Feng Gao
@ 2016-07-28 0:46 ` Philp Prindeville
2016-08-01 7:36 ` Feng Gao
1 sibling, 1 reply; 18+ messages in thread
From: Philp Prindeville @ 2016-07-28 0:46 UTC (permalink / raw)
To: Tom Herbert, fgao
Cc: David S. Miller, Stephen Hemminger, Pravin B Shelar, Alex Duyck,
Linux Kernel Network Developers, gfree.wind
Inline...
On 07/27/2016 06:04 PM, Tom Herbert wrote:
> On Wed, Jul 27, 2016 at 4:42 PM, <fgao@ikuai8.com> wrote:
>> From: Gao Feng <fgao@ikuai8.com>
>>
>> The PPTP is encapsulated by GRE header with that GRE_VERSION bits
>> must contain one. But current GRE RPS needs the GRE_VERSION must be
>> zero. So RPS does not work for PPTP traffic.
>>
>> In my test environment, there are four MIPS cores, and all traffic
>> are passed through by PPTP. As a result, only one core is 100% busy
>> while other three cores are very idle. After this patch, the usage
>> of four cores are balanced well.
>>
>> Signed-off-by: Gao Feng <fgao@ikuai8.com>
>> ---
>> v1: Initial patch
>>
>> include/uapi/linux/if_tunnel.h | 5 +-
>> net/core/flow_dissector.c | 138 ++++++++++++++++++++++++++---------------
>> 2 files changed, 92 insertions(+), 51 deletions(-)
>>
>> diff --git a/include/uapi/linux/if_tunnel.h b/include/uapi/linux/if_tunnel.h
>> index 1046f55..dda4e4b 100644
>> --- a/include/uapi/linux/if_tunnel.h
>> +++ b/include/uapi/linux/if_tunnel.h
>> @@ -24,9 +24,12 @@
>> #define GRE_SEQ __cpu_to_be16(0x1000)
>> #define GRE_STRICT __cpu_to_be16(0x0800)
>> #define GRE_REC __cpu_to_be16(0x0700)
>> -#define GRE_FLAGS __cpu_to_be16(0x00F8)
>> +#define GRE_ACK __cpu_to_be16(0x0080)
>> +#define GRE_FLAGS __cpu_to_be16(0x0078)
>> #define GRE_VERSION __cpu_to_be16(0x0007)
>>
>> +#define GRE_PROTO_PPP __cpu_to_be16(0x880b)
>> +
>> struct ip_tunnel_parm {
>> char name[IFNAMSIZ];
>> int link;
>> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>> index 61ad43f..d95e060 100644
>> --- a/net/core/flow_dissector.c
>> +++ b/net/core/flow_dissector.c
>> @@ -346,63 +346,101 @@ ip_proto_again:
>> hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data, hlen, &_hdr);
>> if (!hdr)
>> goto out_bad;
>> - /*
>> - * Only look inside GRE if version zero and no
>> - * routing
>> - */
>> - if (hdr->flags & (GRE_VERSION | GRE_ROUTING))
>> - break;
>>
>> - proto = hdr->proto;
>> - nhoff += 4;
>> - if (hdr->flags & GRE_CSUM)
>> + /*
>> + * Only look inside GRE if version zero and no
>> + * routing
> This comment is no longer true, GRE version 1 is being handled.
>
>> + */
>> + if (!(hdr->flags & (GRE_VERSION | GRE_ROUTING))) {
>> + proto = hdr->proto;
>> nhoff += 4;
>> - if (hdr->flags & GRE_KEY) {
>> - const __be32 *keyid;
>> - __be32 _keyid;
>> + if (hdr->flags & GRE_CSUM)
>> + nhoff += 4;
>> + if (hdr->flags & GRE_KEY) {
>> + const __be32 *keyid;
>> + __be32 _keyid;
>> +
>> + keyid = __skb_header_pointer(skb, nhoff, sizeof(_keyid),
>> + data, hlen, &_keyid);
>> +
>> + if (!keyid)
>> + goto out_bad;
>> +
>> + if (dissector_uses_key(flow_dissector,
>> + FLOW_DISSECTOR_KEY_GRE_KEYID)) {
>> + key_keyid = skb_flow_dissector_target(flow_dissector,
>> + FLOW_DISSECTOR_KEY_GRE_KEYID,
>> + target_container);
>> + key_keyid->keyid = *keyid;
>> + }
>> + nhoff += 4;
>> + }
>> + if (hdr->flags & GRE_SEQ)
>> + nhoff += 4;
>> + if (proto == htons(ETH_P_TEB)) {
>> + const struct ethhdr *eth;
>> + struct ethhdr _eth;
>> +
>> + eth = __skb_header_pointer(skb, nhoff,
>> + sizeof(_eth),
>> + data, hlen, &_eth);
>> + if (!eth)
>> + goto out_bad;
>> + proto = eth->h_proto;
>> + nhoff += sizeof(*eth);
>> +
>> + /* Cap headers that we access via pointers at the
>> + * end of the Ethernet header as our maximum alignment
>> + * at that point is only 2 bytes.
>> + */
>> + if (NET_IP_ALIGN)
>> + hlen = nhoff;
>> + }
>>
>> - keyid = __skb_header_pointer(skb, nhoff, sizeof(_keyid),
>> - data, hlen, &_keyid);
>> + key_control->flags |= FLOW_DIS_ENCAPSULATION;
>> + if (flags & FLOW_DISSECTOR_F_STOP_AT_ENCAP)
>> + goto out_good;
>>
>> - if (!keyid)
>> + goto again;
>> + } else if (hdr->proto == GRE_PROTO_PPP && (hdr->flags & GRE_VERSION) &&
>> + (hdr->flags & GRE_KEY) &&
>> + !(hdr->flags & (GRE_CSUM | GRE_ROUTING | GRE_STRICT | GRE_REC | GRE_FLAGS))) {
>> + /* It should be the PPTP in GRE
>> + * We have checked the flags according to the
>> + * RFC 2637
>> + */
>> + struct ppp_frame {
>> + /* address & control, just ignore it */
>> + __be16 ignore;
>> + __be16 proto;
>> + } *frame, _frame;
> Isn't there a common definition of a PPP frame?
If there were, it would be in <net/ppp_defs.h> but it's not. It's sort
of implied the macros PPP_ADDRESS(), PPP_CONTROL(), and PPP_PROTOCOL().
Please rewrite the patch to use these.
-Philip
>> + int offset = 0;
>> +
>> + /* Skip GRE header */
>> + offset += 4;
>> + /* Skip payload length and call id */
>> + offset += 4;
>> +
>> + if (hdr->flags & GRE_SEQ)
>> + offset += 4;
>> +
>> + if (hdr->flags & GRE_ACK)
>> + offset += 4;
>> +
>> + frame = skb_header_pointer(skb, nhoff + offset, sizeof(_frame), &_frame);
>> + if (!frame)
>> goto out_bad;
>> -
>> - if (dissector_uses_key(flow_dissector,
>> - FLOW_DISSECTOR_KEY_GRE_KEYID)) {
>> - key_keyid = skb_flow_dissector_target(flow_dissector,
>> - FLOW_DISSECTOR_KEY_GRE_KEYID,
>> - target_container);
>> - key_keyid->keyid = *keyid;
>> + if (frame->proto == __constant_htons(PPP_IP)) {
>> + nhoff += (sizeof(*frame) + offset);
>> + proto = __constant_htons(ETH_P_IP);
>> + goto again;
>> + } else if (frame->proto == __constant_htons(PPP_IPV6)) {
>> + nhoff += (sizeof(*frame) + offset);
>> + proto = __constant_htons(ETH_P_IPV6);
>> + goto again;
> There's a lot of code replication here with the version 0 path. Please
> consolidate these. Also, this looks an awful lot like ETH_P_PPP_SES,
> can this code leverage that code?
>
>> }
>> - nhoff += 4;
>> }
>> - if (hdr->flags & GRE_SEQ)
>> - nhoff += 4;
>> - if (proto == htons(ETH_P_TEB)) {
>> - const struct ethhdr *eth;
>> - struct ethhdr _eth;
>> -
>> - eth = __skb_header_pointer(skb, nhoff,
>> - sizeof(_eth),
>> - data, hlen, &_eth);
>> - if (!eth)
>> - goto out_bad;
>> - proto = eth->h_proto;
>> - nhoff += sizeof(*eth);
>> -
>> - /* Cap headers that we access via pointers at the
>> - * end of the Ethernet header as our maximum alignment
>> - * at that point is only 2 bytes.
>> - */
>> - if (NET_IP_ALIGN)
>> - hlen = nhoff;
>> - }
>> -
>> - key_control->flags |= FLOW_DIS_ENCAPSULATION;
>> - if (flags & FLOW_DISSECTOR_F_STOP_AT_ENCAP)
>> - goto out_good;
>> -
>> - goto again;
>> + break;
>> }
>> case NEXTHDR_HOP:
>> case NEXTHDR_ROUTING:
>> --
>> 1.9.1
>>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash
2016-07-28 0:46 ` Philp Prindeville
@ 2016-08-01 7:36 ` Feng Gao
0 siblings, 0 replies; 18+ messages in thread
From: Feng Gao @ 2016-08-01 7:36 UTC (permalink / raw)
To: Philp Prindeville
Cc: Tom Herbert, fgao, David S. Miller, Stephen Hemminger,
Pravin B Shelar, Alex Duyck, Linux Kernel Network Developers
Hi Philp & Tom,
Thanks your advice.
I have send the update patch some days ago.
The link is http://www.spinics.net/lists/netdev/msg388083.html.
Could you help review it please?
Best Regards
Feng
On Thu, Jul 28, 2016 at 8:46 AM, Philp Prindeville
<philipp@redfish-solutions.com> wrote:
> Inline...
>
>
>
> On 07/27/2016 06:04 PM, Tom Herbert wrote:
>>
>> On Wed, Jul 27, 2016 at 4:42 PM, <fgao@ikuai8.com> wrote:
>>>
>>> From: Gao Feng <fgao@ikuai8.com>
>>>
>>> The PPTP is encapsulated by GRE header with that GRE_VERSION bits
>>> must contain one. But current GRE RPS needs the GRE_VERSION must be
>>> zero. So RPS does not work for PPTP traffic.
>>>
>>> In my test environment, there are four MIPS cores, and all traffic
>>> are passed through by PPTP. As a result, only one core is 100% busy
>>> while other three cores are very idle. After this patch, the usage
>>> of four cores are balanced well.
>>>
>>> Signed-off-by: Gao Feng <fgao@ikuai8.com>
>>> ---
>>> v1: Initial patch
>>>
>>> include/uapi/linux/if_tunnel.h | 5 +-
>>> net/core/flow_dissector.c | 138
>>> ++++++++++++++++++++++++++---------------
>>> 2 files changed, 92 insertions(+), 51 deletions(-)
>>>
>>> diff --git a/include/uapi/linux/if_tunnel.h
>>> b/include/uapi/linux/if_tunnel.h
>>> index 1046f55..dda4e4b 100644
>>> --- a/include/uapi/linux/if_tunnel.h
>>> +++ b/include/uapi/linux/if_tunnel.h
>>> @@ -24,9 +24,12 @@
>>> #define GRE_SEQ __cpu_to_be16(0x1000)
>>> #define GRE_STRICT __cpu_to_be16(0x0800)
>>> #define GRE_REC __cpu_to_be16(0x0700)
>>> -#define GRE_FLAGS __cpu_to_be16(0x00F8)
>>> +#define GRE_ACK __cpu_to_be16(0x0080)
>>> +#define GRE_FLAGS __cpu_to_be16(0x0078)
>>> #define GRE_VERSION __cpu_to_be16(0x0007)
>>>
>>> +#define GRE_PROTO_PPP __cpu_to_be16(0x880b)
>>> +
>>> struct ip_tunnel_parm {
>>> char name[IFNAMSIZ];
>>> int link;
>>> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>>> index 61ad43f..d95e060 100644
>>> --- a/net/core/flow_dissector.c
>>> +++ b/net/core/flow_dissector.c
>>> @@ -346,63 +346,101 @@ ip_proto_again:
>>> hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr),
>>> data, hlen, &_hdr);
>>> if (!hdr)
>>> goto out_bad;
>>> - /*
>>> - * Only look inside GRE if version zero and no
>>> - * routing
>>> - */
>>> - if (hdr->flags & (GRE_VERSION | GRE_ROUTING))
>>> - break;
>>>
>>> - proto = hdr->proto;
>>> - nhoff += 4;
>>> - if (hdr->flags & GRE_CSUM)
>>> + /*
>>> + * Only look inside GRE if version zero and no
>>> + * routing
>>
>> This comment is no longer true, GRE version 1 is being handled.
>>
>>> + */
>>> + if (!(hdr->flags & (GRE_VERSION | GRE_ROUTING))) {
>>> + proto = hdr->proto;
>>> nhoff += 4;
>>> - if (hdr->flags & GRE_KEY) {
>>> - const __be32 *keyid;
>>> - __be32 _keyid;
>>> + if (hdr->flags & GRE_CSUM)
>>> + nhoff += 4;
>>> + if (hdr->flags & GRE_KEY) {
>>> + const __be32 *keyid;
>>> + __be32 _keyid;
>>> +
>>> + keyid = __skb_header_pointer(skb, nhoff,
>>> sizeof(_keyid),
>>> + data, hlen,
>>> &_keyid);
>>> +
>>> + if (!keyid)
>>> + goto out_bad;
>>> +
>>> + if (dissector_uses_key(flow_dissector,
>>> +
>>> FLOW_DISSECTOR_KEY_GRE_KEYID)) {
>>> + key_keyid =
>>> skb_flow_dissector_target(flow_dissector,
>>> +
>>> FLOW_DISSECTOR_KEY_GRE_KEYID,
>>> +
>>> target_container);
>>> + key_keyid->keyid = *keyid;
>>> + }
>>> + nhoff += 4;
>>> + }
>>> + if (hdr->flags & GRE_SEQ)
>>> + nhoff += 4;
>>> + if (proto == htons(ETH_P_TEB)) {
>>> + const struct ethhdr *eth;
>>> + struct ethhdr _eth;
>>> +
>>> + eth = __skb_header_pointer(skb, nhoff,
>>> + sizeof(_eth),
>>> + data, hlen,
>>> &_eth);
>>> + if (!eth)
>>> + goto out_bad;
>>> + proto = eth->h_proto;
>>> + nhoff += sizeof(*eth);
>>> +
>>> + /* Cap headers that we access via
>>> pointers at the
>>> + * end of the Ethernet header as our
>>> maximum alignment
>>> + * at that point is only 2 bytes.
>>> + */
>>> + if (NET_IP_ALIGN)
>>> + hlen = nhoff;
>>> + }
>>>
>>> - keyid = __skb_header_pointer(skb, nhoff,
>>> sizeof(_keyid),
>>> - data, hlen,
>>> &_keyid);
>>> + key_control->flags |= FLOW_DIS_ENCAPSULATION;
>>> + if (flags & FLOW_DISSECTOR_F_STOP_AT_ENCAP)
>>> + goto out_good;
>>>
>>> - if (!keyid)
>>> + goto again;
>>> + } else if (hdr->proto == GRE_PROTO_PPP && (hdr->flags &
>>> GRE_VERSION) &&
>>> + (hdr->flags & GRE_KEY) &&
>>> + !(hdr->flags & (GRE_CSUM | GRE_ROUTING |
>>> GRE_STRICT | GRE_REC | GRE_FLAGS))) {
>>> + /* It should be the PPTP in GRE
>>> + * We have checked the flags according to the
>>> + * RFC 2637
>>> + */
>>> + struct ppp_frame {
>>> + /* address & control, just ignore it */
>>> + __be16 ignore;
>>> + __be16 proto;
>>> + } *frame, _frame;
>>
>> Isn't there a common definition of a PPP frame?
>
>
> If there were, it would be in <net/ppp_defs.h> but it's not. It's sort of
> implied the macros PPP_ADDRESS(), PPP_CONTROL(), and PPP_PROTOCOL().
>
> Please rewrite the patch to use these.
>
> -Philip
>
>
>>> + int offset = 0;
>>> +
>>> + /* Skip GRE header */
>>> + offset += 4;
>>> + /* Skip payload length and call id */
>>> + offset += 4;
>>> +
>>> + if (hdr->flags & GRE_SEQ)
>>> + offset += 4;
>>> +
>>> + if (hdr->flags & GRE_ACK)
>>> + offset += 4;
>>> +
>>> + frame = skb_header_pointer(skb, nhoff + offset,
>>> sizeof(_frame), &_frame);
>>> + if (!frame)
>>> goto out_bad;
>>> -
>>> - if (dissector_uses_key(flow_dissector,
>>> -
>>> FLOW_DISSECTOR_KEY_GRE_KEYID)) {
>>> - key_keyid =
>>> skb_flow_dissector_target(flow_dissector,
>>> -
>>> FLOW_DISSECTOR_KEY_GRE_KEYID,
>>> -
>>> target_container);
>>> - key_keyid->keyid = *keyid;
>>> + if (frame->proto == __constant_htons(PPP_IP)) {
>>> + nhoff += (sizeof(*frame) + offset);
>>> + proto = __constant_htons(ETH_P_IP);
>>> + goto again;
>>> + } else if (frame->proto ==
>>> __constant_htons(PPP_IPV6)) {
>>> + nhoff += (sizeof(*frame) + offset);
>>> + proto = __constant_htons(ETH_P_IPV6);
>>> + goto again;
>>
>> There's a lot of code replication here with the version 0 path. Please
>> consolidate these. Also, this looks an awful lot like ETH_P_PPP_SES,
>> can this code leverage that code?
>>
>>> }
>>> - nhoff += 4;
>>> }
>>> - if (hdr->flags & GRE_SEQ)
>>> - nhoff += 4;
>>> - if (proto == htons(ETH_P_TEB)) {
>>> - const struct ethhdr *eth;
>>> - struct ethhdr _eth;
>>> -
>>> - eth = __skb_header_pointer(skb, nhoff,
>>> - sizeof(_eth),
>>> - data, hlen, &_eth);
>>> - if (!eth)
>>> - goto out_bad;
>>> - proto = eth->h_proto;
>>> - nhoff += sizeof(*eth);
>>> -
>>> - /* Cap headers that we access via pointers at the
>>> - * end of the Ethernet header as our maximum
>>> alignment
>>> - * at that point is only 2 bytes.
>>> - */
>>> - if (NET_IP_ALIGN)
>>> - hlen = nhoff;
>>> - }
>>> -
>>> - key_control->flags |= FLOW_DIS_ENCAPSULATION;
>>> - if (flags & FLOW_DISSECTOR_F_STOP_AT_ENCAP)
>>> - goto out_good;
>>> -
>>> - goto again;
>>> + break;
>>> }
>>> case NEXTHDR_HOP:
>>> case NEXTHDR_ROUTING:
>>> --
>>> 1.9.1
>>>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2016-08-03 15:03 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-28 7:14 [PATCH 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash fgao
2016-08-02 1:36 ` Philp Prindeville
2016-08-02 6:10 ` Feng Gao
2016-08-02 17:33 ` Philp Prindeville
2016-08-03 0:22 ` Feng Gao
2016-08-02 20:35 ` Tom Herbert
2016-08-03 0:28 ` Feng Gao
2016-08-03 1:11 ` Philip Prindeville
2016-08-03 1:59 ` Tom Herbert
2016-08-03 2:17 ` Feng Gao
2016-08-03 4:01 ` Tom Herbert
2016-08-03 15:02 ` Feng Gao
-- strict thread matches above, loose matches on Subject: below --
2016-07-27 23:42 fgao
2016-07-28 0:04 ` Tom Herbert
2016-07-28 0:24 ` Feng Gao
2016-07-28 0:35 ` Feng Gao
2016-07-28 0:46 ` Philp Prindeville
2016-08-01 7:36 ` Feng Gao
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.