On 08.01.2020 18:37, Stokes, Ian wrote: > > > 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 <[email protected]> >>>>> >>>>> --- >>>> >>>> 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.
We're not missing core info since flow dumps are printed per-PMD: flow-dump from the main thread: <...> flow-dump from the pmd thread on core X: <...> numa could be easily checked by by the core number and I'm not sure if we really need this information here. Flow dumps always contains 'in_port' fileld and the port name will be printed instead of port number if you'll pass --names to the dump command. > >> '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? Not ovs-ofctl - It dumps OF tables, not the datapath flow tables - but 'ovs-dpctl dump-flows' or 'ovs-appctl dpctl/dump-flows'. dpctl and appctl are using same code from lib/dpctl.c which is fairly simple. I'm suggesting to add this information to dpif_flow.attrs and dpif-netdev will fill this information while sending dumped flow in dp_netdev_flow_to_dpif_flow(). Should not be hard. It's not parsing and breaking out, we have a netdev_flow while dumping and we only need to store a bit more information from it into dpif_flow. > > Would it even be acceptable for 2.13 as well? it would be a new v1 and we're > past the soft freeze. Technically, soft freeze wasn't announced yet. =) And we also could make it a v3. > >> >> 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 Not sure about that. I'd like to not have both. >> >>> >>> 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. >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
