> > + ehca_lock_hcalls = !(cur_cpu_spec->cpu_user_features
> > + & PPC_FEATURE_ARCH_2_05);
> We already talked about this yesterday, but I still feel that checking the
> instruction set of the CPU should not be used to determine whether a
> specific device driver implementation is used int hypervisor.
I had the same reaction... is testing cpu_user_features really the
best way to detect this issue?
I'll hold off applying this for a few days so you guys can decide the
best thing to do. We'll definitely get some fix into 2.6.24 but we
have time to make a good decision.
> Regarding the performance problem, have you checked whether converting all
> your spin_lock_irqsave to spin_lock/spin_lock_irq improves your performance
> on the older machines? Maybe it's already fast enough that way.
It does seem that the only places that the hcall_lock is taken also
use msleep, so they must always be in process context. So you can
safely just use spin_lock(), right?
- R.
On Thursday 06 December 2007, Roland Dreier wrote:
=A0> Regarding the performance problem, have you checked whether converti=
ng all
=A0> your spin_lock_irqsave to spin_lock/spin_lock_irq improves your perf=
ormance
=A0> on the older machines? Maybe it's already fast enough that way.
=20
It does seem that the only places that the hcall_lock is taken also
use msleep, so they must always be in process context. =A0So you can
safely just use spin_lock(), right?
I think it needs some more inspection. The msleep in there is only called
for hcalls that return H_IS_LONG_BUSY(). In theory, you can call
ehca_plpar_hcall_norets() from inside an interrupt handler if the
hcall in question never returns long busy.
Arnd <><
Roland Dreier [off-list ref] wrote on 06.12.2007 19:27:09:
> > + ehca_lock_hcalls =
!(cur_cpu_spec->cpu_user_features
> > + & PPC_FEATURE_ARCH_2_05);
> We already talked about this yesterday, but I still feel that
checking the
> instruction set of the CPU should not be used to determine whether a
> specific device driver implementation is used int hypervisor.
I had the same reaction... is testing cpu_user_features really the
best way to detect this issue?
I concur it's not nice, but it was the only feasible method we could find
without adding a "bug fixed" feature flag to the partition<->firmware
interface. The firmware version reported in the OFDT is not a reliable
enough source, and even if it were, it would require a lot of string
parsing and matching against tables.
We're taking this to the firmware architects at the moment, but they're
not very fond of the idea of reporting the absence of bugs through
capability flags, as this could quickly lead to the exhaustion of flag
bits. We'll let the discussion stew for a bit, but if we don't get this
flag, we'll have to resort to the CPU features.
I'll hold off applying this for a few days so you guys can decide the
best thing to do. We'll definitely get some fix into 2.6.24 but we
have time to make a good decision.
Right.
> Regarding the performance problem, have you checked whether
converting all
> your spin_lock_irqsave to spin_lock/spin_lock_irq improves your
performance
> on the older machines? Maybe it's already fast enough that way.
It does seem that the only places that the hcall_lock is taken also
use msleep, so they must always be in process context. So you can
safely just use spin_lock(), right?
As Arnd said, there are hCalls that will never return H_LONG_BUSY_*, such
as H_QUERY_PORT and chums, so they will never sleep. The surrounding
functions, though, are not prepared to be called from interrupt context
(GFP_KERNEL comes to mind), so I agree that a simple spin_lock() will
suffice. Thanks, Arnd, for pointing this out.
We'll keep you guys posted on the feature flag discussion. Until then,
have a nice weekend!
Joachim