[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





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.

BTW, I am not responsible of this code, so if the REST maintainers are happy with that then fine.



diff --git a/xen/include/asm-arm/arm32/system.h b/xen/include/asm-
arm/arm32/system.h
index c617b40..20871ad 100644
--- a/xen/include/asm-arm/arm32/system.h
+++ b/xen/include/asm-arm/arm32/system.h
@@ -4,6 +4,8 @@

 #include <asm/arm32/cmpxchg.h>

+#include <xen/trace.h>
+
 #define local_irq_disable() asm volatile ( "cpsid i @
local_irq_disable\n" : : : "cc" )
 #define local_irq_enable()  asm volatile ( "cpsie i @
local_irq_enable\n" : : : "cc" )

@@ -41,6 +43,16 @@ static inline int local_irq_is_enabled(void)
 #define local_abort_enable() __asm__("cpsie a  @ __sta\n" : : :
"memory", "cc")
 #define local_abort_disable() __asm__("cpsid a @ __sta\n" : : :
"memory", "cc")

+/* We do not support tracing (at all) yet */

I know that some bits are missing for ARM, but the patch #5
contradicts
this comment as you have CONFIG_TRACE=y by default.

No sure what you mean. Tracing is de facto on by default right now,
despite it not being implemented for ARM. So what I'm doing is
basically keeping things as they are.

If you think it should be off by default, well, let's talk about it...
but I'm not sure this is really what you are saying/asking.

I mean that CONFIG_TRACE=y by default gives the impression that tracing
is supported on ARM. This is not the case, hence your comment. So I
think it should be off my default until we get the support. BTW, it is
only a couple of simple patches but they never made upstreamed :/

+#define trace_irq_disable_ret()   do { } while ( 0 )
+#define trace_irq_enable_ret()    do { } while ( 0 )
+#define trace_irq_save_ret(_x)    do { } while ( 0 )
+#define trace_irq_restore_ret(_x) do { } while ( 0 )
+#define _local_irq_disable()      local_irq_disable()
+#define _local_irq_enable()       local_irq_enable()
+#define _local_irq_save(_x)       local_irq_save(_x)
+#define _local_irq_restore(_x)    local_irq_restore(_x)
+

This does not need to be duplicated in both asm-
arm/arm{32,64}/system.h.
You could directly implement them in asm-arm/system.h.

Ok, thanks.

Also, I would prefer if you stay consistent with x86. I.e non-
underscore
versions are calling the underscore versions and not the invert.

Well, I know it is counterintuitive, but it's the easiest way of
getting over this, i.e., without the need of a lot of stubs, and
touching as few existing code as possible.

Can you detail here? The only things I was asking is to do:

#define _local_irq_disable() ......

#define local_irq_disable() _local_irq_disable.

The callers would still use local_irq_disable() and the code would be
consistent with x86.

Cheers,


--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.