Re: [Xen-devel] [PATCH 06/15] xen: trace IRQ enabling/disabling

[Julien Grall <julien.grall@xxxxxxx>]

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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel