Date: Mon, 02 Jul 2012 20:17:27 +0300 From: Andriy Gapon <avg@FreeBSD.org> To: Sean Bruno <seanbru@yahoo-inc.com> Cc: freebsd-acpi@FreeBSD.org, John Baldwin <jhb@FreeBSD.org> Subject: Re: [CFT] Sparse Cstate Support -- Its possible, that I don't know what I'm doing. Message-ID: <4FF1D7A7.6000906@FreeBSD.org> In-Reply-To: <1340756603.2898.12.camel@powernoodle.corp.yahoo.com> References: <1340756603.2898.12.camel@powernoodle.corp.yahoo.com>
next in thread | previous in thread | raw e-mail | index | archive | help
on 27/06/2012 03:23 Sean Bruno said the following: > Ok, version 2 now in effect. I removed the return of BUS_PROBE_DEFAULT > and this seems to do the right thing in the cases I have. > > If there are no objections to this, I'll chuck it over into -head soonish. > > > http://people.freebsd.org/~sbruno/acpi_cpu_cstate_sparse.txt Some sort of a review... First, going over what seems to be cosmetic changes in the patch. The change to etc/rc.d/power_profile seems to be a NOP, is it needed? New comment in the first hunk of acpi_cpu.c seems to be redundant and its placement violates style(9). What do we achieve by renaming cpu_cx_lowest static file-scope variable to global_lowest_cstate? I am not necessarily against the change, but it needs to be justified and it should likely be made in a separate commit. Besides it breaks an apparent (and perhaps useless) naming convention of prefixing everything with "cpu_". Other comments that you seem to have added as notes to self while working on the code - they are good, but better be done in a separate comments-only commit. Now to the essence of the patch. acpi_cpu_notify hunk - I am not sure if I completely follow the logic of the current code in that function. For some reason that I can not grasp it doesn't update per-CPU cpu_cx_lowest if it is (was) equal or greater than the global cpu_cx_lowest limit. Otherwise the per-CPU limit is set to the shallower of the global limit and the deepest available state. The patch seems to do something that make less sense in the latter case, it tries to set per-CPU cpu_cx_lowest based on a new C-state that resides at old per-CPU cpu_cx_lowest index. Essentially this means that (1) cpu_cx_lowest maybe point to a junk portion of cpu_cx_states after C-states change; (2) whatever the change, if we do not run into (1), then cpu_cx_lowest value won't change and the lowest C-state would be whichever happens to be at that index. About change in acpi_cpu_set_cx_lowest - I am not sure that it should fail if the exact match can not be found. Maybe cx_lowest should be set to the best match in that case? Where best match is the deepest state that is not deeper than the requested state. Also, the ACPI spec permits multiple _CST entries with the same C-state type. Not sure if this ever happens in practice but wouldn't rule it out. Previously they would be disambiguated by their indexes (perhaps incorrectly from ACPI point of view). I think that with the proposed change we need to have some policy on which of e.g. four C3 states to select as _the_ C3 state. acpi_cpu_global_cx_lowest_sysctl change - unlike the current code where acpi_cpu_set_cx_lowest never fails, the new code may change cx_lowest on some CPUs and leave other CPUs un-updated in the case of an error. Also, it seems that with this patch cpu_cx_count becomes a write-only variable. Maybe it should be removed or maybe it could be re-purposed/replaced with a deepest Cx type available. So that the sysctl handlers could check against this value like they did before (instead of MAX_CX_STATES). I think that the patch is not ready yet to be committed. Perhaps we need to discuss some things before the next review request. E.g. how the following three should interact: manually set global Cx limit, manually set Cx level for an individual CPU (thus overriding global limit) and ACPI platform change of available Cx levels. Thank you very much for your work on this driver! It really needs a bit of do-over. P.S. A diff produced with svn diff -x -p is much more convenient to review than plain svn diff. -- Andriy Gapon
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4FF1D7A7.6000906>