[RFC PATCH 00/11] mini-os: xenbus changes for rump kernels

[RFC PATCH 00/11] mini-os: xenbus changes for rump kernels

[Ian Jackson]

I have been working on a facility for the NetBSD-based rump kernels to
allow programs to make xenstore queries using libxenstore.  IMO the
best way to do this is to provide a working /dev/xen/xenbus.  I have
achieved this, and will be submitting the results to the rump kernel
project.

However, mini-os's xenbus driver was not suited to properly
multiplexing and ordering requests from multiple callers, and some of
the mini-os headers were a bit hostile towards the NetBSD kernel
environment the see when they are #included from a rump kernel device
driver component.  So I had to make some changes to minios.

The rump kernel project has its own fork of minios (although the
divergence is reasonably limited).  So my changes to minios are
actually changes to rumpuser-xen's fork of minios.

But before I ask the rump kernel developers to pull my nontrivial
changes, I felt I should give the Xen community a chance to object.
Hence this RFC series.

The code including my complete working branch of rumpuser-xen is also
available here:
    git://xenbits.xen.org/people/iwj/rumpuser-xen.git
in the commits
    base.dev-xen-xenbus.v1..dev-xen-xenbus.v1
or via gitweb:
    http://xenbits.xen.org/gitweb/?p=people/iwj/rumpuser-xen.git;a=log;h=refs/tags/dev-xen-xenbus.v1

The changes I would like Xen community review for are these:

  01/11 mini-os: Make some headers more rumpkernel-friendly
  02/11 mini-os: Provide <mini-os/queue.h>
  03/11 mini-os/xenbus: Add missing locks to xb_write
  04/11 mini-os/xenbus: Change type of xenbus_event_queue
  05/11 mini-os/xenbus: Use MINIOS_LIST for the list of watches
  06/11 mini-os/xenbus: Rename xenbus_events to xenbus_default_watch_queue
  07/11 mini-os/xenbus: Unify watch and reply queues
  08/11 mini-os/xenbus: Expose lower-level interface
  09/11 mini-os/xenbus: Sort out request and watch locking
  10/11 mini-os/xenbus: Provide queue->wakeup hook
  11/11 mini-os/xenbus: Provide xenbus_free

Thanks,
Ian.

[PATCH 01/11] mini-os: Make some headers more rumpkernel-friendly

[Ian Jackson]

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 include/mini-os/os.h           |   15 ++++++++++-----
 include/mini-os/spinlock.h     |    2 ++
 include/mini-os/x86/mm.h       |    4 ++++
 include/mini-os/x86/spinlock.h |    3 +++
 4 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/include/mini-os/os.h b/include/mini-os/os.h
index 9938d31..675a2bf 100644
--- a/include/mini-os/os.h
+++ b/include/mini-os/os.h
@@ -1,17 +1,20 @@
 #ifndef _MINIOS_OS_H_
 #define _MINIOS_OS_H_
 
-#if __GNUC__ == 2 && __GNUC_MINOR__ < 96
-#define __builtin_expect(x, expected_value) (x)
-#endif
+#define smp_processor_id() 0
 #define unlikely(x)  __builtin_expect((x),0)
 #define likely(x)  __builtin_expect((x),1)
 
-#define smp_processor_id() 0
+#include <mini-os/hypervisor.h>
+
+#ifndef __RUMP_KERNEL__
+
+#if __GNUC__ == 2 && __GNUC_MINOR__ < 96
+#define __builtin_expect(x, expected_value) (x)
+#endif
 
 #ifndef __ASSEMBLY__
 #include <mini-os/types.h>
-#include <mini-os/hypervisor.h>
 #include <mini-os/kernel.h>
 #endif
 
@@ -21,4 +24,6 @@
 
 #include <mini-os/machine/os.h>
 
+#endif /* !__RUMP_KERNEL__ */
+
 #endif /* _MINIOS_OS_H_ */
diff --git a/include/mini-os/spinlock.h b/include/mini-os/spinlock.h
index 5e662bb..b87ffe5 100644
--- a/include/mini-os/spinlock.h
+++ b/include/mini-os/spinlock.h
@@ -1,7 +1,9 @@
 #ifndef __MINIOS_ASM_SPINLOCK_H
 #define __MINIOS_ASM_SPINLOCK_H
 
+#ifndef __RUMP_KERNEL__
 #include <mini-os/lib.h>
+#endif
 
 /*
  * Your basic SMP spinlocks, allowing only a single CPU anywhere
diff --git a/include/mini-os/x86/mm.h b/include/mini-os/x86/mm.h
index e41ef74..2a73d08 100644
--- a/include/mini-os/x86/mm.h
+++ b/include/mini-os/x86/mm.h
@@ -36,6 +36,8 @@
 #endif
 #endif
 
+#ifndef __RUMP_KERNEL__
+
 #define L1_FRAME                1
 #define L2_FRAME                2
 #define L3_FRAME                3
@@ -231,4 +233,6 @@ static __inline__ paddr_t machine_to_phys(maddr_t machine)
 pgentry_t *need_pgt(unsigned long addr);
 int mfn_is_ram(unsigned long mfn);
 
+#endif /* !__RUMP_KERNEL__ */
+
 #endif /* _ARCH_MM_H_ */
diff --git a/include/mini-os/x86/spinlock.h b/include/mini-os/x86/spinlock.h
index 4b8faf7..b22f2a4 100644
--- a/include/mini-os/x86/spinlock.h
+++ b/include/mini-os/x86/spinlock.h
@@ -3,7 +3,10 @@
 #ifndef __ARCH_ASM_SPINLOCK_H
 #define __ARCH_ASM_SPINLOCK_H
 
+#ifndef __RUMP_KERNEL__
 #include <mini-os/lib.h>
+#endif
+
 #include "os.h"
 
 
-- 
1.7.10.4

Re: [PATCH 01/11] mini-os: Make some headers more rumpkernel-friendly

[Andrew Cooper]

On 20/06/14 20:04, Ian Jackson wrote:
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  include/mini-os/os.h           |   15 ++++++++++-----
>  include/mini-os/spinlock.h     |    2 ++
>  include/mini-os/x86/mm.h       |    4 ++++
>  include/mini-os/x86/spinlock.h |    3 +++
>  4 files changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/include/mini-os/os.h b/include/mini-os/os.h
> index 9938d31..675a2bf 100644
> --- a/include/mini-os/os.h
> +++ b/include/mini-os/os.h
> @@ -1,17 +1,20 @@
>  #ifndef _MINIOS_OS_H_
>  #define _MINIOS_OS_H_
>  
> -#if __GNUC__ == 2 && __GNUC_MINOR__ < 96
> -#define __builtin_expect(x, expected_value) (x)
> -#endif
> +#define smp_processor_id() 0
>  #define unlikely(x)  __builtin_expect((x),0)
>  #define likely(x)  __builtin_expect((x),1)

Not relevant to the patch itself, but as identified in c/s e5545fb6d0,
these constructs are buggy.

>  
> -#define smp_processor_id() 0
> +#include <mini-os/hypervisor.h>
> +
> +#ifndef __RUMP_KERNEL__
> +
> +#if __GNUC__ == 2 && __GNUC_MINOR__ < 96
> +#define __builtin_expect(x, expected_value) (x)
> +#endif
>  

Do we honestly care about compilers this old?

~Andrew

>  #ifndef __ASSEMBLY__
>  #include <mini-os/types.h>
> -#include <mini-os/hypervisor.h>
>  #include <mini-os/kernel.h>
>  #endif
>  
> @@ -21,4 +24,6 @@
>  
>  #include <mini-os/machine/os.h>
>  
> +#endif /* !__RUMP_KERNEL__ */
> +
>  #endif /* _MINIOS_OS_H_ */
> diff --git a/include/mini-os/spinlock.h b/include/mini-os/spinlock.h
> index 5e662bb..b87ffe5 100644
> --- a/include/mini-os/spinlock.h
> +++ b/include/mini-os/spinlock.h
> @@ -1,7 +1,9 @@
>  #ifndef __MINIOS_ASM_SPINLOCK_H
>  #define __MINIOS_ASM_SPINLOCK_H
>  
> +#ifndef __RUMP_KERNEL__
>  #include <mini-os/lib.h>
> +#endif
>  
>  /*
>   * Your basic SMP spinlocks, allowing only a single CPU anywhere
> diff --git a/include/mini-os/x86/mm.h b/include/mini-os/x86/mm.h
> index e41ef74..2a73d08 100644
> --- a/include/mini-os/x86/mm.h
> +++ b/include/mini-os/x86/mm.h
> @@ -36,6 +36,8 @@
>  #endif
>  #endif
>  
> +#ifndef __RUMP_KERNEL__
> +
>  #define L1_FRAME                1
>  #define L2_FRAME                2
>  #define L3_FRAME                3
> @@ -231,4 +233,6 @@ static __inline__ paddr_t machine_to_phys(maddr_t machine)
>  pgentry_t *need_pgt(unsigned long addr);
>  int mfn_is_ram(unsigned long mfn);
>  
> +#endif /* !__RUMP_KERNEL__ */
> +
>  #endif /* _ARCH_MM_H_ */
> diff --git a/include/mini-os/x86/spinlock.h b/include/mini-os/x86/spinlock.h
> index 4b8faf7..b22f2a4 100644
> --- a/include/mini-os/x86/spinlock.h
> +++ b/include/mini-os/x86/spinlock.h
> @@ -3,7 +3,10 @@
>  #ifndef __ARCH_ASM_SPINLOCK_H
>  #define __ARCH_ASM_SPINLOCK_H
>  
> +#ifndef __RUMP_KERNEL__
>  #include <mini-os/lib.h>
> +#endif
> +
>  #include "os.h"
>  
>  

Re: [PATCH 01/11] mini-os: Make some headers more rumpkernel-friendly

[Ian Jackson]

