Re: [PPC64] Functions to reserve performance monitor hardware

[Olof Johansson <olof@austin.ibm.com>]

On Tue, Jan 11, 2005 at 05:01:27AM +1100, David Gibson wrote:

> This patch creates functions to reserve and release the performance
> monitor hardware (including its interrupt), and makes oprofile use
> them.

I don't see where you make oprofile use the functions? op_model_*
changes aren't included in the patch.

> +int reserve_pmc_hardware(perf_irq_t new_perf_irq)
> +{
> +	int err = -EBUSY;;

Keeping an extra semicolon around in case you need one in a hurry? :)

> +	spin_lock(&pmc_owner_lock);
> +
> +	if (pmc_owner_caller) {
> +		printk(KERN_WARNING "reserve_pmc_hardware: "
> +		       "PMC hardware busy (reserved by caller %p)\n",
> +		       pmc_owner_caller);
> +		goto out;
> +	}
> +
> +	pmc_owner_caller = __builtin_return_address(0);
> +	perf_irq = new_perf_irq ? : dummy_perf;
> +
> +	err = 0;

Maybe I'm the only one with such an opinion, but I find it more readable
to set the error code in the error case (if section above) instead of
defaulting to error and clearing it before returning. :)

> +	pmc_owner_caller = NULL;
> +	perf_irq = dummy_perf;
> +
> +	spin_unlock(&pmc_owner_lock);

Current oprofile code has an implicit mb(); after restoring perf_irq. I
think the implied lwsync in spin_unlock is sufficient, but I wanted to
mention it.

How do you expect the function to be used, will there really be users
reserving the hardware without registering the interrupt handler? If
there are no such users then it could be nice to reserve using the
handler instead of the return address.


-Olof