Hi Ilya, Please find my replies inline.
> -----Original Message----- > From: Ilya Maximets <i.maxim...@ovn.org> > Sent: Wednesday, March 30, 2022 10:23 PM > To: Amber, Kumar <kumar.am...@intel.com>; ovs-dev@openvswitch.org; > Stokes, Ian <ian.sto...@intel.com> > Cc: i.maxim...@ovn.org; f...@sysclose.org; Eelco Chaudron > <echau...@redhat.com>; Ferriter, Cian <cian.ferri...@intel.com> > Subject: Re: [ovs-dev] [PATCH v5] dpcls: Change info-get function to fetch > dpcls usage stats. > > On 3/21/22 16:03, Amber, Kumar wrote: > > Hi Ilya, > > > > Thanks for the comments. > > Replies are inline. > > > >> -----Original Message----- > >> From: Ilya Maximets <i.maxim...@ovn.org> > >> Sent: Thursday, March 17, 2022 5:30 PM > >> To: Amber, Kumar <kumar.am...@intel.com>; ovs- > d...@openvswitch.org; > >> Stokes, Ian <ian.sto...@intel.com> > >> Cc: f...@sysclose.org; i.maxim...@ovn.org; Eelco Chaudron > >> <echau...@redhat.com>; Ferriter, Cian <cian.ferri...@intel.com> > >> 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- > >> 125194-1-git-send-email-emma.f...@intel.com/ > >> > >> 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? > We think the DPIF, MFEX and DPCLS commands should all provide the same workflow to the user. By having "subtable-lookup-prio-get" return the list (including "use count" item) allows the get/set pair of commands to both modify and check the results of setting the DPCLS implementations. This patch makes the output and workflow the same as DPIF and MFEX. > That doesn't seem straightforward. Especially if we'll have that information > reported in a different place already without need for extra infrastructure. > The "dump-flows" approach provides information between the flow and the subtable implementaiton which is also useful, however it does not provide the above workflow like DPIF and MFEX. We are willing to also accept the "dump-flows" approach, but the "subtable lookup prio get" patch/workflow is required. Regards Amber > > > > Regards > > Amber > >> Best regards, Ilya Maximets. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
