[PATCH net-next 0/2] flow_dissector: dissect tunnel info

[PATCH net-next 0/2] flow_dissector: dissect tunnel info

[Simon Horman]

Move dissection of tunnel info from the flower classifier to the flow
dissector where all other dissection occurs.  This should not have any
behavioural affect on other users of the flow dissector.

Simon Horman (2):
  net/dst: Make skb parameter of skb{metadata_dst,tunnel_info}() const
  flow_dissector: dissect tunnel info

 include/net/dst_metadata.h |   5 ++-
 net/core/flow_dissector.c  | 100 +++++++++++++++++++++++++++++++++++++++++++++
 net/sched/cls_flower.c     |  25 ------------
 3 files changed, 103 insertions(+), 27 deletions(-)

-- 
2.1.4

[PATCH net-next 1/2] net/dst: Make skb parameter of skb{metadata_dst,tunnel_info}() const

[Simon Horman]

Make the skb parameter of skb_metadata_dst() and skb_tunnel_info()
const as they are not modified. This is in preparation for using
them in call-sites where skb is const.

Signed-off-by: Simon Horman <simon.horman@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 include/net/dst_metadata.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/net/dst_metadata.h b/include/net/dst_metadata.h
index a803129a4849..9fba2ebf6dda 100644
--- a/include/net/dst_metadata.h
+++ b/include/net/dst_metadata.h
@@ -24,7 +24,7 @@ struct metadata_dst {
 	} u;
 };
 
-static inline struct metadata_dst *skb_metadata_dst(struct sk_buff *skb)
+static inline struct metadata_dst *skb_metadata_dst(const struct sk_buff *skb)
 {
 	struct metadata_dst *md_dst = (struct metadata_dst *) skb_dst(skb);
 
@@ -34,7 +34,8 @@ static inline struct metadata_dst *skb_metadata_dst(struct sk_buff *skb)
 	return NULL;
 }
 
-static inline struct ip_tunnel_info *skb_tunnel_info(struct sk_buff *skb)
+static inline struct ip_tunnel_info *
+skb_tunnel_info(const struct sk_buff *skb)
 {
 	struct metadata_dst *md_dst = skb_metadata_dst(skb);
 	struct dst_entry *dst;
-- 
2.1.4

[PATCH net-next 2/2] flow_dissector: dissect tunnel info

[Simon Horman]

Move dissection of tunnel info from the flower classifier to the flow
dissector where all other dissection occurs.  This should not have any
behavioural affect on other users of the flow dissector.

Signed-off-by: Simon Horman <simon.horman@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 net/core/flow_dissector.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++
 net/sched/cls_flower.c    |  25 ------------
 2 files changed, 100 insertions(+), 25 deletions(-)

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 0a977373d003..1f5caafb4492 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -5,6 +5,7 @@
 #include <linux/ipv6.h>
 #include <linux/if_vlan.h>
 #include <net/dsa.h>
+#include <net/dst_metadata.h>
 #include <net/ip.h>
 #include <net/ipv6.h>
 #include <net/gre.h>
@@ -115,6 +116,102 @@ __be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
 }
 EXPORT_SYMBOL(__skb_flow_get_ports);
 
+static void
+skb_flow_dissect_set_enc_addr_type(enum flow_dissector_key_id type,
+				   struct flow_dissector *flow_dissector,
+				   void *target_container)
+{
+	struct flow_dissector_key_control *ctrl;
+
+	if (!dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ENC_CONTROL))
+		return;
+
+	ctrl = skb_flow_dissector_target(flow_dissector,
+					 FLOW_DISSECTOR_KEY_ENC_CONTROL,
+					 target_container);
+	ctrl->addr_type = type;
+}
+
+static void
+__skb_flow_dissect_tunnel_info(const struct sk_buff *skb,
+			       struct flow_dissector *flow_dissector,
+			       void *target_container)
+{
+	struct ip_tunnel_info *info;
+	struct ip_tunnel_key *key;
+
+	/* A quick check to see if there might be something to do. */
+	if (!dissector_uses_key(flow_dissector,
+				FLOW_DISSECTOR_KEY_ENC_KEYID) &&
+	    !dissector_uses_key(flow_dissector,
+				FLOW_DISSECTOR_KEY_ENC_IPV4_ADDRS) &&
+	    !dissector_uses_key(flow_dissector,
+				FLOW_DISSECTOR_KEY_ENC_IPV6_ADDRS) &&
+	    !dissector_uses_key(flow_dissector,
+				FLOW_DISSECTOR_KEY_ENC_CONTROL) &&
+	    !dissector_uses_key(flow_dissector,
+				FLOW_DISSECTOR_KEY_ENC_PORTS))
+		return;
+
+	info = skb_tunnel_info(skb);
+	if (!info)
+		return;
+
+	key = &info->key;
+
+	switch (ip_tunnel_info_af(info)) {
+	case AF_INET:
+		skb_flow_dissect_set_enc_addr_type(FLOW_DISSECTOR_KEY_IPV4_ADDRS,
+						   flow_dissector,
+						   target_container);
+		if (dissector_uses_key(flow_dissector,
+				       FLOW_DISSECTOR_KEY_ENC_IPV4_ADDRS)) {
+			struct flow_dissector_key_ipv4_addrs *ipv4;
+
+			ipv4 = skb_flow_dissector_target(flow_dissector,
+							 FLOW_DISSECTOR_KEY_ENC_IPV4_ADDRS,
+							 target_container);
+			ipv4->src = key->u.ipv4.src;
+			ipv4->dst = key->u.ipv4.dst;
+		}
+		break;
+	case AF_INET6:
+		skb_flow_dissect_set_enc_addr_type(FLOW_DISSECTOR_KEY_IPV6_ADDRS,
+						   flow_dissector,
+						   target_container);
+		if (dissector_uses_key(flow_dissector,
+				       FLOW_DISSECTOR_KEY_ENC_IPV6_ADDRS)) {
+			struct flow_dissector_key_ipv6_addrs *ipv6;
+
+			ipv6 = skb_flow_dissector_target(flow_dissector,
+							 FLOW_DISSECTOR_KEY_ENC_IPV6_ADDRS,
+							 target_container);
+			ipv6->src = key->u.ipv6.src;
+			ipv6->dst = key->u.ipv6.dst;
+		}
+		break;
+	}
+
+	if (dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ENC_KEYID)) {
+		struct flow_dissector_key_keyid *keyid;
+
+		keyid = skb_flow_dissector_target(flow_dissector,
+						  FLOW_DISSECTOR_KEY_ENC_KEYID,
+						  target_container);
+		keyid->keyid = tunnel_id_to_key32(key->tun_id);
+	}
+
+	if (dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ENC_PORTS)) {
+		struct flow_dissector_key_ports *tp;
+
+		tp = skb_flow_dissector_target(flow_dissector,
+					       FLOW_DISSECTOR_KEY_ENC_PORTS,
+					       target_container);
+		tp->src = key->tp_src;
+		tp->dst = key->tp_dst;
+	}
+}
+
 static enum flow_dissect_ret
 __skb_flow_dissect_mpls(const struct sk_buff *skb,
 			struct flow_dissector *flow_dissector,
@@ -478,6 +575,9 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
 					      FLOW_DISSECTOR_KEY_BASIC,
 					      target_container);
 
