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. Regards Amber > Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