(Fixed Keir's address.  Sorry, Keir.)

Andrew Cooper writes ("Re: [Xen-devel] [PATCH 01/11] mini-os: Make some headers more rumpkernel-friendly"):
> On 20/06/14 20:04, Ian Jackson wrote:
> > -#if __GNUC__ == 2 && __GNUC_MINOR__ < 96
> > -#define __builtin_expect(x, expected_value) (x)
> > -#endif
> > +#define smp_processor_id() 0
> >  #define unlikely(x)  __builtin_expect((x),0)
> >  #define likely(x)  __builtin_expect((x),1)
> 
> Not relevant to the patch itself, but as identified in c/s e5545fb6d0,
> these constructs are buggy.

Oh, yes.  Joy.  I don't propose to do anything about that in
rumpuser-xen's minios at this stage.

> > -#define smp_processor_id() 0
> > +#include <mini-os/hypervisor.h>
> > +
> > +#ifndef __RUMP_KERNEL__
> > +
> > +#if __GNUC__ == 2 && __GNUC_MINOR__ < 96
> > +#define __builtin_expect(x, expected_value) (x)
> > +#endif
> 
> Do we honestly care about compilers this old?

No.  This is just code motion, though.

Thanks,
Ian.

Re: [PATCH 01/11] mini-os: Make some headers more rumpkernel-friendly

[Samuel Thibault]

Ian Jackson, le Fri 20 Jun 2014 20:04:40 +0100, a écrit :
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
>  include/mini-os/os.h           |   15 ++++++++++-----
>  include/mini-os/spinlock.h     |    2 ++
>  include/mini-os/x86/mm.h       |    4 ++++
>  include/mini-os/x86/spinlock.h |    3 +++
>  4 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/include/mini-os/os.h b/include/mini-os/os.h
> index 9938d31..675a2bf 100644
> --- a/include/mini-os/os.h
> +++ b/include/mini-os/os.h
> @@ -1,17 +1,20 @@
>  #ifndef _MINIOS_OS_H_
>  #define _MINIOS_OS_H_
>  
> -#if __GNUC__ == 2 && __GNUC_MINOR__ < 96
> -#define __builtin_expect(x, expected_value) (x)
> -#endif
> +#define smp_processor_id() 0
>  #define unlikely(x)  __builtin_expect((x),0)
>  #define likely(x)  __builtin_expect((x),1)
>  
> -#define smp_processor_id() 0
> +#include <mini-os/hypervisor.h>
> +
> +#ifndef __RUMP_KERNEL__
> +
> +#if __GNUC__ == 2 && __GNUC_MINOR__ < 96
> +#define __builtin_expect(x, expected_value) (x)
> +#endif
>  
>  #ifndef __ASSEMBLY__
>  #include <mini-os/types.h>
> -#include <mini-os/hypervisor.h>
>  #include <mini-os/kernel.h>
>  #endif
>  
> @@ -21,4 +24,6 @@
>  
>  #include <mini-os/machine/os.h>
>  
> +#endif /* !__RUMP_KERNEL__ */
> +
>  #endif /* _MINIOS_OS_H_ */
> diff --git a/include/mini-os/spinlock.h b/include/mini-os/spinlock.h
> index 5e662bb..b87ffe5 100644
> --- a/include/mini-os/spinlock.h
> +++ b/include/mini-os/spinlock.h
> @@ -1,7 +1,9 @@
>  #ifndef __MINIOS_ASM_SPINLOCK_H
>  #define __MINIOS_ASM_SPINLOCK_H
>  
> +#ifndef __RUMP_KERNEL__
>  #include <mini-os/lib.h>
> +#endif
>  
>  /*
>   * Your basic SMP spinlocks, allowing only a single CPU anywhere
> diff --git a/include/mini-os/x86/mm.h b/include/mini-os/x86/mm.h
> index e41ef74..2a73d08 100644
> --- a/include/mini-os/x86/mm.h
> +++ b/include/mini-os/x86/mm.h
> @@ -36,6 +36,8 @@
>  #endif
>  #endif
>  
> +#ifndef __RUMP_KERNEL__
> +
>  #define L1_FRAME                1
>  #define L2_FRAME                2
>  #define L3_FRAME                3
> @@ -231,4 +233,6 @@ static __inline__ paddr_t machine_to_phys(maddr_t machine)
>  pgentry_t *need_pgt(unsigned long addr);
>  int mfn_is_ram(unsigned long mfn);
>  
> +#endif /* !__RUMP_KERNEL__ */
> +
>  #endif /* _ARCH_MM_H_ */
> diff --git a/include/mini-os/x86/spinlock.h b/include/mini-os/x86/spinlock.h
> index 4b8faf7..b22f2a4 100644
> --- a/include/mini-os/x86/spinlock.h
> +++ b/include/mini-os/x86/spinlock.h
> @@ -3,7 +3,10 @@
>  #ifndef __ARCH_ASM_SPINLOCK_H
>  #define __ARCH_ASM_SPINLOCK_H
>  
> +#ifndef __RUMP_KERNEL__
>  #include <mini-os/lib.h>
> +#endif
> +
>  #include "os.h"
>  
>  
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

-- 
Samuel
/* Halley */

	(Halley's comment.)

[PATCH 02/11] mini-os: Provide <mini-os/queue.h>

[Ian Jackson]

Upstream (xen.git) minios has MINIOS_{,S}{LIST,TAILQ}_* (eg,
MINIOS_LIST_INSERT).  rumpuser-xen has {,S}{LIST,TAILQ}_* (eg,
LIST_INSERT) because it can #include <sys/queue.h>.

We want to try to make this code upstreamable (or at least no less
upstreamable than it already is).

So provide <mini-os/queue.h> which provides MINIOS_* in terms of
<sys/queue.h>.  That allows us to use MINIOS_* in the bulk of minios.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 include/mini-os/queue.h |   74 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)
 create mode 100644 include/mini-os/queue.h

diff --git a/include/mini-os/queue.h b/include/mini-os/queue.h
new file mode 100644
index 0000000..bdf52af
--- /dev/null
+++ b/include/mini-os/queue.h
@@ -0,0 +1,74 @@
+#ifndef MINIOS_QUEUE_H__
+#define MINIOS_QUEUE_H__
+
+#include <sys/queue.h>
+
+#define MINIOS_SLIST_EMPTY		SLIST_EMPTY
+#define MINIOS_SLIST_ENTRY		SLIST_ENTRY
+#define MINIOS_SLIST_FIRST		SLIST_FIRST
+#define MINIOS_SLIST_FOREACH		SLIST_FOREACH
+#define MINIOS_SLIST_FOREACH_SAFE	SLIST_FOREACH_SAFE
+#define MINIOS_SLIST_HEAD		SLIST_HEAD
+#define MINIOS_SLIST_HEAD_INITIALIZER	SLIST_HEAD_INITIALIZER
+#define MINIOS_SLIST_INIT		SLIST_INIT
+#define MINIOS_SLIST_INSERT_AFTER	SLIST_INSERT_AFTER
+#define MINIOS_SLIST_INSERT_HEAD	SLIST_INSERT_HEAD
+#define MINIOS_SLIST_NEXT		SLIST_NEXT
+#define MINIOS_SLIST_REMOVE_AFTER	SLIST_REMOVE_AFTER
+#define MINIOS_SLIST_REMOVE_HEAD	SLIST_REMOVE_HEAD
+#define MINIOS_SLIST_REMOVE		SLIST_REMOVE
+#define MINIOS_SLIST_SWAP		SLIST_SWAP
+#define MINIOS_STAILQ_CONCAT		STAILQ_CONCAT
+#define MINIOS_STAILQ_EMPTY		STAILQ_EMPTY
+#define MINIOS_STAILQ_ENTRY		STAILQ_ENTRY
+#define MINIOS_STAILQ_FIRST		STAILQ_FIRST
+#define MINIOS_STAILQ_FOREACH		STAILQ_FOREACH
+#define MINIOS_STAILQ_FOREACH_SAFE	STAILQ_FOREACH_SAFE
+#define MINIOS_STAILQ_HEAD		STAILQ_HEAD
+#define MINIOS_STAILQ_HEAD_INITIALIZER	STAILQ_HEAD_INITIALIZER
+#define MINIOS_STAILQ_INIT		STAILQ_INIT
+#define MINIOS_STAILQ_INSERT_AFTER	STAILQ_INSERT_AFTER
+#define MINIOS_STAILQ_INSERT_HEAD	STAILQ_INSERT_HEAD
+#define MINIOS_STAILQ_INSERT_TAIL	STAILQ_INSERT_TAIL
+#define MINIOS_STAILQ_LAST		STAILQ_LAST
+#define MINIOS_STAILQ_NEXT		STAILQ_NEXT
+#define MINIOS_STAILQ_REMOVE_AFTER	STAILQ_REMOVE_AFTER
+#define MINIOS_STAILQ_REMOVE_HEAD	STAILQ_REMOVE_HEAD
+#define MINIOS_STAILQ_REMOVE		STAILQ_REMOVE
+#define MINIOS_STAILQ_SWAP		STAILQ_SWAP
+#define MINIOS_LIST_EMPTY		LIST_EMPTY
+#define MINIOS_LIST_ENTRY		LIST_ENTRY
+#define MINIOS_LIST_FIRST		LIST_FIRST
+#define MINIOS_LIST_FOREACH		LIST_FOREACH
+#define MINIOS_LIST_FOREACH_SAFE	LIST_FOREACH_SAFE
+#define MINIOS_LIST_HEAD		LIST_HEAD
+#define MINIOS_LIST_HEAD_INITIALIZER	LIST_HEAD_INITIALIZER
+#define MINIOS_LIST_INIT		LIST_INIT
+#define MINIOS_LIST_INSERT_AFTER	LIST_INSERT_AFTER
+#define MINIOS_LIST_INSERT_BEFORE	LIST_INSERT_BEFORE
+#define MINIOS_LIST_INSERT_HEAD		LIST_INSERT_HEAD
+#define MINIOS_LIST_NEXT		LIST_NEXT
+#define MINIOS_LIST_REMOVE		LIST_REMOVE
+#define MINIOS_LIST_SWAP		LIST_SWAP
+#define MINIOS_TAILQ_CONCAT		TAILQ_CONCAT
+#define MINIOS_TAILQ_EMPTY		TAILQ_EMPTY
+#define MINIOS_TAILQ_ENTRY		TAILQ_ENTRY
+#define MINIOS_TAILQ_FIRST		TAILQ_FIRST
+#define MINIOS_TAILQ_FOREACH		TAILQ_FOREACH
+#define MINIOS_TAILQ_FOREACH_SAFE	TAILQ_FOREACH_SAFE
+#define MINIOS_TAILQ_FOREACH_REVERSE	TAILQ_FOREACH_REVERSE
+#define MINIOS_TAILQ_FOREACH_REVERSE_SAFE TAILQ_FOREACH_REVERSE_SAFE
+#define MINIOS_TAILQ_HEAD		TAILQ_HEAD
+#define MINIOS_TAILQ_HEAD_INITIALIZER	TAILQ_HEAD_INITIALIZER
+#define MINIOS_TAILQ_INIT		TAILQ_INIT
+#define MINIOS_TAILQ_INSERT_AFTER	TAILQ_INSERT_AFTER
+#define MINIOS_TAILQ_INSERT_BEFORE	TAILQ_INSERT_BEFORE
+#define MINIOS_TAILQ_INSERT_HEAD	TAILQ_INSERT_HEAD
+#define MINIOS_TAILQ_INSERT_TAIL	TAILQ_INSERT_TAIL
+#define MINIOS_TAILQ_LAST		TAILQ_LAST
+#define MINIOS_TAILQ_NEXT		TAILQ_NEXT
+#define MINIOS_TAILQ_PREV		TAILQ_PREV
+#define MINIOS_TAILQ_REMOVE		TAILQ_REMOVE
+#define MINIOS_TAILQ_SWAP		TAILQ_SWAP
+
+#endif /* MINIOS_QUEUE_H__ */
-- 
1.7.10.4

Re: [PATCH 02/11] mini-os: Provide <mini-os/queue.h>

[Samuel Thibault]

Ian Jackson, le Fri 20 Jun 2014 20:04:41 +0100, a écrit :
> Upstream (xen.git) minios has MINIOS_{,S}{LIST,TAILQ}_* (eg,
> MINIOS_LIST_INSERT).  rumpuser-xen has {,S}{LIST,TAILQ}_* (eg,
> LIST_INSERT) because it can #include <sys/queue.h>.
> 
> We want to try to make this code upstreamable (or at least no less
> upstreamable than it already is).
> 
> So provide <mini-os/queue.h> which provides MINIOS_* in terms of
> <sys/queue.h>.  That allows us to use MINIOS_* in the bulk of minios.

We had prefixed those with MINIOS_ to avoid clashing with applications'
own lists implementations. Doing the #define as you propose should be
fine indeed.

> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
>  include/mini-os/queue.h |   74 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 74 insertions(+)
>  create mode 100644 include/mini-os/queue.h
> 
> diff --git a/include/mini-os/queue.h b/include/mini-os/queue.h
> new file mode 100644
> index 0000000..bdf52af
> --- /dev/null
> +++ b/include/mini-os/queue.h
> @@ -0,0 +1,74 @@
> +#ifndef MINIOS_QUEUE_H__
> +#define MINIOS_QUEUE_H__
> +
> +#include <sys/queue.h>
> +
> +#define MINIOS_SLIST_EMPTY		SLIST_EMPTY
> +#define MINIOS_SLIST_ENTRY		SLIST_ENTRY
> +#define MINIOS_SLIST_FIRST		SLIST_FIRST
> +#define MINIOS_SLIST_FOREACH		SLIST_FOREACH
> +#define MINIOS_SLIST_FOREACH_SAFE	SLIST_FOREACH_SAFE
> +#define MINIOS_SLIST_HEAD		SLIST_HEAD
> +#define MINIOS_SLIST_HEAD_INITIALIZER	SLIST_HEAD_INITIALIZER
> +#define MINIOS_SLIST_INIT		SLIST_INIT
> +#define MINIOS_SLIST_INSERT_AFTER	SLIST_INSERT_AFTER
> +#define MINIOS_SLIST_INSERT_HEAD	SLIST_INSERT_HEAD
> +#define MINIOS_SLIST_NEXT		SLIST_NEXT
> +#define MINIOS_SLIST_REMOVE_AFTER	SLIST_REMOVE_AFTER
> +#define MINIOS_SLIST_REMOVE_HEAD	SLIST_REMOVE_HEAD
> +#define MINIOS_SLIST_REMOVE		SLIST_REMOVE
> +#define MINIOS_SLIST_SWAP		SLIST_SWAP
> +#define MINIOS_STAILQ_CONCAT		STAILQ_CONCAT
> +#define MINIOS_STAILQ_EMPTY		STAILQ_EMPTY
> +#define MINIOS_STAILQ_ENTRY		STAILQ_ENTRY
> +#define MINIOS_STAILQ_FIRST		STAILQ_FIRST
> +#define MINIOS_STAILQ_FOREACH		STAILQ_FOREACH
> +#define MINIOS_STAILQ_FOREACH_SAFE	STAILQ_FOREACH_SAFE
> +#define MINIOS_STAILQ_HEAD		STAILQ_HEAD
> +#define MINIOS_STAILQ_HEAD_INITIALIZER	STAILQ_HEAD_INITIALIZER
> +#define MINIOS_STAILQ_INIT		STAILQ_INIT
> +#define MINIOS_STAILQ_INSERT_AFTER	STAILQ_INSERT_AFTER
> +#define MINIOS_STAILQ_INSERT_HEAD	STAILQ_INSERT_HEAD
> +#define MINIOS_STAILQ_INSERT_TAIL	STAILQ_INSERT_TAIL
> +#define MINIOS_STAILQ_LAST		STAILQ_LAST
> +#define MINIOS_STAILQ_NEXT		STAILQ_NEXT
> +#define MINIOS_STAILQ_REMOVE_AFTER	STAILQ_REMOVE_AFTER
> +#define MINIOS_STAILQ_REMOVE_HEAD	STAILQ_REMOVE_HEAD
> +#define MINIOS_STAILQ_REMOVE		STAILQ_REMOVE
> +#define MINIOS_STAILQ_SWAP		STAILQ_SWAP
> +#define MINIOS_LIST_EMPTY		LIST_EMPTY
> +#define MINIOS_LIST_ENTRY		LIST_ENTRY
> +#define MINIOS_LIST_FIRST		LIST_FIRST
> +#define MINIOS_LIST_FOREACH		LIST_FOREACH
> +#define MINIOS_LIST_FOREACH_SAFE	LIST_FOREACH_SAFE
> +#define MINIOS_LIST_HEAD		LIST_HEAD
> +#define MINIOS_LIST_HEAD_INITIALIZER	LIST_HEAD_INITIALIZER
> +#define MINIOS_LIST_INIT		LIST_INIT
> +#define MINIOS_LIST_INSERT_AFTER	LIST_INSERT_AFTER
> +#define MINIOS_LIST_INSERT_BEFORE	LIST_INSERT_BEFORE
> +#define MINIOS_LIST_INSERT_HEAD		LIST_INSERT_HEAD
> +#define MINIOS_LIST_NEXT		LIST_NEXT
> +#define MINIOS_LIST_REMOVE		LIST_REMOVE
> +#define MINIOS_LIST_SWAP		LIST_SWAP
> +#define MINIOS_TAILQ_CONCAT		TAILQ_CONCAT
> +#define MINIOS_TAILQ_EMPTY		TAILQ_EMPTY
> +#define MINIOS_TAILQ_ENTRY		TAILQ_ENTRY
> +#define MINIOS_TAILQ_FIRST		TAILQ_FIRST
> +#define MINIOS_TAILQ_FOREACH		TAILQ_FOREACH
> +#define MINIOS_TAILQ_FOREACH_SAFE	TAILQ_FOREACH_SAFE
> +#define MINIOS_TAILQ_FOREACH_REVERSE	TAILQ_FOREACH_REVERSE
> +#define MINIOS_TAILQ_FOREACH_REVERSE_SAFE TAILQ_FOREACH_REVERSE_SAFE
> +#define MINIOS_TAILQ_HEAD		TAILQ_HEAD
> +#define MINIOS_TAILQ_HEAD_INITIALIZER	TAILQ_HEAD_INITIALIZER
> +#define MINIOS_TAILQ_INIT		TAILQ_INIT
> +#define MINIOS_TAILQ_INSERT_AFTER	TAILQ_INSERT_AFTER
> +#define MINIOS_TAILQ_INSERT_BEFORE	TAILQ_INSERT_BEFORE
> +#define MINIOS_TAILQ_INSERT_HEAD	TAILQ_INSERT_HEAD
> +#define MINIOS_TAILQ_INSERT_TAIL	TAILQ_INSERT_TAIL
> +#define MINIOS_TAILQ_LAST		TAILQ_LAST
> +#define MINIOS_TAILQ_NEXT		TAILQ_NEXT
> +#define MINIOS_TAILQ_PREV		TAILQ_PREV
> +#define MINIOS_TAILQ_REMOVE		TAILQ_REMOVE
> +#define MINIOS_TAILQ_SWAP		TAILQ_SWAP
> +
> +#endif /* MINIOS_QUEUE_H__ */
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

-- 
Samuel
<N> je déteste import
<N> parce que lorsque tu fais du python et que tu oublies le #!/bin/env python et que tu mets le fichier exécutable
<N> import est exécuté
 -+- #ens-mim - pourquoi mon script python change le curseur de la souris ?! -+-

[PATCH 03/11] mini-os/xenbus: Add missing locks to xb_write

[Ian Jackson]

xb_write was missing any locking against concurrent calls.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 xen/xenbus/xenbus.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/xen/xenbus/xenbus.c b/xen/xenbus/xenbus.c
index 0958604..a06dc30 100644
--- a/xen/xenbus/xenbus.c
+++ b/xen/xenbus/xenbus.c
@@ -45,6 +45,7 @@
 
 static struct xenstore_domain_interface *xenstore_buf;
 static DECLARE_WAIT_QUEUE_HEAD(xb_waitq);
+static spinlock_t xb_lock = SPIN_LOCK_UNLOCKED; /* protects xenbus req ring */
 DECLARE_WAIT_QUEUE_HEAD(xenbus_watch_queue);
 
 xenbus_event_queue xenbus_events;
@@ -372,6 +373,8 @@ static void xb_write(int type, int req_id, xenbus_transaction_t trans_id,
     cur_req = &header_req;
 
     BUG_ON(len > XENSTORE_RING_SIZE);
+
+    spin_lock(&xb_lock);
     /* Wait for the ring to drain to the point where we can send the
        message. */
     prod = xenstore_buf->req_prod;
@@ -380,9 +383,11 @@ static void xb_write(int type, int req_id, xenbus_transaction_t trans_id,
         /* Wait for there to be space on the ring */
         DEBUG("prod %d, len %d, cons %d, size %d; waiting.\n",
                 prod, len, xenstore_buf->req_cons, XENSTORE_RING_SIZE);
+        spin_unlock(&xb_lock);
         wait_event(xb_waitq,
                 xenstore_buf->req_prod + len - xenstore_buf->req_cons <=
                 XENSTORE_RING_SIZE);
+        spin_lock(&xb_lock);
         DEBUG("Back from wait.\n");
         prod = xenstore_buf->req_prod;
     }
@@ -419,6 +424,7 @@ static void xb_write(int type, int req_id, xenbus_transaction_t trans_id,
     wmb();
 
     xenstore_buf->req_prod += len;
+    spin_unlock(&xb_lock);
 
     /* Send evtchn to notify remote */
     notify_remote_via_evtchn(start_info.store_evtchn);
-- 
1.7.10.4

Re: [PATCH 03/11] mini-os/xenbus: Add missing locks to xb_write

[Samuel Thibault]

Ian Jackson, le Fri 20 Jun 2014 20:04:42 +0100, a écrit :
> xb_write was missing any locking against concurrent calls.

Well, yes, in its current form mini-os was not really meant to run with
multiple processors. There are probably quite other issues like that in
there.

> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
>  xen/xenbus/xenbus.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/xen/xenbus/xenbus.c b/xen/xenbus/xenbus.c
> index 0958604..a06dc30 100644
> --- a/xen/xenbus/xenbus.c
> +++ b/xen/xenbus/xenbus.c
> @@ -45,6 +45,7 @@
>  
>  static struct xenstore_domain_interface *xenstore_buf;
>  static DECLARE_WAIT_QUEUE_HEAD(xb_waitq);
> +static spinlock_t xb_lock = SPIN_LOCK_UNLOCKED; /* protects xenbus req ring */
>  DECLARE_WAIT_QUEUE_HEAD(xenbus_watch_queue);
>  
>  xenbus_event_queue xenbus_events;
> @@ -372,6 +373,8 @@ static void xb_write(int type, int req_id, xenbus_transaction_t trans_id,
>      cur_req = &header_req;
>  
>      BUG_ON(len > XENSTORE_RING_SIZE);
> +
> +    spin_lock(&xb_lock);
>      /* Wait for the ring to drain to the point where we can send the
>         message. */
>      prod = xenstore_buf->req_prod;
> @@ -380,9 +383,11 @@ static void xb_write(int type, int req_id, xenbus_transaction_t trans_id,
>          /* Wait for there to be space on the ring */
>          DEBUG("prod %d, len %d, cons %d, size %d; waiting.\n",
>                  prod, len, xenstore_buf->req_cons, XENSTORE_RING_SIZE);
> +        spin_unlock(&xb_lock);
>          wait_event(xb_waitq,
>                  xenstore_buf->req_prod + len - xenstore_buf->req_cons <=
>                  XENSTORE_RING_SIZE);
> +        spin_lock(&xb_lock);
>          DEBUG("Back from wait.\n");
>          prod = xenstore_buf->req_prod;
>      }
> @@ -419,6 +424,7 @@ static void xb_write(int type, int req_id, xenbus_transaction_t trans_id,
>      wmb();
>  
>      xenstore_buf->req_prod += len;
> +    spin_unlock(&xb_lock);
>  
>      /* Send evtchn to notify remote */
>      notify_remote_via_evtchn(start_info.store_evtchn);
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

-- 
Samuel
<N>  sl  -  display animations aimed to correct users who accidentally enter
<N>        sl instead of ls.

Re: [PATCH 03/11] mini-os/xenbus: Add missing locks to xb_write

[Ian Jackson]

Thanks for your reviews.  FYI this series has already been applied to
the NetBSD rump kernels' fork of mini-os. I still intend to merge
these together at some point, but as I say that won't be very soon.

Samuel Thibault writes ("Re: [Xen-devel] [PATCH 03/11] mini-os/xenbus: Add missing locks to xb_write"):
> Ian Jackson, le Fri 20 Jun 2014 20:04:42 +0100, a écrit :
> > xb_write was missing any locking against concurrent calls.
> 
> Well, yes, in its current form mini-os was not really meant to run with
> multiple processors. There are probably quite other issues like that in
> there.

Right.  My approach has been that as I'm looking at this code in
detail and restructuring it, I ought to sort out the locking.  It is
much easier to add locking as one goes along when working on a piece
of code, than to come along later and try to retrofit it.

Thanks,
Ian.

Re: [PATCH 03/11] mini-os/xenbus: Add missing locks to xb_write

[Samuel Thibault]

Ian Jackson, le Mon 30 Jun 2014 13:47:46 +0100, a écrit :
> > Well, yes, in its current form mini-os was not really meant to run with
> > multiple processors. There are probably quite other issues like that in
> > there.
> 
> Right.  My approach has been that as I'm looking at this code in
> detail and restructuring it, I ought to sort out the locking.  It is
> much easier to add locking as one goes along when working on a piece
> of code, than to come along later and try to retrofit it.

Indeed :)

Samuel

[PATCH 04/11] mini-os/xenbus: Change type of xenbus_event_queue

[Ian Jackson]

We change xenbus_event_queue from a pointer typedef to a struct, for
two reasons:

1. In a moment we are going to want to extend this struct to include
   a minios scheduler wait queue.

2. We can replace the open-coded list with a MINIOS_STAILQ.

All the xenbus users need to call the new initialisation function
instead of simply initialising the struct to NULL, and have their
parameter type changed.

There is a functional side-effect: now we are using a tail queue,
rather than a tailless queue, we insert events at the end rather than
the beginning.  So watch events now come out in chronological order,
rather than their order possibly being scrambled in the queue.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 include/mini-os/console.h |    2 +-
 include/mini-os/xenbus.h  |   22 +++++++++++++---------
 xen/blkfront.c            |    4 ++--
 xen/console/xenbus.c      |    2 +-
 xen/netfront.c            |    4 ++--
 xen/pcifront.c            |    7 ++++---
 xen/xenbus/xenbus.c       |   31 +++++++++++++++++++------------
 7 files changed, 42 insertions(+), 30 deletions(-)

diff --git a/include/mini-os/console.h b/include/mini-os/console.h
index 1b04b13..49990b9 100644
--- a/include/mini-os/console.h
+++ b/include/mini-os/console.h
@@ -56,7 +56,7 @@ struct consfront_dev {
     char *nodename;
     char *backend;
 
-    xenbus_event_queue events;
+    struct xenbus_event_queue events;
 
 };
 
diff --git a/include/mini-os/xenbus.h b/include/mini-os/xenbus.h
index f3594cb..4dad4c8 100644
--- a/include/mini-os/xenbus.h
+++ b/include/mini-os/xenbus.h
@@ -2,6 +2,7 @@
 #define MINIOS_XENBUS_H__
 
 #include <xen/io/xenbus.h>
+#include <mini-os/queue.h>
 
 typedef unsigned long xenbus_transaction_t;
 #define XBT_NIL ((xenbus_transaction_t)0)
@@ -25,22 +26,25 @@ struct xenbus_event {
     /* Keep these two as this for xs.c */
     char *path;
     char *token;
-    struct xenbus_event *next;
+    MINIOS_STAILQ_ENTRY(xenbus_event) entry;
 };
-typedef struct xenbus_event *xenbus_event_queue;
+struct xenbus_event_queue {
+    MINIOS_STAILQ_HEAD(, xenbus_event) events;
+};
+
+void xenbus_event_queue_init(struct xenbus_event_queue *queue);
 
-char *xenbus_watch_path_token(xenbus_transaction_t xbt, const char *path, const char *token, xenbus_event_queue *events);
+char *xenbus_watch_path_token(xenbus_transaction_t xbt, const char *path, const char *token, struct xenbus_event_queue *events);
 char *xenbus_unwatch_path_token(xenbus_transaction_t xbt, const char *path, const char *token);
-extern struct wait_queue_head xenbus_watch_queue;
-void xenbus_wait_for_watch(xenbus_event_queue *queue);
-char **xenbus_wait_for_watch_return(xenbus_event_queue *queue);
-char* xenbus_wait_for_value(const char *path, const char *value, xenbus_event_queue *queue);
-char *xenbus_wait_for_state_change(const char* path, XenbusState *state, xenbus_event_queue *queue);
+void xenbus_wait_for_watch(struct xenbus_event_queue *queue);
+char **xenbus_wait_for_watch_return(struct xenbus_event_queue *queue);
+char* xenbus_wait_for_value(const char *path, const char *value, struct xenbus_event_queue *queue);
+char *xenbus_wait_for_state_change(const char* path, XenbusState *state, struct xenbus_event_queue *queue);
 char *xenbus_switch_state(xenbus_transaction_t xbt, const char* path, XenbusState state);
 
 /* When no token is provided, use a global queue. */
 #define XENBUS_WATCH_PATH_TOKEN "xenbus_watch_path"
-extern xenbus_event_queue xenbus_events;
+extern struct xenbus_event_queue xenbus_events;
 #define xenbus_watch_path(xbt, path) xenbus_watch_path_token(xbt, path, XENBUS_WATCH_PATH_TOKEN, NULL)
 #define xenbus_unwatch_path(xbt, path) xenbus_unwatch_path_token(xbt, path, XENBUS_WATCH_PATH_TOKEN)
 
diff --git a/xen/blkfront.c b/xen/blkfront.c
index 347c68c..a5da53d 100644
--- a/xen/blkfront.c
+++ b/xen/blkfront.c
@@ -47,7 +47,7 @@ struct blkfront_dev {
     char *backend;
     struct blkfront_info info;
 
-    xenbus_event_queue events;
+    struct xenbus_event_queue events;
 
 };
 
@@ -103,7 +103,7 @@ struct blkfront_dev *init_blkfront(char *_nodename, struct blkfront_info *info)
 
     dev->ring_ref = gnttab_grant_access(dev->dom,virt_to_mfn(s),0);
 
-    dev->events = NULL;
+    xenbus_event_queue_init(&dev->events);
 
 again:
     err = xenbus_transaction_start(&xbt);
diff --git a/xen/console/xenbus.c b/xen/console/xenbus.c
index d848af8..a164314 100644
--- a/xen/console/xenbus.c
+++ b/xen/console/xenbus.c
@@ -96,7 +96,7 @@ struct consfront_dev *init_consfront(char *_nodename)
     memset(dev->ring, 0, PAGE_SIZE);
     dev->ring_ref = gnttab_grant_access(dev->dom, virt_to_mfn(dev->ring), 0);
 
-    dev->events = NULL;
+    xenbus_event_queue_init(&dev->events);
 
 again:
     err = xenbus_transaction_start(&xbt);
diff --git a/xen/netfront.c b/xen/netfront.c
index 9e17a86..a89a0dd 100644
--- a/xen/netfront.c
+++ b/xen/netfront.c
@@ -51,7 +51,7 @@ struct netfront_dev {
     char *backend;
     char *mac;
 
-    xenbus_event_queue events;
+    struct xenbus_event_queue events;
 
 
     void (*netif_rx)(struct netfront_dev *, unsigned char* data, int len);
@@ -313,7 +313,7 @@ struct netfront_dev *init_netfront(char *_nodename, void (*thenetif_rx)(struct n
 
     dev->netif_rx = thenetif_rx;
 
-    dev->events = NULL;
+    xenbus_event_queue_init(&dev->events);
 
 again:
     err = xenbus_transaction_start(&xbt);
diff --git a/xen/pcifront.c b/xen/pcifront.c
index 6f52b77..7bd3788 100644
--- a/xen/pcifront.c
+++ b/xen/pcifront.c
@@ -31,7 +31,7 @@ struct pcifront_dev {
     char *nodename;
     char *backend;
 
-    xenbus_event_queue events;
+    struct xenbus_event_queue events;
 };
 
 void pcifront_handler(evtchn_port_t port, struct pt_regs *regs, void *data)
@@ -64,7 +64,8 @@ void pcifront_watches(void *opaque)
     char* nodename = opaque ? opaque : "device/pci/0";
     char path[strlen(nodename) + 9];
     char fe_state[strlen(nodename) + 7];
-    xenbus_event_queue events = NULL;
+    struct xenbus_event_queue events;
+    xenbus_event_queue_init(&events);
 
     snprintf(path, sizeof(path), "%s/backend", nodename);
     snprintf(fe_state, sizeof(fe_state), "%s/state", nodename);
@@ -174,7 +175,7 @@ struct pcifront_dev *init_pcifront(char *_nodename)
 
     dev->info_ref = gnttab_grant_access(dev->dom,virt_to_mfn(dev->info),0);
 
-    dev->events = NULL;
+    xenbus_event_queue_init(&dev->events);
 
 again:
     err = xenbus_transaction_start(&xbt);
diff --git a/xen/xenbus/xenbus.c b/xen/xenbus/xenbus.c
index a06dc30..8a14c3b 100644
--- a/xen/xenbus/xenbus.c
+++ b/xen/xenbus/xenbus.c
@@ -48,10 +48,10 @@ static DECLARE_WAIT_QUEUE_HEAD(xb_waitq);
 static spinlock_t xb_lock = SPIN_LOCK_UNLOCKED; /* protects xenbus req ring */
 DECLARE_WAIT_QUEUE_HEAD(xenbus_watch_queue);
 
-xenbus_event_queue xenbus_events;
+struct xenbus_event_queue xenbus_events;
 static struct watch {
     char *token;
-    xenbus_event_queue *events;
+    struct xenbus_event_queue *events;
     struct watch *next;
 } *watches;
 struct xenbus_req_info 
@@ -61,6 +61,13 @@ struct xenbus_req_info
     void *reply;
 };
 
+
+void xenbus_event_queue_init(struct xenbus_event_queue *queue)
+{
+    MINIOS_STAILQ_INIT(&queue->events);
+}
+
+
 #define NR_REQS 32
 static struct xenbus_req_info req_info[NR_REQS];
 
@@ -78,22 +85,22 @@ static void memcpy_from_ring(const void *Ring,
     memcpy(dest + c1, ring, c2);
 }
 
-char **xenbus_wait_for_watch_return(xenbus_event_queue *queue)
+char **xenbus_wait_for_watch_return(struct xenbus_event_queue *queue)
 {
     struct xenbus_event *event;
     DEFINE_WAIT(w);
     if (!queue)
         queue = &xenbus_events;
-    while (!(event = *queue)) {
+    while (!(event = MINIOS_STAILQ_FIRST(&queue->events))) {
         add_waiter(w, xenbus_watch_queue);
         schedule();
     }
     remove_waiter(w, xenbus_watch_queue);
-    *queue = event->next;
+    MINIOS_STAILQ_REMOVE_HEAD(&queue->events, entry);
     return &event->path;
 }
 
-void xenbus_wait_for_watch(xenbus_event_queue *queue)
+void xenbus_wait_for_watch(struct xenbus_event_queue *queue)
 {
     char **ret;
     if (!queue)
@@ -105,7 +112,7 @@ void xenbus_wait_for_watch(xenbus_event_queue *queue)
         printk("unexpected path returned by watch\n");
 }
 
-char* xenbus_wait_for_value(const char* path, const char* value, xenbus_event_queue *queue)
+char* xenbus_wait_for_value(const char* path, const char* value, struct xenbus_event_queue *queue)
 {
     if (!queue)
         queue = &xenbus_events;
@@ -168,7 +175,7 @@ exit:
     return msg;
 }
 
-char *xenbus_wait_for_state_change(const char* path, XenbusState *state, xenbus_event_queue *queue)
+char *xenbus_wait_for_state_change(const char* path, XenbusState *state, struct xenbus_event_queue *queue)
 {
     if (!queue)
         queue = &xenbus_events;
@@ -227,7 +234,7 @@ static void xenbus_thread_func(void *ign)
             if(msg.type == XS_WATCH_EVENT)
             {
 		struct xenbus_event *event = malloc(sizeof(*event) + msg.len);
-                xenbus_event_queue *events = NULL;
+                struct xenbus_event_queue *events = NULL;
 		char *data = (char*)event + sizeof(*event);
                 struct watch *watch;
 
@@ -248,8 +255,7 @@ static void xenbus_thread_func(void *ign)
                     }
 
                 if (events) {
-                    event->next = *events;
-                    *events = event;
+                    MINIOS_STAILQ_INSERT_TAIL(&events->events, event, entry);
                     wake_up(&xenbus_watch_queue);
                 } else {
                     printk("unexpected watch token %s\n", event->token);
@@ -332,6 +338,7 @@ void init_xenbus(void)
 {
     int err;
     DEBUG("init_xenbus called.\n");
+    xenbus_event_queue_init(&xenbus_events);
     xenstore_buf = mfn_to_virt(start_info.store_mfn);
     create_thread("xenstore", xenbus_thread_func, NULL);
     DEBUG("buf at %p.\n", xenstore_buf);
@@ -561,7 +568,7 @@ char *xenbus_write(xenbus_transaction_t xbt, const char *path, const char *value
     return NULL;
 }
 
-char* xenbus_watch_path_token( xenbus_transaction_t xbt, const char *path, const char *token, xenbus_event_queue *events)
+char* xenbus_watch_path_token( xenbus_transaction_t xbt, const char *path, const char *token, struct xenbus_event_queue *events)
 {
     struct xsd_sockmsg *rep;
 
-- 
1.7.10.4

Re: [PATCH 04/11] mini-os/xenbus: Change type of xenbus_event_queue

[Samuel Thibault]

Ian Jackson, le Fri 20 Jun 2014 20:04:43 +0100, a écrit :
> We change xenbus_event_queue from a pointer typedef to a struct, for
> two reasons:
> 
> 1. In a moment we are going to want to extend this struct to include
>    a minios scheduler wait queue.
> 
> 2. We can replace the open-coded list with a MINIOS_STAILQ.
> 
> All the xenbus users need to call the new initialisation function
> instead of simply initialising the struct to NULL, and have their
> parameter type changed.
> 
> There is a functional side-effect: now we are using a tail queue,
> rather than a tailless queue, we insert events at the end rather than
> the beginning.  So watch events now come out in chronological order,
> rather than their order possibly being scrambled in the queue.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
>  include/mini-os/console.h |    2 +-
>  include/mini-os/xenbus.h  |   22 +++++++++++++---------
>  xen/blkfront.c            |    4 ++--
>  xen/console/xenbus.c      |    2 +-
>  xen/netfront.c            |    4 ++--
>  xen/pcifront.c            |    7 ++++---
>  xen/xenbus/xenbus.c       |   31 +++++++++++++++++++------------
>  7 files changed, 42 insertions(+), 30 deletions(-)
> 
> diff --git a/include/mini-os/console.h b/include/mini-os/console.h
> index 1b04b13..49990b9 100644
> --- a/include/mini-os/console.h
> +++ b/include/mini-os/console.h
> @@ -56,7 +56,7 @@ struct consfront_dev {
>      char *nodename;
>      char *backend;
>  
> -    xenbus_event_queue events;
> +    struct xenbus_event_queue events;
>  
>  };
>  
> diff --git a/include/mini-os/xenbus.h b/include/mini-os/xenbus.h
> index f3594cb..4dad4c8 100644
> --- a/include/mini-os/xenbus.h
> +++ b/include/mini-os/xenbus.h
> @@ -2,6 +2,7 @@
>  #define MINIOS_XENBUS_H__
>  
>  #include <xen/io/xenbus.h>
> +#include <mini-os/queue.h>
>  
>  typedef unsigned long xenbus_transaction_t;
>  #define XBT_NIL ((xenbus_transaction_t)0)
> @@ -25,22 +26,25 @@ struct xenbus_event {
>      /* Keep these two as this for xs.c */
>      char *path;
>      char *token;
> -    struct xenbus_event *next;
> +    MINIOS_STAILQ_ENTRY(xenbus_event) entry;
>  };
> -typedef struct xenbus_event *xenbus_event_queue;
> +struct xenbus_event_queue {
> +    MINIOS_STAILQ_HEAD(, xenbus_event) events;
> +};
> +
> +void xenbus_event_queue_init(struct xenbus_event_queue *queue);
>  
> -char *xenbus_watch_path_token(xenbus_transaction_t xbt, const char *path, const char *token, xenbus_event_queue *events);
> +char *xenbus_watch_path_token(xenbus_transaction_t xbt, const char *path, const char *token, struct xenbus_event_queue *events);
>  char *xenbus_unwatch_path_token(xenbus_transaction_t xbt, const char *path, const char *token);
> -extern struct wait_queue_head xenbus_watch_queue;
> -void xenbus_wait_for_watch(xenbus_event_queue *queue);
> -char **xenbus_wait_for_watch_return(xenbus_event_queue *queue);
> -char* xenbus_wait_for_value(const char *path, const char *value, xenbus_event_queue *queue);
> -char *xenbus_wait_for_state_change(const char* path, XenbusState *state, xenbus_event_queue *queue);
> +void xenbus_wait_for_watch(struct xenbus_event_queue *queue);
> +char **xenbus_wait_for_watch_return(struct xenbus_event_queue *queue);
> +char* xenbus_wait_for_value(const char *path, const char *value, struct xenbus_event_queue *queue);
> +char *xenbus_wait_for_state_change(const char* path, XenbusState *state, struct xenbus_event_queue *queue);
>  char *xenbus_switch_state(xenbus_transaction_t xbt, const char* path, XenbusState state);
>  
>  /* When no token is provided, use a global queue. */
>  #define XENBUS_WATCH_PATH_TOKEN "xenbus_watch_path"
> -extern xenbus_event_queue xenbus_events;
> +extern struct xenbus_event_queue xenbus_events;
>  #define xenbus_watch_path(xbt, path) xenbus_watch_path_token(xbt, path, XENBUS_WATCH_PATH_TOKEN, NULL)
>  #define xenbus_unwatch_path(xbt, path) xenbus_unwatch_path_token(xbt, path, XENBUS_WATCH_PATH_TOKEN)
>  
> diff --git a/xen/blkfront.c b/xen/blkfront.c
> index 347c68c..a5da53d 100644
> --- a/xen/blkfront.c
> +++ b/xen/blkfront.c
> @@ -47,7 +47,7 @@ struct blkfront_dev {
>      char *backend;
>      struct blkfront_info info;
>  
> -    xenbus_event_queue events;
> +    struct xenbus_event_queue events;
>  
>  };
>  
> @@ -103,7 +103,7 @@ struct blkfront_dev *init_blkfront(char *_nodename, struct blkfront_info *info)
>  
>      dev->ring_ref = gnttab_grant_access(dev->dom,virt_to_mfn(s),0);
>  
> -    dev->events = NULL;
> +    xenbus_event_queue_init(&dev->events);
>  
>  again:
>      err = xenbus_transaction_start(&xbt);
> diff --git a/xen/console/xenbus.c b/xen/console/xenbus.c
> index d848af8..a164314 100644
> --- a/xen/console/xenbus.c
> +++ b/xen/console/xenbus.c
> @@ -96,7 +96,7 @@ struct consfront_dev *init_consfront(char *_nodename)
>      memset(dev->ring, 0, PAGE_SIZE);
>      dev->ring_ref = gnttab_grant_access(dev->dom, virt_to_mfn(dev->ring), 0);
>  
> -    dev->events = NULL;
> +    xenbus_event_queue_init(&dev->events);
>  
>  again:
>      err = xenbus_transaction_start(&xbt);
> diff --git a/xen/netfront.c b/xen/netfront.c
> index 9e17a86..a89a0dd 100644
> --- a/xen/netfront.c
> +++ b/xen/netfront.c
> @@ -51,7 +51,7 @@ struct netfront_dev {
>      char *backend;
>      char *mac;
>  
> -    xenbus_event_queue events;
> +    struct xenbus_event_queue events;
>  
>  
>      void (*netif_rx)(struct netfront_dev *, unsigned char* data, int len);
> @@ -313,7 +313,7 @@ struct netfront_dev *init_netfront(char *_nodename, void (*thenetif_rx)(struct n
>  
>      dev->netif_rx = thenetif_rx;
>  
> -    dev->events = NULL;
> +    xenbus_event_queue_init(&dev->events);
>  
>  again:
>      err = xenbus_transaction_start(&xbt);
> diff --git a/xen/pcifront.c b/xen/pcifront.c
> index 6f52b77..7bd3788 100644
> --- a/xen/pcifront.c
> +++ b/xen/pcifront.c
> @@ -31,7 +31,7 @@ struct pcifront_dev {
>      char *nodename;
>      char *backend;
>  
> -    xenbus_event_queue events;
> +    struct xenbus_event_queue events;
>  };
>  
>  void pcifront_handler(evtchn_port_t port, struct pt_regs *regs, void *data)
> @@ -64,7 +64,8 @@ void pcifront_watches(void *opaque)
>      char* nodename = opaque ? opaque : "device/pci/0";
>      char path[strlen(nodename) + 9];
>      char fe_state[strlen(nodename) + 7];
> -    xenbus_event_queue events = NULL;
> +    struct xenbus_event_queue events;
> +    xenbus_event_queue_init(&events);
>  
>      snprintf(path, sizeof(path), "%s/backend", nodename);
>      snprintf(fe_state, sizeof(fe_state), "%s/state", nodename);
> @@ -174,7 +175,7 @@ struct pcifront_dev *init_pcifront(char *_nodename)
>  
>      dev->info_ref = gnttab_grant_access(dev->dom,virt_to_mfn(dev->info),0);
>  
> -    dev->events = NULL;
> +    xenbus_event_queue_init(&dev->events);
>  
>  again:
>      err = xenbus_transaction_start(&xbt);
> diff --git a/xen/xenbus/xenbus.c b/xen/xenbus/xenbus.c
> index a06dc30..8a14c3b 100644
> --- a/xen/xenbus/xenbus.c
> +++ b/xen/xenbus/xenbus.c
> @@ -48,10 +48,10 @@ static DECLARE_WAIT_QUEUE_HEAD(xb_waitq);
>  static spinlock_t xb_lock = SPIN_LOCK_UNLOCKED; /* protects xenbus req ring */
>  DECLARE_WAIT_QUEUE_HEAD(xenbus_watch_queue);
>  
> -xenbus_event_queue xenbus_events;
> +struct xenbus_event_queue xenbus_events;
>  static struct watch {
>      char *token;
> -    xenbus_event_queue *events;
> +    struct xenbus_event_queue *events;
>      struct watch *next;
>  } *watches;
>  struct xenbus_req_info 
> @@ -61,6 +61,13 @@ struct xenbus_req_info
>      void *reply;
>  };
>  
> +
> +void xenbus_event_queue_init(struct xenbus_event_queue *queue)
> +{
> +    MINIOS_STAILQ_INIT(&queue->events);
> +}
> +
> +
>  #define NR_REQS 32
>  static struct xenbus_req_info req_info[NR_REQS];
>  
> @@ -78,22 +85,22 @@ static void memcpy_from_ring(const void *Ring,
>      memcpy(dest + c1, ring, c2);
>  }
>  
> -char **xenbus_wait_for_watch_return(xenbus_event_queue *queue)
> +char **xenbus_wait_for_watch_return(struct xenbus_event_queue *queue)
>  {
>      struct xenbus_event *event;
>      DEFINE_WAIT(w);
>      if (!queue)
>          queue = &xenbus_events;
> -    while (!(event = *queue)) {
> +    while (!(event = MINIOS_STAILQ_FIRST(&queue->events))) {
>          add_waiter(w, xenbus_watch_queue);
>          schedule();
>      }
>      remove_waiter(w, xenbus_watch_queue);
> -    *queue = event->next;
> +    MINIOS_STAILQ_REMOVE_HEAD(&queue->events, entry);
>      return &event->path;
>  }
>  
> -void xenbus_wait_for_watch(xenbus_event_queue *queue)
> +void xenbus_wait_for_watch(struct xenbus_event_queue *queue)
>  {
>      char **ret;
>      if (!queue)
> @@ -105,7 +112,7 @@ void xenbus_wait_for_watch(xenbus_event_queue *queue)
>          printk("unexpected path returned by watch\n");
>  }
>  
> -char* xenbus_wait_for_value(const char* path, const char* value, xenbus_event_queue *queue)
> +char* xenbus_wait_for_value(const char* path, const char* value, struct xenbus_event_queue *queue)
>  {
>      if (!queue)
>          queue = &xenbus_events;
> @@ -168,7 +175,7 @@ exit:
>      return msg;
>  }
>  
> -char *xenbus_wait_for_state_change(const char* path, XenbusState *state, xenbus_event_queue *queue)
> +char *xenbus_wait_for_state_change(const char* path, XenbusState *state, struct xenbus_event_queue *queue)
>  {
>      if (!queue)
>          queue = &xenbus_events;
> @@ -227,7 +234,7 @@ static void xenbus_thread_func(void *ign)
>              if(msg.type == XS_WATCH_EVENT)
>              {
>  		struct xenbus_event *event = malloc(sizeof(*event) + msg.len);
> -                xenbus_event_queue *events = NULL;
> +                struct xenbus_event_queue *events = NULL;
>  		char *data = (char*)event + sizeof(*event);
>                  struct watch *watch;
>  
> @@ -248,8 +255,7 @@ static void xenbus_thread_func(void *ign)
>                      }
>  
>                  if (events) {
> -                    event->next = *events;
> -                    *events = event;
> +                    MINIOS_STAILQ_INSERT_TAIL(&events->events, event, entry);
>                      wake_up(&xenbus_watch_queue);
>                  } else {
>                      printk("unexpected watch token %s\n", event->token);
> @@ -332,6 +338,7 @@ void init_xenbus(void)
>  {
>      int err;
>      DEBUG("init_xenbus called.\n");
> +    xenbus_event_queue_init(&xenbus_events);
>      xenstore_buf = mfn_to_virt(start_info.store_mfn);
>      create_thread("xenstore", xenbus_thread_func, NULL);
>      DEBUG("buf at %p.\n", xenstore_buf);
> @@ -561,7 +568,7 @@ char *xenbus_write(xenbus_transaction_t xbt, const char *path, const char *value
>      return NULL;
>  }
>  
> -char* xenbus_watch_path_token( xenbus_transaction_t xbt, const char *path, const char *token, xenbus_event_queue *events)
> +char* xenbus_watch_path_token( xenbus_transaction_t xbt, const char *path, const char *token, struct xenbus_event_queue *events)
>  {
>      struct xsd_sockmsg *rep;
>  
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

-- 
Samuel
<P> moo
<N> moo ?
<D> P: keski t'arrive? :))
<P> moooo
<N> moooooo ?
<P> rien le net marche je suis content :)
 -+- #ens-mim - accro du net -+-

[PATCH 05/11] mini-os/xenbus: Use MINIOS_LIST for the list of watches

[Ian Jackson]

Remove the open-coded singly-linked list manipulation.

We replace it with a doubly-linked list because in forthcoming patches
we are going to want the ability to remove a watch from the middle of
the list without hunting for it first.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 xen/xenbus/xenbus.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/xen/xenbus/xenbus.c b/xen/xenbus/xenbus.c
index 8a14c3b..66fdc8a 100644
--- a/xen/xenbus/xenbus.c
+++ b/xen/xenbus/xenbus.c
@@ -49,11 +49,12 @@ static spinlock_t xb_lock = SPIN_LOCK_UNLOCKED; /* protects xenbus req ring */
 DECLARE_WAIT_QUEUE_HEAD(xenbus_watch_queue);
 
 struct xenbus_event_queue xenbus_events;
-static struct watch {
+struct watch {
     char *token;
     struct xenbus_event_queue *events;
-    struct watch *next;
-} *watches;
+    MINIOS_LIST_ENTRY(watch) entry;
+};
+static MINIOS_LIST_HEAD(, watch) watches;
 struct xenbus_req_info 
 {
     int in_use:1;
@@ -248,7 +249,7 @@ static void xenbus_thread_func(void *ign)
 
                 xenstore_buf->rsp_cons += msg.len + sizeof(msg);
 
-                for (watch = watches; watch; watch = watch->next)
+                MINIOS_LIST_FOREACH(watch, &watches, entry)
                     if (!strcmp(watch->token, event->token)) {
                         events = watch->events;
                         break;
@@ -586,8 +587,8 @@ char* xenbus_watch_path_token( xenbus_transaction_t xbt, const char *path, const
 
     watch->token = strdup(token);
     watch->events = events;
-    watch->next = watches;
-    watches = watch;
+
+    MINIOS_LIST_INSERT_HEAD(&watches, watch, entry);
 
     rep = xenbus_msg_reply(XS_WATCH, xbt, req, ARRAY_SIZE(req));
 
@@ -607,7 +608,7 @@ char* xenbus_unwatch_path_token( xenbus_transaction_t xbt, const char *path, con
 	{token, strlen(token) + 1},
     };
 
-    struct watch *watch, **prev;
+    struct watch *watch;
 
     char *msg;
 
@@ -617,10 +618,10 @@ char* xenbus_unwatch_path_token( xenbus_transaction_t xbt, const char *path, con
     if (msg) return msg;
     free(rep);
 
-    for (prev = &watches, watch = *prev; watch; prev = &watch->next, watch = *prev)
+    MINIOS_LIST_FOREACH(watch, &watches, entry)
         if (!strcmp(watch->token, token)) {
             free(watch->token);
-            *prev = watch->next;
+            MINIOS_LIST_REMOVE(watch, entry);
             free(watch);
             break;
         }
-- 
1.7.10.4

Re: [PATCH 05/11] mini-os/xenbus: Use MINIOS_LIST for the list of watches

[Samuel Thibault]

Ian Jackson, le Fri 20 Jun 2014 20:04:44 +0100, a écrit :
> Remove the open-coded singly-linked list manipulation.
> 
> We replace it with a doubly-linked list because in forthcoming patches
> we are going to want the ability to remove a watch from the middle of
> the list without hunting for it first.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
>  xen/xenbus/xenbus.c |   19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/xenbus/xenbus.c b/xen/xenbus/xenbus.c
> index 8a14c3b..66fdc8a 100644
> --- a/xen/xenbus/xenbus.c
> +++ b/xen/xenbus/xenbus.c
> @@ -49,11 +49,12 @@ static spinlock_t xb_lock = SPIN_LOCK_UNLOCKED; /* protects xenbus req ring */
>  DECLARE_WAIT_QUEUE_HEAD(xenbus_watch_queue);
>  
>  struct xenbus_event_queue xenbus_events;
> -static struct watch {
> +struct watch {
>      char *token;
>      struct xenbus_event_queue *events;
> -    struct watch *next;
> -} *watches;
> +    MINIOS_LIST_ENTRY(watch) entry;
> +};
> +static MINIOS_LIST_HEAD(, watch) watches;
>  struct xenbus_req_info 
>  {
>      int in_use:1;
> @@ -248,7 +249,7 @@ static void xenbus_thread_func(void *ign)
>  
>                  xenstore_buf->rsp_cons += msg.len + sizeof(msg);
>  
> -                for (watch = watches; watch; watch = watch->next)
> +                MINIOS_LIST_FOREACH(watch, &watches, entry)
>                      if (!strcmp(watch->token, event->token)) {
>                          events = watch->events;
>                          break;
> @@ -586,8 +587,8 @@ char* xenbus_watch_path_token( xenbus_transaction_t xbt, const char *path, const
>  
>      watch->token = strdup(token);
>      watch->events = events;
> -    watch->next = watches;
> -    watches = watch;
> +
> +    MINIOS_LIST_INSERT_HEAD(&watches, watch, entry);
>  
>      rep = xenbus_msg_reply(XS_WATCH, xbt, req, ARRAY_SIZE(req));
>  
> @@ -607,7 +608,7 @@ char* xenbus_unwatch_path_token( xenbus_transaction_t xbt, const char *path, con
>  	{token, strlen(token) + 1},
>      };
>  
> -    struct watch *watch, **prev;
> +    struct watch *watch;
>  
>      char *msg;
>  
> @@ -617,10 +618,10 @@ char* xenbus_unwatch_path_token( xenbus_transaction_t xbt, const char *path, con
>      if (msg) return msg;
>      free(rep);
>  
> -    for (prev = &watches, watch = *prev; watch; prev = &watch->next, watch = *prev)
> +    MINIOS_LIST_FOREACH(watch, &watches, entry)
>          if (!strcmp(watch->token, token)) {
>              free(watch->token);
> -            *prev = watch->next;
> +            MINIOS_LIST_REMOVE(watch, entry);
>              free(watch);
>              break;
>          }
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

-- 
Samuel
<xterm> The problem with America is stupidity. I'm not saying there should be a capital punishment for stupidity, but why don't we just take the safety labels off of everything and let the problem solve itself?

[PATCH 06/11] mini-os/xenbus: Rename xenbus_events to xenbus_default_watch_queue

[Ian Jackson]

This is only used for watch events, and only if xenbus's caller
doesn't specify a queue of their own.

Rename the variable because future patches are going to make the
current name confusing, because "event" is going to mean not just
watch events.

perl -i~ -pe 's/\bxenbus_events\b/xenbus_default_watch_queue/' xen/xenbus/xenbus.c include/mini-os/xenbus.h

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 include/mini-os/xenbus.h |    2 +-
 xen/xenbus/xenbus.c      |   14 +++++++-------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/mini-os/xenbus.h b/include/mini-os/xenbus.h
index 4dad4c8..abf8765 100644
--- a/include/mini-os/xenbus.h
+++ b/include/mini-os/xenbus.h
@@ -44,7 +44,7 @@ char *xenbus_switch_state(xenbus_transaction_t xbt, const char* path, XenbusStat
 
 /* When no token is provided, use a global queue. */
 #define XENBUS_WATCH_PATH_TOKEN "xenbus_watch_path"
-extern struct xenbus_event_queue xenbus_events;
+extern struct xenbus_event_queue xenbus_default_watch_queue;
 #define xenbus_watch_path(xbt, path) xenbus_watch_path_token(xbt, path, XENBUS_WATCH_PATH_TOKEN, NULL)
 #define xenbus_unwatch_path(xbt, path) xenbus_unwatch_path_token(xbt, path, XENBUS_WATCH_PATH_TOKEN)
 
diff --git a/xen/xenbus/xenbus.c b/xen/xenbus/xenbus.c
index 66fdc8a..947b5c8 100644
--- a/xen/xenbus/xenbus.c
+++ b/xen/xenbus/xenbus.c
@@ -48,7 +48,7 @@ static DECLARE_WAIT_QUEUE_HEAD(xb_waitq);
 static spinlock_t xb_lock = SPIN_LOCK_UNLOCKED; /* protects xenbus req ring */
 DECLARE_WAIT_QUEUE_HEAD(xenbus_watch_queue);
 
-struct xenbus_event_queue xenbus_events;
+struct xenbus_event_queue xenbus_default_watch_queue;
 struct watch {
     char *token;
     struct xenbus_event_queue *events;
@@ -91,7 +91,7 @@ char **xenbus_wait_for_watch_return(struct xenbus_event_queue *queue)
     struct xenbus_event *event;
     DEFINE_WAIT(w);
     if (!queue)
-        queue = &xenbus_events;
+        queue = &xenbus_default_watch_queue;
     while (!(event = MINIOS_STAILQ_FIRST(&queue->events))) {
         add_waiter(w, xenbus_watch_queue);
         schedule();
@@ -105,7 +105,7 @@ void xenbus_wait_for_watch(struct xenbus_event_queue *queue)
 {
     char **ret;
     if (!queue)
-        queue = &xenbus_events;
+        queue = &xenbus_default_watch_queue;
     ret = xenbus_wait_for_watch_return(queue);
     if (ret)
         free(ret);
@@ -116,7 +116,7 @@ void xenbus_wait_for_watch(struct xenbus_event_queue *queue)
 char* xenbus_wait_for_value(const char* path, const char* value, struct xenbus_event_queue *queue)
 {
     if (!queue)
-        queue = &xenbus_events;
+        queue = &xenbus_default_watch_queue;
     for(;;)
     {
         char *res, *msg;
@@ -179,7 +179,7 @@ exit:
 char *xenbus_wait_for_state_change(const char* path, XenbusState *state, struct xenbus_event_queue *queue)
 {
     if (!queue)
-        queue = &xenbus_events;
+        queue = &xenbus_default_watch_queue;
     for(;;)
     {
         char *res, *msg;
@@ -339,7 +339,7 @@ void init_xenbus(void)
 {
     int err;
     DEBUG("init_xenbus called.\n");
-    xenbus_event_queue_init(&xenbus_events);
+    xenbus_event_queue_init(&xenbus_default_watch_queue);
     xenstore_buf = mfn_to_virt(start_info.store_mfn);
     create_thread("xenstore", xenbus_thread_func, NULL);
     DEBUG("buf at %p.\n", xenstore_buf);
@@ -583,7 +583,7 @@ char* xenbus_watch_path_token( xenbus_transaction_t xbt, const char *path, const
     char *msg;
 
     if (!events)
-        events = &xenbus_events;
+        events = &xenbus_default_watch_queue;
 
     watch->token = strdup(token);
     watch->events = events;
-- 
1.7.10.4

Re: [PATCH 06/11] mini-os/xenbus: Rename xenbus_events to xenbus_default_watch_queue

[Samuel Thibault]

Ian Jackson, le Fri 20 Jun 2014 20:04:45 +0100, a écrit :
> This is only used for watch events, and only if xenbus's caller
> doesn't specify a queue of their own.
> 
> Rename the variable because future patches are going to make the
> current name confusing, because "event" is going to mean not just
> watch events.
> 
> perl -i~ -pe 's/\bxenbus_events\b/xenbus_default_watch_queue/' xen/xenbus/xenbus.c include/mini-os/xenbus.h
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
>  include/mini-os/xenbus.h |    2 +-
>  xen/xenbus/xenbus.c      |   14 +++++++-------
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/include/mini-os/xenbus.h b/include/mini-os/xenbus.h
> index 4dad4c8..abf8765 100644
> --- a/include/mini-os/xenbus.h
> +++ b/include/mini-os/xenbus.h
> @@ -44,7 +44,7 @@ char *xenbus_switch_state(xenbus_transaction_t xbt, const char* path, XenbusStat
>  
>  /* When no token is provided, use a global queue. */
>  #define XENBUS_WATCH_PATH_TOKEN "xenbus_watch_path"
> -extern struct xenbus_event_queue xenbus_events;
> +extern struct xenbus_event_queue xenbus_default_watch_queue;
>  #define xenbus_watch_path(xbt, path) xenbus_watch_path_token(xbt, path, XENBUS_WATCH_PATH_TOKEN, NULL)
>  #define xenbus_unwatch_path(xbt, path) xenbus_unwatch_path_token(xbt, path, XENBUS_WATCH_PATH_TOKEN)
>  
> diff --git a/xen/xenbus/xenbus.c b/xen/xenbus/xenbus.c
> index 66fdc8a..947b5c8 100644
> --- a/xen/xenbus/xenbus.c
> +++ b/xen/xenbus/xenbus.c
> @@ -48,7 +48,7 @@ static DECLARE_WAIT_QUEUE_HEAD(xb_waitq);
>  static spinlock_t xb_lock = SPIN_LOCK_UNLOCKED; /* protects xenbus req ring */
>  DECLARE_WAIT_QUEUE_HEAD(xenbus_watch_queue);
>  
> -struct xenbus_event_queue xenbus_events;
> +struct xenbus_event_queue xenbus_default_watch_queue;
>  struct watch {
>      char *token;
>      struct xenbus_event_queue *events;
> @@ -91,7 +91,7 @@ char **xenbus_wait_for_watch_return(struct xenbus_event_queue *queue)
>      struct xenbus_event *event;
>      DEFINE_WAIT(w);
>      if (!queue)
> -        queue = &xenbus_events;
> +        queue = &xenbus_default_watch_queue;
>      while (!(event = MINIOS_STAILQ_FIRST(&queue->events))) {
>          add_waiter(w, xenbus_watch_queue);
>          schedule();
> @@ -105,7 +105,7 @@ void xenbus_wait_for_watch(struct xenbus_event_queue *queue)
>  {
>      char **ret;
>      if (!queue)
> -        queue = &xenbus_events;
> +        queue = &xenbus_default_watch_queue;
>      ret = xenbus_wait_for_watch_return(queue);
>      if (ret)
>          free(ret);
> @@ -116,7 +116,7 @@ void xenbus_wait_for_watch(struct xenbus_event_queue *queue)
>  char* xenbus_wait_for_value(const char* path, const char* value, struct xenbus_event_queue *queue)
>  {
>      if (!queue)
> -        queue = &xenbus_events;
> +        queue = &xenbus_default_watch_queue;
>      for(;;)
>      {
>          char *res, *msg;
> @@ -179,7 +179,7 @@ exit:
>  char *xenbus_wait_for_state_change(const char* path, XenbusState *state, struct xenbus_event_queue *queue)
>  {
>      if (!queue)
> -        queue = &xenbus_events;
> +        queue = &xenbus_default_watch_queue;
>      for(;;)
>      {
>          char *res, *msg;
> @@ -339,7 +339,7 @@ void init_xenbus(void)
>  {
>      int err;
>      DEBUG("init_xenbus called.\n");
> -    xenbus_event_queue_init(&xenbus_events);
> +    xenbus_event_queue_init(&xenbus_default_watch_queue);
>      xenstore_buf = mfn_to_virt(start_info.store_mfn);
>      create_thread("xenstore", xenbus_thread_func, NULL);
>      DEBUG("buf at %p.\n", xenstore_buf);
> @@ -583,7 +583,7 @@ char* xenbus_watch_path_token( xenbus_transaction_t xbt, const char *path, const
>      char *msg;
>  
>      if (!events)
> -        events = &xenbus_events;
> +        events = &xenbus_default_watch_queue;
>  
>      watch->token = strdup(token);
>      watch->events = events;
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

-- 
Samuel
 > Quelqu'un aurait-il une solution pour réinitialiser un MBR
 Si tu veux qu'il soit complètement blanc (pas souhaitable, à mon avis) :
 dd if=/dev/zero of=/dev/hda bs=512 count=1 (sous Linux)
 -+- OT in Guide du linuxien (très) pervers - "Pour les K difficiles" -+-

[PATCH 07/11] mini-os/xenbus: Unify watch and reply queues

[Ian Jackson]

We are going to want to provide an interface to xenbus which does not
reorder messages for a particular user.  In particular, the reply to a
watch or unwatch should not be reordered with respect to watch events.

To this end we arrange that both replies and watches use the same kind
of queue inside the xenbus driver.  Currently this queue type is only
exposed outside the xenbus driver for use with watches, as before.

Important functional changes in this patch include:

* There is a separate scheduler wait queue for each reply queue,
  rather than one for all watches and one for each outstanding reply.
  This wait queue lives in the reply queue struct.

* There are abstracted-away internal functions for removing (and,
  indeed, awaiting) events.  xenbus_wait_for_watch_return becomes a
  trivial wrapper around await_event.

* Handling of the replies to requests is formalised, using the new
  queues.  Now a single reply queue might be used for multiple
  requests (although there are no callers that do this).

Other changes are:

* The latent bug in xenbus_msg_reply, which assumed no spurious
  wakeups, is gone.

* The "in_use" flag in the request array can be done away with, since
  we can use the reply_queue pointer value instead.

* The callers of allocate_xenbus_id (currently, only
  xenbus_msg_reply), have to initialise a xenbus_event_queue and
  provide it to allocate_xenbus_id.

* Abolished the xenbus_watch_queue waitq in favour of the waitq inside
  the xenbus_default_watch_events event queue.

* Abolished a duplicate assignment to in_use in release_xenbus_id.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 include/mini-os/xenbus.h |    3 ++
 xen/xenbus/xenbus.c      |   82 +++++++++++++++++++++++++++++-----------------
 2 files changed, 55 insertions(+), 30 deletions(-)

diff --git a/include/mini-os/xenbus.h b/include/mini-os/xenbus.h
index abf8765..7e70de0 100644
--- a/include/mini-os/xenbus.h
+++ b/include/mini-os/xenbus.h
@@ -2,6 +2,8 @@
 #define MINIOS_XENBUS_H__
 
 #include <xen/io/xenbus.h>
+#include <mini-os/sched.h>
+#include <mini-os/waittypes.h>
 #include <mini-os/queue.h>
 
 typedef unsigned long xenbus_transaction_t;
@@ -30,6 +32,7 @@ struct xenbus_event {
 };
 struct xenbus_event_queue {
     MINIOS_STAILQ_HEAD(, xenbus_event) events;
+    struct wait_queue_head waitq;
 };
 
 void xenbus_event_queue_init(struct xenbus_event_queue *queue);
diff --git a/xen/xenbus/xenbus.c b/xen/xenbus/xenbus.c
index 947b5c8..d2e59b3 100644
--- a/xen/xenbus/xenbus.c
+++ b/xen/xenbus/xenbus.c
@@ -46,7 +46,6 @@
 static struct xenstore_domain_interface *xenstore_buf;
 static DECLARE_WAIT_QUEUE_HEAD(xb_waitq);
 static spinlock_t xb_lock = SPIN_LOCK_UNLOCKED; /* protects xenbus req ring */
-DECLARE_WAIT_QUEUE_HEAD(xenbus_watch_queue);
 
 struct xenbus_event_queue xenbus_default_watch_queue;
 struct watch {
@@ -57,8 +56,8 @@ struct watch {
 static MINIOS_LIST_HEAD(, watch) watches;
 struct xenbus_req_info 
 {
-    int in_use:1;
-    struct wait_queue_head waitq;
+    struct xenbus_event_queue *reply_queue; /* non-0 iff in use */
+    struct xenbus_event *for_queue;
     void *reply;
 };
 
@@ -66,6 +65,39 @@ struct xenbus_req_info
 void xenbus_event_queue_init(struct xenbus_event_queue *queue)
 {
     MINIOS_STAILQ_INIT(&queue->events);
+    init_waitqueue_head(&queue->waitq);
+}
+
+static struct xenbus_event *remove_event(struct xenbus_event_queue *queue)
+{
+    struct xenbus_event *event;
+
+    event = MINIOS_STAILQ_FIRST(&queue->events);
+    if (!event)
+        goto out;
+    MINIOS_STAILQ_REMOVE_HEAD(&queue->events, entry);
+
+ out:
+    return event;
+}
+
+static void queue_event(struct xenbus_event_queue *queue,
+                        struct xenbus_event *event)
+{
+    MINIOS_STAILQ_INSERT_TAIL(&queue->events, event, entry);
+    wake_up(&queue->waitq);
+}
+
+static struct xenbus_event *await_event(struct xenbus_event_queue *queue)
+{
+    struct xenbus_event *event;
+    DEFINE_WAIT(w);
+    while (!(event = remove_event(queue))) {
+        add_waiter(w, queue->waitq);
+        schedule();
+    }
+    remove_waiter(w, queue->waitq);
+    return event;
 }
 
 
@@ -89,15 +121,9 @@ static void memcpy_from_ring(const void *Ring,
 char **xenbus_wait_for_watch_return(struct xenbus_event_queue *queue)
 {
     struct xenbus_event *event;
-    DEFINE_WAIT(w);
     if (!queue)
         queue = &xenbus_default_watch_queue;
-    while (!(event = MINIOS_STAILQ_FIRST(&queue->events))) {
-        add_waiter(w, xenbus_watch_queue);
-        schedule();
-    }
-    remove_waiter(w, xenbus_watch_queue);
-    MINIOS_STAILQ_REMOVE_HEAD(&queue->events, entry);
+    event = await_event(queue);
     return &event->path;
 }
 
@@ -256,8 +282,7 @@ static void xenbus_thread_func(void *ign)
                     }
 
                 if (events) {
-                    MINIOS_STAILQ_INSERT_TAIL(&events->events, event, entry);
-                    wake_up(&xenbus_watch_queue);
+                    queue_event(events, event);
                 } else {
                     printk("unexpected watch token %s\n", event->token);
                     free(event);
@@ -272,7 +297,8 @@ static void xenbus_thread_func(void *ign)
                     MASK_XENSTORE_IDX(xenstore_buf->rsp_cons),
                     msg.len + sizeof(msg));
                 xenstore_buf->rsp_cons += msg.len + sizeof(msg);
-                wake_up(&req_info[msg.req_id].waitq);
+                queue_event(req_info[msg.req_id].reply_queue,
+                            req_info[msg.req_id].for_queue);
             }
         }
     }
@@ -291,11 +317,10 @@ static DECLARE_WAIT_QUEUE_HEAD(req_wq);
 /* Release a xenbus identifier */
 static void release_xenbus_id(int id)
 {
-    BUG_ON(!req_info[id].in_use);
+    BUG_ON(!req_info[id].reply_queue);
     spin_lock(&req_lock);
-    req_info[id].in_use = 0;
+    req_info[id].reply_queue = 0;
     nr_live_reqs--;
-    req_info[id].in_use = 0;
     if (nr_live_reqs == NR_REQS - 1)
         wake_up(&req_wq);
     spin_unlock(&req_lock);
@@ -303,7 +328,8 @@ static void release_xenbus_id(int id)
 
 /* Allocate an identifier for a xenbus request.  Blocks if none are
    available. */
-static int allocate_xenbus_id(void)
+static int allocate_xenbus_id(struct xenbus_event_queue *reply_queue,
+                              struct xenbus_event *for_queue)
 {
     static int probe;
     int o_probe;
@@ -320,16 +346,16 @@ static int allocate_xenbus_id(void)
     o_probe = probe;
     for (;;) 
     {
-        if (!req_info[o_probe].in_use)
+        if (!req_info[o_probe].reply_queue)
             break;
         o_probe = (o_probe + 1) % NR_REQS;
         BUG_ON(o_probe == probe);
     }
     nr_live_reqs++;
-    req_info[o_probe].in_use = 1;
+    req_info[o_probe].reply_queue = reply_queue;
+    req_info[o_probe].for_queue = for_queue;
     probe = (o_probe + 1) % NR_REQS;
     spin_unlock(&req_lock);
-    init_waitqueue_head(&req_info[o_probe].waitq);
 
     return o_probe;
 }
@@ -448,22 +474,18 @@ xenbus_msg_reply(int type,
 		 int nr_reqs)
 {
     int id;
-    DEFINE_WAIT(w);
     struct xsd_sockmsg *rep;
+    struct xenbus_event_queue queue;
+    struct xenbus_event event_buf;
 
-    /*
-     * XXX: should use a predicate loop instead of blindly trusting
-     * that $someone didn't wake us up
-     */
+    xenbus_event_queue_init(&queue);
 
-    id = allocate_xenbus_id();
-    add_waiter(w, req_info[id].waitq);
+    id = allocate_xenbus_id(&queue,&event_buf);
 
     xb_write(type, id, trans, io, nr_reqs);
 
-    schedule();
-    remove_waiter(w, req_info[id].waitq);
-    wake(current);
+    struct xenbus_event *event = await_event(&queue);
+    BUG_ON(event != &event_buf);
 
     rep = req_info[id].reply;
     BUG_ON(rep->req_id != id);
-- 
1.7.10.4

Re: [PATCH 07/11] mini-os/xenbus: Unify watch and reply queues

[Samuel Thibault]

Ian Jackson, le Fri 20 Jun 2014 20:04:46 +0100, a écrit :
> We are going to want to provide an interface to xenbus which does not
> reorder messages for a particular user.  In particular, the reply to a
> watch or unwatch should not be reordered with respect to watch events.
> 
> To this end we arrange that both replies and watches use the same kind
> of queue inside the xenbus driver.  Currently this queue type is only
> exposed outside the xenbus driver for use with watches, as before.
> 
> Important functional changes in this patch include:
> 
> * There is a separate scheduler wait queue for each reply queue,
>   rather than one for all watches and one for each outstanding reply.
>   This wait queue lives in the reply queue struct.
> 
> * There are abstracted-away internal functions for removing (and,
>   indeed, awaiting) events.  xenbus_wait_for_watch_return becomes a
>   trivial wrapper around await_event.
> 
> * Handling of the replies to requests is formalised, using the new
>   queues.  Now a single reply queue might be used for multiple
>   requests (although there are no callers that do this).
> 
> Other changes are:
> 
> * The latent bug in xenbus_msg_reply, which assumed no spurious
>   wakeups, is gone.
> 
> * The "in_use" flag in the request array can be done away with, since
>   we can use the reply_queue pointer value instead.
> 
> * The callers of allocate_xenbus_id (currently, only
>   xenbus_msg_reply), have to initialise a xenbus_event_queue and
>   provide it to allocate_xenbus_id.
> 
> * Abolished the xenbus_watch_queue waitq in favour of the waitq inside
>   the xenbus_default_watch_events event queue.
> 
> * Abolished a duplicate assignment to in_use in release_xenbus_id.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
>  include/mini-os/xenbus.h |    3 ++
>  xen/xenbus/xenbus.c      |   82 +++++++++++++++++++++++++++++-----------------
>  2 files changed, 55 insertions(+), 30 deletions(-)
> 
> diff --git a/include/mini-os/xenbus.h b/include/mini-os/xenbus.h
> index abf8765..7e70de0 100644
> --- a/include/mini-os/xenbus.h
> +++ b/include/mini-os/xenbus.h
> @@ -2,6 +2,8 @@
>  #define MINIOS_XENBUS_H__
>  
>  #include <xen/io/xenbus.h>
> +#include <mini-os/sched.h>
> +#include <mini-os/waittypes.h>
>  #include <mini-os/queue.h>
>  
>  typedef unsigned long xenbus_transaction_t;
> @@ -30,6 +32,7 @@ struct xenbus_event {
>  };
>  struct xenbus_event_queue {
>      MINIOS_STAILQ_HEAD(, xenbus_event) events;
> +    struct wait_queue_head waitq;
>  };
>  
>  void xenbus_event_queue_init(struct xenbus_event_queue *queue);
> diff --git a/xen/xenbus/xenbus.c b/xen/xenbus/xenbus.c
> index 947b5c8..d2e59b3 100644
> --- a/xen/xenbus/xenbus.c
> +++ b/xen/xenbus/xenbus.c
> @@ -46,7 +46,6 @@
>  static struct xenstore_domain_interface *xenstore_buf;
>  static DECLARE_WAIT_QUEUE_HEAD(xb_waitq);
>  static spinlock_t xb_lock = SPIN_LOCK_UNLOCKED; /* protects xenbus req ring */
> -DECLARE_WAIT_QUEUE_HEAD(xenbus_watch_queue);
>  
>  struct xenbus_event_queue xenbus_default_watch_queue;
>  struct watch {
> @@ -57,8 +56,8 @@ struct watch {
>  static MINIOS_LIST_HEAD(, watch) watches;
>  struct xenbus_req_info 
>  {
> -    int in_use:1;
> -    struct wait_queue_head waitq;
> +    struct xenbus_event_queue *reply_queue; /* non-0 iff in use */
> +    struct xenbus_event *for_queue;
>      void *reply;
>  };
>  
> @@ -66,6 +65,39 @@ struct xenbus_req_info
>  void xenbus_event_queue_init(struct xenbus_event_queue *queue)
>  {
>      MINIOS_STAILQ_INIT(&queue->events);
> +    init_waitqueue_head(&queue->waitq);
> +}
> +
> +static struct xenbus_event *remove_event(struct xenbus_event_queue *queue)
> +{
> +    struct xenbus_event *event;
> +
> +    event = MINIOS_STAILQ_FIRST(&queue->events);
> +    if (!event)
> +        goto out;
> +    MINIOS_STAILQ_REMOVE_HEAD(&queue->events, entry);
> +
> + out:
> +    return event;
> +}
> +
> +static void queue_event(struct xenbus_event_queue *queue,
> +                        struct xenbus_event *event)
> +{
> +    MINIOS_STAILQ_INSERT_TAIL(&queue->events, event, entry);
> +    wake_up(&queue->waitq);
> +}
> +
> +static struct xenbus_event *await_event(struct xenbus_event_queue *queue)
> +{
> +    struct xenbus_event *event;
> +    DEFINE_WAIT(w);
> +    while (!(event = remove_event(queue))) {
> +        add_waiter(w, queue->waitq);
> +        schedule();
> +    }
> +    remove_waiter(w, queue->waitq);
> +    return event;
>  }
>  
>  
> @@ -89,15 +121,9 @@ static void memcpy_from_ring(const void *Ring,
>  char **xenbus_wait_for_watch_return(struct xenbus_event_queue *queue)
>  {
>      struct xenbus_event *event;
> -    DEFINE_WAIT(w);
>      if (!queue)
>          queue = &xenbus_default_watch_queue;
> -    while (!(event = MINIOS_STAILQ_FIRST(&queue->events))) {
> -        add_waiter(w, xenbus_watch_queue);
> -        schedule();
> -    }
> -    remove_waiter(w, xenbus_watch_queue);
> -    MINIOS_STAILQ_REMOVE_HEAD(&queue->events, entry);
> +    event = await_event(queue);
>      return &event->path;
>  }
>  
> @@ -256,8 +282,7 @@ static void xenbus_thread_func(void *ign)
>                      }
>  
>                  if (events) {
> -                    MINIOS_STAILQ_INSERT_TAIL(&events->events, event, entry);
> -                    wake_up(&xenbus_watch_queue);
> +                    queue_event(events, event);
>                  } else {
>                      printk("unexpected watch token %s\n", event->token);
>                      free(event);
> @@ -272,7 +297,8 @@ static void xenbus_thread_func(void *ign)
>                      MASK_XENSTORE_IDX(xenstore_buf->rsp_cons),
>                      msg.len + sizeof(msg));
>                  xenstore_buf->rsp_cons += msg.len + sizeof(msg);
> -                wake_up(&req_info[msg.req_id].waitq);
> +                queue_event(req_info[msg.req_id].reply_queue,
> +                            req_info[msg.req_id].for_queue);
>              }
>          }
>      }
> @@ -291,11 +317,10 @@ static DECLARE_WAIT_QUEUE_HEAD(req_wq);
>  /* Release a xenbus identifier */
>  static void release_xenbus_id(int id)
>  {
> -    BUG_ON(!req_info[id].in_use);
> +    BUG_ON(!req_info[id].reply_queue);
>      spin_lock(&req_lock);
> -    req_info[id].in_use = 0;
> +    req_info[id].reply_queue = 0;
>      nr_live_reqs--;
> -    req_info[id].in_use = 0;
>      if (nr_live_reqs == NR_REQS - 1)
>          wake_up(&req_wq);
>      spin_unlock(&req_lock);
> @@ -303,7 +328,8 @@ static void release_xenbus_id(int id)
>  
>  /* Allocate an identifier for a xenbus request.  Blocks if none are
>     available. */
> -static int allocate_xenbus_id(void)
> +static int allocate_xenbus_id(struct xenbus_event_queue *reply_queue,
> +                              struct xenbus_event *for_queue)
>  {
>      static int probe;
>      int o_probe;
> @@ -320,16 +346,16 @@ static int allocate_xenbus_id(void)
>      o_probe = probe;
>      for (;;) 
>      {
> -        if (!req_info[o_probe].in_use)
> +        if (!req_info[o_probe].reply_queue)
>              break;
>          o_probe = (o_probe + 1) % NR_REQS;
>          BUG_ON(o_probe == probe);
>      }
>      nr_live_reqs++;
> -    req_info[o_probe].in_use = 1;
> +    req_info[o_probe].reply_queue = reply_queue;
> +    req_info[o_probe].for_queue = for_queue;
>      probe = (o_probe + 1) % NR_REQS;
>      spin_unlock(&req_lock);
> -    init_waitqueue_head(&req_info[o_probe].waitq);
>  
>      return o_probe;
>  }
> @@ -448,22 +474,18 @@ xenbus_msg_reply(int type,
>  		 int nr_reqs)
>  {
>      int id;
> -    DEFINE_WAIT(w);
>      struct xsd_sockmsg *rep;
> +    struct xenbus_event_queue queue;
> +    struct xenbus_event event_buf;
>  
> -    /*
> -     * XXX: should use a predicate loop instead of blindly trusting
> -     * that $someone didn't wake us up
> -     */
> +    xenbus_event_queue_init(&queue);
>  
> -    id = allocate_xenbus_id();
> -    add_waiter(w, req_info[id].waitq);
> +    id = allocate_xenbus_id(&queue,&event_buf);
>  
>      xb_write(type, id, trans, io, nr_reqs);
>  
> -    schedule();
> -    remove_waiter(w, req_info[id].waitq);
> -    wake(current);
> +    struct xenbus_event *event = await_event(&queue);
> +    BUG_ON(event != &event_buf);
>  
>      rep = req_info[id].reply;
>      BUG_ON(rep->req_id != id);
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

-- 
Samuel
 La carte réseau fournie par cybercable (sn2000) ne va-t-elle que sur
 bus isa ou peut-on aussi la mettre sur bus PCI.
 Merci de m'éclairer.
 -+- JP in le Neuneu Pète un Câble : Une carte dans chaque port -+-

[PATCH 08/11] mini-os/xenbus: Expose lower-level interface

[Ian Jackson]

Provide an interface that allows a xenbus user to explicitly allocate
ids, deal with responses asynchronously, specify the queues to be used
for responses and watches, etc.

More specifically:

* Enhance xenbus_event to be capable of dealing with both watches and
  command replies.  In particular, arrange that it will contain a
  pointer to the watch.  We leave the old fields undisturbed because
  of the way that this struct is already used in various places.

* Provide that a xenbus_event for a command response contains a copy
  of the pointer to the reply message, rather than putting it in the
  req_info (which is visible only internally).

* Rename `struct watch' to `struct xenbus_watch' because it needs
  to be in the public interface.

* allocate_xenbus_id becomes xenbus_id_allocate; same for release.

* Make xb_write into a public function, xenbus_xb_write.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 include/mini-os/xenbus.h |   74 +++++++++++++++++++++++++++++++++++++++++++---
 xen/xenbus/xenbus.c      |   66 +++++++++++++++++++++++++----------------
 2 files changed, 110 insertions(+), 30 deletions(-)

diff --git a/include/mini-os/xenbus.h b/include/mini-os/xenbus.h
index 7e70de0..b8d152d 100644
--- a/include/mini-os/xenbus.h
+++ b/include/mini-os/xenbus.h
@@ -23,11 +23,18 @@ static inline void init_xenbus(void)
    set to a malloc'd copy of the value. */
 char *xenbus_read(xenbus_transaction_t xbt, const char *path, char **value);
 
-/* Watch event queue */
+/* Queue for events (watches or async request replies - see below) */
 struct xenbus_event {
-    /* Keep these two as this for xs.c */
-    char *path;
-    char *token;
+    union {
+        struct {
+            /* must be first, both for the bare minios xs.c, and for
+             * xenbus_wait_for_watch's handling */
+            char *path;
+            char *token;
+        };
+        struct xsd_sockmsg *reply;
+    };
+    struct xenbus_watch *watch;
     MINIOS_STAILQ_ENTRY(xenbus_event) entry;
 };
 struct xenbus_event_queue {
@@ -111,6 +118,65 @@ char* xenbus_printf(xenbus_transaction_t xbt,
 /* Utility function to figure out our domain id */
 domid_t xenbus_get_self_id(void);
 
+/*
+ * ----- asynchronous low-level interface -----
+ */
+
+/* Allocate an identifier for a xenbus request.  Blocks if none are
+ * available.  Cannot fail.  On return, we may use the returned value
+ * as the id in a xenbus request.
+ *
+ * for_queue must already be allocated, but may be uninitialised.
+ *
+ * for_queue->watch is not touched by the xenbus machinery for
+ * handling requests/replies but should probably be initialised by the
+ * caller (probably to NULL) because this will help the caller
+ * distinguish the reply from any watch events which might end up in
+ * the same queue.
+ *
+ * reply_queue must exist and have been initialised.
+ *
+ * When the response arrives, the reply message will stored in
+ * for_queue->reply and for_queue will be queued on reply_queue.  The
+ * id must be then explicitly released (or, used again, if desired).
+ * After ->reply is done with the caller must pass it to free().
+ * (Do not use the id for more than one request at a time.) */
+int xenbus_id_allocate(struct xenbus_event_queue *reply_queue,
+                       struct xenbus_event *for_queue);
+void xenbus_id_release(int id);
+
+/* Allocating a token for a watch.
+ *
+ * To use this:
+ *  - Include struct xenbus_watch in your own struct.
+ *  - Set events; then call prepare.  This will set token.
+ *    You may then use token in a WATCH request.
+ *  - You must UNWATCH before you call release.
+ * Do not modify token yourself.
+ * entry is private for the xenbus driver.
+ *
+ * When the watch fires, a new struct xenbus_event will be allocated
+ * and queued on events.  The field xenbus_event->watch will have been
+ * set to watch by the xenbus machinery, and xenbus_event->path will
+ * be the watch path.  After the caller is done with the event,
+ * its pointer should simply be passed to free(). */
+struct xenbus_watch {
+    char *token;
+    struct xenbus_event_queue *events;
+    MINIOS_LIST_ENTRY(xenbus_watch) entry;
+};
+void xenbus_watch_init(struct xenbus_watch *watch); /* makes release a noop */
+void xenbus_watch_prepare(struct xenbus_watch *watch); /* need not be init'd */
+void xenbus_watch_release(struct xenbus_watch *watch); /* idempotent */
+
+
+/* Send data to xenbus.  This can block.  All of the requests are seen
+ * by xenbus as if sent atomically.  The header is added
+ * automatically, using type %type, req_id %req_id, and trans_id
+ * %trans_id. */
+void xenbus_xb_write(int type, int req_id, xenbus_transaction_t trans_id,
+		     const struct write_req *req, int nr_reqs);
+
 #ifdef CONFIG_XENBUS
 /* Reset the XenBus system. */
 void fini_xenbus(void);
diff --git a/xen/xenbus/xenbus.c b/xen/xenbus/xenbus.c
index d2e59b3..bf4bb45 100644
--- a/xen/xenbus/xenbus.c
+++ b/xen/xenbus/xenbus.c
@@ -48,17 +48,11 @@ static DECLARE_WAIT_QUEUE_HEAD(xb_waitq);
 static spinlock_t xb_lock = SPIN_LOCK_UNLOCKED; /* protects xenbus req ring */
 
 struct xenbus_event_queue xenbus_default_watch_queue;
-struct watch {
-    char *token;
-    struct xenbus_event_queue *events;
-    MINIOS_LIST_ENTRY(watch) entry;
-};
-static MINIOS_LIST_HEAD(, watch) watches;
+static MINIOS_LIST_HEAD(, xenbus_watch) watches;
 struct xenbus_req_info 
 {
     struct xenbus_event_queue *reply_queue; /* non-0 iff in use */
     struct xenbus_event *for_queue;
-    void *reply;
 };
 
 
@@ -263,7 +257,7 @@ static void xenbus_thread_func(void *ign)
 		struct xenbus_event *event = malloc(sizeof(*event) + msg.len);
                 struct xenbus_event_queue *events = NULL;
 		char *data = (char*)event + sizeof(*event);
-                struct watch *watch;
+                struct xenbus_watch *watch;
 
                 memcpy_from_ring(xenstore_buf->rsp,
 		    data,
@@ -277,6 +271,7 @@ static void xenbus_thread_func(void *ign)
 
                 MINIOS_LIST_FOREACH(watch, &watches, entry)
                     if (!strcmp(watch->token, event->token)) {
+                        event->watch = watch;
                         events = watch->events;
                         break;
                     }
@@ -291,9 +286,10 @@ static void xenbus_thread_func(void *ign)
 
             else
             {
-                req_info[msg.req_id].reply = malloc(sizeof(msg) + msg.len);
+                req_info[msg.req_id].for_queue->reply =
+                    malloc(sizeof(msg) + msg.len);
                 memcpy_from_ring(xenstore_buf->rsp,
-                    req_info[msg.req_id].reply,
+                    req_info[msg.req_id].for_queue->reply,
                     MASK_XENSTORE_IDX(xenstore_buf->rsp_cons),
                     msg.len + sizeof(msg));
                 xenstore_buf->rsp_cons += msg.len + sizeof(msg);
@@ -315,7 +311,7 @@ static spinlock_t req_lock = SPIN_LOCK_UNLOCKED;
 static DECLARE_WAIT_QUEUE_HEAD(req_wq);
 
 /* Release a xenbus identifier */
-static void release_xenbus_id(int id)
+void xenbus_id_release(int id)
 {
     BUG_ON(!req_info[id].reply_queue);
     spin_lock(&req_lock);
@@ -326,10 +322,8 @@ static void release_xenbus_id(int id)
     spin_unlock(&req_lock);
 }
 
-/* Allocate an identifier for a xenbus request.  Blocks if none are
-   available. */
-static int allocate_xenbus_id(struct xenbus_event_queue *reply_queue,
-                              struct xenbus_event *for_queue)
+int xenbus_id_allocate(struct xenbus_event_queue *reply_queue,
+                       struct xenbus_event *for_queue)
 {
     static int probe;
     int o_probe;
@@ -360,6 +354,30 @@ static int allocate_xenbus_id(struct xenbus_event_queue *reply_queue,
     return o_probe;
 }
 
+void xenbus_watch_init(struct xenbus_watch *watch)
+{
+    watch->token = 0;
+}
+
+void xenbus_watch_prepare(struct xenbus_watch *watch)
+{
+    BUG_ON(!watch->events);
+    size_t size = sizeof(void*)*2 + 5;
+    watch->token = malloc(size);
+    int r = snprintf(watch->token,size,"*%p",(void*)watch);
+    BUG_ON(!(r > 0 && r < size));
+    MINIOS_LIST_INSERT_HEAD(&watches, watch, entry);
+}
+
+void xenbus_watch_release(struct xenbus_watch *watch)
+{
+    if (!watch->token)
+        return;
+    MINIOS_LIST_REMOVE(watch, entry);
+    free(watch->token);
+    watch->token = 0;
+}
+
 /* Initialise xenbus. */
 void init_xenbus(void)
 {
@@ -381,11 +399,7 @@ void fini_xenbus(void)
 {
 }
 
-/* Send data to xenbus.  This can block.  All of the requests are seen
-   by xenbus as if sent atomically.  The header is added
-   automatically, using type %type, req_id %req_id, and trans_id
-   %trans_id. */
-static void xb_write(int type, int req_id, xenbus_transaction_t trans_id,
+void xenbus_xb_write(int type, int req_id, xenbus_transaction_t trans_id,
 		     const struct write_req *req, int nr_reqs)
 {
     XENSTORE_RING_IDX prod;
@@ -480,16 +494,16 @@ xenbus_msg_reply(int type,
 
     xenbus_event_queue_init(&queue);
 
-    id = allocate_xenbus_id(&queue,&event_buf);
+    id = xenbus_id_allocate(&queue,&event_buf);
 
-    xb_write(type, id, trans, io, nr_reqs);
+    xenbus_xb_write(type, id, trans, io, nr_reqs);
 
     struct xenbus_event *event = await_event(&queue);
     BUG_ON(event != &event_buf);
 
-    rep = req_info[id].reply;
+    rep = req_info[id].for_queue->reply;
     BUG_ON(rep->req_id != id);
-    release_xenbus_id(id);
+    xenbus_id_release(id);
     return rep;
 }
 
@@ -600,7 +614,7 @@ char* xenbus_watch_path_token( xenbus_transaction_t xbt, const char *path, const
 	{token, strlen(token) + 1},
     };
 
-    struct watch *watch = malloc(sizeof(*watch));
+    struct xenbus_watch *watch = malloc(sizeof(*watch));
 
     char *msg;
 
@@ -630,7 +644,7 @@ char* xenbus_unwatch_path_token( xenbus_transaction_t xbt, const char *path, con
 	{token, strlen(token) + 1},
     };
 
-    struct watch *watch;
+    struct xenbus_watch *watch;
 
     char *msg;
 
-- 
1.7.10.4

Re: [PATCH 08/11] mini-os/xenbus: Expose lower-level interface

[Samuel Thibault]

Ian Jackson, le Fri 20 Jun 2014 20:04:47 +0100, a écrit :
> Provide an interface that allows a xenbus user to explicitly allocate
> ids, deal with responses asynchronously, specify the queues to be used
> for responses and watches, etc.
> 
> More specifically:
> 
> * Enhance xenbus_event to be capable of dealing with both watches and
>   command replies.  In particular, arrange that it will contain a
>   pointer to the watch.  We leave the old fields undisturbed because
>   of the way that this struct is already used in various places.
> 
> * Provide that a xenbus_event for a command response contains a copy
>   of the pointer to the reply message, rather than putting it in the
>   req_info (which is visible only internally).
> 
> * Rename `struct watch' to `struct xenbus_watch' because it needs
>   to be in the public interface.
> 
> * allocate_xenbus_id becomes xenbus_id_allocate; same for release.
> 
> * Make xb_write into a public function, xenbus_xb_write.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
>  include/mini-os/xenbus.h |   74 +++++++++++++++++++++++++++++++++++++++++++---
>  xen/xenbus/xenbus.c      |   66 +++++++++++++++++++++++++----------------
>  2 files changed, 110 insertions(+), 30 deletions(-)
> 
> diff --git a/include/mini-os/xenbus.h b/include/mini-os/xenbus.h
> index 7e70de0..b8d152d 100644
> --- a/include/mini-os/xenbus.h
> +++ b/include/mini-os/xenbus.h
> @@ -23,11 +23,18 @@ static inline void init_xenbus(void)
>     set to a malloc'd copy of the value. */
>  char *xenbus_read(xenbus_transaction_t xbt, const char *path, char **value);
>  
> -/* Watch event queue */
> +/* Queue for events (watches or async request replies - see below) */
>  struct xenbus_event {
> -    /* Keep these two as this for xs.c */
> -    char *path;
> -    char *token;
> +    union {
> +        struct {
> +            /* must be first, both for the bare minios xs.c, and for
> +             * xenbus_wait_for_watch's handling */
> +            char *path;
> +            char *token;
> +        };
> +        struct xsd_sockmsg *reply;
> +    };
> +    struct xenbus_watch *watch;
>      MINIOS_STAILQ_ENTRY(xenbus_event) entry;
>  };
>  struct xenbus_event_queue {
> @@ -111,6 +118,65 @@ char* xenbus_printf(xenbus_transaction_t xbt,
>  /* Utility function to figure out our domain id */
>  domid_t xenbus_get_self_id(void);
>  
> +/*
> + * ----- asynchronous low-level interface -----
> + */
> +
> +/* Allocate an identifier for a xenbus request.  Blocks if none are
> + * available.  Cannot fail.  On return, we may use the returned value
> + * as the id in a xenbus request.
> + *
> + * for_queue must already be allocated, but may be uninitialised.
> + *
> + * for_queue->watch is not touched by the xenbus machinery for
> + * handling requests/replies but should probably be initialised by the
> + * caller (probably to NULL) because this will help the caller
> + * distinguish the reply from any watch events which might end up in
> + * the same queue.
> + *
> + * reply_queue must exist and have been initialised.
> + *
> + * When the response arrives, the reply message will stored in
> + * for_queue->reply and for_queue will be queued on reply_queue.  The
> + * id must be then explicitly released (or, used again, if desired).
> + * After ->reply is done with the caller must pass it to free().
> + * (Do not use the id for more than one request at a time.) */
> +int xenbus_id_allocate(struct xenbus_event_queue *reply_queue,
> +                       struct xenbus_event *for_queue);
> +void xenbus_id_release(int id);
> +
> +/* Allocating a token for a watch.
> + *
> + * To use this:
> + *  - Include struct xenbus_watch in your own struct.
> + *  - Set events; then call prepare.  This will set token.
> + *    You may then use token in a WATCH request.
> + *  - You must UNWATCH before you call release.
> + * Do not modify token yourself.
> + * entry is private for the xenbus driver.
> + *
> + * When the watch fires, a new struct xenbus_event will be allocated
> + * and queued on events.  The field xenbus_event->watch will have been
> + * set to watch by the xenbus machinery, and xenbus_event->path will
> + * be the watch path.  After the caller is done with the event,
> + * its pointer should simply be passed to free(). */
> +struct xenbus_watch {
> +    char *token;
> +    struct xenbus_event_queue *events;
> +    MINIOS_LIST_ENTRY(xenbus_watch) entry;
> +};
> +void xenbus_watch_init(struct xenbus_watch *watch); /* makes release a noop */
> +void xenbus_watch_prepare(struct xenbus_watch *watch); /* need not be init'd */
> +void xenbus_watch_release(struct xenbus_watch *watch); /* idempotent */
> +
> +
> +/* Send data to xenbus.  This can block.  All of the requests are seen
> + * by xenbus as if sent atomically.  The header is added
> + * automatically, using type %type, req_id %req_id, and trans_id
> + * %trans_id. */
> +void xenbus_xb_write(int type, int req_id, xenbus_transaction_t trans_id,
> +		     const struct write_req *req, int nr_reqs);
> +
>  #ifdef CONFIG_XENBUS
>  /* Reset the XenBus system. */
>  void fini_xenbus(void);
> diff --git a/xen/xenbus/xenbus.c b/xen/xenbus/xenbus.c
> index d2e59b3..bf4bb45 100644
> --- a/xen/xenbus/xenbus.c
> +++ b/xen/xenbus/xenbus.c
> @@ -48,17 +48,11 @@ static DECLARE_WAIT_QUEUE_HEAD(xb_waitq);
>  static spinlock_t xb_lock = SPIN_LOCK_UNLOCKED; /* protects xenbus req ring */
>  
>  struct xenbus_event_queue xenbus_default_watch_queue;
> -struct watch {
> -    char *token;
> -    struct xenbus_event_queue *events;
> -    MINIOS_LIST_ENTRY(watch) entry;
> -};
> -static MINIOS_LIST_HEAD(, watch) watches;
> +static MINIOS_LIST_HEAD(, xenbus_watch) watches;
>  struct xenbus_req_info 
>  {
>      struct xenbus_event_queue *reply_queue; /* non-0 iff in use */
>      struct xenbus_event *for_queue;
> -    void *reply;
>  };
>  
>  
> @@ -263,7 +257,7 @@ static void xenbus_thread_func(void *ign)
>  		struct xenbus_event *event = malloc(sizeof(*event) + msg.len);
>                  struct xenbus_event_queue *events = NULL;
>  		char *data = (char*)event + sizeof(*event);
> -                struct watch *watch;
> +                struct xenbus_watch *watch;
>  
>                  memcpy_from_ring(xenstore_buf->rsp,
>  		    data,
> @@ -277,6 +271,7 @@ static void xenbus_thread_func(void *ign)
>  
>                  MINIOS_LIST_FOREACH(watch, &watches, entry)
>                      if (!strcmp(watch->token, event->token)) {
> +                        event->watch = watch;
>                          events = watch->events;
>                          break;
>                      }
> @@ -291,9 +286,10 @@ static void xenbus_thread_func(void *ign)
>  
>              else
>              {
> -                req_info[msg.req_id].reply = malloc(sizeof(msg) + msg.len);
> +                req_info[msg.req_id].for_queue->reply =
> +                    malloc(sizeof(msg) + msg.len);
>                  memcpy_from_ring(xenstore_buf->rsp,
> -                    req_info[msg.req_id].reply,
> +                    req_info[msg.req_id].for_queue->reply,
>                      MASK_XENSTORE_IDX(xenstore_buf->rsp_cons),
>                      msg.len + sizeof(msg));
>                  xenstore_buf->rsp_cons += msg.len + sizeof(msg);
> @@ -315,7 +311,7 @@ static spinlock_t req_lock = SPIN_LOCK_UNLOCKED;
>  static DECLARE_WAIT_QUEUE_HEAD(req_wq);
>  
>  /* Release a xenbus identifier */
> -static void release_xenbus_id(int id)
> +void xenbus_id_release(int id)
>  {
>      BUG_ON(!req_info[id].reply_queue);
>      spin_lock(&req_lock);
> @@ -326,10 +322,8 @@ static void release_xenbus_id(int id)
>      spin_unlock(&req_lock);
>  }
>  
> -/* Allocate an identifier for a xenbus request.  Blocks if none are
> -   available. */
> -static int allocate_xenbus_id(struct xenbus_event_queue *reply_queue,
> -                              struct xenbus_event *for_queue)
> +int xenbus_id_allocate(struct xenbus_event_queue *reply_queue,
> +                       struct xenbus_event *for_queue)
>  {
>      static int probe;
>      int o_probe;
> @@ -360,6 +354,30 @@ static int allocate_xenbus_id(struct xenbus_event_queue *reply_queue,
>      return o_probe;
>  }
>  
> +void xenbus_watch_init(struct xenbus_watch *watch)
> +{
> +    watch->token = 0;
> +}
> +
> +void xenbus_watch_prepare(struct xenbus_watch *watch)
> +{
> +    BUG_ON(!watch->events);
> +    size_t size = sizeof(void*)*2 + 5;
> +    watch->token = malloc(size);
> +    int r = snprintf(watch->token,size,"*%p",(void*)watch);
> +    BUG_ON(!(r > 0 && r < size));
> +    MINIOS_LIST_INSERT_HEAD(&watches, watch, entry);
> +}
> +
> +void xenbus_watch_release(struct xenbus_watch *watch)
> +{
> +    if (!watch->token)
> +        return;
> +    MINIOS_LIST_REMOVE(watch, entry);
> +    free(watch->token);
> +    watch->token = 0;
> +}
> +
>  /* Initialise xenbus. */
>  void init_xenbus(void)
>  {
> @@ -381,11 +399,7 @@ void fini_xenbus(void)
>  {
>  }
>  
> -/* Send data to xenbus.  This can block.  All of the requests are seen
> -   by xenbus as if sent atomically.  The header is added
> -   automatically, using type %type, req_id %req_id, and trans_id
> -   %trans_id. */
> -static void xb_write(int type, int req_id, xenbus_transaction_t trans_id,
> +void xenbus_xb_write(int type, int req_id, xenbus_transaction_t trans_id,
>  		     const struct write_req *req, int nr_reqs)
>  {
>      XENSTORE_RING_IDX prod;
> @@ -480,16 +494,16 @@ xenbus_msg_reply(int type,
>  
>      xenbus_event_queue_init(&queue);
>  
> -    id = allocate_xenbus_id(&queue,&event_buf);
> +    id = xenbus_id_allocate(&queue,&event_buf);
>  
> -    xb_write(type, id, trans, io, nr_reqs);
> +    xenbus_xb_write(type, id, trans, io, nr_reqs);
>  
>      struct xenbus_event *event = await_event(&queue);
>      BUG_ON(event != &event_buf);
>  
> -    rep = req_info[id].reply;
> +    rep = req_info[id].for_queue->reply;
>      BUG_ON(rep->req_id != id);
> -    release_xenbus_id(id);
> +    xenbus_id_release(id);
>      return rep;
>  }
>  
> @@ -600,7 +614,7 @@ char* xenbus_watch_path_token( xenbus_transaction_t xbt, const char *path, const
>  	{token, strlen(token) + 1},
>      };
>  
> -    struct watch *watch = malloc(sizeof(*watch));
> +    struct xenbus_watch *watch = malloc(sizeof(*watch));
>  
>      char *msg;
>  
> @@ -630,7 +644,7 @@ char* xenbus_unwatch_path_token( xenbus_transaction_t xbt, const char *path, con
>  	{token, strlen(token) + 1},
>      };
>  
> -    struct watch *watch;
> +    struct xenbus_watch *watch;
>  
>      char *msg;
>  
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

-- 
Samuel
As usual, this being a 1.3.x release, I haven't even compiled this
kernel yet.  So if it works, you should be doubly impressed.
(Linus Torvalds, announcing kernel 1.3.3 on the linux-kernel mailing list.)

[PATCH 09/11] mini-os/xenbus: Sort out request and watch locking

[Ian Jackson]

Make the xenbus_req_lock public, and lock it everywhere it is needed.
It needs to protect not just the xenbus request ring itself, but also
a number of internal data structures.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 include/mini-os/xenbus.h |    7 +++++++
 xen/xenbus/xenbus.c      |   42 ++++++++++++++++++++++++++++++++++++------
 2 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/include/mini-os/xenbus.h b/include/mini-os/xenbus.h
index b8d152d..a811c19 100644
--- a/include/mini-os/xenbus.h
+++ b/include/mini-os/xenbus.h
@@ -5,6 +5,7 @@
 #include <mini-os/sched.h>
 #include <mini-os/waittypes.h>
 #include <mini-os/queue.h>
+#include <mini-os/spinlock.h>
 
 typedef unsigned long xenbus_transaction_t;
 #define XBT_NIL ((xenbus_transaction_t)0)
@@ -23,6 +24,12 @@ static inline void init_xenbus(void)
    set to a malloc'd copy of the value. */
 char *xenbus_read(xenbus_transaction_t xbt, const char *path, char **value);
 
+/* All accesses to an active xenbus_event_queue must occur with this
+ * lock held.  The public functions here will do that for you, but
+ * your own accesses to the queue (including the contained waitq)
+ * must be protected by the lock. */
+extern spinlock_t xenbus_req_lock;
+
 /* Queue for events (watches or async request replies - see below) */
 struct xenbus_event {
     union {
diff --git a/xen/xenbus/xenbus.c b/xen/xenbus/xenbus.c
index bf4bb45..7b391c5 100644
--- a/xen/xenbus/xenbus.c
+++ b/xen/xenbus/xenbus.c
@@ -56,6 +56,17 @@ struct xenbus_req_info
 };
 
 
+spinlock_t xenbus_req_lock = SPIN_LOCK_UNLOCKED;
+/*
+ * This lock protects:
+ *    the xenbus request ring
+ *    req_info[]
+ *    all live struct xenbus_event_queue (including xenbus_default_watch_queue)
+ *    nr_live_reqs
+ *    req_wq
+ *    watches
+ */
+
 void xenbus_event_queue_init(struct xenbus_event_queue *queue)
 {
     MINIOS_STAILQ_INIT(&queue->events);
@@ -64,6 +75,7 @@ void xenbus_event_queue_init(struct xenbus_event_queue *queue)
 
 static struct xenbus_event *remove_event(struct xenbus_event_queue *queue)
 {
+    /* Called with lock held */
     struct xenbus_event *event;
 
     event = MINIOS_STAILQ_FIRST(&queue->events);
@@ -78,6 +90,7 @@ static struct xenbus_event *remove_event(struct xenbus_event_queue *queue)
 static void queue_event(struct xenbus_event_queue *queue,
                         struct xenbus_event *event)
 {
+    /* Called with lock held */
     MINIOS_STAILQ_INSERT_TAIL(&queue->events, event, entry);
     wake_up(&queue->waitq);
 }
@@ -86,11 +99,15 @@ static struct xenbus_event *await_event(struct xenbus_event_queue *queue)
 {
     struct xenbus_event *event;
     DEFINE_WAIT(w);
+    spin_lock(&xenbus_req_lock);
     while (!(event = remove_event(queue))) {
         add_waiter(w, queue->waitq);
+        spin_unlock(&xenbus_req_lock);
         schedule();
+        spin_lock(&xenbus_req_lock);
     }
     remove_waiter(w, queue->waitq);
+    spin_unlock(&xenbus_req_lock);
     return event;
 }
 
@@ -269,6 +286,8 @@ static void xenbus_thread_func(void *ign)
 
                 xenstore_buf->rsp_cons += msg.len + sizeof(msg);
 
+                spin_lock(&xenbus_req_lock);
+
                 MINIOS_LIST_FOREACH(watch, &watches, entry)
                     if (!strcmp(watch->token, event->token)) {
                         event->watch = watch;
@@ -282,6 +301,8 @@ static void xenbus_thread_func(void *ign)
                     printk("unexpected watch token %s\n", event->token);
                     free(event);
                 }
+
+                spin_unlock(&xenbus_req_lock);
             }
 
             else
@@ -293,8 +314,10 @@ static void xenbus_thread_func(void *ign)
                     MASK_XENSTORE_IDX(xenstore_buf->rsp_cons),
                     msg.len + sizeof(msg));
                 xenstore_buf->rsp_cons += msg.len + sizeof(msg);
+                spin_lock(&xenbus_req_lock);
                 queue_event(req_info[msg.req_id].reply_queue,
                             req_info[msg.req_id].for_queue);
+                spin_unlock(&xenbus_req_lock);
             }
         }
     }
@@ -307,19 +330,18 @@ static void xenbus_evtchn_handler(evtchn_port_t port, struct pt_regs *regs,
 }
 
 static int nr_live_reqs;
-static spinlock_t req_lock = SPIN_LOCK_UNLOCKED;
 static DECLARE_WAIT_QUEUE_HEAD(req_wq);
 
 /* Release a xenbus identifier */
 void xenbus_id_release(int id)
 {
     BUG_ON(!req_info[id].reply_queue);
-    spin_lock(&req_lock);
+    spin_lock(&xenbus_req_lock);
     req_info[id].reply_queue = 0;
     nr_live_reqs--;
     if (nr_live_reqs == NR_REQS - 1)
         wake_up(&req_wq);
-    spin_unlock(&req_lock);
+    spin_unlock(&xenbus_req_lock);
 }
 
 int xenbus_id_allocate(struct xenbus_event_queue *reply_queue,
@@ -330,10 +352,10 @@ int xenbus_id_allocate(struct xenbus_event_queue *reply_queue,
 
     while (1) 
     {
-        spin_lock(&req_lock);
+        spin_lock(&xenbus_req_lock);
         if (nr_live_reqs < NR_REQS)
             break;
-        spin_unlock(&req_lock);
+        spin_unlock(&xenbus_req_lock);
         wait_event(req_wq, (nr_live_reqs < NR_REQS));
     }
 
@@ -349,7 +371,7 @@ int xenbus_id_allocate(struct xenbus_event_queue *reply_queue,
     req_info[o_probe].reply_queue = reply_queue;
     req_info[o_probe].for_queue = for_queue;
     probe = (o_probe + 1) % NR_REQS;
-    spin_unlock(&req_lock);
+    spin_unlock(&xenbus_req_lock);
 
     return o_probe;
 }
@@ -366,14 +388,18 @@ void xenbus_watch_prepare(struct xenbus_watch *watch)
     watch->token = malloc(size);
     int r = snprintf(watch->token,size,"*%p",(void*)watch);
     BUG_ON(!(r > 0 && r < size));
+    spin_lock(&xenbus_req_lock);
     MINIOS_LIST_INSERT_HEAD(&watches, watch, entry);
+    spin_unlock(&xenbus_req_lock);
 }
 
 void xenbus_watch_release(struct xenbus_watch *watch)
 {
     if (!watch->token)
         return;
+    spin_lock(&xenbus_req_lock);
     MINIOS_LIST_REMOVE(watch, entry);
+    spin_unlock(&xenbus_req_lock);
     free(watch->token);
     watch->token = 0;
 }
@@ -624,7 +650,9 @@ char* xenbus_watch_path_token( xenbus_transaction_t xbt, const char *path, const
     watch->token = strdup(token);
     watch->events = events;
 
+    spin_lock(&xenbus_req_lock);
     MINIOS_LIST_INSERT_HEAD(&watches, watch, entry);
+    spin_unlock(&xenbus_req_lock);
 
     rep = xenbus_msg_reply(XS_WATCH, xbt, req, ARRAY_SIZE(req));
 
@@ -654,6 +682,7 @@ char* xenbus_unwatch_path_token( xenbus_transaction_t xbt, const char *path, con
     if (msg) return msg;
     free(rep);
 
+    spin_lock(&xenbus_req_lock);
     MINIOS_LIST_FOREACH(watch, &watches, entry)
         if (!strcmp(watch->token, token)) {
             free(watch->token);
@@ -661,6 +690,7 @@ char* xenbus_unwatch_path_token( xenbus_transaction_t xbt, const char *path, con
             free(watch);
             break;
         }
+    spin_unlock(&xenbus_req_lock);
 
     return NULL;
 }
-- 
1.7.10.4

Re: [PATCH 09/11] mini-os/xenbus: Sort out request and watch locking

[Samuel Thibault]

Ian Jackson, le Fri 20 Jun 2014 20:04:48 +0100, a écrit :
> Make the xenbus_req_lock public, and lock it everywhere it is needed.
> It needs to protect not just the xenbus request ring itself, but also
> a number of internal data structures.
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
>  include/mini-os/xenbus.h |    7 +++++++
>  xen/xenbus/xenbus.c      |   42 ++++++++++++++++++++++++++++++++++++------
>  2 files changed, 43 insertions(+), 6 deletions(-)
> 
> diff --git a/include/mini-os/xenbus.h b/include/mini-os/xenbus.h
> index b8d152d..a811c19 100644
> --- a/include/mini-os/xenbus.h
> +++ b/include/mini-os/xenbus.h
> @@ -5,6 +5,7 @@
>  #include <mini-os/sched.h>
>  #include <mini-os/waittypes.h>
>  #include <mini-os/queue.h>
> +#include <mini-os/spinlock.h>
>  
>  typedef unsigned long xenbus_transaction_t;
>  #define XBT_NIL ((xenbus_transaction_t)0)
> @@ -23,6 +24,12 @@ static inline void init_xenbus(void)
>     set to a malloc'd copy of the value. */
>  char *xenbus_read(xenbus_transaction_t xbt, const char *path, char **value);
>  
> +/* All accesses to an active xenbus_event_queue must occur with this
> + * lock held.  The public functions here will do that for you, but
> + * your own accesses to the queue (including the contained waitq)
> + * must be protected by the lock. */
> +extern spinlock_t xenbus_req_lock;
> +
>  /* Queue for events (watches or async request replies - see below) */
>  struct xenbus_event {
>      union {
> diff --git a/xen/xenbus/xenbus.c b/xen/xenbus/xenbus.c
> index bf4bb45..7b391c5 100644
> --- a/xen/xenbus/xenbus.c
> +++ b/xen/xenbus/xenbus.c
> @@ -56,6 +56,17 @@ struct xenbus_req_info
>  };
>  
>  
> +spinlock_t xenbus_req_lock = SPIN_LOCK_UNLOCKED;
> +/*
> + * This lock protects:
> + *    the xenbus request ring
> + *    req_info[]
> + *    all live struct xenbus_event_queue (including xenbus_default_watch_queue)
> + *    nr_live_reqs
> + *    req_wq
> + *    watches
> + */
> +
>  void xenbus_event_queue_init(struct xenbus_event_queue *queue)
>  {
>      MINIOS_STAILQ_INIT(&queue->events);
> @@ -64,6 +75,7 @@ void xenbus_event_queue_init(struct xenbus_event_queue *queue)
>  
>  static struct xenbus_event *remove_event(struct xenbus_event_queue *queue)
>  {
> +    /* Called with lock held */
>      struct xenbus_event *event;
>  
>      event = MINIOS_STAILQ_FIRST(&queue->events);
> @@ -78,6 +90,7 @@ static struct xenbus_event *remove_event(struct xenbus_event_queue *queue)
>  static void queue_event(struct xenbus_event_queue *queue,
>                          struct xenbus_event *event)
>  {
> +    /* Called with lock held */
>      MINIOS_STAILQ_INSERT_TAIL(&queue->events, event, entry);
>      wake_up(&queue->waitq);
>  }
> @@ -86,11 +99,15 @@ static struct xenbus_event *await_event(struct xenbus_event_queue *queue)
>  {
>      struct xenbus_event *event;
>      DEFINE_WAIT(w);
> +    spin_lock(&xenbus_req_lock);
>      while (!(event = remove_event(queue))) {
>          add_waiter(w, queue->waitq);
> +        spin_unlock(&xenbus_req_lock);
>          schedule();
> +        spin_lock(&xenbus_req_lock);
>      }
>      remove_waiter(w, queue->waitq);
> +    spin_unlock(&xenbus_req_lock);
>      return event;
>  }
>  
> @@ -269,6 +286,8 @@ static void xenbus_thread_func(void *ign)
>  
>                  xenstore_buf->rsp_cons += msg.len + sizeof(msg);
>  
> +                spin_lock(&xenbus_req_lock);
> +
>                  MINIOS_LIST_FOREACH(watch, &watches, entry)
>                      if (!strcmp(watch->token, event->token)) {
>                          event->watch = watch;
> @@ -282,6 +301,8 @@ static void xenbus_thread_func(void *ign)
>                      printk("unexpected watch token %s\n", event->token);
>                      free(event);
>                  }
> +
> +                spin_unlock(&xenbus_req_lock);
>              }
>  
>              else
> @@ -293,8 +314,10 @@ static void xenbus_thread_func(void *ign)
>                      MASK_XENSTORE_IDX(xenstore_buf->rsp_cons),
>                      msg.len + sizeof(msg));
>                  xenstore_buf->rsp_cons += msg.len + sizeof(msg);
> +                spin_lock(&xenbus_req_lock);
>                  queue_event(req_info[msg.req_id].reply_queue,
>                              req_info[msg.req_id].for_queue);
> +                spin_unlock(&xenbus_req_lock);
>              }
>          }
>      }
> @@ -307,19 +330,18 @@ static void xenbus_evtchn_handler(evtchn_port_t port, struct pt_regs *regs,
>  }
>  
>  static int nr_live_reqs;
> -static spinlock_t req_lock = SPIN_LOCK_UNLOCKED;
>  static DECLARE_WAIT_QUEUE_HEAD(req_wq);
>  
>  /* Release a xenbus identifier */
>  void xenbus_id_release(int id)
>  {
>      BUG_ON(!req_info[id].reply_queue);
> -    spin_lock(&req_lock);
> +    spin_lock(&xenbus_req_lock);
>      req_info[id].reply_queue = 0;
>      nr_live_reqs--;
>      if (nr_live_reqs == NR_REQS - 1)
>          wake_up(&req_wq);
> -    spin_unlock(&req_lock);
> +    spin_unlock(&xenbus_req_lock);
>  }
>  
>  int xenbus_id_allocate(struct xenbus_event_queue *reply_queue,
> @@ -330,10 +352,10 @@ int xenbus_id_allocate(struct xenbus_event_queue *reply_queue,
>  
>      while (1) 
>      {
> -        spin_lock(&req_lock);
> +        spin_lock(&xenbus_req_lock);
>          if (nr_live_reqs < NR_REQS)
>              break;
> -        spin_unlock(&req_lock);
> +        spin_unlock(&xenbus_req_lock);
>          wait_event(req_wq, (nr_live_reqs < NR_REQS));
>      }
>  
> @@ -349,7 +371,7 @@ int xenbus_id_allocate(struct xenbus_event_queue *reply_queue,
>      req_info[o_probe].reply_queue = reply_queue;
>      req_info[o_probe].for_queue = for_queue;
>      probe = (o_probe + 1) % NR_REQS;
> -    spin_unlock(&req_lock);
> +    spin_unlock(&xenbus_req_lock);
>  
>      return o_probe;
>  }
> @@ -366,14 +388,18 @@ void xenbus_watch_prepare(struct xenbus_watch *watch)
>      watch->token = malloc(size);
>      int r = snprintf(watch->token,size,"*%p",(void*)watch);
>      BUG_ON(!(r > 0 && r < size));
> +    spin_lock(&xenbus_req_lock);
>      MINIOS_LIST_INSERT_HEAD(&watches, watch, entry);
> +    spin_unlock(&xenbus_req_lock);
>  }
>  
>  void xenbus_watch_release(struct xenbus_watch *watch)
>  {
>      if (!watch->token)
>          return;
> +    spin_lock(&xenbus_req_lock);
>      MINIOS_LIST_REMOVE(watch, entry);
> +    spin_unlock(&xenbus_req_lock);
>      free(watch->token);
>      watch->token = 0;
>  }
> @@ -624,7 +650,9 @@ char* xenbus_watch_path_token( xenbus_transaction_t xbt, const char *path, const
>      watch->token = strdup(token);
>      watch->events = events;
>  
> +    spin_lock(&xenbus_req_lock);
>      MINIOS_LIST_INSERT_HEAD(&watches, watch, entry);
> +    spin_unlock(&xenbus_req_lock);
>  
>      rep = xenbus_msg_reply(XS_WATCH, xbt, req, ARRAY_SIZE(req));
>  
> @@ -654,6 +682,7 @@ char* xenbus_unwatch_path_token( xenbus_transaction_t xbt, const char *path, con
>      if (msg) return msg;
>      free(rep);
>  
> +    spin_lock(&xenbus_req_lock);
>      MINIOS_LIST_FOREACH(watch, &watches, entry)
>          if (!strcmp(watch->token, token)) {
>              free(watch->token);
> @@ -661,6 +690,7 @@ char* xenbus_unwatch_path_token( xenbus_transaction_t xbt, const char *path, con
>              free(watch);
>              break;
>          }
> +    spin_unlock(&xenbus_req_lock);
>  
>      return NULL;
>  }
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

-- 
Samuel
<g> r: et la marmotte, elle écrit un papier IPDPS

[PATCH 10/11] mini-os/xenbus: Provide queue->wakeup hook

[Ian Jackson]

This allows xenbus's caller to get called back when an item is put on
the queue, rather than simply having the waitq signaled.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 include/mini-os/xenbus.h |   15 +++++++++++++++
 xen/xenbus/xenbus.c      |    8 +++++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/include/mini-os/xenbus.h b/include/mini-os/xenbus.h
index a811c19..1900e55 100644
--- a/include/mini-os/xenbus.h
+++ b/include/mini-os/xenbus.h
@@ -46,6 +46,7 @@ struct xenbus_event {
 };
 struct xenbus_event_queue {
     MINIOS_STAILQ_HEAD(, xenbus_event) events;
+    void (*wakeup)(struct xenbus_event_queue*); /* can be safely ignored */
     struct wait_queue_head waitq;
 };
 
@@ -129,6 +130,20 @@ domid_t xenbus_get_self_id(void);
  * ----- asynchronous low-level interface -----
  */
 
+/*
+ * Use of queue->wakeup:
+ *
+ * If queue->wakeup is set, it will be called instead of
+ * wake_up(&queue->waitq);
+ *
+ * queue->wakeup is initialised (to a function which just calls
+ * wake_up) by xenbus_event_queue_init.  The user who wants something
+ * different should set ->wakeup after the init, but before the queue
+ * is used for xenbus_id_allocate or xenbus_watch_prepare.
+ *
+ * queue->wakeup() is called with the req_lock held.
+ */
+
 /* Allocate an identifier for a xenbus request.  Blocks if none are
  * available.  Cannot fail.  On return, we may use the returned value
  * as the id in a xenbus request.
diff --git a/xen/xenbus/xenbus.c b/xen/xenbus/xenbus.c
index 7b391c5..e5d7f36 100644
--- a/xen/xenbus/xenbus.c
+++ b/xen/xenbus/xenbus.c
@@ -67,9 +67,15 @@ spinlock_t xenbus_req_lock = SPIN_LOCK_UNLOCKED;
  *    watches
  */
 
+static void queue_wakeup(struct xenbus_event_queue *queue)
+{
+    wake_up(&queue->waitq);
+}
+
 void xenbus_event_queue_init(struct xenbus_event_queue *queue)
 {
     MINIOS_STAILQ_INIT(&queue->events);
+    queue->wakeup = queue_wakeup;
     init_waitqueue_head(&queue->waitq);
 }
 
@@ -92,7 +98,7 @@ static void queue_event(struct xenbus_event_queue *queue,
 {
     /* Called with lock held */
     MINIOS_STAILQ_INSERT_TAIL(&queue->events, event, entry);
-    wake_up(&queue->waitq);
+    queue->wakeup(queue);
 }
 
 static struct xenbus_event *await_event(struct xenbus_event_queue *queue)
-- 
1.7.10.4

Re: [PATCH 10/11] mini-os/xenbus: Provide queue->wakeup hook

[Samuel Thibault]

Ian Jackson, le Fri 20 Jun 2014 20:04:49 +0100, a écrit :
> This allows xenbus's caller to get called back when an item is put on
> the queue, rather than simply having the waitq signaled.

Mmm, so the callback will be made with lock held, is that not a problem
in the use you plan?

Although I don't usually like callbacks precisely for that kind of
reason, I'm not against it, but it should at least be documented.

> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  include/mini-os/xenbus.h |   15 +++++++++++++++
>  xen/xenbus/xenbus.c      |    8 +++++++-
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/include/mini-os/xenbus.h b/include/mini-os/xenbus.h
> index a811c19..1900e55 100644
> --- a/include/mini-os/xenbus.h
> +++ b/include/mini-os/xenbus.h
> @@ -46,6 +46,7 @@ struct xenbus_event {
>  };
>  struct xenbus_event_queue {
>      MINIOS_STAILQ_HEAD(, xenbus_event) events;
> +    void (*wakeup)(struct xenbus_event_queue*); /* can be safely ignored */
>      struct wait_queue_head waitq;
>  };
>  
> @@ -129,6 +130,20 @@ domid_t xenbus_get_self_id(void);
>   * ----- asynchronous low-level interface -----
>   */
>  
> +/*
> + * Use of queue->wakeup:
> + *
> + * If queue->wakeup is set, it will be called instead of
> + * wake_up(&queue->waitq);
> + *
> + * queue->wakeup is initialised (to a function which just calls
> + * wake_up) by xenbus_event_queue_init.  The user who wants something
> + * different should set ->wakeup after the init, but before the queue
> + * is used for xenbus_id_allocate or xenbus_watch_prepare.
> + *
> + * queue->wakeup() is called with the req_lock held.
> + */
> +
>  /* Allocate an identifier for a xenbus request.  Blocks if none are
>   * available.  Cannot fail.  On return, we may use the returned value
>   * as the id in a xenbus request.
> diff --git a/xen/xenbus/xenbus.c b/xen/xenbus/xenbus.c
> index 7b391c5..e5d7f36 100644
> --- a/xen/xenbus/xenbus.c
> +++ b/xen/xenbus/xenbus.c
> @@ -67,9 +67,15 @@ spinlock_t xenbus_req_lock = SPIN_LOCK_UNLOCKED;
>   *    watches
>   */
>  
> +static void queue_wakeup(struct xenbus_event_queue *queue)
> +{
> +    wake_up(&queue->waitq);
> +}
> +
>  void xenbus_event_queue_init(struct xenbus_event_queue *queue)
>  {
>      MINIOS_STAILQ_INIT(&queue->events);
> +    queue->wakeup = queue_wakeup;
>      init_waitqueue_head(&queue->waitq);
>  }
>  
> @@ -92,7 +98,7 @@ static void queue_event(struct xenbus_event_queue *queue,
>  {
>      /* Called with lock held */
>      MINIOS_STAILQ_INSERT_TAIL(&queue->events, event, entry);
> -    wake_up(&queue->waitq);
> +    queue->wakeup(queue);
>  }
>  
>  static struct xenbus_event *await_event(struct xenbus_event_queue *queue)
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

-- 
Samuel
<m> argh, pi est plus grand que 2. Ca casse tout
 -+- #ens-mim -+-

Re: [PATCH 10/11] mini-os/xenbus: Provide queue->wakeup hook

[Ian Jackson]

Samuel Thibault writes ("Re: [Xen-devel] [PATCH 10/11] mini-os/xenbus: Provide queue->wakeup hook"):
> Ian Jackson, le Fri 20 Jun 2014 20:04:49 +0100, a écrit :
> > This allows xenbus's caller to get called back when an item is put on
> > the queue, rather than simply having the waitq signaled.
> 
> Mmm, so the callback will be made with lock held, is that not a problem
> in the use you plan?

No.  My callback function just needs to call a NetBSD kernel file
descriptor poll() wakeup hook.

> Although I don't usually like callbacks precisely for that kind of
> reason, I'm not against it, but it should at least be documented.
...
> > +/*
> > + * Use of queue->wakeup:
> > + *
> > + * If queue->wakeup is set, it will be called instead of
> > + * wake_up(&queue->waitq);
> > + *
> > + * queue->wakeup is initialised (to a function which just calls
> > + * wake_up) by xenbus_event_queue_init.  The user who wants something
> > + * different should set ->wakeup after the init, but before the queue
> > + * is used for xenbus_id_allocate or xenbus_watch_prepare.
> > + *
> > + * queue->wakeup() is called with the req_lock held.
> > + */

Here it is :-).

Thanks,
Ian.

[PATCH 11/11] mini-os/xenbus: Provide xenbus_free

[Ian Jackson]

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 include/mini-os/xenbus.h |    5 +++++
 xen/xenbus/xenbus.c      |    2 ++
 2 files changed, 7 insertions(+)

diff --git a/include/mini-os/xenbus.h b/include/mini-os/xenbus.h
index 1900e55..0e45d47 100644
--- a/include/mini-os/xenbus.h
+++ b/include/mini-os/xenbus.h
@@ -199,6 +199,11 @@ void xenbus_watch_release(struct xenbus_watch *watch); /* idempotent */
 void xenbus_xb_write(int type, int req_id, xenbus_transaction_t trans_id,
 		     const struct write_req *req, int nr_reqs);
 
+void xenbus_free(void*);
+/* If the caller is in a scope which uses a different malloc arena,
+ * it must use this rather than free() when freeing data received
+ * from xenbus. */
+
 #ifdef CONFIG_XENBUS
 /* Reset the XenBus system. */
 void fini_xenbus(void);
diff --git a/xen/xenbus/xenbus.c b/xen/xenbus/xenbus.c
index e5d7f36..77b9603 100644
--- a/xen/xenbus/xenbus.c
+++ b/xen/xenbus/xenbus.c
@@ -539,6 +539,8 @@ xenbus_msg_reply(int type,
     return rep;
 }
 
+void xenbus_free(void *p) { free(p); }
+
 static char *errmsg(struct xsd_sockmsg *rep)
 {
     char *res;
-- 
1.7.10.4

Re: [PATCH 11/11] mini-os/xenbus: Provide xenbus_free

[Samuel Thibault]

Ian Jackson, le Fri 20 Jun 2014 20:04:50 +0100, a écrit :
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
>  include/mini-os/xenbus.h |    5 +++++
>  xen/xenbus/xenbus.c      |    2 ++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/include/mini-os/xenbus.h b/include/mini-os/xenbus.h
> index 1900e55..0e45d47 100644
> --- a/include/mini-os/xenbus.h
> +++ b/include/mini-os/xenbus.h
> @@ -199,6 +199,11 @@ void xenbus_watch_release(struct xenbus_watch *watch); /* idempotent */
>  void xenbus_xb_write(int type, int req_id, xenbus_transaction_t trans_id,
>  		     const struct write_req *req, int nr_reqs);
>  
> +void xenbus_free(void*);
> +/* If the caller is in a scope which uses a different malloc arena,
> + * it must use this rather than free() when freeing data received
> + * from xenbus. */
> +
>  #ifdef CONFIG_XENBUS
>  /* Reset the XenBus system. */
>  void fini_xenbus(void);
> diff --git a/xen/xenbus/xenbus.c b/xen/xenbus/xenbus.c
> index e5d7f36..77b9603 100644
> --- a/xen/xenbus/xenbus.c
> +++ b/xen/xenbus/xenbus.c
> @@ -539,6 +539,8 @@ xenbus_msg_reply(int type,
>      return rep;
>  }
>  
> +void xenbus_free(void *p) { free(p); }
> +
>  static char *errmsg(struct xsd_sockmsg *rep)
>  {
>      char *res;
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

-- 
Samuel
Fatal Error: Found [MS-Windows] System -> Repartitioning Disk for Linux...
(By cbbrown@io.org, Christopher Browne)

Re: [RFC PATCH 00/11] mini-os: xenbus changes for rump kernels

[George Dunlap]

On Fri, Jun 20, 2014 at 8:04 PM, Ian Jackson <ian.jackson@eu.citrix.com> wrote:
> I have been working on a facility for the NetBSD-based rump kernels to
> allow programs to make xenstore queries using libxenstore.  IMO the
> best way to do this is to provide a working /dev/xen/xenbus.  I have
> achieved this, and will be submitting the results to the rump kernel
> project.
>
> However, mini-os's xenbus driver was not suited to properly
> multiplexing and ordering requests from multiple callers, and some of
> the mini-os headers were a bit hostile towards the NetBSD kernel
> environment the see when they are #included from a rump kernel device
> driver component.  So I had to make some changes to minios.
>
> The rump kernel project has its own fork of minios (although the
> divergence is reasonably limited).  So my changes to minios are
> actually changes to rumpuser-xen's fork of minios.
>
> But before I ask the rump kernel developers to pull my nontrivial
> changes, I felt I should give the Xen community a chance to object.
> Hence this RFC series.
>
> The code including my complete working branch of rumpuser-xen is also
> available here:
>     git://xenbits.xen.org/people/iwj/rumpuser-xen.git
> in the commits
>     base.dev-xen-xenbus.v1..dev-xen-xenbus.v1
> or via gitweb:
>     http://xenbits.xen.org/gitweb/?p=people/iwj/rumpuser-xen.git;a=log;h=refs/tags/dev-xen-xenbus.v1
>
> The changes I would like Xen community review for are these:

CC'ing the minios maintainer.

 -G

>
>   01/11 mini-os: Make some headers more rumpkernel-friendly
>   02/11 mini-os: Provide <mini-os/queue.h>
>   03/11 mini-os/xenbus: Add missing locks to xb_write
>   04/11 mini-os/xenbus: Change type of xenbus_event_queue
>   05/11 mini-os/xenbus: Use MINIOS_LIST for the list of watches
>   06/11 mini-os/xenbus: Rename xenbus_events to xenbus_default_watch_queue
>   07/11 mini-os/xenbus: Unify watch and reply queues
>   08/11 mini-os/xenbus: Expose lower-level interface
>   09/11 mini-os/xenbus: Sort out request and watch locking
>   10/11 mini-os/xenbus: Provide queue->wakeup hook
>   11/11 mini-os/xenbus: Provide xenbus_free
>
> Thanks,
> Ian.
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [RFC PATCH 00/11] mini-os: xenbus changes for rump kernels

[Ian Jackson]

George Dunlap writes ("Re: [Xen-devel] [RFC PATCH 00/11] mini-os: xenbus changes for rump kernels"):
> On Fri, Jun 20, 2014 at 8:04 PM, Ian Jackson <ian.jackson@eu.citrix.com> wrote:
> > But before I ask the rump kernel developers to pull my nontrivial
> > changes, I felt I should give the Xen community a chance to object.
> > Hence this RFC series.
...
> > The changes I would like Xen community review for are these:
> 
> CC'ing the minios maintainer.

Thanks.  Sorry for omitting to do that myself; I did search
MAINTAINERS but I must have made a typo in my search term.

Ian.

Re: [RFC PATCH 00/11] mini-os: xenbus changes for rump kernels

[Andrew Cooper]

On 20/06/14 20:04, Ian Jackson wrote:
> I have been working on a facility for the NetBSD-based rump kernels to
> allow programs to make xenstore queries using libxenstore.  IMO the
> best way to do this is to provide a working /dev/xen/xenbus.  I have
> achieved this, and will be submitting the results to the rump kernel
> project.
>
> However, mini-os's xenbus driver was not suited to properly
> multiplexing and ordering requests from multiple callers, and some of
> the mini-os headers were a bit hostile towards the NetBSD kernel
> environment the see when they are #included from a rump kernel device
> driver component.  So I had to make some changes to minios.
>
> The rump kernel project has its own fork of minios (although the
> divergence is reasonably limited).  So my changes to minios are
> actually changes to rumpuser-xen's fork of minios.
>
> But before I ask the rump kernel developers to pull my nontrivial
> changes, I felt I should give the Xen community a chance to object.
> Hence this RFC series.

Are these changes indented for the Xen tree, or the rump fork tree?

It would be preferable to try and avoid forks, if that can be achieved
by cleaning up the existing minios tree to be more amenable to extending
without having to hack at it?  I seem to remember the Mirage people
expressing similar views.

~Andrew

>
> The code including my complete working branch of rumpuser-xen is also
> available here:
>     git://xenbits.xen.org/people/iwj/rumpuser-xen.git
> in the commits
>     base.dev-xen-xenbus.v1..dev-xen-xenbus.v1
> or via gitweb:
>     http://xenbits.xen.org/gitweb/?p=people/iwj/rumpuser-xen.git;a=log;h=refs/tags/dev-xen-xenbus.v1
>
> The changes I would like Xen community review for are these:
>
>   01/11 mini-os: Make some headers more rumpkernel-friendly
>   02/11 mini-os: Provide <mini-os/queue.h>
>   03/11 mini-os/xenbus: Add missing locks to xb_write
>   04/11 mini-os/xenbus: Change type of xenbus_event_queue
>   05/11 mini-os/xenbus: Use MINIOS_LIST for the list of watches
>   06/11 mini-os/xenbus: Rename xenbus_events to xenbus_default_watch_queue
>   07/11 mini-os/xenbus: Unify watch and reply queues
>   08/11 mini-os/xenbus: Expose lower-level interface
>   09/11 mini-os/xenbus: Sort out request and watch locking
>   10/11 mini-os/xenbus: Provide queue->wakeup hook
>   11/11 mini-os/xenbus: Provide xenbus_free
>
> Thanks,
> Ian.
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [RFC PATCH 00/11] mini-os: xenbus changes for rump kernels

[Ian Jackson]

Andrew Cooper writes ("Re: [Xen-devel] [RFC PATCH 00/11] mini-os: xenbus changes for rump kernels"):
> On 20/06/14 20:04, Ian Jackson wrote:
...
> > But before I ask the rump kernel developers to pull my nontrivial
> > changes, I felt I should give the Xen community a chance to object.
> > Hence this RFC series.
> 
> Are these changes indented for the Xen tree, or the rump fork tree?

These are for the rump fork tree, which already exists and has
diverged (and not by me).  I intend to try to unify them again but not
now.

> It would be preferable to try and avoid forks, if that can be achieved
> by cleaning up the existing minios tree to be more amenable to extending
> without having to hack at it?

Contributions of effort welcome :-).

>  I seem to remember the Mirage people expressing similar views.

Yes.

Ian.

Re: [RFC PATCH 00/11] mini-os: xenbus changes for rump kernels

[Ian Campbell]

On Mon, 2014-06-23 at 15:26 +0100, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [Xen-devel] [RFC PATCH 00/11] mini-os: xenbus changes for rump kernels"):
> > On 20/06/14 20:04, Ian Jackson wrote:
> ...
> > > But before I ask the rump kernel developers to pull my nontrivial
> > > changes, I felt I should give the Xen community a chance to object.
> > > Hence this RFC series.
> > 
> > Are these changes indented for the Xen tree, or the rump fork tree?
> 
> These are for the rump fork tree

So for the purposes of having my committer hat on I should certainly
ignore all these, right?

I take it you would at some point like some review though?

I notice you aren't CCing and rumpkernels lists, was that deliberate?

Ian.

Re: [RFC PATCH 00/11] mini-os: xenbus changes for rump kernels

[Samuel Thibault]

Ian Campbell, le Fri 27 Jun 2014 12:52:22 +0100, a écrit :
> On Mon, 2014-06-23 at 15:26 +0100, Ian Jackson wrote:
> > Andrew Cooper writes ("Re: [Xen-devel] [RFC PATCH 00/11] mini-os: xenbus changes for rump kernels"):
> > > On 20/06/14 20:04, Ian Jackson wrote:
> > ...
> > > > But before I ask the rump kernel developers to pull my nontrivial
> > > > changes, I felt I should give the Xen community a chance to object.
> > > > Hence this RFC series.
> > > 
> > > Are these changes indented for the Xen tree, or the rump fork tree?
> > 
> > These are for the rump fork tree
> 
> So for the purposes of having my committer hat on I should certainly
> ignore all these, right?

Yes, they aren't even meant to apply on the xen tree :)

> I take it you would at some point like some review though?

Well, I guess my Acked-by-s will propagate up to when they'll get back
submitted to the xen tree.

Samuel

Re: [RFC PATCH 00/11] mini-os: xenbus changes for rump kernels

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [RFC PATCH 00/11] mini-os: xenbus changes for rump kernels"):
> So for the purposes of having my committer hat on I should certainly
> ignore all these, right?

Absolutely.

> I take it you would at some point like some review though?

Samuel has helpfully reviewed them now.

> I notice you aren't CCing and rumpkernels lists, was that deliberate?

Yes.  They don't really do patchbombs in the same way we do.  I drew
their attention to it in a different way.  It's in their tree now.

Thanks,
Ian.