On 09/06/17 11:55, George Dunlap wrote:
On 09/06/17 11:51, Julien Grall wrote:
On 07/06/17 16:22, Dario Faggioli wrote:
On Wed, 2017-06-07 at 12:16 +0100, Julien Grall wrote:
Hi Dario,
Hi,
On 01/06/17 18:34, Dario Faggioli wrote:
diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
index 2a06406..33b903e 100644
--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -150,7 +150,9 @@ void _spin_lock(spinlock_t *lock)
void _spin_lock_irq(spinlock_t *lock)
{
ASSERT(local_irq_is_enabled());
- local_irq_disable();
+ _local_irq_disable();
+ if ( unlikely(tb_init_done) )
__trace_var already contain a "if ( !tb_init_done )". It sounds
pointless to do it twice, and also I think it is not obvious to
understand the meaning of the check (i.e what is tb_init_done).
Would it be possible to hide this check and avoid check tb_init_done
twice?
I totally agree. And in fact, in another patch, I remove the
tb_init_done check in __trace_var(). Reason behind this is that all the
callers of __trace_var() (the main one being trace_var()), already
checks for tb_init_done themselves.
In fact, the explicit check in the caller, is the (only) basis on which
one decides to call either trace_var() or __trace_var().
This here is one of the call sites where I think the check is better
done in the caller.
I admit, it is a bit confusing to require the caller to check
tb_init_done. It adds more code and IHMO counterintuitive.
Because a number of the traces include a certain amount if "marshalling"
work ahead of time; there's no point in making space for a struct on the
stack, carefully reading and sorting all kinds if things to put into it,
only to have the data ignored because tracing isn't enabled (which is by
far the most common case).
At the moment all the callers have the semantic:
if ( !unlikely(tb_init_done) )
trace_irq_*
So why not hiding that in the macro trace_irq_disable/trace_irq_enable...?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel