[ovs-dev] [v1] dpif-netdev: Add ovs-appctl dpif-netdev/subtable-show.

[Stokes, Ian]

On 1/8/2020 4:38 PM, Ilya Maximets wrote:
> On 07.01.2020 11:51, Stokes, Ian wrote:
>>
>>
>> On 1/2/2020 12:27 PM, Ilya Maximets wrote:
>>> On 20.12.2019 16:28, Emma Finn wrote:
>>>> Add an ovs-appctl command to iterate through the dpcls
>>>> and for each subtable output the miniflow bits for any
>>>> existing table.
>>>>
>>>> $ ovs-appctl dpif-netdev/subtable-show
>>>> pmd thread numa_id 0
>>>>     dpcls port 2:
>>>>       subtable:
>>>>         unit_0: 2 (0x5)
>>>>         unit_1: 1 (0x1)
>>>> pmd thread numa_id 1
>>>>     dpcls port 3:
>>>>       subtable:
>>>>         unit_0: 2 (0x5)
>>>>         unit_1: 1 (0x1)
>>>>
>>>> Signed-off-by: Emma Finn <emma.finn at intel.com>
>>>>
>>>> ---
>>>
>>> So, what about my suggestions and thoughts about alternative solutions
>>> that I posted in reply to RFC?  It's still unclear why we need to disturb
>>> the running datapath to get this information, especially if we could get
>>> it "offline" from the flow dump.
>>
>> Hi Ilya,
>>
>> apologies I've only spotted this post now.
>>
>> I guess to my mind there are a few reasons why this is being added as a new command and not a separate application to process/parse the flow dumps of a datapath.
>>
>> (i) Ease of implementation, it seems straight forward enough to follow the existing commands structure such as dpif-netdev/pmd-rxq-show to implement this command. It had the required elements (required data structures etc.) so minimum plumbing was required to get access to that info and it will be familiar to any other developers who have already worked or will work in that area of the code in the future.
>>
>> I agree this could be done offline without the need to lock the datapath, but from my understanding I don't think the intention here is to run this command at a frequent or high interval so I would think that the lock should not be an issue unless the command is being executed continually (again similar to pmd-rxq-show, it would be called when needed only).
>>
>> The concern of adding a new separate application for parsing the dump flow as you suggested came down to it being another separate app within OVS to maintain as well as the work required to plumb or parse all required info.
> 
> It's not hard to parse.  Just a few function calls.  Most of the required
> functionality exists in 'lib' code.
> 
> If you don't like a separate app or script, this information could be printed

I think it would be better to keep it in OVS rather than a separate 
app/script.

> in a flow-dump in a some special field named like 'dp-extra-info' or whatever.
> Like this:
>    recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), dp-extra-info:miniflow_bits(0x5, 0x1), actions:hash(sym_l4(0)),recirc(0x1)
> 

This is interesting approach, but are we missing info such as the in 
ports, numa nodes etc? Maybe Harry can comment to this as it was his 
idea originally. It seems with either approach you will be missing some 
info depending on how you want to approach the debug usecase.

> 'dp-extra-info' might be printed with verbosity enabled if dpif provided this
> information.

I haven't looked at ovs-ofctl myself, not sure how much work would be 
needed to access this info or if you are back to parsing, again I think 
the idea here is we have this info available in the current approach as 
is why parse and break out these values?

Would it even be acceptable for 2.13 as well? it would be a new v1 and 
we're past the soft freeze.

> 
> I really feel that this information (miniflow bits) should be bonded with flow-dump
> somehow just because it belongs there.

I guess we're looking at this from different points of view, don't shoot 
me but would it make sense to have both approaches? :-D
> 
>>
>> After posting the RFC we had a number of users already applying the patch and using it in their deployments, we spoke about it at the conference and didn't hear any objections so I this is why the patch has continued with this approach for the 2.13 release.
> 
> They didn't have any alternatives to use. =)

Agree, but the choice provided worked well so it may not have been a 
problem :D.

Best Regards
Ian
> 
> Best regards, Ilya Maximets.
>