Re: [PATCH v4 net-next 0/8] flow_dissector: Protocol specific flow dissector offload

[Jiri Pirko <jiri@resnulli.us>]

Tue, Oct 03, 2017 at 08:35:54PM CEST, tom@quantonium.net wrote:
>On Tue, Oct 3, 2017 at 12:46 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Fri, Sep 29, 2017 at 07:59:35PM CEST, tom@herbertland.com wrote:
>>>On Fri, Sep 29, 2017 at 10:42 AM, David Miller <davem@davemloft.net> wrote:
>>>> From: Tom Herbert <tom@herbertland.com>
>>>> Date: Fri, 29 Sep 2017 08:48:55 -0700
>>>>
>>>>> The flow_dissector interface is not a uAPI.
>>>>
>>>> That's not true, insofar as cls_flower.c uses the flow_dissector
>>>> therefore if you change the flow_dissector in certain ways then
>>>> cls_flower.c might have it's behavior changed and that is in fact UAPI
>>>> facing.
>>>
>>>Then I would suggest adding another flag like FLOW_DISSECTOR_F_FLOWER
>>>and when anyone puts new code into flow_dissector they can wrap it
>>>with "if !(flags & FLOW_DISSECTOR_F_FLOWER)". If the flower uAPI is
>>>subsequently update then the conditional can be removed. This way
>>>flower can support maintain its APIs, but we can still still extend
>>>and improve flow_dissector for othersuse cases.
>>
>> This is not flower-specific problem. Flow_dissector is a servant of many.
>
>Besides flower, what other use cases of flow_dissector have made
>flow_dissector interface a uAPI? Any use of hashing does not do this.
>Maybe OVS does?

It may be that currently it affects only flower. That does not mean you
should add flower-specific quirk. All I say is this should be handled in
a generic way, independent on the caller.


>
>> As such, it is instructed what should it do. If you want to
>> change the way inner headers are parsed, you should either:
>
>Why would that only affect the way inner headers are parsed? Wouldn't
>we need to consider any change to flow_dissector that might affect the
>output in any way. For instance, the depth limits I added would change
>to output for someone that was parsing thirty-five layers of
>encapsulation so it it looks like that feature needs a flag. What if
>someone adds a new Ethernet protocol or a new encap protocol?

Sure, what I ment was any change of behaviour.


>
>> 1) change the callers so they are behaving the same as before
>> 2) make the flow_dissection change optional so the caller can say if he
>>    wants original or new behaviour.
>
>I guess we can do that, but am concerned about the overhead this will
>generate if were adding a flag each time anyone modifies the function.
>There are performance critical use cases of flow_dissector that will
>be impacted by such changes.

I don't think that the overhead would be much different from what you
proposed.


>
>Tom
>
>
>>