Re: [PATCH v2 net-next 2/2] net/sched: allow flower to match tunnel options

Jiri Pirko
2017-09-27T13:47:52Z

Wed, Sep 27, 2017 at 03:37:33PM CEST, simon.horman@netronome.com wrote:
>On Wed, Sep 27, 2017 at 02:56:03PM +0200, Jiri Pirko wrote:
>> Wed, Sep 27, 2017 at 02:52:06PM CEST, simon.horman@netronome.com wrote:
>> >On Wed, Sep 27, 2017 at 01:08:22PM +0200, Jiri Pirko wrote:
>> >> Wed, Sep 27, 2017 at 11:27:33AM CEST, simon.horman@netronome.com wrote:
>> >> >On Wed, Sep 27, 2017 at 11:10:05AM +0200, Jiri Pirko wrote:
>> >> >> Wed, Sep 27, 2017 at 10:16:34AM CEST, simon.horman@netronome.com wrote:
>> >
>> >...
>> >
>> >> >> > enum flow_dissector_key_id {
>> >> >> > FLOW_DISSECTOR_KEY_CONTROL, /* struct flow_dissector_key_control */
>> >> >> > FLOW_DISSECTOR_KEY_BASIC, /* struct flow_dissector_key_basic */
>> >> >> >@@ -205,6 +217,7 @@ enum flow_dissector_key_id {
>> >> >> > FLOW_DISSECTOR_KEY_MPLS, /* struct flow_dissector_key_mpls */
>> >> >> > FLOW_DISSECTOR_KEY_TCP, /* struct flow_dissector_key_tcp */
>> >> >> > FLOW_DISSECTOR_KEY_IP, /* struct flow_dissector_key_ip */
>> >> >> >+ FLOW_DISSECTOR_KEY_ENC_OPTS, /* struct flow_dissector_key_enc_opts */
>> >> >>
>> >> >> I don't see the actual dissection implementation. Where is it?
>> >> >> Did you test the patchset?
>> >> >
>> >> >Yes, I did test it. But it is also possible something went astray along the
>> >> >way and I will retest.
>> >> >
>> >> >I think that the code you are looking for is in
>> >> >fl_classify() in this patch.
>> >>
>> >> The dissection should be done in the flow_dissector. That's the whole
>> >> point in having it generic. You should move it there.
>> >
>> >Coming back to this after lunch, I believe what I have done in this patch
>> >is consistent with handling of other enc fields, which are set in
>> >fl_classify() rather than the dissector. In particular the ip_tunnel_info,
>> >which is used by this patch, is already used in fl_classify().
>>
>> That means the current code is wrong. The dissection should be done in
>> flow_dissector, not in fl_classify.
>
>Would an better approach be to move the fl_classify() below into, say,
>skb_flow_dissect_tunnel_info() and call that from fl_classify().

No. There is one flow dissection function and you just set it up in a
way you need it. Makes no sense to me to split it up in any way.

>
>The reason I suggest this rather than moving the code into
>__skb_flow_dissect() is that currently flower assumes that tunnel_info
>is used if present. While I assume other users of () assume tunnel_info
>is not used even if present.

__skb_flow_dissect should look at what caller wants, then check skb_tunnel_info
only in case it is needed.

>
>> >Without this patch I see:
>> >
>> >
>> >static int fl_classify(struct sk_buff *skb, const struct tcf_proto *tp,
>> > struct tcf_result *res)
>> >{
>> > ...
>> > struct ip_tunnel_info *info;
>> >
>> > ...
>> >
>> > 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;
>> > }
>> >
>> > ...
>> >}
>> >
>> >This patch adds the following inside the if() clause above:
>> >
>> > if (info->options_len) {
>> > skb_key.enc_opts.len = info->options_len;
>> > ip_tunnel_info_opts_get(skb_key.enc_opts.data, info);
>> > }
>> >
>> >