+	__skb_flow_dissect_tunnel_info(skb, flow_dissector,
+				       target_container);
+
 	if (dissector_uses_key(flow_dissector,
 			       FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
 		struct ethhdr *eth = eth_hdr(skb);
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index d230cb4c8094..db831ac708f6 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -152,37 +152,12 @@ static int fl_classify(struct sk_buff *skb, const struct tcf_proto *tp,
 	struct cls_fl_filter *f;
 	struct fl_flow_key skb_key;
 	struct fl_flow_key skb_mkey;
-	struct ip_tunnel_info *info;
 
 	if (!atomic_read(&head->ht.nelems))
 		return -1;
 
 	fl_clear_masked_range(&skb_key, &head->mask);
 
-	info = skb_tunnel_info(skb);
-	if (info) {
-		struct ip_tunnel_key *key = &info->key;
-
-		switch (ip_tunnel_info_af(info)) {
-		case AF_INET:
-			skb_key.enc_control.addr_type =
-				FLOW_DISSECTOR_KEY_IPV4_ADDRS;
-			skb_key.enc_ipv4.src = key->u.ipv4.src;
-			skb_key.enc_ipv4.dst = key->u.ipv4.dst;
-			break;
-		case AF_INET6:
-			skb_key.enc_control.addr_type =
-				FLOW_DISSECTOR_KEY_IPV6_ADDRS;
-			skb_key.enc_ipv6.src = key->u.ipv6.src;
-			skb_key.enc_ipv6.dst = key->u.ipv6.dst;
-			break;
-		}
-
-		skb_key.enc_key_id.keyid = tunnel_id_to_key32(key->tun_id);
-		skb_key.enc_tp.src = key->tp_src;
-		skb_key.enc_tp.dst = key->tp_dst;
-	}
-
 	skb_key.indev_ifindex = skb->skb_iif;
 	/* skb_flow_dissect() does not set n_proto in case an unknown protocol,
 	 * so do it rather here.
-- 
2.1.4

Re: [PATCH net-next 2/2] flow_dissector: dissect tunnel info

[Jiri Pirko]

Mon, Oct 02, 2017 at 10:41:16AM CEST, simon.horman@netronome.com wrote:
>Move dissection of tunnel info from the flower classifier to the flow
>dissector where all other dissection occurs.  This should not have any
>behavioural affect on other users of the flow dissector.
>
>Signed-off-by: Simon Horman <simon.horman@netronome.com>
>Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>

Acked-by: Jiri Pirko <jiri@mellanox.com>

Thanks for fixing this up Simon!

Re: [PATCH net-next 0/2] flow_dissector: dissect tunnel info

[David Miller]

From: Simon Horman <simon.horman@netronome.com>
Date: Mon,  2 Oct 2017 10:41:14 +0200

> Move dissection of tunnel info from the flower classifier to the flow
> dissector where all other dissection occurs.  This should not have any
> behavioural affect on other users of the flow dissector.

Series applied, thanks Simon.

Re: [PATCH net-next 2/2] flow_dissector: dissect tunnel info

[Tom Herbert]

On Mon, Oct 2, 2017 at 1:41 AM, Simon Horman <simon.horman@netronome.com> wrote:
> Move dissection of tunnel info from the flower classifier to the flow
> dissector where all other dissection occurs.  This should not have any
> behavioural affect on other users of the flow dissector.
>
> Signed-off-by: Simon Horman <simon.horman@netronome.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
>  net/core/flow_dissector.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++
>  net/sched/cls_flower.c    |  25 ------------
>  2 files changed, 100 insertions(+), 25 deletions(-)
>
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 0a977373d003..1f5caafb4492 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -5,6 +5,7 @@
>  #include <linux/ipv6.h>
>  #include <linux/if_vlan.h>
>  #include <net/dsa.h>
> +#include <net/dst_metadata.h>
>  #include <net/ip.h>
>  #include <net/ipv6.h>
>  #include <net/gre.h>
> @@ -115,6 +116,102 @@ __be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
>  }
>  EXPORT_SYMBOL(__skb_flow_get_ports);
>
> +static void
> +skb_flow_dissect_set_enc_addr_type(enum flow_dissector_key_id type,
> +                                  struct flow_dissector *flow_dissector,
> +                                  void *target_container)
> +{
> +       struct flow_dissector_key_control *ctrl;
> +
> +       if (!dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ENC_CONTROL))
> +               return;
> +
> +       ctrl = skb_flow_dissector_target(flow_dissector,
> +                                        FLOW_DISSECTOR_KEY_ENC_CONTROL,
> +                                        target_container);
> +       ctrl->addr_type = type;
> +}
> +
> +static void
> +__skb_flow_dissect_tunnel_info(const struct sk_buff *skb,
> +                              struct flow_dissector *flow_dissector,
> +                              void *target_container)
> +{
> +       struct ip_tunnel_info *info;
> +       struct ip_tunnel_key *key;
> +
> +       /* A quick check to see if there might be something to do. */
> +       if (!dissector_uses_key(flow_dissector,
> +                               FLOW_DISSECTOR_KEY_ENC_KEYID) &&
> +           !dissector_uses_key(flow_dissector,
> +                               FLOW_DISSECTOR_KEY_ENC_IPV4_ADDRS) &&
> +           !dissector_uses_key(flow_dissector,
> +                               FLOW_DISSECTOR_KEY_ENC_IPV6_ADDRS) &&
> +           !dissector_uses_key(flow_dissector,
> +                               FLOW_DISSECTOR_KEY_ENC_CONTROL) &&
> +           !dissector_uses_key(flow_dissector,
> +                               FLOW_DISSECTOR_KEY_ENC_PORTS))
> +               return;

This complex conditional is now in the path of every call to flow
dissector regardless of whether a classifier is enabled or tunnels
are. What does the assembly show in terms of number of branches? Can
we at least get this down to one check (might be a use case for
FLOW_DISSECTOR_F_FLOWER ;-) ), or even better use the static key when
encap or is enabled?

> +
> +       info = skb_tunnel_info(skb);
> +       if (!info)
> +               return;
> +
> +       key = &info->key;
> +
> +       switch (ip_tunnel_info_af(info)) {
> +       case AF_INET:
> +               skb_flow_dissect_set_enc_addr_type(FLOW_DISSECTOR_KEY_IPV4_ADDRS,
> +                                                  flow_dissector,
> +                                                  target_container);
> +               if (dissector_uses_key(flow_dissector,
> +                                      FLOW_DISSECTOR_KEY_ENC_IPV4_ADDRS)) {
> +                       struct flow_dissector_key_ipv4_addrs *ipv4;
> +
> +                       ipv4 = skb_flow_dissector_target(flow_dissector,
> +                                                        FLOW_DISSECTOR_KEY_ENC_IPV4_ADDRS,
> +                                                        target_container);
> +                       ipv4->src = key->u.ipv4.src;
> +                       ipv4->dst = key->u.ipv4.dst;
> +               }
> +               break;
> +       case AF_INET6:
> +               skb_flow_dissect_set_enc_addr_type(FLOW_DISSECTOR_KEY_IPV6_ADDRS,
> +                                                  flow_dissector,
> +                                                  target_container);
> +               if (dissector_uses_key(flow_dissector,
> +                                      FLOW_DISSECTOR_KEY_ENC_IPV6_ADDRS)) {
> +                       struct flow_dissector_key_ipv6_addrs *ipv6;
> +
> +                       ipv6 = skb_flow_dissector_target(flow_dissector,
> +                                                        FLOW_DISSECTOR_KEY_ENC_IPV6_ADDRS,
> +                                                        target_container);
> +                       ipv6->src = key->u.ipv6.src;
> +                       ipv6->dst = key->u.ipv6.dst;
> +               }
> +               break;
> +       }
> +
> +       if (dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ENC_KEYID)) {
> +               struct flow_dissector_key_keyid *keyid;
> +
> +               keyid = skb_flow_dissector_target(flow_dissector,
> +                                                 FLOW_DISSECTOR_KEY_ENC_KEYID,
> +                                                 target_container);
> +               keyid->keyid = tunnel_id_to_key32(key->tun_id);
> +       }
> +
> +       if (dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ENC_PORTS)) {
> +               struct flow_dissector_key_ports *tp;
> +
> +               tp = skb_flow_dissector_target(flow_dissector,
> +                                              FLOW_DISSECTOR_KEY_ENC_PORTS,
> +                                              target_container);
> +               tp->src = key->tp_src;
> +               tp->dst = key->tp_dst;
> +       }
> +}
> +
>  static enum flow_dissect_ret
>  __skb_flow_dissect_mpls(const struct sk_buff *skb,
>                         struct flow_dissector *flow_dissector,
> @@ -478,6 +575,9 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>                                               FLOW_DISSECTOR_KEY_BASIC,
>                                               target_container);
>
> +       __skb_flow_dissect_tunnel_info(skb, flow_dissector,
> +                                      target_container);
> +
>         if (dissector_uses_key(flow_dissector,
>                                FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
>                 struct ethhdr *eth = eth_hdr(skb);
> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> index d230cb4c8094..db831ac708f6 100644
> --- a/net/sched/cls_flower.c
> +++ b/net/sched/cls_flower.c
> @@ -152,37 +152,12 @@ static int fl_classify(struct sk_buff *skb, const struct tcf_proto *tp,
>         struct cls_fl_filter *f;
>         struct fl_flow_key skb_key;
>         struct fl_flow_key skb_mkey;
> -       struct ip_tunnel_info *info;
>
>         if (!atomic_read(&head->ht.nelems))
>                 return -1;
>
>         fl_clear_masked_range(&skb_key, &head->mask);
>
> -       info = skb_tunnel_info(skb);
> -       if (info) {
> -               struct ip_tunnel_key *key = &info->key;
> -
> -               switch (ip_tunnel_info_af(info)) {
> -               case AF_INET:
> -                       skb_key.enc_control.addr_type =
> -                               FLOW_DISSECTOR_KEY_IPV4_ADDRS;
> -                       skb_key.enc_ipv4.src = key->u.ipv4.src;
> -                       skb_key.enc_ipv4.dst = key->u.ipv4.dst;
> -                       break;
> -               case AF_INET6:
> -                       skb_key.enc_control.addr_type =
> -                               FLOW_DISSECTOR_KEY_IPV6_ADDRS;
> -                       skb_key.enc_ipv6.src = key->u.ipv6.src;
> -                       skb_key.enc_ipv6.dst = key->u.ipv6.dst;
> -                       break;
> -               }
> -
> -               skb_key.enc_key_id.keyid = tunnel_id_to_key32(key->tun_id);
> -               skb_key.enc_tp.src = key->tp_src;
> -               skb_key.enc_tp.dst = key->tp_dst;
> -       }
> -
>         skb_key.indev_ifindex = skb->skb_iif;
>         /* skb_flow_dissect() does not set n_proto in case an unknown protocol,
>          * so do it rather here.
> --
> 2.1.4
>

Re: [oss-drivers] Re: [PATCH net-next 2/2] flow_dissector: dissect tunnel info

[Simon Horman]

On Mon, Oct 02, 2017 at 12:36:33PM -0700, Tom Herbert wrote:
> On Mon, Oct 2, 2017 at 1:41 AM, Simon Horman <simon.horman@netronome.com> wrote:
> > Move dissection of tunnel info from the flower classifier to the flow
> > dissector where all other dissection occurs.  This should not have any
> > behavioural affect on other users of the flow dissector.

...

> > +static void
> > +__skb_flow_dissect_tunnel_info(const struct sk_buff *skb,
> > +                              struct flow_dissector *flow_dissector,
> > +                              void *target_container)
> > +{
> > +       struct ip_tunnel_info *info;
> > +       struct ip_tunnel_key *key;
> > +
> > +       /* A quick check to see if there might be something to do. */
> > +       if (!dissector_uses_key(flow_dissector,
> > +                               FLOW_DISSECTOR_KEY_ENC_KEYID) &&
> > +           !dissector_uses_key(flow_dissector,
> > +                               FLOW_DISSECTOR_KEY_ENC_IPV4_ADDRS) &&
> > +           !dissector_uses_key(flow_dissector,
> > +                               FLOW_DISSECTOR_KEY_ENC_IPV6_ADDRS) &&
> > +           !dissector_uses_key(flow_dissector,
> > +                               FLOW_DISSECTOR_KEY_ENC_CONTROL) &&
> > +           !dissector_uses_key(flow_dissector,
> > +                               FLOW_DISSECTOR_KEY_ENC_PORTS))
> > +               return;
> 
> This complex conditional is now in the path of every call to flow
> dissector regardless of whether a classifier is enabled or tunnels
> are. What does the assembly show in terms of number of branches? Can
> we at least get this down to one check (might be a use case for
> FLOW_DISSECTOR_F_FLOWER ;-) ), or even better use the static key when
> encap or is enabled?

Hi Tom,

it appears to me (a little to my surprise but I did check before
posting) that the compiler turns the above into a single comparison.

$ gcc --version
gcc (Debian 4.9.2-10) 4.9.2
Copyright (C) 2014 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Re: [PATCH net-next 2/2] flow_dissector: dissect tunnel info

[Tom Herbert]

On Mon, Oct 2, 2017 at 1:41 AM, Simon Horman <simon.horman@netronome.com> wrote:
> Move dissection of tunnel info from the flower classifier to the flow
> dissector where all other dissection occurs.  This should not have any
> behavioural affect on other users of the flow dissector.
>
> Signed-off-by: Simon Horman <simon.horman@netronome.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
>  net/core/flow_dissector.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++
>  net/sched/cls_flower.c    |  25 ------------
>  2 files changed, 100 insertions(+), 25 deletions(-)
>
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 0a977373d003..1f5caafb4492 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -5,6 +5,7 @@
>  #include <linux/ipv6.h>
>  #include <linux/if_vlan.h>
>  #include <net/dsa.h>
> +#include <net/dst_metadata.h>
>  #include <net/ip.h>
>  #include <net/ipv6.h>
>  #include <net/gre.h>
> @@ -115,6 +116,102 @@ __be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
>  }
>  EXPORT_SYMBOL(__skb_flow_get_ports);
>
> +static void
> +skb_flow_dissect_set_enc_addr_type(enum flow_dissector_key_id type,
> +                                  struct flow_dissector *flow_dissector,
> +                                  void *target_container)
> +{
> +       struct flow_dissector_key_control *ctrl;
> +
> +       if (!dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ENC_CONTROL))
> +               return;
> +
> +       ctrl = skb_flow_dissector_target(flow_dissector,
> +                                        FLOW_DISSECTOR_KEY_ENC_CONTROL,
> +                                        target_container);
> +       ctrl->addr_type = type;
> +}
> +
> +static void
> +__skb_flow_dissect_tunnel_info(const struct sk_buff *skb,
> +                              struct flow_dissector *flow_dissector,
> +                              void *target_container)
> +{
> +       struct ip_tunnel_info *info;
> +       struct ip_tunnel_key *key;
> +
> +       /* A quick check to see if there might be something to do. */
> +       if (!dissector_uses_key(flow_dissector,
> +                               FLOW_DISSECTOR_KEY_ENC_KEYID) &&
> +           !dissector_uses_key(flow_dissector,
> +                               FLOW_DISSECTOR_KEY_ENC_IPV4_ADDRS) &&
> +           !dissector_uses_key(flow_dissector,
> +                               FLOW_DISSECTOR_KEY_ENC_IPV6_ADDRS) &&
> +           !dissector_uses_key(flow_dissector,
> +                               FLOW_DISSECTOR_KEY_ENC_CONTROL) &&
> +           !dissector_uses_key(flow_dissector,
> +                               FLOW_DISSECTOR_KEY_ENC_PORTS))
> +               return;
> +
> +       info = skb_tunnel_info(skb);
> +       if (!info)
> +               return;
> +
> +       key = &info->key;
> +
> +       switch (ip_tunnel_info_af(info)) {
> +       case AF_INET:
> +               skb_flow_dissect_set_enc_addr_type(FLOW_DISSECTOR_KEY_IPV4_ADDRS,
> +                                                  flow_dissector,
> +                                                  target_container);
> +               if (dissector_uses_key(flow_dissector,
> +                                      FLOW_DISSECTOR_KEY_ENC_IPV4_ADDRS)) {
> +                       struct flow_dissector_key_ipv4_addrs *ipv4;
> +
> +                       ipv4 = skb_flow_dissector_target(flow_dissector,
> +                                                        FLOW_DISSECTOR_KEY_ENC_IPV4_ADDRS,
> +                                                        target_container);
> +                       ipv4->src = key->u.ipv4.src;
> +                       ipv4->dst = key->u.ipv4.dst;
> +               }
> +               break;
> +       case AF_INET6:
> +               skb_flow_dissect_set_enc_addr_type(FLOW_DISSECTOR_KEY_IPV6_ADDRS,
> +                                                  flow_dissector,
> +                                                  target_container);
> +               if (dissector_uses_key(flow_dissector,
> +                                      FLOW_DISSECTOR_KEY_ENC_IPV6_ADDRS)) {
> +                       struct flow_dissector_key_ipv6_addrs *ipv6;
> +
> +                       ipv6 = skb_flow_dissector_target(flow_dissector,
> +                                                        FLOW_DISSECTOR_KEY_ENC_IPV6_ADDRS,
> +                                                        target_container);
> +                       ipv6->src = key->u.ipv6.src;
> +                       ipv6->dst = key->u.ipv6.dst;

Simon,

I think I'm missing something fundamental here. This code is
populating flow dissector keys not based on the contents of the packet
like rest of the flow dissector, but on external meta data related to
the packet which I believe is constant during the whole flow
dissection. Why can't this be handled by the caller? Also, if I read
this correctly, this code could be called multiple times and it seems
like it does the exact same thing in each call.

Tom

> +               }
> +               break;
> +       }
> +
> +       if (dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ENC_KEYID)) {
> +               struct flow_dissector_key_keyid *keyid;
> +
> +               keyid = skb_flow_dissector_target(flow_dissector,
> +                                                 FLOW_DISSECTOR_KEY_ENC_KEYID,
> +                                                 target_container);
> +               keyid->keyid = tunnel_id_to_key32(key->tun_id);
> +       }
> +
> +       if (dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ENC_PORTS)) {
> +               struct flow_dissector_key_ports *tp;
> +
> +               tp = skb_flow_dissector_target(flow_dissector,
> +                                              FLOW_DISSECTOR_KEY_ENC_PORTS,
> +                                              target_container);
> +               tp->src = key->tp_src;
> +               tp->dst = key->tp_dst;
> +       }
> +}
> +
>  static enum flow_dissect_ret
>  __skb_flow_dissect_mpls(const struct sk_buff *skb,
>                         struct flow_dissector *flow_dissector,
> @@ -478,6 +575,9 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>                                               FLOW_DISSECTOR_KEY_BASIC,
>                                               target_container);
>
> +       __skb_flow_dissect_tunnel_info(skb, flow_dissector,
> +                                      target_container);
> +
>         if (dissector_uses_key(flow_dissector,
>                                FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
>                 struct ethhdr *eth = eth_hdr(skb);
> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> index d230cb4c8094..db831ac708f6 100644
> --- a/net/sched/cls_flower.c
> +++ b/net/sched/cls_flower.c
> @@ -152,37 +152,12 @@ static int fl_classify(struct sk_buff *skb, const struct tcf_proto *tp,
>         struct cls_fl_filter *f;
>         struct fl_flow_key skb_key;
>         struct fl_flow_key skb_mkey;
> -       struct ip_tunnel_info *info;
>
>         if (!atomic_read(&head->ht.nelems))
>                 return -1;
>
>         fl_clear_masked_range(&skb_key, &head->mask);
>
> -       info = skb_tunnel_info(skb);
> -       if (info) {
> -               struct ip_tunnel_key *key = &info->key;
> -
> -               switch (ip_tunnel_info_af(info)) {
> -               case AF_INET:
> -                       skb_key.enc_control.addr_type =
> -                               FLOW_DISSECTOR_KEY_IPV4_ADDRS;
> -                       skb_key.enc_ipv4.src = key->u.ipv4.src;
> -                       skb_key.enc_ipv4.dst = key->u.ipv4.dst;
> -                       break;
> -               case AF_INET6:
> -                       skb_key.enc_control.addr_type =
> -                               FLOW_DISSECTOR_KEY_IPV6_ADDRS;
> -                       skb_key.enc_ipv6.src = key->u.ipv6.src;
> -                       skb_key.enc_ipv6.dst = key->u.ipv6.dst;
> -                       break;
> -               }
> -
> -               skb_key.enc_key_id.keyid = tunnel_id_to_key32(key->tun_id);
> -               skb_key.enc_tp.src = key->tp_src;
> -               skb_key.enc_tp.dst = key->tp_dst;
> -       }
> -
>         skb_key.indev_ifindex = skb->skb_iif;
>         /* skb_flow_dissect() does not set n_proto in case an unknown protocol,
>          * so do it rather here.
> --
> 2.1.4
>

Re: [PATCH net-next 2/2] flow_dissector: dissect tunnel info

[Simon Horman]

On Mon, Oct 02, 2017 at 01:37:55PM -0700, Tom Herbert wrote:
> On Mon, Oct 2, 2017 at 1:41 AM, Simon Horman <simon.horman@netronome.com> wrote:
> > Move dissection of tunnel info from the flower classifier to the flow
> > dissector where all other dissection occurs.  This should not have any
> > behavioural affect on other users of the flow dissector.

...

Hi Tom,

> Simon,
> 
> I think I'm missing something fundamental here. This code is
> populating flow dissector keys not based on the contents of the packet
> like rest of the flow dissector, but on external meta data related to
> the packet which I believe is constant during the whole flow
> dissection.

Yes, I believe that is correct on all counts.

> Why can't this be handled by the caller?

It certainly can be. And indeed it was before this patch. But it seems odd
for some population of dissector keys to occur in the dissector and some
elsewhere.

I feel that we are circling back the perennial issue of flower using the
flow dissector in a somewhat broader/different way than many/all other
users of the flow dissector.

> Also, if I read this correctly, this code could be called multiple times
> and it seems like it does the exact same thing in each call.

I'm not sure what you are getting at there. If there are flower classifiers
for the same device at different priority levels then the dissection
will be called multiple times and the data in question cannot have changed
as far as I know. But this was also the case before this patch.

Re: [PATCH net-next 2/2] flow_dissector: dissect tunnel info

[Tom Herbert]

On Tue, Oct 3, 2017 at 2:40 AM, Simon Horman <simon.horman@netronome.com> wrote:
> On Mon, Oct 02, 2017 at 01:37:55PM -0700, Tom Herbert wrote:
>> On Mon, Oct 2, 2017 at 1:41 AM, Simon Horman <simon.horman@netronome.com> wrote:
>> > Move dissection of tunnel info from the flower classifier to the flow
>> > dissector where all other dissection occurs.  This should not have any
>> > behavioural affect on other users of the flow dissector.
>
> ...

> I feel that we are circling back the perennial issue of flower using the
> flow dissector in a somewhat broader/different way than many/all other
> users of the flow dissector.
>
Simon,

It's more like __skb_flow_dissect is already an incredibly complex
function and because of that it's difficult to maintain. We need to
measure changes against that fact. For this patch, there is precisely
one user (cls_flower.c) and it's not at all clear to me if there will
be ever any more (e.g. for hashing we don't need tunnel info). IMO, it
should be just as easy and less convolution for everyone to have
flower call __skb_flow_dissect_tunnel_info directly and not call if
from __skb_flow_dissect.

Tom

Re: [PATCH net-next 2/2] flow_dissector: dissect tunnel info

[Simon Horman]

On Tue, Oct 03, 2017 at 11:17:46AM -0700, Tom Herbert wrote:
> On Tue, Oct 3, 2017 at 2:40 AM, Simon Horman <simon.horman@netronome.com> wrote:
> > On Mon, Oct 02, 2017 at 01:37:55PM -0700, Tom Herbert wrote:
> >> On Mon, Oct 2, 2017 at 1:41 AM, Simon Horman <simon.horman@netronome.com> wrote:
> >> > Move dissection of tunnel info from the flower classifier to the flow
> >> > dissector where all other dissection occurs.  This should not have any
> >> > behavioural affect on other users of the flow dissector.
> >
> > ...
> 
> > I feel that we are circling back the perennial issue of flower using the
> > flow dissector in a somewhat broader/different way than many/all other
> > users of the flow dissector.
> >
> Simon,
> 
> It's more like __skb_flow_dissect is already an incredibly complex
> function and because of that it's difficult to maintain. We need to
> measure changes against that fact. For this patch, there is precisely
> one user (cls_flower.c) and it's not at all clear to me if there will
> be ever any more (e.g. for hashing we don't need tunnel info). IMO, it
> should be just as easy and less convolution for everyone to have
> flower call __skb_flow_dissect_tunnel_info directly and not call if
> from __skb_flow_dissect.

Hi Tom,

my original suggestion was just that, but Jiri indicated a strong preference
for the approach taken by this patch. I think we need to widen the
participants in this discussion.

Re: [PATCH net-next 2/2] flow_dissector: dissect tunnel info

[Jiri Pirko]

Wed, Oct 04, 2017 at 10:08:57AM CEST, simon.horman@netronome.com wrote:
>On Tue, Oct 03, 2017 at 11:17:46AM -0700, Tom Herbert wrote:
>> On Tue, Oct 3, 2017 at 2:40 AM, Simon Horman <simon.horman@netronome.com> wrote:
>> > On Mon, Oct 02, 2017 at 01:37:55PM -0700, Tom Herbert wrote:
>> >> On Mon, Oct 2, 2017 at 1:41 AM, Simon Horman <simon.horman@netronome.com> wrote:
>> >> > Move dissection of tunnel info from the flower classifier to the flow
>> >> > dissector where all other dissection occurs.  This should not have any
>> >> > behavioural affect on other users of the flow dissector.
>> >
>> > ...
>> 
>> > I feel that we are circling back the perennial issue of flower using the
>> > flow dissector in a somewhat broader/different way than many/all other
>> > users of the flow dissector.
>> >
>> Simon,
>> 
>> It's more like __skb_flow_dissect is already an incredibly complex
>> function and because of that it's difficult to maintain. We need to
>> measure changes against that fact. For this patch, there is precisely
>> one user (cls_flower.c) and it's not at all clear to me if there will
>> be ever any more (e.g. for hashing we don't need tunnel info). IMO, it
>> should be just as easy and less convolution for everyone to have
>> flower call __skb_flow_dissect_tunnel_info directly and not call if
>> from __skb_flow_dissect.
>
>Hi Tom,
>
>my original suggestion was just that, but Jiri indicated a strong preference
>for the approach taken by this patch. I think we need to widen the
>participants in this discussion.

I like the __skb_flow_dissect to be the function to call and it will do
the job according to the configuration. I don't like to split in
multiple calls. Does not make sense in the most of the cases as the
dissection state would have to be carried in between calls.

Re: [PATCH net-next 2/2] flow_dissector: dissect tunnel info

[Tom Herbert]

On Wed, Oct 4, 2017 at 1:15 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Wed, Oct 04, 2017 at 10:08:57AM CEST, simon.horman@netronome.com wrote:
>>On Tue, Oct 03, 2017 at 11:17:46AM -0700, Tom Herbert wrote:
>>> On Tue, Oct 3, 2017 at 2:40 AM, Simon Horman <simon.horman@netronome.com> wrote:
>>> > On Mon, Oct 02, 2017 at 01:37:55PM -0700, Tom Herbert wrote:
>>> >> On Mon, Oct 2, 2017 at 1:41 AM, Simon Horman <simon.horman@netronome.com> wrote:
>>> >> > Move dissection of tunnel info from the flower classifier to the flow
>>> >> > dissector where all other dissection occurs.  This should not have any
>>> >> > behavioural affect on other users of the flow dissector.
>>> >
>>> > ...
>>>
>>> > I feel that we are circling back the perennial issue of flower using the
>>> > flow dissector in a somewhat broader/different way than many/all other
>>> > users of the flow dissector.
>>> >
>>> Simon,
>>>
>>> It's more like __skb_flow_dissect is already an incredibly complex
>>> function and because of that it's difficult to maintain. We need to
>>> measure changes against that fact. For this patch, there is precisely
>>> one user (cls_flower.c) and it's not at all clear to me if there will
>>> be ever any more (e.g. for hashing we don't need tunnel info). IMO, it
>>> should be just as easy and less convolution for everyone to have
>>> flower call __skb_flow_dissect_tunnel_info directly and not call if
>>> from __skb_flow_dissect.
>>
>>Hi Tom,
>>
>>my original suggestion was just that, but Jiri indicated a strong preference
>>for the approach taken by this patch. I think we need to widen the
>>participants in this discussion.
>
> I like the __skb_flow_dissect to be the function to call and it will do
> the job according to the configuration. I don't like to split in
> multiple calls.

Those are not technical arguments. As I already mentioned, I don't
like it when we add stuff for the benefit of a 1% use case that
negatively impacts the rest of the 99% cases which is what I believe
is happening here.

> Does not make sense in the most of the cases as the
> dissection state would have to be carried in between calls.

Please elaborate. This code is being moved into __skb_flow_dissect, so
the functionality was already there. I don't see any description in
this discussion that things were broken and that this patch is a
necessary fix.

Thanks,
Tom

Re: [PATCH net-next 2/2] flow_dissector: dissect tunnel info

[Jiri Pirko]

Wed, Oct 04, 2017 at 05:52:54PM CEST, tom@herbertland.com wrote:
>On Wed, Oct 4, 2017 at 1:15 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Wed, Oct 04, 2017 at 10:08:57AM CEST, simon.horman@netronome.com wrote:
>>>On Tue, Oct 03, 2017 at 11:17:46AM -0700, Tom Herbert wrote:
>>>> On Tue, Oct 3, 2017 at 2:40 AM, Simon Horman <simon.horman@netronome.com> wrote:
>>>> > On Mon, Oct 02, 2017 at 01:37:55PM -0700, Tom Herbert wrote:
>>>> >> On Mon, Oct 2, 2017 at 1:41 AM, Simon Horman <simon.horman@netronome.com> wrote:
>>>> >> > Move dissection of tunnel info from the flower classifier to the flow
>>>> >> > dissector where all other dissection occurs.  This should not have any
>>>> >> > behavioural affect on other users of the flow dissector.
>>>> >
>>>> > ...
>>>>
>>>> > I feel that we are circling back the perennial issue of flower using the
>>>> > flow dissector in a somewhat broader/different way than many/all other
>>>> > users of the flow dissector.
>>>> >
>>>> Simon,
>>>>
>>>> It's more like __skb_flow_dissect is already an incredibly complex
>>>> function and because of that it's difficult to maintain. We need to
>>>> measure changes against that fact. For this patch, there is precisely
>>>> one user (cls_flower.c) and it's not at all clear to me if there will
>>>> be ever any more (e.g. for hashing we don't need tunnel info). IMO, it
>>>> should be just as easy and less convolution for everyone to have
>>>> flower call __skb_flow_dissect_tunnel_info directly and not call if
>>>> from __skb_flow_dissect.
>>>
>>>Hi Tom,
>>>
>>>my original suggestion was just that, but Jiri indicated a strong preference
>>>for the approach taken by this patch. I think we need to widen the
>>>participants in this discussion.
>>
>> I like the __skb_flow_dissect to be the function to call and it will do
>> the job according to the configuration. I don't like to split in
>> multiple calls.
>
>Those are not technical arguments. As I already mentioned, I don't
>like it when we add stuff for the benefit of a 1% use case that
>negatively impacts the rest of the 99% cases which is what I believe
>is happening here.

Yeah. I just wanted the flow dissector to stay compact. But if needed,
could be split. I just fear that it will become a mess that's all.


>
>> Does not make sense in the most of the cases as the
>> dissection state would have to be carried in between calls.
>
>Please elaborate. This code is being moved into __skb_flow_dissect, so
>the functionality was already there. I don't see any description in
>this discussion that things were broken and that this patch is a
>necessary fix.

Yeah, you are right.


>
>Thanks,
>Tom

Re: [PATCH net-next 2/2] flow_dissector: dissect tunnel info

[Simon Horman]

On Wed, Oct 04, 2017 at 08:07:15PM +0200, Jiri Pirko wrote:
> Wed, Oct 04, 2017 at 05:52:54PM CEST, tom@herbertland.com wrote:
> >On Wed, Oct 4, 2017 at 1:15 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> >> Wed, Oct 04, 2017 at 10:08:57AM CEST, simon.horman@netronome.com wrote:
> >>>On Tue, Oct 03, 2017 at 11:17:46AM -0700, Tom Herbert wrote:
> >>>> On Tue, Oct 3, 2017 at 2:40 AM, Simon Horman <simon.horman@netronome.com> wrote:
> >>>> > On Mon, Oct 02, 2017 at 01:37:55PM -0700, Tom Herbert wrote:
> >>>> >> On Mon, Oct 2, 2017 at 1:41 AM, Simon Horman <simon.horman@netronome.com> wrote:
> >>>> >> > Move dissection of tunnel info from the flower classifier to the flow
> >>>> >> > dissector where all other dissection occurs.  This should not have any
> >>>> >> > behavioural affect on other users of the flow dissector.
> >>>> >
> >>>> > ...
> >>>>
> >>>> > I feel that we are circling back the perennial issue of flower using the
> >>>> > flow dissector in a somewhat broader/different way than many/all other
> >>>> > users of the flow dissector.
> >>>> >
> >>>> Simon,
> >>>>
> >>>> It's more like __skb_flow_dissect is already an incredibly complex
> >>>> function and because of that it's difficult to maintain. We need to
> >>>> measure changes against that fact. For this patch, there is precisely
> >>>> one user (cls_flower.c) and it's not at all clear to me if there will
> >>>> be ever any more (e.g. for hashing we don't need tunnel info). IMO, it
> >>>> should be just as easy and less convolution for everyone to have
> >>>> flower call __skb_flow_dissect_tunnel_info directly and not call if
> >>>> from __skb_flow_dissect.
> >>>
> >>>Hi Tom,
> >>>
> >>>my original suggestion was just that, but Jiri indicated a strong preference
> >>>for the approach taken by this patch. I think we need to widen the
> >>>participants in this discussion.
> >>
> >> I like the __skb_flow_dissect to be the function to call and it will do
> >> the job according to the configuration. I don't like to split in
> >> multiple calls.
> >
> >Those are not technical arguments. As I already mentioned, I don't
> >like it when we add stuff for the benefit of a 1% use case that
> >negatively impacts the rest of the 99% cases which is what I believe
> >is happening here.
> 
> Yeah. I just wanted the flow dissector to stay compact. But if needed,
> could be split. I just fear that it will become a mess that's all.
> 
> 
> >
> >> Does not make sense in the most of the cases as the
> >> dissection state would have to be carried in between calls.
> >
> >Please elaborate. This code is being moved into __skb_flow_dissect, so
> >the functionality was already there. I don't see any description in
> >this discussion that things were broken and that this patch is a
> >necessary fix.
> 
> Yeah, you are right.

Hi Tom, Hi Jiri,

I'm happy to make a patch to move the call to
__skb_flow_dissect_tunnel_info() from __skb_flow_dissect() to
fl_classify(). It seems that approach has been agreed on above.

Re: [PATCH net-next 2/2] flow_dissector: dissect tunnel info

[Jiri Pirko]

Wed, Oct 04, 2017 at 09:07:17PM CEST, simon.horman@netronome.com wrote:
>On Wed, Oct 04, 2017 at 08:07:15PM +0200, Jiri Pirko wrote:
>> Wed, Oct 04, 2017 at 05:52:54PM CEST, tom@herbertland.com wrote:
>> >On Wed, Oct 4, 2017 at 1:15 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> >> Wed, Oct 04, 2017 at 10:08:57AM CEST, simon.horman@netronome.com wrote:
>> >>>On Tue, Oct 03, 2017 at 11:17:46AM -0700, Tom Herbert wrote:
>> >>>> On Tue, Oct 3, 2017 at 2:40 AM, Simon Horman <simon.horman@netronome.com> wrote:
>> >>>> > On Mon, Oct 02, 2017 at 01:37:55PM -0700, Tom Herbert wrote:
>> >>>> >> On Mon, Oct 2, 2017 at 1:41 AM, Simon Horman <simon.horman@netronome.com> wrote:
>> >>>> >> > Move dissection of tunnel info from the flower classifier to the flow
>> >>>> >> > dissector where all other dissection occurs.  This should not have any
>> >>>> >> > behavioural affect on other users of the flow dissector.
>> >>>> >
>> >>>> > ...
>> >>>>
>> >>>> > I feel that we are circling back the perennial issue of flower using the
>> >>>> > flow dissector in a somewhat broader/different way than many/all other
>> >>>> > users of the flow dissector.
>> >>>> >
>> >>>> Simon,
>> >>>>
>> >>>> It's more like __skb_flow_dissect is already an incredibly complex
>> >>>> function and because of that it's difficult to maintain. We need to
>> >>>> measure changes against that fact. For this patch, there is precisely
>> >>>> one user (cls_flower.c) and it's not at all clear to me if there will
>> >>>> be ever any more (e.g. for hashing we don't need tunnel info). IMO, it
>> >>>> should be just as easy and less convolution for everyone to have
>> >>>> flower call __skb_flow_dissect_tunnel_info directly and not call if
>> >>>> from __skb_flow_dissect.
>> >>>
>> >>>Hi Tom,
>> >>>
>> >>>my original suggestion was just that, but Jiri indicated a strong preference
>> >>>for the approach taken by this patch. I think we need to widen the
>> >>>participants in this discussion.
>> >>
>> >> I like the __skb_flow_dissect to be the function to call and it will do
>> >> the job according to the configuration. I don't like to split in
>> >> multiple calls.
>> >
>> >Those are not technical arguments. As I already mentioned, I don't
>> >like it when we add stuff for the benefit of a 1% use case that
>> >negatively impacts the rest of the 99% cases which is what I believe
>> >is happening here.
>> 
>> Yeah. I just wanted the flow dissector to stay compact. But if needed,
>> could be split. I just fear that it will become a mess that's all.
>> 
>> 
>> >
>> >> Does not make sense in the most of the cases as the
>> >> dissection state would have to be carried in between calls.
>> >
>> >Please elaborate. This code is being moved into __skb_flow_dissect, so
>> >the functionality was already there. I don't see any description in
>> >this discussion that things were broken and that this patch is a
>> >necessary fix.
>> 
>> Yeah, you are right.
>
>Hi Tom, Hi Jiri,
>
>I'm happy to make a patch to move the call to
>__skb_flow_dissect_tunnel_info() from __skb_flow_dissect() to
>fl_classify(). It seems that approach has been agreed on above.

If the consensus is that the right way is to cut-out flow dissector,
so be it. But first, I believe it is reasonable to request to see some
numbers that would indicate that it actually resolves anything.