Re: CPA patchset

Ingo Molnar
2008-01-10T09:31:58Z

Andi,

finally managed to get the time to review your CPA patchset, and i
fundamentally agree with most of the detail changes done in it. But here
are a few structural high-level observations:

- firstly, there's no rationale given. So we'll change ioremap()/etc.
from doing a cflush-range instruction instead of a WBINVD. But why?
WBINVD isnt particular fast (takes a few msecs), but why is that a
problem? Drivers dont do high-frequency ioremap-ing. It's typically
only done at driver/device startup and that's it. Whether module load
time takes 1254 msecs instead of 1250 msecs is no big deal.

- secondly, obviously doing a 'flush some' instead of a 'flush all'
operation is risky. There's not many ways we can get the 'flush all'
WBINVD instruction wrong (as long as we do it on all cpus, etc.). But
with this specific range flush we've got all the risks of accidentally
not flushing _enough_. Especially if some boundary of a mapped area is
imprecisely. Accidentally leaving aliases around in the cache is
asking for trouble as it's very hard to debug and the operations here
(ioremap) are typically done only once per system bootup. So we'll add
_fundamental_ fragility to an already historically fragile and
bug-ridden piece of code. That does not sound too smart to me and you
do not analyze these concerns in your submission.

- the bugfixes and cleanups to pgattr.c i like very much - but shouldnt
they come first in the series, instead of being mixed into it?

so i'd suggest for you to reshape this ontop of the PAT patchset done by
Venki and Suresh, with the cflush stuff kept at the very end of the
series, and even that should be boot and .config configurable, with
default off - at least initially. The cflush performance advantages seem
dubious at best, and they bring in very real regression dangers.

Ingo