Messages in this thread | | | Date | Mon, 10 Jan 2005 16:23:40 -0600 | From | Olof Johansson <> | Subject | Re: [PPC64] Functions to reserve performance monitor hardware |
| |
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
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |