On 3/21/22 16:03, Amber, Kumar wrote: > Hi Ilya, > > Thanks for the comments. > Replies are inline. > >> -----Original Message----- >> From: Ilya Maximets <[email protected]> >> Sent: Thursday, March 17, 2022 5:30 PM >> To: Amber, Kumar <[email protected]>; [email protected]; >> Stokes, Ian <[email protected]> >> Cc: [email protected]; [email protected]; Eelco Chaudron >> <[email protected]>; Ferriter, Cian <[email protected]> >> Subject: Re: [ovs-dev] [PATCH v5] dpcls: Change info-get function to fetch >> dpcls usage stats. >> >> On 12/15/21 05:15, Kumar Amber wrote: >>> Modified the dplcs info-get command output to include the count for >>> different dpcls implementations. >>> >>> $ovs-appctl dpif-netdev/subtable-lookup-prio-get >>> >>> Available dpcls implementations: >>> autovalidator (Use count: 1, Priority: 5) >>> generic (Use count: 0, Priority: 1) >>> avx512_gather (Use count: 0, Priority: 3) >>> >> >> Hi, folks. >> >> Sorry for jumping in so late again, but Mike's recent patch gave me >> flashbacks of the following discussion of a very similar functionality: >> https://patchwork.ozlabs.org/project/openvswitch/patch/1576855703- >> [email protected]/ >> >> We wanted to see which miniflow bits are in each subtable, so we ended up >> reporting this information alongside the datapath flow information in the >> output of dpctl/dump-flows via dp_extra_info. >> >> This new patch is intended to show which subtable lookup implementation is >> used. But this depends only on the miniflow bits and the current priorities. >> And doesn't change in runtime (unless priorities changed manually). What >> I'm saying is that it's extremely similar information to what we're already >> printing as part of dp_extra_info. >> >> Maybe instead of introducing extra counters, we could just add lookup >> implementation to the extra info? >> >> Something like this: >> >> >> recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=n >> o), \ >> dp-extra-info:miniflow_bits(5,1),lookup(avx512_gather) \ >> actions:hash(sym_l4(0)),recirc(0x1) >> >> I think, this format gives all the same information, because flows are dumped >> per-PMD, so can be counted if needed (I'm not sure though how the value of >> the counter is actually useful beside being zero or non-zero). >> But also we have a visual representation of the actual flow and we can see >> which lookup method is used for it. >> >> Implementation should also be simpler, because the infrastructure is already >> in place. 'dp_extra_info' pointer can be protected by rcu to avoid race >> conditions while re-generating the string on re-probe. >> dpcls_subtable_get_best_impl() can be extended to return a name as a >> constant string via the 'out' argument. >> >> Thoughts? >> > > I have already made a POC based on the Discussion above and will put that > patch in few days with the > 'dp_extra_info' showing DPCLS as well. > As for this Patch is also important as currently the get-command doesn’t > show much info and hence this improvement will make things clear for the user > through the dpcl-get-command. > Hence request this usability feature to be merged. > I will follow this patch with the 'dp_extra_info' showing DPCLS functions > next to give absolute clarity to the user.
Why the 'subtable-lookup-prio-get' should return anything except for the priority of subtable lookup implementations? That doesn't seem straightforward. Especially if we'll have that information reported in a different place already without need for extra infrastructure. > > Regards > Amber >> Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
