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 <t...@herbertland.com> wrote:
>
> On Wed, Jul 27, 2016 at 4:42 PM, <f...@ikuai8.com> wrote:
> > From: Gao Feng <f...@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 <f...@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
> >
