From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Wang, Wei W" Subject: Re: [PATCH v4 10/11] x86/intel_pstate: support the use of intel_pstate in pmstat.c Date: Thu, 10 Sep 2015 09:33:10 +0000 Message-ID: <286AC319A985734F985F78AFA26841F7A263C9@shsmsx102.ccr.corp.intel.com> References: <1435231033-22806-1-git-send-email-wei.w.wang@intel.com> <55B264B802000078000953C0@prv-mh.provo.novell.com> <286AC319A985734F985F78AFA26841F7A23F12@shsmsx102.ccr.corp.intel.com> <55F00A7C02000078000A1231@prv-mh.provo.novell.com> <286AC319A985734F985F78AFA26841F7A2441E@shsmsx102.ccr.corp.intel.com> <55F0114B02000078000A126B@prv-mh.provo.novell.com> <286AC319A985734F985F78AFA26841F7A245E8@shsmsx102.ccr.corp.intel.com> <55F0216A02000078000A1331@prv-mh.provo.novell.com> <286AC319A985734F985F78AFA26841F7A247E6@shsmsx102.ccr.corp.intel.com> <55F03ABD02000078000A1410@prv-mh.provo.novell.com> <286AC319A985734F985F78AFA26841F7A24B3B@shsmsx102.ccr.corp.intel.com> <55F04C4902000078000A14BD@prv-mh.provo.novell.com> <286AC319A985734F985F78AFA26841F7A24FBE@shsmsx102.ccr.corp.intel.com> <55F0727D02000078000A166A@prv-mh.provo.novell.com> <286AC319A985734F985F78AFA26841F7A25C20@shsmsx102.ccr.corp.intel.com> <55F1588302000078000A18C7@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <55F1588302000078000A18C7@prv-mh.provo.novell.com> Content-Language: en-US List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: "andrew.cooper3@citrix.com" , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 09/09/2015 16:17, Jan Beulich wrote: >>> On 10.09.15 at 07:35, wrote: > On 09/09/2015 23:55, Jan Beulich wrote: >>>> On 09.09.15 at 17:16, wrote: >> On 09/09/2015 21:12, Jan Beulich wrote: >>>>> On 09.09.15 at 14:56, wrote: >>> Can you please explain more why it doesn't scale? >>> From my point of view, any other future value representation can be >>> passed from the producer to the related consumer through this method. >> >>> Did you read all of my earlier replies? I already said there "Just >>> consider >> what happens to the code when we end up gaining a few >>> more drivers providing percentage values, and perhaps another one >>> providing >> a third variant of output representation." >> >> Yes, I have read that. I am not sure if I got your point, but my >> meaning was when we add new drivers to the code, e.g. xx_pstate >> driver, we can still have the name, "xx_pstate", assigned to >> "p_cpufreq->scaling_driver" to distinguish one another. If the driver >> uses a different variant of output representation, which cannot be >> held by " uint32_t scaling_max_perf" (it needs "uint64_t" for >> example, then > that driver developer needs to add a new field here like " >> uint64_t scaling_max_perf_xx"). >> What is the scaling problem? > >> if (strcmp() == 0 || >> strcmp() == 0 || >> strcmp() == 0) { >> ... >> } else if (strcmp() == 0) { >> ... >> } else { >> ... >> } > >> is just ugly, and gets all the uglier the more strcmp()s get added. >> Have a boolean or enumeration indicating what kind of data there is, >> and the > above changes to > >> switch (kind) { >> case absolute: ... >> case percentage: ... >> } > > Ok. I will replace the default "scaling_driver[CPUFREQ_NAME_LEN]" with > an enum type, like this following ... > - char scaling_driver[CPUFREQ_NAME_LEN]; > + enum scaling_driver_flag scaling_driver; > ... > > We cannot keep both of the above two there, because there is a 128Byte > size limit. Then somewhere, we need to translate the > character-represented scaling_driver to our new enum-represented > scaling_driver. For example, in pmstat.c, the following: > > if ( cpufreq_driver->name[0] ) > strlcpy(op->u.get_para.scaling_driver, > cpufreq_driver->name, CPUFREQ_NAME_LEN); else > strlcpy(op->u.get_para.scaling_driver, "Unknown", > CPUFREQ_NAME_LEN); > > needs to be changed to: > if ( strncmp(cpufreq_driver->name[0], "intel_pstate", CPUFREQ_NAME_LEN) == 0 ) > op->u.get_para.scaling_driver = INTEL_PSTATE; else if ( > strncmp(cpufreq_driver->name[0], "acpi_cpufreq", CPUFREQ_NAME_LEN) == > 0 ) > op->u.get_para.scaling_driver = ACPI_CPUFREQ; ... > > Seems we still cannot get rid of these strncmp()s. Is this acceptable, > or should we change "struct cpufreq_driver" to use enum represented > driver name as well, or do you have a better suggestion? > The one I explained before: Express the data representation type in an enum, not the driver kind. But even if we went with the > above, the strcmp() ugliness would at least be limited to the producer, not enforced onto any number of consumers. No. I think in our previous discussion, there is no problem with "the data representation type", any future data representation, as long as it is in "uint32_t", it can use "uint32_t scaling_max_perf" to hold that value representation. Your concern was that the following doesn't scale well. + if (!strncmp(p_cpufreq->scaling_driver, + "intel_pstate", CPUFREQ_NAME_LEN) ) So we are trying to change the driver name thing to be in enum. Best, Wei