| Message ID | 1506933676-20121-3-git-send-email-simon.horman@netronome.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | flow_dissector: dissect tunnel info | expand |
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!
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 >
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.
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 >
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.
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
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.
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.
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
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
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.
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.
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.