Re: [PATCH iproute2-next v3 2/2] tc: implement support for terse dump

[Vlad Buslov <vladbu@nvidia.com>]

On Thu 22 Oct 2020 at 17:05, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 2020-10-21 4:19 a.m., Vlad Buslov wrote:
>>
>> On Tue 20 Oct 2020 at 15:29, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>> On 2020-10-19 11:18 a.m., Vlad Buslov wrote:
>>> My worry is you have a very specific use case for your hardware or
>>> maybe it is ovs - where counters are uniquely tied to filters and
>>> there is no sharing. And possibly maybe only one counter can be tied
>>> to a filter (was not sure if you could handle more than one action
>>> in the terse from looking at the code).
>>
>> OVS uses cookie to uniquely identify the flow and it does support
>> multiple actions per flow.
>
>
> ok, so they use it like a flowid/classid to identify the flow.
> In our use case the cookie stores all kinds of other state that
> the controller can avoid to lookup after a query.
> index otoh is universal i.e two different users can intepret it
> per action tying it specific stats.
> IOW: I dont think it replaces the index.
> Do they set cookies on all actions in a flow?

AFAIK on only one action per flow.

>
>
>>> Our assumptions so far had no such constraints.
>>> Maybe a new TERSE_OPTIONS TLV, and then add an extra flag
>>> to indicate interest in the tlv? Peharps store the stats in it as well.
>>
>> Maybe, but wouldn't that require making it a new dump mode? Current
>> terse dump is already in released kernel and this seems like a
>> backward-incompatible change.
>>
>
> I meant you would set a new flag(in addition to TERSE) in a request to
> the kernel to ask for the index to be made available on the response.
> Response comes back in a TLV with just index in it for now.

Makes sense.

>
>>>
>>>> This wouldn't be much of a terse dump anymore. What prevents user that
>>>> needs all action info from calling regular dump? It is not like terse
>>>> dump substitutes it or somehow makes it harder to use.
>>>
>>> Both scaling and correctness are important. You have the cookie
>>> in the terse dump, thats a lot of data.
>>
>> Cookie only consumes space in resulting netlink packet if used set the
>> cookie during action init. Otherwise, the cookie attribute is omitted.
>
> True, but: I am wondering why it is even considered in when terseness
> was a requirement (and index was left out).

There was several reasons for me to include it:

- As I wrote in previous email its TLV is only included in dump if user
  set the cookie. Users who don't use cookies don't lose any performance
  of terse dump.

- Including it didn't require any changes to tc_action_ops->dump() (like
  passing 'terse' flag or introducing dedicated terse_dump() callback)
  because it is processed in tcf_action_dump_1().

- OVS was the main use-case for us because it relies on filter dump for
  flow revalidation and uses cookie to identify the flows.

>
>>> In our case we totally bypass filters to reduce the amount of data
>>> crossing to user space (tc action ls). Theres still a lot of data
>>> crossing which we could trim with a terse dump. All we are interested
>>> in are stats. Another alternative is perhaps to introduce the index for
>>> the direct dump.
>>
>> What is the direct dump?
>
> tc action ls ...
> Like i said in our use cases to get the stats we just dumped the actions
> we wanted. It is a lot less data than having the filter + actions.
> And with your idea of terseness we can trim down further how much
> data by removing all the action attributes coming back if we set TERSE
> flag in such a request. But the index has to be there to make sense.

Yes, that makes sense. I guess introducing something like 'tc action -br
ls ..' mode implemented by means of existing terse flag + new 'also
output action index' flag would achieve that goal.

>
> cheers,
> jamal