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

Simon Horman
2017-09-27T13:50:49Z

On Wed, Sep 27, 2017 at 03:47:50PM +0200, Jiri Pirko wrote:
> 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.

Ok, do you think it is sufficient for __skb_flow_dissect to look at the
tunnel keys, say FLOW_DISSECTOR_KEY_ENC_*? I am a bit concerned this may
break flower as it look at the tunnel info unconditionally.