* Re: [PATCH net-next 2/2] flow_dissector: dissect tunnel info
2017-10-02 8:41 ` [PATCH net-next 2/2] flow_dissector: dissect tunnel info Simon Horman
@ 2017-10-02 10:00 ` Jiri Pirko
2017-10-02 19:36 ` Tom Herbert
2017-10-02 20:37 ` Tom Herbert
2 siblings, 0 replies; 16+ messages in thread
From: Jiri Pirko @ 2017-10-02 10:00 UTC (permalink / raw)
To: Simon Horman
Cc: David Miller, Jiri Pirko, Jamal Hadi Salim, Cong Wang, netdev,
oss-drivers
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!
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next 2/2] flow_dissector: dissect tunnel info
2017-10-02 8:41 ` [PATCH net-next 2/2] flow_dissector: dissect tunnel info Simon Horman
2017-10-02 10:00 ` Jiri Pirko
@ 2017-10-02 19:36 ` Tom Herbert
2017-10-02 20:04 ` [oss-drivers] " Simon Horman
2017-10-02 20:37 ` Tom Herbert
2 siblings, 1 reply; 16+ messages in thread
From: Tom Herbert @ 2017-10-02 19:36 UTC (permalink / raw)
To: Simon Horman
Cc: David Miller, Jiri Pirko, Jamal Hadi Salim, Cong Wang,
Linux Kernel Network Developers, oss-drivers
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
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [oss-drivers] Re: [PATCH net-next 2/2] flow_dissector: dissect tunnel info
2017-10-02 19:36 ` Tom Herbert
@ 2017-10-02 20:04 ` Simon Horman
0 siblings, 0 replies; 16+ messages in thread
From: Simon Horman @ 2017-10-02 20:04 UTC (permalink / raw)
To: Tom Herbert
Cc: David Miller, Jiri Pirko, Jamal Hadi Salim, Cong Wang,
Linux Kernel Network Developers, oss-drivers
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next 2/2] flow_dissector: dissect tunnel info
2017-10-02 8:41 ` [PATCH net-next 2/2] flow_dissector: dissect tunnel info Simon Horman
2017-10-02 10:00 ` Jiri Pirko
2017-10-02 19:36 ` Tom Herbert
@ 2017-10-02 20:37 ` Tom Herbert
2017-10-03 9:40 ` Simon Horman
2 siblings, 1 reply; 16+ messages in thread
From: Tom Herbert @ 2017-10-02 20:37 UTC (permalink / raw)
To: Simon Horman
Cc: David Miller, Jiri Pirko, Jamal Hadi Salim, Cong Wang,
Linux Kernel Network Developers, oss-drivers
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
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH net-next 2/2] flow_dissector: dissect tunnel info
2017-10-02 20:37 ` Tom Herbert
@ 2017-10-03 9:40 ` Simon Horman
2017-10-03 18:17 ` Tom Herbert
0 siblings, 1 reply; 16+ messages in thread
From: Simon Horman @ 2017-10-03 9:40 UTC (permalink / raw)
To: Tom Herbert
Cc: David Miller, Jiri Pirko, Jamal Hadi Salim, Cong Wang,
Linux Kernel Network Developers, oss-drivers
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next 2/2] flow_dissector: dissect tunnel info
2017-10-03 9:40 ` Simon Horman
@ 2017-10-03 18:17 ` Tom Herbert
2017-10-04 8:08 ` Simon Horman
0 siblings, 1 reply; 16+ messages in thread
From: Tom Herbert @ 2017-10-03 18:17 UTC (permalink / raw)
To: Simon Horman
Cc: David Miller, Jiri Pirko, Jamal Hadi Salim, Cong Wang,
Linux Kernel Network Developers, oss-drivers
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next 2/2] flow_dissector: dissect tunnel info
2017-10-03 18:17 ` Tom Herbert
@ 2017-10-04 8:08 ` Simon Horman
2017-10-04 8:15 ` Jiri Pirko
0 siblings, 1 reply; 16+ messages in thread
From: Simon Horman @ 2017-10-04 8:08 UTC (permalink / raw)
To: Tom Herbert
Cc: David Miller, Jiri Pirko, Jamal Hadi Salim, Cong Wang,
Linux Kernel Network Developers, oss-drivers
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next 2/2] flow_dissector: dissect tunnel info
2017-10-04 8:08 ` Simon Horman
@ 2017-10-04 8:15 ` Jiri Pirko
2017-10-04 15:52 ` Tom Herbert
0 siblings, 1 reply; 16+ messages in thread
From: Jiri Pirko @ 2017-10-04 8:15 UTC (permalink / raw)
To: Simon Horman
Cc: Tom Herbert, David Miller, Jiri Pirko, Jamal Hadi Salim,
Cong Wang, Linux Kernel Network Developers, oss-drivers
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next 2/2] flow_dissector: dissect tunnel info
2017-10-04 8:15 ` Jiri Pirko
@ 2017-10-04 15:52 ` Tom Herbert
2017-10-04 18:07 ` Jiri Pirko
0 siblings, 1 reply; 16+ messages in thread
From: Tom Herbert @ 2017-10-04 15:52 UTC (permalink / raw)
To: Jiri Pirko
Cc: Simon Horman, David Miller, Jiri Pirko, Jamal Hadi Salim,
Cong Wang, Linux Kernel Network Developers, oss-drivers
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next 2/2] flow_dissector: dissect tunnel info
2017-10-04 15:52 ` Tom Herbert
@ 2017-10-04 18:07 ` Jiri Pirko
2017-10-04 19:07 ` Simon Horman
0 siblings, 1 reply; 16+ messages in thread
From: Jiri Pirko @ 2017-10-04 18:07 UTC (permalink / raw)
To: Tom Herbert
Cc: Simon Horman, David Miller, Jiri Pirko, Jamal Hadi Salim,
Cong Wang, Linux Kernel Network Developers, oss-drivers
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next 2/2] flow_dissector: dissect tunnel info
2017-10-04 18:07 ` Jiri Pirko
@ 2017-10-04 19:07 ` Simon Horman
2017-10-04 19:17 ` Jiri Pirko
0 siblings, 1 reply; 16+ messages in thread
From: Simon Horman @ 2017-10-04 19:07 UTC (permalink / raw)
To: Jiri Pirko
Cc: Tom Herbert, David Miller, Jiri Pirko, Jamal Hadi Salim,
Cong Wang, Linux Kernel Network Developers, oss-drivers
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next 2/2] flow_dissector: dissect tunnel info
2017-10-04 19:07 ` Simon Horman
@ 2017-10-04 19:17 ` Jiri Pirko
0 siblings, 0 replies; 16+ messages in thread
From: Jiri Pirko @ 2017-10-04 19:17 UTC (permalink / raw)
To: Simon Horman
Cc: Tom Herbert, David Miller, Jiri Pirko, Jamal Hadi Salim,
Cong Wang, Linux Kernel Network Developers, oss-drivers
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.
^ permalink raw reply [flat|nested] 16+ messages in thread