[Xenomai-core] [PATCH v3 0/9] heap setup/cleanup fixes, refactorings & more

[Xenomai-core] [PATCH v3 0/9] heap setup/cleanup fixes, refactorings & more

[Jan Kiszka]

Third round of this series. This time I dug deeper into the heap
management, trying to simplify its use which should remove a few race
conditions of the existing code.

The central change is patch 5: xnheap_destroy_mapped will no longer
return an error, it will always start deferred deletion in case the
heap's memory is still in use. This simplifies the use a lot, allowing
among other things to drop -EBUSY from the list of possible return codes
of rt_queue/heap_delete.

The series furthermore contains an attempt to fix RTAI's shm code, fixes
the know leakages of rt_mutex/queue/heap auto-deletion and introduces
complete heap statistics (this time without using a Linux spin lock).

Please pull the series (or cherry-pick individual patches) from

    git://xenomai.org/xenomai-jki.git for-upstream

if there are no concerns.

Jan Kiszka (9):
      native: Release fastlock to the proper heap
      nucleus: Use Linux spin lock for heap list management
      nucleus: Fix race window in heap mapping procedure
      nucleus: xnheap_destroy does not fail
      nucleus: Avoid returning errors from xnheap_destroy_mapped
      rtai: Try to fix _shm_free
      native: Do not requeue on auto-cleanup errors
      native: Fix memory leak on heap/queue auto-deletion
      nucleus: Include all heaps in statistics

 include/asm-generic/bits/heap.h |    2 +-
 include/asm-generic/system.h    |    2 +-
 include/native/ppd.h            |   16 +--
 include/nucleus/heap.h          |   32 +++--
 ksrc/drivers/ipc/iddp.c         |    3 +-
 ksrc/drivers/ipc/xddp.c         |    6 +-
 ksrc/nucleus/heap.c             |  258 +++++++++++++++++++++++++++------------
 ksrc/nucleus/module.c           |    2 +-
 ksrc/nucleus/pod.c              |    5 +-
 ksrc/nucleus/shadow.c           |    5 +-
 ksrc/skins/native/heap.c        |   41 +++---
 ksrc/skins/native/mutex.c       |   14 ++-
 ksrc/skins/native/pipe.c        |    4 +-
 ksrc/skins/native/queue.c       |   34 +++---
 ksrc/skins/native/syscall.c     |   25 +---
 ksrc/skins/posix/shm.c          |    4 +-
 ksrc/skins/psos+/rn.c           |    6 +-
 ksrc/skins/rtai/shm.c           |   47 ++++---
 ksrc/skins/vrtx/heap.c          |    6 +-
 ksrc/skins/vrtx/syscall.c       |    3 +-
 20 files changed, 317 insertions(+), 198 deletions(-)

[1] http://thread.gmane.org/gmane.linux.real-time.xenomai.devel/6559

[Xenomai-core] [PATCH v3 4/9] nucleus: xnheap_destroy does not fail

[Jan Kiszka]

There is no error returned by xnheap_destroy. So drop its return value
and update all callers.

Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
---

 include/nucleus/heap.h    |   12 ++++++------
 ksrc/nucleus/heap.c       |   17 +++++++----------
 ksrc/skins/native/heap.c  |    2 +-
 ksrc/skins/native/queue.c |    2 +-
 ksrc/skins/rtai/shm.c     |    5 ++---
 5 files changed, 17 insertions(+), 21 deletions(-)

diff --git a/include/nucleus/heap.h b/include/nucleus/heap.h
index f39ef64..ca691bf 100644
--- a/include/nucleus/heap.h
+++ b/include/nucleus/heap.h
@@ -226,12 +226,12 @@ int xnheap_init(xnheap_t *heap,
 		u_long heapsize,
 		u_long pagesize);
 
-int xnheap_destroy(xnheap_t *heap,
-		   void (*flushfn)(xnheap_t *heap,
-				   void *extaddr,
-				   u_long extsize,
-				   void *cookie),
-		   void *cookie);
+void xnheap_destroy(xnheap_t *heap,
+		    void (*flushfn)(xnheap_t *heap,
+				    void *extaddr,
+				    u_long extsize,
+				    void *cookie),
+		    void *cookie);
 
 int xnheap_extend(xnheap_t *heap,
 		  void *extaddr,
diff --git a/ksrc/nucleus/heap.c b/ksrc/nucleus/heap.c
index 0a9bfdf..4958daa 100644
--- a/ksrc/nucleus/heap.c
+++ b/ksrc/nucleus/heap.c
@@ -253,9 +253,6 @@ EXPORT_SYMBOL_GPL(xnheap_init);
  * @param cookie If @a flushfn is non-NULL, @a cookie is an opaque
  * pointer which will be passed unmodified to @a flushfn.
  *
- * @return 0 is returned on success, or -EBUSY if external mappings
- * are still pending on the heap memory.
- *
  * Environments:
  *
  * This service can be called from:
@@ -267,16 +264,17 @@ EXPORT_SYMBOL_GPL(xnheap_init);
  * Rescheduling: never.
  */
 
-int xnheap_destroy(xnheap_t *heap,
-		   void (*flushfn) (xnheap_t *heap,
+void xnheap_destroy(xnheap_t *heap,
+		    void (*flushfn)(xnheap_t *heap,
 				    void *extaddr,
-				    u_long extsize, void *cookie), void *cookie)
+				    u_long extsize, void *cookie),
+		    void *cookie)
 {
 	xnholder_t *holder;
 	spl_t s;
 
 	if (!flushfn)
-		return 0;
+		return;
 
 	xnlock_get_irqsave(&heap->lock, s);
 
@@ -287,8 +285,6 @@ int xnheap_destroy(xnheap_t *heap,
 	}
 
 	xnlock_put_irqrestore(&heap->lock, s);
-
-	return 0;
 }
 EXPORT_SYMBOL_GPL(xnheap_destroy);
 
@@ -1267,7 +1263,8 @@ int xnheap_init_mapped(xnheap_t *heap, u_long heapsize, int memflags)
 int xnheap_destroy_mapped(xnheap_t *heap, void (*release)(struct xnheap *heap),
 			  void __user *mapaddr)
 {
-	return xnheap_destroy(heap, &xnheap_free_extent, NULL);
+	xnheap_destroy(heap, &xnheap_free_extent, NULL);
+	return 0;
 }
 #endif /* !CONFIG_XENO_OPT_PERVASIVE */
 
diff --git a/ksrc/skins/native/heap.c b/ksrc/skins/native/heap.c
index 9c3810c..0d7ad83 100644
--- a/ksrc/skins/native/heap.c
+++ b/ksrc/skins/native/heap.c
@@ -416,7 +416,7 @@ int rt_heap_delete_inner(RT_HEAP *heap, void __user *mapaddr)
 					    __heap_post_release, mapaddr);
 	else
 #endif /* CONFIG_XENO_OPT_PERVASIVE */
-		err = xnheap_destroy(&heap->heap_base, &__heap_flush_private, NULL);
+		xnheap_destroy(&heap->heap_base, &__heap_flush_private, NULL);
 
 	xnlock_get_irqsave(&nklock, s);
 
diff --git a/ksrc/skins/native/queue.c b/ksrc/skins/native/queue.c
index 527bde8..f913675 100644
--- a/ksrc/skins/native/queue.c
+++ b/ksrc/skins/native/queue.c
@@ -377,7 +377,7 @@ int rt_queue_delete_inner(RT_QUEUE *q, void __user *mapaddr)
 					    __queue_post_release, mapaddr);
 	else
 #endif /* CONFIG_XENO_OPT_PERVASIVE */
-		err = xnheap_destroy(&q->bufpool, &__queue_flush_private, NULL);
+		xnheap_destroy(&q->bufpool, &__queue_flush_private, NULL);
 
 	xnlock_get_irqsave(&nklock, s);
 
diff --git a/ksrc/skins/rtai/shm.c b/ksrc/skins/rtai/shm.c
index 81de434..fddf455 100644
--- a/ksrc/skins/rtai/shm.c
+++ b/ksrc/skins/rtai/shm.c
@@ -295,9 +295,8 @@ static int _shm_free(unsigned long name)
 #ifdef CONFIG_XENO_OPT_PERVASIVE
 				ret = xnheap_destroy_mapped(p->heap, NULL, NULL);
 #else /* !CONFIG_XENO_OPT_PERVASIVE */
-				ret =
-				    xnheap_destroy(p->heap,
-						   &__heap_flush_private, NULL);
+				xnheap_destroy(p->heap,
+					       &__heap_flush_private, NULL);
 #endif /* !CONFIG_XENO_OPT_PERVASIVE */
 				if (ret)
 					goto unlock_and_exit;

[Xenomai-core] [PATCH v3 3/9] nucleus: Fix race window in heap mapping procedure

[Jan Kiszka]

There is a race window between setting the heap address via the IOCTL
and actually mapping it. A heap that is invalidated after the IOCTL can
still be mapped by user space. Fix this by pushing the heap validation
into xnheap_mmap.

Moreover, make sure that we update archdep.numaps along with declaring
the heap valid. Otherwise xnheap_destroy_mapped may draw wrong
conclusions about the heap mapping state.

As archdep.numaps is now exclusively modified under heapq_lock, we can
switch it to a non-atomic counter.

Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
---

 include/asm-generic/bits/heap.h |    2 +
 include/asm-generic/system.h    |    2 +
 ksrc/nucleus/heap.c             |   55 +++++++++++++++++++--------------------
 ksrc/skins/native/heap.c        |    7 +++--
 4 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/include/asm-generic/bits/heap.h b/include/asm-generic/bits/heap.h
index 40d31c8..4d0c41a 100644
--- a/include/asm-generic/bits/heap.h
+++ b/include/asm-generic/bits/heap.h
@@ -27,7 +27,7 @@
 static inline void xnarch_init_heapcb (xnarch_heapcb_t *hcb)
 
 {
-    atomic_set(&hcb->numaps,0);
+    hcb->numaps = 0;
     hcb->kmflags = 0;
     hcb->heapbase = NULL;
 }
diff --git a/include/asm-generic/system.h b/include/asm-generic/system.h
index fcec613..c0f1698 100644
--- a/include/asm-generic/system.h
+++ b/include/asm-generic/system.h
@@ -270,7 +270,7 @@ struct xnheap;
 
 typedef struct xnarch_heapcb {
 
-	atomic_t numaps;	/* # of active user-space mappings. */
+	unsigned long numaps;	/* # of active user-space mappings. */
 	int kmflags;		/* Kernel memory flags (0 if vmalloc()). */
 	void *heapbase;		/* Shared heap memory base. */
 	void (*release)(struct xnheap *heap); /* callback upon last unmap */
diff --git a/ksrc/nucleus/heap.c b/ksrc/nucleus/heap.c
index 9ba5227..0a9bfdf 100644
--- a/ksrc/nucleus/heap.c
+++ b/ksrc/nucleus/heap.c
@@ -1027,7 +1027,7 @@ static void xnheap_vmclose(struct vm_area_struct *vma)
 
 	spin_lock(&kheapq_lock);
 
-	if (atomic_dec_and_test(&heap->archdep.numaps)) {
+	if (--heap->archdep.numaps == 0) {
 		if (heap->archdep.release) {
 			removeq(&kheapq, &heap->link);
 			spin_unlock(&kheapq_lock);
@@ -1067,31 +1067,15 @@ static inline xnheap_t *__validate_heap_addr(void *addr)
 static int xnheap_ioctl(struct inode *inode,
 			struct file *file, unsigned int cmd, unsigned long arg)
 {
-	xnheap_t *heap;
-	int err = 0;
-
-	spin_lock(&kheapq_lock);
-
-	heap = __validate_heap_addr((void *)arg);
-
-	if (!heap) {
-		err = -EINVAL;
-		goto unlock_and_exit;
-	}
-
-	file->private_data = heap;
-
-      unlock_and_exit:
-
-	spin_unlock(&kheapq_lock);
-
-	return err;
+	file->private_data = (void *)arg;
+	return 0;
 }
 
 static int xnheap_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	unsigned long offset, size, vaddr;
 	xnheap_t *heap;
+	int err;
 
 	if (vma->vm_ops != NULL || file->private_data == NULL)
 		/* Caller should mmap() once for a given file instance, after
@@ -1103,21 +1087,34 @@ static int xnheap_mmap(struct file *file, struct vm_area_struct *vma)
 
 	offset = vma->vm_pgoff << PAGE_SHIFT;
 	size = vma->vm_end - vma->vm_start;
-	heap = (xnheap_t *)file->private_data;
 
+	spin_lock(&kheapq_lock);
+
+	heap = __validate_heap_addr(file->private_data);
+	if (!heap) {
+		spin_unlock(&kheapq_lock);
+		return -EINVAL;
+	}
+
+	heap->archdep.numaps++;
+
+	spin_unlock(&kheapq_lock);
+
+	err = -ENXIO;
 	if (offset + size > xnheap_extentsize(heap))
-		return -ENXIO;
+		goto deref_out;
 
 	if (countq(&heap->extents) > 1)
 		/* Cannot map multi-extent heaps, we need the memory
 		   area we map from to be contiguous. */
-		return -ENXIO;
+		goto deref_out;
 
 	vma->vm_ops = &xnheap_vmops;
 	vma->vm_private_data = file->private_data;
 
 	vaddr = (unsigned long)heap->archdep.heapbase + offset;
 
+	err = -EAGAIN;
 	if ((heap->archdep.kmflags & ~XNHEAP_GFP_NONCACHED) == 0) {
 		unsigned long maddr = vma->vm_start;
 
@@ -1126,7 +1123,7 @@ static int xnheap_mmap(struct file *file, struct vm_area_struct *vma)
 
 		while (size > 0) {
 			if (xnarch_remap_vm_page(vma, maddr, vaddr))
-				return -EAGAIN;
+				goto deref_out;
 
 			maddr += PAGE_SIZE;
 			vaddr += PAGE_SIZE;
@@ -1136,13 +1133,15 @@ static int xnheap_mmap(struct file *file, struct vm_area_struct *vma)
 					      vma->vm_start,
 					      virt_to_phys((void *)vaddr),
 					      size, vma->vm_page_prot))
-		return -EAGAIN;
+		goto deref_out;
 
 	xnarch_fault_range(vma);
 
-	atomic_inc(&heap->archdep.numaps);
-
 	return 0;
+
+deref_out:
+	xnheap_vmclose(vma);
+	return err;
 }
 
 int xnheap_init_mapped(xnheap_t *heap, u_long heapsize, int memflags)
@@ -1188,7 +1187,7 @@ int xnheap_destroy_mapped(xnheap_t *heap, void (*release)(struct xnheap *heap),
 
 	spin_lock(&kheapq_lock);
 
-	if (atomic_read(&heap->archdep.numaps) > ccheck) {
+	if (heap->archdep.numaps > ccheck) {
 		heap->archdep.release = release;
 		spin_unlock(&kheapq_lock);
 		return -EBUSY;
diff --git a/ksrc/skins/native/heap.c b/ksrc/skins/native/heap.c
index 0a24735..9c3810c 100644
--- a/ksrc/skins/native/heap.c
+++ b/ksrc/skins/native/heap.c
@@ -60,11 +60,12 @@ static int __heap_read_proc(char *page,
 	int len;
 	spl_t s;
 
-	p += sprintf(p, "type=%s:size=%lu:used=%lu:numaps=%d\n",
+	p += sprintf(p, "type=%s:size=%lu:used=%lu:numaps=%lu\n",
 		     (heap->mode & H_SHARED) == H_SHARED ? "shared" :
 		     (heap->mode & H_MAPPABLE) ? "mappable" : "kernel",
-		     xnheap_usable_mem(&heap->heap_base), xnheap_used_mem(&heap->heap_base),
-		     atomic_read(&heap->heap_base.archdep.numaps));
+		     xnheap_usable_mem(&heap->heap_base),
+		     xnheap_used_mem(&heap->heap_base),
+		     heap->heap_base.archdep.numaps);
 
 	xnlock_get_irqsave(&nklock, s);
 

[Xenomai-core] [PATCH v3 1/9] native: Release fastlock to the proper heap

[Jan Kiszka]

Don't assume rt_task_delete is only called for in-kernel users, it may
be triggered via auto-cleanup also on user space objects. So check for
the creator and release the fastlock to the correct heap.

Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
---

 ksrc/skins/native/mutex.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/ksrc/skins/native/mutex.c b/ksrc/skins/native/mutex.c
index 20eb484..6cf7eb1 100644
--- a/ksrc/skins/native/mutex.c
+++ b/ksrc/skins/native/mutex.c
@@ -47,6 +47,7 @@
 #include <nucleus/pod.h>
 #include <nucleus/registry.h>
 #include <nucleus/heap.h>
+#include <nucleus/sys_ppd.h>
 #include <native/task.h>
 #include <native/mutex.h>
 
@@ -316,8 +317,17 @@ int rt_mutex_delete(RT_MUTEX *mutex)
 	err = rt_mutex_delete_inner(mutex);
 
 #ifdef CONFIG_XENO_FASTSYNCH
-	if (!err)
-		xnfree(mutex->synch_base.fastlock);
+	if (!err) {
+#ifdef CONFIG_XENO_OPT_PERVASIVE
+		if (mutex->cpid) {
+			int global = xnsynch_test_flags(&mutex->synch_base,
+							RT_MUTEX_EXPORTED);
+			xnheap_free(&xnsys_ppd_get(global)->sem_heap,
+				    mutex->synch_base.fastlock);
+		} else
+#endif /* CONFIG_XENO_OPT_PERVASIVE */
+			xnfree(mutex->synch_base.fastlock);
+	}
 #endif /* CONFIG_XENO_FASTSYNCH */
 
 	return err;

[Xenomai-core] [PATCH v3 5/9] nucleus: Avoid returning errors from xnheap_destroy_mapped

[Jan Kiszka]

Allowing xnheap_delete_mapped to return an error and then attempting to
recover from it does not work out very well: Corner cases are racy,
intransparent to the user, and proper error handling imposes a lot of
complexity on the caller - if it actually bothers to check the return
value...

Fortunately, there is no reason for this function to fail: If the heap
is still mapped, just install the provide cleanup handler and switch to
deferred removal. If the unmapping fails, we either raced with some
other caller of unmap or user space provided a bogus address, or
something else is wrong. In any case, leaving the cleanup callback
behind is the best we can do anyway.

Removing the return value immediately allows to simplify the callers,
namemly rt_queue_delete and rt_heap_delete.

Note: This is still not 100% waterproof. If we issue
xnheap_destroy_mapped from module cleanup passing a release handler
that belongs to the module text, deferred release will cause a crash.
But this corner case is no new regression, so let's keep the head in the
sand.

Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
---

 include/nucleus/heap.h    |    6 +++---
 ksrc/nucleus/heap.c       |   45 +++++++++++++++++++++++++++------------------
 ksrc/skins/native/heap.c  |   21 ++++++---------------
 ksrc/skins/native/queue.c |   21 ++++++---------------
 ksrc/skins/rtai/shm.c     |    6 +-----
 5 files changed, 43 insertions(+), 56 deletions(-)

diff --git a/include/nucleus/heap.h b/include/nucleus/heap.h
index ca691bf..44db738 100644
--- a/include/nucleus/heap.h
+++ b/include/nucleus/heap.h
@@ -204,9 +204,9 @@ int xnheap_init_mapped(xnheap_t *heap,
 		       u_long heapsize,
 		       int memflags);
 
-int xnheap_destroy_mapped(xnheap_t *heap,
-			  void (*release)(struct xnheap *heap),
-			  void __user *mapaddr);
+void xnheap_destroy_mapped(xnheap_t *heap,
+			   void (*release)(struct xnheap *heap),
+			   void __user *mapaddr);
 
 #define xnheap_mapped_offset(heap,ptr) \
 (((caddr_t)(ptr)) - ((caddr_t)(heap)->archdep.heapbase))
diff --git a/ksrc/nucleus/heap.c b/ksrc/nucleus/heap.c
index 4958daa..96c46f8 100644
--- a/ksrc/nucleus/heap.c
+++ b/ksrc/nucleus/heap.c
@@ -1173,42 +1173,51 @@ int xnheap_init_mapped(xnheap_t *heap, u_long heapsize, int memflags)
 	return 0;
 }
 
-int xnheap_destroy_mapped(xnheap_t *heap, void (*release)(struct xnheap *heap),
-			  void __user *mapaddr)
+void xnheap_destroy_mapped(xnheap_t *heap,
+			   void (*release)(struct xnheap *heap),
+			   void __user *mapaddr)
 {
-	int ret = 0, ccheck;
 	unsigned long len;
 
-	ccheck = mapaddr ? 1 : 0;
+	/*
+	 * Trying to unmap user memory without providing a release handler for
+	 * deferred cleanup is a bug.
+	 */
+	XENO_ASSERT(NUCLEUS, !mapaddr || release, /* nop */);
 
 	spin_lock(&kheapq_lock);
 
-	if (heap->archdep.numaps > ccheck) {
-		heap->archdep.release = release;
-		spin_unlock(&kheapq_lock);
-		return -EBUSY;
-	}
-
 	removeq(&kheapq, &heap->link); /* Prevent further mapping. */
+
+	heap->archdep.release = release;
+
+	if (heap->archdep.numaps == 0)
+		mapaddr = NULL; /* nothing left to unmap */
+	else
+		release = NULL; /* will be called by Linux on unmap */
+
 	spin_unlock(&kheapq_lock);
 
 	len = xnheap_extentsize(heap);
 
 	if (mapaddr) {
 		down_write(&current->mm->mmap_sem);
-		heap->archdep.release = NULL;
-		ret = do_munmap(current->mm, (unsigned long)mapaddr, len);
+		do_munmap(current->mm, (unsigned long)mapaddr, len);
 		up_write(&current->mm->mmap_sem);
 	}
 
-	if (ret == 0) {
+	if (heap->archdep.numaps > 0) {
+		/* The release handler is supposed to clean up the rest. */
+		XENO_ASSERT(NUCLEUS, heap->archdep.release, /* nop */);
+		return;
+	}
+
+	if (!mapaddr) {
 		__unreserve_and_free_heap(heap->archdep.heapbase, len,
 					  heap->archdep.kmflags);
 		if (release)
 			release(heap);
 	}
-
-	return ret;
 }
 
 static struct file_operations xnheap_fops = {
@@ -1260,11 +1269,11 @@ int xnheap_init_mapped(xnheap_t *heap, u_long heapsize, int memflags)
 	return -ENOMEM;
 }
 
-int xnheap_destroy_mapped(xnheap_t *heap, void (*release)(struct xnheap *heap),
-			  void __user *mapaddr)
+void xnheap_destroy_mapped(xnheap_t *heap,
+			   void (*release)(struct xnheap *heap),
+			   void __user *mapaddr)
 {
 	xnheap_destroy(heap, &xnheap_free_extent, NULL);
-	return 0;
 }
 #endif /* !CONFIG_XENO_OPT_PERVASIVE */
 
diff --git a/ksrc/skins/native/heap.c b/ksrc/skins/native/heap.c
index 0d7ad83..f7411e8 100644
--- a/ksrc/skins/native/heap.c
+++ b/ksrc/skins/native/heap.c
@@ -357,9 +357,6 @@ static void __heap_post_release(struct xnheap *h)
  *
  * @return 0 is returned upon success. Otherwise:
  *
- * - -EBUSY is returned if @a heap is in use by another process and the
- * descriptor is not destroyed.
- *
  * - -EINVAL is returned if @a heap is not a heap descriptor.
  *
  * - -EIDRM is returned if @a heap is a deleted heap descriptor.
@@ -379,7 +376,7 @@ static void __heap_post_release(struct xnheap *h)
 
 int rt_heap_delete_inner(RT_HEAP *heap, void __user *mapaddr)
 {
-	int err = 0;
+	int err;
 	spl_t s;
 
 	if (!xnpod_root_p())
@@ -412,22 +409,16 @@ int rt_heap_delete_inner(RT_HEAP *heap, void __user *mapaddr)
 
 #ifdef CONFIG_XENO_OPT_PERVASIVE
 	if (heap->mode & H_MAPPABLE)
-		err = xnheap_destroy_mapped(&heap->heap_base,
-					    __heap_post_release, mapaddr);
+		xnheap_destroy_mapped(&heap->heap_base,
+				      __heap_post_release, mapaddr);
 	else
 #endif /* CONFIG_XENO_OPT_PERVASIVE */
+	{
 		xnheap_destroy(&heap->heap_base, &__heap_flush_private, NULL);
-
-	xnlock_get_irqsave(&nklock, s);
-
-	if (err)
-		heap->magic = XENO_HEAP_MAGIC;
-	else if (!(heap->mode & H_MAPPABLE))
 		__heap_post_release(&heap->heap_base);
+	}
 
-	xnlock_put_irqrestore(&nklock, s);
-
-	return err;
+	return 0;
 }
 
 int rt_heap_delete(RT_HEAP *heap)
diff --git a/ksrc/skins/native/queue.c b/ksrc/skins/native/queue.c
index f913675..3592a4a 100644
--- a/ksrc/skins/native/queue.c
+++ b/ksrc/skins/native/queue.c
@@ -326,9 +326,6 @@ static void __queue_post_release(struct xnheap *heap)
  * - -EPERM is returned if this service was called from an
  * asynchronous context.
  *
- * - -EBUSY is returned if an attempt is made to delete a shared queue
- * which is still bound to a process.
- *
  * Environments:
  *
  * This service can be called from:
@@ -341,7 +338,7 @@ static void __queue_post_release(struct xnheap *heap)
 
 int rt_queue_delete_inner(RT_QUEUE *q, void __user *mapaddr)
 {
-	int err = 0;
+	int err;
 	spl_t s;
 
 	if (xnpod_asynch_p())
@@ -373,22 +370,16 @@ int rt_queue_delete_inner(RT_QUEUE *q, void __user *mapaddr)
 
 #ifdef CONFIG_XENO_OPT_PERVASIVE
 	if (q->mode & Q_SHARED)
-		err = xnheap_destroy_mapped(&q->bufpool,
-					    __queue_post_release, mapaddr);
+		xnheap_destroy_mapped(&q->bufpool,
+				      __queue_post_release, mapaddr);
 	else
 #endif /* CONFIG_XENO_OPT_PERVASIVE */
+	{
 		xnheap_destroy(&q->bufpool, &__queue_flush_private, NULL);
-
-	xnlock_get_irqsave(&nklock, s);
-
-	if (err)
-		q->magic = XENO_QUEUE_MAGIC;
-	else if (!(q->mode & Q_SHARED))
 		__queue_post_release(&q->bufpool);
+	}
 
-	xnlock_put_irqrestore(&nklock, s);
-
-	return err;
+	return 0;
 }
 
 int rt_queue_delete(RT_QUEUE *q)
diff --git a/ksrc/skins/rtai/shm.c b/ksrc/skins/rtai/shm.c
index fddf455..4c56495 100644
--- a/ksrc/skins/rtai/shm.c
+++ b/ksrc/skins/rtai/shm.c
@@ -293,13 +293,11 @@ static int _shm_free(unsigned long name)
 				 * [YES!]
 				 */
 #ifdef CONFIG_XENO_OPT_PERVASIVE
-				ret = xnheap_destroy_mapped(p->heap, NULL, NULL);
+				xnheap_destroy_mapped(p->heap, NULL, NULL);
 #else /* !CONFIG_XENO_OPT_PERVASIVE */
 				xnheap_destroy(p->heap,
 					       &__heap_flush_private, NULL);
 #endif /* !CONFIG_XENO_OPT_PERVASIVE */
-				if (ret)
-					goto unlock_and_exit;
 				xnheap_free(&kheap, p->heap);
 			}
 			removeq(&xnshm_allocq, &p->link);
@@ -311,8 +309,6 @@ static int _shm_free(unsigned long name)
 		holder = nextq(&xnshm_allocq, holder);
 	}
 
-      unlock_and_exit:
-
 	xnlock_put_irqrestore(&nklock, s);
 
 	return ret;

Re: [Xenomai-core] [PATCH v3 5/9] nucleus: Avoid returning errors from xnheap_destroy_mapped

[Philippe Gerum]

On Tue, 2009-10-20 at 13:37 +0200, Jan Kiszka wrote:
> Allowing xnheap_delete_mapped to return an error and then attempting to
> recover from it does not work out very well: Corner cases are racy,
> intransparent to the user, and proper error handling imposes a lot of
> complexity on the caller - if it actually bothers to check the return
> value...
> 
> Fortunately, there is no reason for this function to fail: If the heap
> is still mapped, just install the provide cleanup handler and switch to
> deferred removal. If the unmapping fails, we either raced with some
> other caller of unmap or user space provided a bogus address, or
> something else is wrong. In any case, leaving the cleanup callback
> behind is the best we can do anyway.
> 
> Removing the return value immediately allows to simplify the callers,
> namemly rt_queue_delete and rt_heap_delete.
> 
> Note: This is still not 100% waterproof. If we issue
> xnheap_destroy_mapped from module cleanup passing a release handler
> that belongs to the module text, deferred release will cause a crash.
> But this corner case is no new regression, so let's keep the head in the
> sand.

I agree with this one, eventually. This does make things clearer, and
removes some opportunities for the upper interfaces to shot themselves
in the foot. Merged, thanks.

> 
> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
> ---
> 
>  include/nucleus/heap.h    |    6 +++---
>  ksrc/nucleus/heap.c       |   45 +++++++++++++++++++++++++++------------------
>  ksrc/skins/native/heap.c  |   21 ++++++---------------
>  ksrc/skins/native/queue.c |   21 ++++++---------------
>  ksrc/skins/rtai/shm.c     |    6 +-----
>  5 files changed, 43 insertions(+), 56 deletions(-)
> 
> diff --git a/include/nucleus/heap.h b/include/nucleus/heap.h
> index ca691bf..44db738 100644
> --- a/include/nucleus/heap.h
> +++ b/include/nucleus/heap.h
> @@ -204,9 +204,9 @@ int xnheap_init_mapped(xnheap_t *heap,
>  		       u_long heapsize,
>  		       int memflags);
>  
> -int xnheap_destroy_mapped(xnheap_t *heap,
> -			  void (*release)(struct xnheap *heap),
> -			  void __user *mapaddr);
> +void xnheap_destroy_mapped(xnheap_t *heap,
> +			   void (*release)(struct xnheap *heap),
> +			   void __user *mapaddr);
>  
>  #define xnheap_mapped_offset(heap,ptr) \
>  (((caddr_t)(ptr)) - ((caddr_t)(heap)->archdep.heapbase))
> diff --git a/ksrc/nucleus/heap.c b/ksrc/nucleus/heap.c
> index 4958daa..96c46f8 100644
> --- a/ksrc/nucleus/heap.c
> +++ b/ksrc/nucleus/heap.c
> @@ -1173,42 +1173,51 @@ int xnheap_init_mapped(xnheap_t *heap, u_long heapsize, int memflags)
>  	return 0;
>  }
>  
> -int xnheap_destroy_mapped(xnheap_t *heap, void (*release)(struct xnheap *heap),
> -			  void __user *mapaddr)
> +void xnheap_destroy_mapped(xnheap_t *heap,
> +			   void (*release)(struct xnheap *heap),
> +			   void __user *mapaddr)
>  {
> -	int ret = 0, ccheck;
>  	unsigned long len;
>  
> -	ccheck = mapaddr ? 1 : 0;
> +	/*
> +	 * Trying to unmap user memory without providing a release handler for
> +	 * deferred cleanup is a bug.
> +	 */
> +	XENO_ASSERT(NUCLEUS, !mapaddr || release, /* nop */);
>  
>  	spin_lock(&kheapq_lock);
>  
> -	if (heap->archdep.numaps > ccheck) {
> -		heap->archdep.release = release;
> -		spin_unlock(&kheapq_lock);
> -		return -EBUSY;
> -	}
> -
>  	removeq(&kheapq, &heap->link); /* Prevent further mapping. */
> +
> +	heap->archdep.release = release;
> +
> +	if (heap->archdep.numaps == 0)
> +		mapaddr = NULL; /* nothing left to unmap */
> +	else
> +		release = NULL; /* will be called by Linux on unmap */
> +
>  	spin_unlock(&kheapq_lock);
>  
>  	len = xnheap_extentsize(heap);
>  
>  	if (mapaddr) {
>  		down_write(&current->mm->mmap_sem);
> -		heap->archdep.release = NULL;
> -		ret = do_munmap(current->mm, (unsigned long)mapaddr, len);
> +		do_munmap(current->mm, (unsigned long)mapaddr, len);
>  		up_write(&current->mm->mmap_sem);
>  	}
>  
> -	if (ret == 0) {
> +	if (heap->archdep.numaps > 0) {
> +		/* The release handler is supposed to clean up the rest. */
> +		XENO_ASSERT(NUCLEUS, heap->archdep.release, /* nop */);
> +		return;
> +	}
> +
> +	if (!mapaddr) {
>  		__unreserve_and_free_heap(heap->archdep.heapbase, len,
>  					  heap->archdep.kmflags);
>  		if (release)
>  			release(heap);
>  	}
> -
> -	return ret;
>  }
>  
>  static struct file_operations xnheap_fops = {
> @@ -1260,11 +1269,11 @@ int xnheap_init_mapped(xnheap_t *heap, u_long heapsize, int memflags)
>  	return -ENOMEM;
>  }
>  
> -int xnheap_destroy_mapped(xnheap_t *heap, void (*release)(struct xnheap *heap),
> -			  void __user *mapaddr)
> +void xnheap_destroy_mapped(xnheap_t *heap,
> +			   void (*release)(struct xnheap *heap),
> +			   void __user *mapaddr)
>  {
>  	xnheap_destroy(heap, &xnheap_free_extent, NULL);
> -	return 0;
>  }
>  #endif /* !CONFIG_XENO_OPT_PERVASIVE */
>  
> diff --git a/ksrc/skins/native/heap.c b/ksrc/skins/native/heap.c
> index 0d7ad83..f7411e8 100644
> --- a/ksrc/skins/native/heap.c
> +++ b/ksrc/skins/native/heap.c
> @@ -357,9 +357,6 @@ static void __heap_post_release(struct xnheap *h)
>   *
>   * @return 0 is returned upon success. Otherwise:
>   *
> - * - -EBUSY is returned if @a heap is in use by another process and the
> - * descriptor is not destroyed.
> - *
>   * - -EINVAL is returned if @a heap is not a heap descriptor.
>   *
>   * - -EIDRM is returned if @a heap is a deleted heap descriptor.
> @@ -379,7 +376,7 @@ static void __heap_post_release(struct xnheap *h)
>  
>  int rt_heap_delete_inner(RT_HEAP *heap, void __user *mapaddr)
>  {
> -	int err = 0;
> +	int err;
>  	spl_t s;
>  
>  	if (!xnpod_root_p())
> @@ -412,22 +409,16 @@ int rt_heap_delete_inner(RT_HEAP *heap, void __user *mapaddr)
>  
>  #ifdef CONFIG_XENO_OPT_PERVASIVE
>  	if (heap->mode & H_MAPPABLE)
> -		err = xnheap_destroy_mapped(&heap->heap_base,
> -					    __heap_post_release, mapaddr);
> +		xnheap_destroy_mapped(&heap->heap_base,
> +				      __heap_post_release, mapaddr);
>  	else
>  #endif /* CONFIG_XENO_OPT_PERVASIVE */
> +	{
>  		xnheap_destroy(&heap->heap_base, &__heap_flush_private, NULL);
> -
> -	xnlock_get_irqsave(&nklock, s);
> -
> -	if (err)
> -		heap->magic = XENO_HEAP_MAGIC;
> -	else if (!(heap->mode & H_MAPPABLE))
>  		__heap_post_release(&heap->heap_base);
> +	}
>  
> -	xnlock_put_irqrestore(&nklock, s);
> -
> -	return err;
> +	return 0;
>  }
>  
>  int rt_heap_delete(RT_HEAP *heap)
> diff --git a/ksrc/skins/native/queue.c b/ksrc/skins/native/queue.c
> index f913675..3592a4a 100644
> --- a/ksrc/skins/native/queue.c
> +++ b/ksrc/skins/native/queue.c
> @@ -326,9 +326,6 @@ static void __queue_post_release(struct xnheap *heap)
>   * - -EPERM is returned if this service was called from an
>   * asynchronous context.
>   *
> - * - -EBUSY is returned if an attempt is made to delete a shared queue
> - * which is still bound to a process.
> - *
>   * Environments:
>   *
>   * This service can be called from:
> @@ -341,7 +338,7 @@ static void __queue_post_release(struct xnheap *heap)
>  
>  int rt_queue_delete_inner(RT_QUEUE *q, void __user *mapaddr)
>  {
> -	int err = 0;
> +	int err;
>  	spl_t s;
>  
>  	if (xnpod_asynch_p())
> @@ -373,22 +370,16 @@ int rt_queue_delete_inner(RT_QUEUE *q, void __user *mapaddr)
>  
>  #ifdef CONFIG_XENO_OPT_PERVASIVE
>  	if (q->mode & Q_SHARED)
> -		err = xnheap_destroy_mapped(&q->bufpool,
> -					    __queue_post_release, mapaddr);
> +		xnheap_destroy_mapped(&q->bufpool,
> +				      __queue_post_release, mapaddr);
>  	else
>  #endif /* CONFIG_XENO_OPT_PERVASIVE */
> +	{
>  		xnheap_destroy(&q->bufpool, &__queue_flush_private, NULL);
> -
> -	xnlock_get_irqsave(&nklock, s);
> -
> -	if (err)
> -		q->magic = XENO_QUEUE_MAGIC;
> -	else if (!(q->mode & Q_SHARED))
>  		__queue_post_release(&q->bufpool);
> +	}
>  
> -	xnlock_put_irqrestore(&nklock, s);
> -
> -	return err;
> +	return 0;
>  }
>  
>  int rt_queue_delete(RT_QUEUE *q)
> diff --git a/ksrc/skins/rtai/shm.c b/ksrc/skins/rtai/shm.c
> index fddf455..4c56495 100644
> --- a/ksrc/skins/rtai/shm.c
> +++ b/ksrc/skins/rtai/shm.c
> @@ -293,13 +293,11 @@ static int _shm_free(unsigned long name)
>  				 * [YES!]
>  				 */
>  #ifdef CONFIG_XENO_OPT_PERVASIVE
> -				ret = xnheap_destroy_mapped(p->heap, NULL, NULL);
> +				xnheap_destroy_mapped(p->heap, NULL, NULL);
>  #else /* !CONFIG_XENO_OPT_PERVASIVE */
>  				xnheap_destroy(p->heap,
>  					       &__heap_flush_private, NULL);
>  #endif /* !CONFIG_XENO_OPT_PERVASIVE */
> -				if (ret)
> -					goto unlock_and_exit;
>  				xnheap_free(&kheap, p->heap);
>  			}
>  			removeq(&xnshm_allocq, &p->link);
> @@ -311,8 +309,6 @@ static int _shm_free(unsigned long name)
>  		holder = nextq(&xnshm_allocq, holder);
>  	}
>  
> -      unlock_and_exit:
> -
>  	xnlock_put_irqrestore(&nklock, s);
>  
>  	return ret;
> 
-- 
Philippe.

Re: [Xenomai-core] [PATCH v3 5/9] nucleus: Avoid returning errors from xnheap_destroy_mapped

[Philippe Gerum]

On Sat, 2009-10-24 at 19:22 +0200, Philippe Gerum wrote:
> On Tue, 2009-10-20 at 13:37 +0200, Jan Kiszka wrote:
> > Allowing xnheap_delete_mapped to return an error and then attempting to
> > recover from it does not work out very well: Corner cases are racy,
> > intransparent to the user, and proper error handling imposes a lot of
> > complexity on the caller - if it actually bothers to check the return
> > value...
> > 
> > Fortunately, there is no reason for this function to fail: If the heap
> > is still mapped, just install the provide cleanup handler and switch to
> > deferred removal. If the unmapping fails, we either raced with some
> > other caller of unmap or user space provided a bogus address, or
> > something else is wrong. In any case, leaving the cleanup callback
> > behind is the best we can do anyway.
> > 
> > Removing the return value immediately allows to simplify the callers,
> > namemly rt_queue_delete and rt_heap_delete.
> > 
> > Note: This is still not 100% waterproof. If we issue
> > xnheap_destroy_mapped from module cleanup passing a release handler
> > that belongs to the module text, deferred release will cause a crash.
> > But this corner case is no new regression, so let's keep the head in the
> > sand.
> 
> I agree with this one, eventually. This does make things clearer, and
> removes some opportunities for the upper interfaces to shot themselves
> in the foot. Merged, thanks.

Well, actually, it does make things clearer, but it is broken. Enabling
list debugging makes the nucleus pull the break after a double unlink in
vmclose().

Basically, the issue is that calling rt_queue/heap_delete() explicitly
from userland will break, due to the vmclose() handler being indirectly
called by do_munmap() for the last mapping. The nasty thing is that
without debugs on, kheapq is just silently trashed.

Fix is on its way, along with nommu support for shared heaps as well.

> > 
> > Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
> > ---
> > 
> >  include/nucleus/heap.h    |    6 +++---
> >  ksrc/nucleus/heap.c       |   45 +++++++++++++++++++++++++++------------------
> >  ksrc/skins/native/heap.c  |   21 ++++++---------------
> >  ksrc/skins/native/queue.c |   21 ++++++---------------
> >  ksrc/skins/rtai/shm.c     |    6 +-----
> >  5 files changed, 43 insertions(+), 56 deletions(-)
> > 
> > diff --git a/include/nucleus/heap.h b/include/nucleus/heap.h
> > index ca691bf..44db738 100644
> > --- a/include/nucleus/heap.h
> > +++ b/include/nucleus/heap.h
> > @@ -204,9 +204,9 @@ int xnheap_init_mapped(xnheap_t *heap,
> >  		       u_long heapsize,
> >  		       int memflags);
> >  
> > -int xnheap_destroy_mapped(xnheap_t *heap,
> > -			  void (*release)(struct xnheap *heap),
> > -			  void __user *mapaddr);
> > +void xnheap_destroy_mapped(xnheap_t *heap,
> > +			   void (*release)(struct xnheap *heap),
> > +			   void __user *mapaddr);
> >  
> >  #define xnheap_mapped_offset(heap,ptr) \
> >  (((caddr_t)(ptr)) - ((caddr_t)(heap)->archdep.heapbase))
> > diff --git a/ksrc/nucleus/heap.c b/ksrc/nucleus/heap.c
> > index 4958daa..96c46f8 100644
> > --- a/ksrc/nucleus/heap.c
> > +++ b/ksrc/nucleus/heap.c
> > @@ -1173,42 +1173,51 @@ int xnheap_init_mapped(xnheap_t *heap, u_long heapsize, int memflags)
> >  	return 0;
> >  }
> >  
> > -int xnheap_destroy_mapped(xnheap_t *heap, void (*release)(struct xnheap *heap),
> > -			  void __user *mapaddr)
> > +void xnheap_destroy_mapped(xnheap_t *heap,
> > +			   void (*release)(struct xnheap *heap),
> > +			   void __user *mapaddr)
> >  {
> > -	int ret = 0, ccheck;
> >  	unsigned long len;
> >  
> > -	ccheck = mapaddr ? 1 : 0;
> > +	/*
> > +	 * Trying to unmap user memory without providing a release handler for
> > +	 * deferred cleanup is a bug.
> > +	 */
> > +	XENO_ASSERT(NUCLEUS, !mapaddr || release, /* nop */);
> >  
> >  	spin_lock(&kheapq_lock);
> >  
> > -	if (heap->archdep.numaps > ccheck) {
> > -		heap->archdep.release = release;
> > -		spin_unlock(&kheapq_lock);
> > -		return -EBUSY;
> > -	}
> > -
> >  	removeq(&kheapq, &heap->link); /* Prevent further mapping. */
> > +
> > +	heap->archdep.release = release;
> > +
> > +	if (heap->archdep.numaps == 0)
> > +		mapaddr = NULL; /* nothing left to unmap */
> > +	else
> > +		release = NULL; /* will be called by Linux on unmap */
> > +
> >  	spin_unlock(&kheapq_lock);
> >  
> >  	len = xnheap_extentsize(heap);
> >  
> >  	if (mapaddr) {
> >  		down_write(&current->mm->mmap_sem);
> > -		heap->archdep.release = NULL;
> > -		ret = do_munmap(current->mm, (unsigned long)mapaddr, len);
> > +		do_munmap(current->mm, (unsigned long)mapaddr, len);
> >  		up_write(&current->mm->mmap_sem);
> >  	}
> >  
> > -	if (ret == 0) {
> > +	if (heap->archdep.numaps > 0) {
> > +		/* The release handler is supposed to clean up the rest. */
> > +		XENO_ASSERT(NUCLEUS, heap->archdep.release, /* nop */);
> > +		return;
> > +	}
> > +
> > +	if (!mapaddr) {
> >  		__unreserve_and_free_heap(heap->archdep.heapbase, len,
> >  					  heap->archdep.kmflags);
> >  		if (release)
> >  			release(heap);
> >  	}
> > -
> > -	return ret;
> >  }
> >  
> >  static struct file_operations xnheap_fops = {
> > @@ -1260,11 +1269,11 @@ int xnheap_init_mapped(xnheap_t *heap, u_long heapsize, int memflags)
> >  	return -ENOMEM;
> >  }
> >  
> > -int xnheap_destroy_mapped(xnheap_t *heap, void (*release)(struct xnheap *heap),
> > -			  void __user *mapaddr)
> > +void xnheap_destroy_mapped(xnheap_t *heap,
> > +			   void (*release)(struct xnheap *heap),
> > +			   void __user *mapaddr)
> >  {
> >  	xnheap_destroy(heap, &xnheap_free_extent, NULL);
> > -	return 0;
> >  }
> >  #endif /* !CONFIG_XENO_OPT_PERVASIVE */
> >  
> > diff --git a/ksrc/skins/native/heap.c b/ksrc/skins/native/heap.c
> > index 0d7ad83..f7411e8 100644
> > --- a/ksrc/skins/native/heap.c
> > +++ b/ksrc/skins/native/heap.c
> > @@ -357,9 +357,6 @@ static void __heap_post_release(struct xnheap *h)
> >   *
> >   * @return 0 is returned upon success. Otherwise:
> >   *
> > - * - -EBUSY is returned if @a heap is in use by another process and the
> > - * descriptor is not destroyed.
> > - *
> >   * - -EINVAL is returned if @a heap is not a heap descriptor.
> >   *
> >   * - -EIDRM is returned if @a heap is a deleted heap descriptor.
> > @@ -379,7 +376,7 @@ static void __heap_post_release(struct xnheap *h)
> >  
> >  int rt_heap_delete_inner(RT_HEAP *heap, void __user *mapaddr)
> >  {
> > -	int err = 0;
> > +	int err;
> >  	spl_t s;
> >  
> >  	if (!xnpod_root_p())
> > @@ -412,22 +409,16 @@ int rt_heap_delete_inner(RT_HEAP *heap, void __user *mapaddr)
> >  
> >  #ifdef CONFIG_XENO_OPT_PERVASIVE
> >  	if (heap->mode & H_MAPPABLE)
> > -		err = xnheap_destroy_mapped(&heap->heap_base,
> > -					    __heap_post_release, mapaddr);
> > +		xnheap_destroy_mapped(&heap->heap_base,
> > +				      __heap_post_release, mapaddr);
> >  	else
> >  #endif /* CONFIG_XENO_OPT_PERVASIVE */
> > +	{
> >  		xnheap_destroy(&heap->heap_base, &__heap_flush_private, NULL);
> > -
> > -	xnlock_get_irqsave(&nklock, s);
> > -
> > -	if (err)
> > -		heap->magic = XENO_HEAP_MAGIC;
> > -	else if (!(heap->mode & H_MAPPABLE))
> >  		__heap_post_release(&heap->heap_base);
> > +	}
> >  
> > -	xnlock_put_irqrestore(&nklock, s);
> > -
> > -	return err;
> > +	return 0;
> >  }
> >  
> >  int rt_heap_delete(RT_HEAP *heap)
> > diff --git a/ksrc/skins/native/queue.c b/ksrc/skins/native/queue.c
> > index f913675..3592a4a 100644
> > --- a/ksrc/skins/native/queue.c
> > +++ b/ksrc/skins/native/queue.c
> > @@ -326,9 +326,6 @@ static void __queue_post_release(struct xnheap *heap)
> >   * - -EPERM is returned if this service was called from an
> >   * asynchronous context.
> >   *
> > - * - -EBUSY is returned if an attempt is made to delete a shared queue
> > - * which is still bound to a process.
> > - *
> >   * Environments:
> >   *
> >   * This service can be called from:
> > @@ -341,7 +338,7 @@ static void __queue_post_release(struct xnheap *heap)
> >  
> >  int rt_queue_delete_inner(RT_QUEUE *q, void __user *mapaddr)
> >  {
> > -	int err = 0;
> > +	int err;
> >  	spl_t s;
> >  
> >  	if (xnpod_asynch_p())
> > @@ -373,22 +370,16 @@ int rt_queue_delete_inner(RT_QUEUE *q, void __user *mapaddr)
> >  
> >  #ifdef CONFIG_XENO_OPT_PERVASIVE
> >  	if (q->mode & Q_SHARED)
> > -		err = xnheap_destroy_mapped(&q->bufpool,
> > -					    __queue_post_release, mapaddr);
> > +		xnheap_destroy_mapped(&q->bufpool,
> > +				      __queue_post_release, mapaddr);
> >  	else
> >  #endif /* CONFIG_XENO_OPT_PERVASIVE */
> > +	{
> >  		xnheap_destroy(&q->bufpool, &__queue_flush_private, NULL);
> > -
> > -	xnlock_get_irqsave(&nklock, s);
> > -
> > -	if (err)
> > -		q->magic = XENO_QUEUE_MAGIC;
> > -	else if (!(q->mode & Q_SHARED))
> >  		__queue_post_release(&q->bufpool);
> > +	}
> >  
> > -	xnlock_put_irqrestore(&nklock, s);
> > -
> > -	return err;
> > +	return 0;
> >  }
> >  
> >  int rt_queue_delete(RT_QUEUE *q)
> > diff --git a/ksrc/skins/rtai/shm.c b/ksrc/skins/rtai/shm.c
> > index fddf455..4c56495 100644
> > --- a/ksrc/skins/rtai/shm.c
> > +++ b/ksrc/skins/rtai/shm.c
> > @@ -293,13 +293,11 @@ static int _shm_free(unsigned long name)
> >  				 * [YES!]
> >  				 */
> >  #ifdef CONFIG_XENO_OPT_PERVASIVE
> > -				ret = xnheap_destroy_mapped(p->heap, NULL, NULL);
> > +				xnheap_destroy_mapped(p->heap, NULL, NULL);
> >  #else /* !CONFIG_XENO_OPT_PERVASIVE */
> >  				xnheap_destroy(p->heap,
> >  					       &__heap_flush_private, NULL);
> >  #endif /* !CONFIG_XENO_OPT_PERVASIVE */
> > -				if (ret)
> > -					goto unlock_and_exit;
> >  				xnheap_free(&kheap, p->heap);
> >  			}
> >  			removeq(&xnshm_allocq, &p->link);
> > @@ -311,8 +309,6 @@ static int _shm_free(unsigned long name)
> >  		holder = nextq(&xnshm_allocq, holder);
> >  	}
> >  
> > -      unlock_and_exit:
> > -
> >  	xnlock_put_irqrestore(&nklock, s);
> >  
> >  	return ret;
> > 


-- 
Philippe.

Re: [Xenomai-core] [PATCH v3 5/9] nucleus: Avoid returning errors from xnheap_destroy_mapped

[Jan Kiszka]

Philippe Gerum wrote:
> On Sat, 2009-10-24 at 19:22 +0200, Philippe Gerum wrote:
>> On Tue, 2009-10-20 at 13:37 +0200, Jan Kiszka wrote:
>>> Allowing xnheap_delete_mapped to return an error and then attempting to
>>> recover from it does not work out very well: Corner cases are racy,
>>> intransparent to the user, and proper error handling imposes a lot of
>>> complexity on the caller - if it actually bothers to check the return
>>> value...
>>>
>>> Fortunately, there is no reason for this function to fail: If the heap
>>> is still mapped, just install the provide cleanup handler and switch to
>>> deferred removal. If the unmapping fails, we either raced with some
>>> other caller of unmap or user space provided a bogus address, or
>>> something else is wrong. In any case, leaving the cleanup callback
>>> behind is the best we can do anyway.
>>>
>>> Removing the return value immediately allows to simplify the callers,
>>> namemly rt_queue_delete and rt_heap_delete.
>>>
>>> Note: This is still not 100% waterproof. If we issue
>>> xnheap_destroy_mapped from module cleanup passing a release handler
>>> that belongs to the module text, deferred release will cause a crash.
>>> But this corner case is no new regression, so let's keep the head in the
>>> sand.
>> I agree with this one, eventually. This does make things clearer, and
>> removes some opportunities for the upper interfaces to shot themselves
>> in the foot. Merged, thanks.
> 
> Well, actually, it does make things clearer, but it is broken. Enabling
> list debugging makes the nucleus pull the break after a double unlink in
> vmclose().
> 
> Basically, the issue is that calling rt_queue/heap_delete() explicitly
> from userland will break, due to the vmclose() handler being indirectly
> called by do_munmap() for the last mapping. The nasty thing is that
> without debugs on, kheapq is just silently trashed.
> 
> Fix is on its way, along with nommu support for shared heaps as well.

OK, I see. Just on minor add-on to your fix:

diff --git a/ksrc/nucleus/heap.c b/ksrc/nucleus/heap.c
index ec14f73..1ae6af6 100644
--- a/ksrc/nucleus/heap.c
+++ b/ksrc/nucleus/heap.c
@@ -1241,6 +1241,7 @@ void xnheap_destroy_mapped(xnheap_t *heap,
 		down_write(&current->mm->mmap_sem);
 		heap->archdep.release = NULL;
 		do_munmap(current->mm, (unsigned long)mapaddr, len);
+		heap->archdep.release = release;
 		up_write(&current->mm->mmap_sem);
 	}
 
@@ -1252,7 +1253,6 @@ void xnheap_destroy_mapped(xnheap_t *heap,
 	if (heap->archdep.numaps > 0) {
 		/* The release handler is supposed to clean up the rest. */
 		XENO_ASSERT(NUCLEUS, release != NULL, /* nop */);
-		heap->archdep.release = release;
 		return;
 	}
 

This is safer than leaving a potential race window open between dropping
mmap_sem and fixing up archdep.release again.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [PATCH v3 5/9] nucleus: Avoid returning errors from xnheap_destroy_mapped

[Philippe Gerum]

On Mon, 2009-11-02 at 17:41 +0100, Jan Kiszka wrote:
> Philippe Gerum wrote:
> > On Sat, 2009-10-24 at 19:22 +0200, Philippe Gerum wrote:
> >> On Tue, 2009-10-20 at 13:37 +0200, Jan Kiszka wrote:
> >>> Allowing xnheap_delete_mapped to return an error and then attempting to
> >>> recover from it does not work out very well: Corner cases are racy,
> >>> intransparent to the user, and proper error handling imposes a lot of
> >>> complexity on the caller - if it actually bothers to check the return
> >>> value...
> >>>
> >>> Fortunately, there is no reason for this function to fail: If the heap
> >>> is still mapped, just install the provide cleanup handler and switch to
> >>> deferred removal. If the unmapping fails, we either raced with some
> >>> other caller of unmap or user space provided a bogus address, or
> >>> something else is wrong. In any case, leaving the cleanup callback
> >>> behind is the best we can do anyway.
> >>>
> >>> Removing the return value immediately allows to simplify the callers,
> >>> namemly rt_queue_delete and rt_heap_delete.
> >>>
> >>> Note: This is still not 100% waterproof. If we issue
> >>> xnheap_destroy_mapped from module cleanup passing a release handler
> >>> that belongs to the module text, deferred release will cause a crash.
> >>> But this corner case is no new regression, so let's keep the head in the
> >>> sand.
> >> I agree with this one, eventually. This does make things clearer, and
> >> removes some opportunities for the upper interfaces to shot themselves
> >> in the foot. Merged, thanks.
> > 
> > Well, actually, it does make things clearer, but it is broken. Enabling
> > list debugging makes the nucleus pull the break after a double unlink in
> > vmclose().
> > 
> > Basically, the issue is that calling rt_queue/heap_delete() explicitly
> > from userland will break, due to the vmclose() handler being indirectly
> > called by do_munmap() for the last mapping. The nasty thing is that
> > without debugs on, kheapq is just silently trashed.
> > 
> > Fix is on its way, along with nommu support for shared heaps as well.
> 
> OK, I see. Just on minor add-on to your fix:
> 
> diff --git a/ksrc/nucleus/heap.c b/ksrc/nucleus/heap.c
> index ec14f73..1ae6af6 100644
> --- a/ksrc/nucleus/heap.c
> +++ b/ksrc/nucleus/heap.c
> @@ -1241,6 +1241,7 @@ void xnheap_destroy_mapped(xnheap_t *heap,
>  		down_write(&current->mm->mmap_sem);
>  		heap->archdep.release = NULL;
>  		do_munmap(current->mm, (unsigned long)mapaddr, len);
> +		heap->archdep.release = release;
>  		up_write(&current->mm->mmap_sem);
>  	}
>  
> @@ -1252,7 +1253,6 @@ void xnheap_destroy_mapped(xnheap_t *heap,
>  	if (heap->archdep.numaps > 0) {
>  		/* The release handler is supposed to clean up the rest. */
>  		XENO_ASSERT(NUCLEUS, release != NULL, /* nop */);
> -		heap->archdep.release = release;
>  		return;
>  	}
>  
> 
> This is safer than leaving a potential race window open between dropping
> mmap_sem and fixing up archdep.release again.
> 

Actually, we have to hold the kheap lock, in case weird code starts
mapping randomly from userland without getting a valid descriptor
through a skin call.

> Jan
> 


-- 
Philippe.

Re: [Xenomai-core] [PATCH v3 5/9] nucleus: Avoid returning errors from xnheap_destroy_mapped

[Jan Kiszka]

Philippe Gerum wrote:
> On Mon, 2009-11-02 at 17:41 +0100, Jan Kiszka wrote:
>> Philippe Gerum wrote:
>>> On Sat, 2009-10-24 at 19:22 +0200, Philippe Gerum wrote:
>>>> On Tue, 2009-10-20 at 13:37 +0200, Jan Kiszka wrote:
>>>>> Allowing xnheap_delete_mapped to return an error and then attempting to
>>>>> recover from it does not work out very well: Corner cases are racy,
>>>>> intransparent to the user, and proper error handling imposes a lot of
>>>>> complexity on the caller - if it actually bothers to check the return
>>>>> value...
>>>>>
>>>>> Fortunately, there is no reason for this function to fail: If the heap
>>>>> is still mapped, just install the provide cleanup handler and switch to
>>>>> deferred removal. If the unmapping fails, we either raced with some
>>>>> other caller of unmap or user space provided a bogus address, or
>>>>> something else is wrong. In any case, leaving the cleanup callback
>>>>> behind is the best we can do anyway.
>>>>>
>>>>> Removing the return value immediately allows to simplify the callers,
>>>>> namemly rt_queue_delete and rt_heap_delete.
>>>>>
>>>>> Note: This is still not 100% waterproof. If we issue
>>>>> xnheap_destroy_mapped from module cleanup passing a release handler
>>>>> that belongs to the module text, deferred release will cause a crash.
>>>>> But this corner case is no new regression, so let's keep the head in the
>>>>> sand.
>>>> I agree with this one, eventually. This does make things clearer, and
>>>> removes some opportunities for the upper interfaces to shot themselves
>>>> in the foot. Merged, thanks.
>>> Well, actually, it does make things clearer, but it is broken. Enabling
>>> list debugging makes the nucleus pull the break after a double unlink in
>>> vmclose().
>>>
>>> Basically, the issue is that calling rt_queue/heap_delete() explicitly
>>> from userland will break, due to the vmclose() handler being indirectly
>>> called by do_munmap() for the last mapping. The nasty thing is that
>>> without debugs on, kheapq is just silently trashed.
>>>
>>> Fix is on its way, along with nommu support for shared heaps as well.
>> OK, I see. Just on minor add-on to your fix:
>>
>> diff --git a/ksrc/nucleus/heap.c b/ksrc/nucleus/heap.c
>> index ec14f73..1ae6af6 100644
>> --- a/ksrc/nucleus/heap.c
>> +++ b/ksrc/nucleus/heap.c
>> @@ -1241,6 +1241,7 @@ void xnheap_destroy_mapped(xnheap_t *heap,
>>  		down_write(&current->mm->mmap_sem);
>>  		heap->archdep.release = NULL;
>>  		do_munmap(current->mm, (unsigned long)mapaddr, len);
>> +		heap->archdep.release = release;
>>  		up_write(&current->mm->mmap_sem);
>>  	}
>>  
>> @@ -1252,7 +1253,6 @@ void xnheap_destroy_mapped(xnheap_t *heap,
>>  	if (heap->archdep.numaps > 0) {
>>  		/* The release handler is supposed to clean up the rest. */
>>  		XENO_ASSERT(NUCLEUS, release != NULL, /* nop */);
>> -		heap->archdep.release = release;
>>  		return;
>>  	}
>>  
>>
>> This is safer than leaving a potential race window open between dropping
>> mmap_sem and fixing up archdep.release again.
>>
> 
> Actually, we have to hold the kheap lock, in case weird code starts
> mapping randomly from userland without getting a valid descriptor
> through a skin call.

Yep, that as well.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [PATCH v3 5/9] nucleus: Avoid returning errors from xnheap_destroy_mapped

[Jan Kiszka]

Jan Kiszka wrote:
> Philippe Gerum wrote:
>> On Mon, 2009-11-02 at 17:41 +0100, Jan Kiszka wrote:
>>> Philippe Gerum wrote:
>>>> On Sat, 2009-10-24 at 19:22 +0200, Philippe Gerum wrote:
>>>>> On Tue, 2009-10-20 at 13:37 +0200, Jan Kiszka wrote:
>>>>>> Allowing xnheap_delete_mapped to return an error and then attempting to
>>>>>> recover from it does not work out very well: Corner cases are racy,
>>>>>> intransparent to the user, and proper error handling imposes a lot of
>>>>>> complexity on the caller - if it actually bothers to check the return
>>>>>> value...
>>>>>>
>>>>>> Fortunately, there is no reason for this function to fail: If the heap
>>>>>> is still mapped, just install the provide cleanup handler and switch to
>>>>>> deferred removal. If the unmapping fails, we either raced with some
>>>>>> other caller of unmap or user space provided a bogus address, or
>>>>>> something else is wrong. In any case, leaving the cleanup callback
>>>>>> behind is the best we can do anyway.
>>>>>>
>>>>>> Removing the return value immediately allows to simplify the callers,
>>>>>> namemly rt_queue_delete and rt_heap_delete.
>>>>>>
>>>>>> Note: This is still not 100% waterproof. If we issue
>>>>>> xnheap_destroy_mapped from module cleanup passing a release handler
>>>>>> that belongs to the module text, deferred release will cause a crash.
>>>>>> But this corner case is no new regression, so let's keep the head in the
>>>>>> sand.
>>>>> I agree with this one, eventually. This does make things clearer, and
>>>>> removes some opportunities for the upper interfaces to shot themselves
>>>>> in the foot. Merged, thanks.
>>>> Well, actually, it does make things clearer, but it is broken. Enabling
>>>> list debugging makes the nucleus pull the break after a double unlink in
>>>> vmclose().
>>>>
>>>> Basically, the issue is that calling rt_queue/heap_delete() explicitly
>>>> from userland will break, due to the vmclose() handler being indirectly
>>>> called by do_munmap() for the last mapping. The nasty thing is that
>>>> without debugs on, kheapq is just silently trashed.
>>>>
>>>> Fix is on its way, along with nommu support for shared heaps as well.
>>> OK, I see. Just on minor add-on to your fix:
>>>
>>> diff --git a/ksrc/nucleus/heap.c b/ksrc/nucleus/heap.c
>>> index ec14f73..1ae6af6 100644
>>> --- a/ksrc/nucleus/heap.c
>>> +++ b/ksrc/nucleus/heap.c
>>> @@ -1241,6 +1241,7 @@ void xnheap_destroy_mapped(xnheap_t *heap,
>>>  		down_write(&current->mm->mmap_sem);
>>>  		heap->archdep.release = NULL;
>>>  		do_munmap(current->mm, (unsigned long)mapaddr, len);
>>> +		heap->archdep.release = release;
>>>  		up_write(&current->mm->mmap_sem);
>>>  	}
>>>  
>>> @@ -1252,7 +1253,6 @@ void xnheap_destroy_mapped(xnheap_t *heap,
>>>  	if (heap->archdep.numaps > 0) {
>>>  		/* The release handler is supposed to clean up the rest. */
>>>  		XENO_ASSERT(NUCLEUS, release != NULL, /* nop */);
>>> -		heap->archdep.release = release;
>>>  		return;
>>>  	}
>>>  
>>>
>>> This is safer than leaving a potential race window open between dropping
>>> mmap_sem and fixing up archdep.release again.
>>>
>> Actually, we have to hold the kheap lock, in case weird code starts
>> mapping randomly from userland without getting a valid descriptor
>> through a skin call.
> 
> Yep, that as well.
> 

Note that 6b1a185b46 doesn't obsolete my patch (pull it from my tree if
you like).

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

[Xenomai-help] Xenomai on ARMadeus

[Pierre Ficheux]

Hi,

I've just tried to use Xenomai on ARMadeus board (APF9328, kernel
2.6.29). I got the following message when starting "latency" :

Any idea?

# /usr/xenomai/bin/latency
== Sampling period: 100 us
== Test mode: periodic user-mode task
== All results in microseconds
warming up...
Unhandled fault: external abort on non-linefetch (0x01a) at 0x40006010


# uname -a
Linux armadeus 2.6.29.6 #1 PREEMPT Fri Oct 30 13:11:26 CET 2009 armv4tl
unknown

# cat /proc/ipipe/version
1.13-00


Looks like there was a discussion  about a similar problem some days ago
 with Gilles  (Freescale CPU too).

http://www.mail-archive.com/xenomai@xenomai.org

Thx,

regards


-- 
Pierre FICHEUX -/- CTO OW/OS4I, France -\- pierre.ficheux@domain.hid
                                         http://www.os4i.com
                                         http://www.ficheux.org
I would love to change the world, but they won't give me the source code

Re: [Xenomai-help] Xenomai on ARMadeus

[Gilles Chanteperdrix]

Pierre Ficheux wrote:
> Hi,
> 
> I've just tried to use Xenomai on ARMadeus board (APF9328, kernel
> 2.6.29). I got the following message when starting "latency" :
> 
> Any idea?
> 
> # /usr/xenomai/bin/latency
> == Sampling period: 100 us
> == Test mode: periodic user-mode task
> == All results in microseconds
> warming up...
> Unhandled fault: external abort on non-linefetch (0x01a) at 0x40006010
> 
> 
> # uname -a
> Linux armadeus 2.6.29.6 #1 PREEMPT Fri Oct 30 13:11:26 CET 2009 armv4tl
> unknown
> 
> # cat /proc/ipipe/version
> 1.13-00
> 
> 
> Looks like there was a discussion  about a similar problem some days ago
>  with Gilles  (Freescale CPU too).
> 
> http://www.mail-archive.com/xenomai@xenomai.org

Yes, the result of which is that you need some code to allow code not
running in supervisor mode to access I/O addresses.

You can also configure xenomai with --enable-arm-mach=generic, which
will avoid access to I/Os from user-space.

-- 
					    Gilles.

Re: [Xenomai-help] Xenomai on ARMadeus

[Gilles Chanteperdrix]

Pierre Ficheux wrote:
> Hi,
> 
> I've just tried to use Xenomai on ARMadeus board (APF9328, kernel
> 2.6.29).

By the way, did you make any change to the I-pipe patch to get it
running on this board?


-- 
					    Gilles.

Re: [Xenomai-help] Xenomai on ARMadeus

[gwenhael.goavec]

On Mon, 02 Nov 2009 19:38:51 +0100
Gilles Chanteperdrix <gilles.chanteperdrix@xenomai.org>
wrote:

> By the way, did you make any change to the I-pipe patch to get it
> running on this board?

The only change realized, is made because Armadeus and
I-pipe does the same change on
mach-imx/include/mach/imxfb.h :

--- a/arch/arm/mach-imx/include/mach/imxfb.h
+++ b/arch/arm/mach-imx/include/mach/imxfb.h
@@ -14,7 +14,6 @@
 #define PCR_BPIX_8 (3 << 25)
 #define PCR_BPIX_12    (4 << 25)
 #define PCR_BPIX_16    (4 << 25)
+#define PCR_BPIX_MASK   (7 << 25)
 #define PCR_PIXPOL (1 << 24)
 #define PCR_FLMPOL (1 << 23)
 #define PCR_LPPOL  (1 << 22)

I take this opportunity to ask a question:
on APF27 with imx27, I found that the system freeze
when an interrupt handler is in place and an
interruption happens.
if interrupt handler is in place without interruption no
problems and if interruption happens without interrupt
handler it's ok too.

If someone could help or advise me. 
thank you very much

Gwenhael

Signed-off-by: gwenhael.goavec-merou@domain.hid

Re: [Xenomai-help] Xenomai on ARMadeus

[Gilles Chanteperdrix]

gwenhael.goavec wrote:
> On Mon, 02 Nov 2009 19:38:51 +0100
> Gilles Chanteperdrix <gilles.chanteperdrix@xenomai.org>
> wrote:
> 
>> By the way, did you make any change to the I-pipe patch to get it
>> running on this board?
> 
> The only change realized, is made because Armadeus and
> I-pipe does the same change on
> mach-imx/include/mach/imxfb.h :
> 
> --- a/arch/arm/mach-imx/include/mach/imxfb.h
> +++ b/arch/arm/mach-imx/include/mach/imxfb.h
> @@ -14,7 +14,6 @@
>  #define PCR_BPIX_8 (3 << 25)
>  #define PCR_BPIX_12    (4 << 25)
>  #define PCR_BPIX_16    (4 << 25)
> +#define PCR_BPIX_MASK   (7 << 25)
>  #define PCR_PIXPOL (1 << 24)
>  #define PCR_FLMPOL (1 << 23)
>  #define PCR_LPPOL  (1 << 22)
> 
> I take this opportunity to ask a question:
> on APF27 with imx27, I found that the system freeze
> when an interrupt handler is in place and an
> interruption happens.
> if interrupt handler is in place without interruption no
> problems and if interruption happens without interrupt
> handler it's ok too.
> 
> If someone could help or advise me. 
> thank you very much
> 
> Gwenhael
> 
> Signed-off-by: gwenhael.goavec-merou@domain.hid

What version of the I-pipe patch?

-- 
					    Gilles.

Re: [Xenomai-help] Xenomai on ARMadeus

[gwenhael.goavec]

On Mon, 02 Nov 2009 23:29:31 +0100
Gilles Chanteperdrix <gilles.chanteperdrix@xenomai.org>
wrote:

> gwenhael.goavec wrote:
> > On Mon, 02 Nov 2009 19:38:51 +0100
> > Gilles Chanteperdrix <gilles.chanteperdrix@xenomai.org>
> > wrote:
> > 
> >> By the way, did you make any change to the I-pipe patch to get it
> >> running on this board?
> > 
> > The only change realized, is made because Armadeus and
> > I-pipe does the same change on
> > mach-imx/include/mach/imxfb.h :
> > 
> > --- a/arch/arm/mach-imx/include/mach/imxfb.h
> > +++ b/arch/arm/mach-imx/include/mach/imxfb.h
> > @@ -14,7 +14,6 @@
> >  #define PCR_BPIX_8 (3 << 25)
> >  #define PCR_BPIX_12    (4 << 25)
> >  #define PCR_BPIX_16    (4 << 25)
> > +#define PCR_BPIX_MASK   (7 << 25)
> >  #define PCR_PIXPOL (1 << 24)
> >  #define PCR_FLMPOL (1 << 23)
> >  #define PCR_LPPOL  (1 << 22)
> > 
> > I take this opportunity to ask a question:
> > on APF27 with imx27, I found that the system freeze
> > when an interrupt handler is in place and an
> > interruption happens.
> > if interrupt handler is in place without interruption no
> > problems and if interruption happens without interrupt
> > handler it's ok too.
> > 
> > If someone could help or advise me. 
> > thank you very much
> > 
> > Gwenhael
> > 
> > Signed-off-by: gwenhael.goavec-merou@domain.hid
> 
> What version of the I-pipe patch?
> 
> -- 
> 					    Gilles.

Version 1.13-03 for 2.6.29 

Gwenhael

Signed-off-by: gwenhael.goavec-merou@domain.hid

Re: [Xenomai-help] Xenomai on ARMadeus

[Gilles Chanteperdrix]

gwenhael.goavec wrote:
> On Mon, 02 Nov 2009 23:29:31 +0100
> Gilles Chanteperdrix <gilles.chanteperdrix@xenomai.org>
> wrote:
> 
>> gwenhael.goavec wrote:
>>> On Mon, 02 Nov 2009 19:38:51 +0100
>>> Gilles Chanteperdrix <gilles.chanteperdrix@xenomai.org>
>>> wrote:
>>>
>>>> By the way, did you make any change to the I-pipe patch to get it
>>>> running on this board?
>>> The only change realized, is made because Armadeus and
>>> I-pipe does the same change on
>>> mach-imx/include/mach/imxfb.h :
>>>
>>> --- a/arch/arm/mach-imx/include/mach/imxfb.h
>>> +++ b/arch/arm/mach-imx/include/mach/imxfb.h
>>> @@ -14,7 +14,6 @@
>>>  #define PCR_BPIX_8 (3 << 25)
>>>  #define PCR_BPIX_12    (4 << 25)
>>>  #define PCR_BPIX_16    (4 << 25)
>>> +#define PCR_BPIX_MASK   (7 << 25)
>>>  #define PCR_PIXPOL (1 << 24)
>>>  #define PCR_FLMPOL (1 << 23)
>>>  #define PCR_LPPOL  (1 << 22)
>>>
>>> I take this opportunity to ask a question:
>>> on APF27 with imx27, I found that the system freeze
>>> when an interrupt handler is in place and an
>>> interruption happens.
>>> if interrupt handler is in place without interruption no
>>> problems and if interruption happens without interrupt
>>> handler it's ok too.
>>>
>>> If someone could help or advise me. 
>>> thank you very much

The reason I knew for this problem was if handle_edge_irq was used for
some interrupts. But problems of this kind for IMX GPIOs are fixed in
1.3-03.

Do you have this problem with in-kernel or out-of-tree drivers? With
real-time or non real-time interrupts? Could you show us
/proc/interrupts before the system freezes?

-- 
                                          Gilles

Re: [Xenomai-help] Xenomai on ARMadeus

[Gilles Chanteperdrix]

Gwenhaël Goavec-Merou wrote:
> Sorry for the late
>> gwenhael.goavec wrote:
>>> On Mon, 02 Nov 2009 23:29:31 +0100
>>> Gilles Chanteperdrix <gilles.chanteperdrix@xenomai.org>
>>> wrote:
>>>
>>>> gwenhael.goavec wrote:
>>>>> On Mon, 02 Nov 2009 19:38:51 +0100
>>>>> Gilles Chanteperdrix <gilles.chanteperdrix@xenomai.org>
>>>>> wrote:
>>>>>
>>>>>> By the way, did you make any change to the I-pipe patch to get it
>>>>>> running on this board?
>>>>> The only change realized, is made because Armadeus and
>>>>> I-pipe does the same change on
>>>>> mach-imx/include/mach/imxfb.h :
>>>>>
>>>>> --- a/arch/arm/mach-imx/include/mach/imxfb.h
>>>>> +++ b/arch/arm/mach-imx/include/mach/imxfb.h
>>>>> @@ -14,7 +14,6 @@
>>>>>  #define PCR_BPIX_8 (3 << 25)
>>>>>  #define PCR_BPIX_12    (4 << 25)
>>>>>  #define PCR_BPIX_16    (4 << 25)
>>>>> +#define PCR_BPIX_MASK   (7 << 25)
>>>>>  #define PCR_PIXPOL (1 << 24)
>>>>>  #define PCR_FLMPOL (1 << 23)
>>>>>  #define PCR_LPPOL  (1 << 22)
>>>>>
>>>>> I take this opportunity to ask a question:
>>>>> on APF27 with imx27, I found that the system freeze
>>>>> when an interrupt handler is in place and an
>>>>> interruption happens.
>>>>> if interrupt handler is in place without interruption no
>>>>> problems and if interruption happens without interrupt
>>>>> handler it's ok too.
>>>>>
>>>>> If someone could help or advise me.
>>>>> thank you very much
>> The reason I knew for this problem was if handle_edge_irq was used for
>> some interrupts. But problems of this kind for IMX GPIOs are fixed in
>> 1.3-03.
>>
>> Do you have this problem with in-kernel or out-of-tree drivers? With
>> real-time or non real-time interrupts? Could you show us
>> /proc/interrupts before the system freezes?
>>
> I use an RTDM real-time driver for testing
> GPIO interrupt don't appears in /proc/interrupts
> 
> ~ # cat /proc/interrupts
>            CPU0
>  20:         69           -  IMX-uart
>  26:       1664           -  i.MX Timer Tick
>  29:      84439           -  mxc_nd
>  50:         75           -  fec
>  53:          0           -  VPU_CODEC_IRQ
> Err:          0

Appear in /proc/interrupts only the Linux domain interrupts.
Xenomai domain interrupts appear in /proc/xenomai/irq.

Now, if you are sure to use the latest I-pipe patch, you should try and
follow the course of the interrupts masking, acking, calling the handler
and unmasking, to understand what happens. The most likely problem is
that your interrupt handler gets called in an infinite loop, for some
reason. The reason may be that the interrupt controller is configured
for the wrong type (edge/level) of irq, or simply that your interrupt
handler is bogus, or something else.

We could help you if you showed us the driver code, and told us what
kind of interrupt (edge, level) the hardware is using.

You could also try to implement the driver as a plain linux driver and
run it over the xenomai patched kernel and a plain linux kernel, to see
if the problem is in the interrupt handling code, I-pipe irq handling
code, or something else.

-- 
                                          Gilles

Re: [Xenomai-core] [PATCH v3 5/9] nucleus: Avoid returning errors from xnheap_destroy_mapped

[Philippe Gerum]

On Mon, 2009-11-02 at 19:01 +0100, Jan Kiszka wrote:
> Jan Kiszka wrote:
> > Philippe Gerum wrote:
> >> On Mon, 2009-11-02 at 17:41 +0100, Jan Kiszka wrote:
> >>> Philippe Gerum wrote:
> >>>> On Sat, 2009-10-24 at 19:22 +0200, Philippe Gerum wrote:
> >>>>> On Tue, 2009-10-20 at 13:37 +0200, Jan Kiszka wrote:
> >>>>>> Allowing xnheap_delete_mapped to return an error and then attempting to
> >>>>>> recover from it does not work out very well: Corner cases are racy,
> >>>>>> intransparent to the user, and proper error handling imposes a lot of
> >>>>>> complexity on the caller - if it actually bothers to check the return
> >>>>>> value...
> >>>>>>
> >>>>>> Fortunately, there is no reason for this function to fail: If the heap
> >>>>>> is still mapped, just install the provide cleanup handler and switch to
> >>>>>> deferred removal. If the unmapping fails, we either raced with some
> >>>>>> other caller of unmap or user space provided a bogus address, or
> >>>>>> something else is wrong. In any case, leaving the cleanup callback
> >>>>>> behind is the best we can do anyway.
> >>>>>>
> >>>>>> Removing the return value immediately allows to simplify the callers,
> >>>>>> namemly rt_queue_delete and rt_heap_delete.
> >>>>>>
> >>>>>> Note: This is still not 100% waterproof. If we issue
> >>>>>> xnheap_destroy_mapped from module cleanup passing a release handler
> >>>>>> that belongs to the module text, deferred release will cause a crash.
> >>>>>> But this corner case is no new regression, so let's keep the head in the
> >>>>>> sand.
> >>>>> I agree with this one, eventually. This does make things clearer, and
> >>>>> removes some opportunities for the upper interfaces to shot themselves
> >>>>> in the foot. Merged, thanks.
> >>>> Well, actually, it does make things clearer, but it is broken. Enabling
> >>>> list debugging makes the nucleus pull the break after a double unlink in
> >>>> vmclose().
> >>>>
> >>>> Basically, the issue is that calling rt_queue/heap_delete() explicitly
> >>>> from userland will break, due to the vmclose() handler being indirectly
> >>>> called by do_munmap() for the last mapping. The nasty thing is that
> >>>> without debugs on, kheapq is just silently trashed.
> >>>>
> >>>> Fix is on its way, along with nommu support for shared heaps as well.
> >>> OK, I see. Just on minor add-on to your fix:
> >>>
> >>> diff --git a/ksrc/nucleus/heap.c b/ksrc/nucleus/heap.c
> >>> index ec14f73..1ae6af6 100644
> >>> --- a/ksrc/nucleus/heap.c
> >>> +++ b/ksrc/nucleus/heap.c
> >>> @@ -1241,6 +1241,7 @@ void xnheap_destroy_mapped(xnheap_t *heap,
> >>>  		down_write(&current->mm->mmap_sem);
> >>>  		heap->archdep.release = NULL;
> >>>  		do_munmap(current->mm, (unsigned long)mapaddr, len);
> >>> +		heap->archdep.release = release;
> >>>  		up_write(&current->mm->mmap_sem);
> >>>  	}
> >>>  
> >>> @@ -1252,7 +1253,6 @@ void xnheap_destroy_mapped(xnheap_t *heap,
> >>>  	if (heap->archdep.numaps > 0) {
> >>>  		/* The release handler is supposed to clean up the rest. */
> >>>  		XENO_ASSERT(NUCLEUS, release != NULL, /* nop */);
> >>> -		heap->archdep.release = release;
> >>>  		return;
> >>>  	}
> >>>  
> >>>
> >>> This is safer than leaving a potential race window open between dropping
> >>> mmap_sem and fixing up archdep.release again.
> >>>
> >> Actually, we have to hold the kheap lock, in case weird code starts
> >> mapping randomly from userland without getting a valid descriptor
> >> through a skin call.
> > 
> > Yep, that as well.
> > 
> 
> Note that 6b1a185b46 doesn't obsolete my patch (pull it from my tree if
> you like).

Are you still referring to a race with the vmclose() handler?

> 
> Jan
> 


-- 
Philippe.

Re: [Xenomai-core] [PATCH v3 5/9] nucleus: Avoid returning errors from xnheap_destroy_mapped

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3874 bytes --]

Philippe Gerum wrote:
> On Mon, 2009-11-02 at 19:01 +0100, Jan Kiszka wrote:
>> Jan Kiszka wrote:
>>> Philippe Gerum wrote:
>>>> On Mon, 2009-11-02 at 17:41 +0100, Jan Kiszka wrote:
>>>>> Philippe Gerum wrote:
>>>>>> On Sat, 2009-10-24 at 19:22 +0200, Philippe Gerum wrote:
>>>>>>> On Tue, 2009-10-20 at 13:37 +0200, Jan Kiszka wrote:
>>>>>>>> Allowing xnheap_delete_mapped to return an error and then attempting to
>>>>>>>> recover from it does not work out very well: Corner cases are racy,
>>>>>>>> intransparent to the user, and proper error handling imposes a lot of
>>>>>>>> complexity on the caller - if it actually bothers to check the return
>>>>>>>> value...
>>>>>>>>
>>>>>>>> Fortunately, there is no reason for this function to fail: If the heap
>>>>>>>> is still mapped, just install the provide cleanup handler and switch to
>>>>>>>> deferred removal. If the unmapping fails, we either raced with some
>>>>>>>> other caller of unmap or user space provided a bogus address, or
>>>>>>>> something else is wrong. In any case, leaving the cleanup callback
>>>>>>>> behind is the best we can do anyway.
>>>>>>>>
>>>>>>>> Removing the return value immediately allows to simplify the callers,
>>>>>>>> namemly rt_queue_delete and rt_heap_delete.
>>>>>>>>
>>>>>>>> Note: This is still not 100% waterproof. If we issue
>>>>>>>> xnheap_destroy_mapped from module cleanup passing a release handler
>>>>>>>> that belongs to the module text, deferred release will cause a crash.
>>>>>>>> But this corner case is no new regression, so let's keep the head in the
>>>>>>>> sand.
>>>>>>> I agree with this one, eventually. This does make things clearer, and
>>>>>>> removes some opportunities for the upper interfaces to shot themselves
>>>>>>> in the foot. Merged, thanks.
>>>>>> Well, actually, it does make things clearer, but it is broken. Enabling
>>>>>> list debugging makes the nucleus pull the break after a double unlink in
>>>>>> vmclose().
>>>>>>
>>>>>> Basically, the issue is that calling rt_queue/heap_delete() explicitly
>>>>>> from userland will break, due to the vmclose() handler being indirectly
>>>>>> called by do_munmap() for the last mapping. The nasty thing is that
>>>>>> without debugs on, kheapq is just silently trashed.
>>>>>>
>>>>>> Fix is on its way, along with nommu support for shared heaps as well.
>>>>> OK, I see. Just on minor add-on to your fix:
>>>>>
>>>>> diff --git a/ksrc/nucleus/heap.c b/ksrc/nucleus/heap.c
>>>>> index ec14f73..1ae6af6 100644
>>>>> --- a/ksrc/nucleus/heap.c
>>>>> +++ b/ksrc/nucleus/heap.c
>>>>> @@ -1241,6 +1241,7 @@ void xnheap_destroy_mapped(xnheap_t *heap,
>>>>>  		down_write(&current->mm->mmap_sem);
>>>>>  		heap->archdep.release = NULL;
>>>>>  		do_munmap(current->mm, (unsigned long)mapaddr, len);
>>>>> +		heap->archdep.release = release;
>>>>>  		up_write(&current->mm->mmap_sem);
>>>>>  	}
>>>>>  
>>>>> @@ -1252,7 +1253,6 @@ void xnheap_destroy_mapped(xnheap_t *heap,
>>>>>  	if (heap->archdep.numaps > 0) {
>>>>>  		/* The release handler is supposed to clean up the rest. */
>>>>>  		XENO_ASSERT(NUCLEUS, release != NULL, /* nop */);
>>>>> -		heap->archdep.release = release;
>>>>>  		return;
>>>>>  	}
>>>>>  
>>>>>
>>>>> This is safer than leaving a potential race window open between dropping
>>>>> mmap_sem and fixing up archdep.release again.
>>>>>
>>>> Actually, we have to hold the kheap lock, in case weird code starts
>>>> mapping randomly from userland without getting a valid descriptor
>>>> through a skin call.
>>> Yep, that as well.
>>>
>> Note that 6b1a185b46 doesn't obsolete my patch (pull it from my tree if
>> you like).
> 
> Are you still referring to a race with the vmclose() handler?
> 

Went through it again, and it's safe as it is (my patch would actually
open a new whole) - dropped.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Xenomai-core] [PATCH v3 6/9] rtai: Try to fix _shm_free

[Jan Kiszka]

This is totally untested but should not make things worse than they
already are.

Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
---

 ksrc/skins/rtai/shm.c |   31 +++++++++++++++++++------------
 1 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/ksrc/skins/rtai/shm.c b/ksrc/skins/rtai/shm.c
index 4c56495..21c3b07 100644
--- a/ksrc/skins/rtai/shm.c
+++ b/ksrc/skins/rtai/shm.c
@@ -260,19 +260,24 @@ void *rt_heap_open(unsigned long name, int size, int suprt)
 	return _shm_alloc(name, size, suprt, 0, &opaque);
 }
 
-#ifndef CONFIG_XENO_OPT_PERVASIVE
+#ifdef CONFIG_XENO_OPT_PERVASIVE
+static void __heap_flush_shared(xnheap_t *heap)
+{
+	xnheap_free(&kheap, heap);
+}
+#else /* !CONFIG_XENO_OPT_PERVASIVE */
 static void __heap_flush_private(xnheap_t *heap,
 				 void *heapmem, u_long heapsize, void *cookie)
 {
 	xnarch_free_host_mem(heapmem, heapsize);
 }
-#endif /* CONFIG_XENO_OPT_PERVASIVE */
+#endif /* !CONFIG_XENO_OPT_PERVASIVE */
 
 static int _shm_free(unsigned long name)
 {
-	int ret = 0;
 	xnholder_t *holder;
 	xnshm_a_t *p;
+	int ret;
 	spl_t s;
 
 	xnlock_get_irqsave(&nklock, s);
@@ -283,27 +288,29 @@ static int _shm_free(unsigned long name)
 		p = link2shma(holder);
 
 		if (p->name == name && --p->ref == 0) {
+			removeq(&xnshm_allocq, &p->link);
 			if (p->handle)
 				xnregistry_remove(p->handle);
+
+			xnlock_put_irqrestore(&nklock, s);
+
 			if (p->heap == &kheap)
 				xnheap_free(&kheap, p->chunk);
 			else {
-				/* Should release lock here? 
-				 * Can destroy_mapped suspend ?
-				 * [YES!]
-				 */
 #ifdef CONFIG_XENO_OPT_PERVASIVE
-				xnheap_destroy_mapped(p->heap, NULL, NULL);
+				xnheap_destroy_mapped(p->heap,
+						      __heap_flush_shared,
+						      NULL);
 #else /* !CONFIG_XENO_OPT_PERVASIVE */
 				xnheap_destroy(p->heap,
 					       &__heap_flush_private, NULL);
-#endif /* !CONFIG_XENO_OPT_PERVASIVE */
 				xnheap_free(&kheap, p->heap);
+#endif /* !CONFIG_XENO_OPT_PERVASIVE */
 			}
-			removeq(&xnshm_allocq, &p->link);
 			ret = p->size;
 			xnheap_free(&kheap, p);
-			break;
+
+			return ret;
 		}
 
 		holder = nextq(&xnshm_allocq, holder);
@@ -311,7 +318,7 @@ static int _shm_free(unsigned long name)
 
 	xnlock_put_irqrestore(&nklock, s);
 
-	return ret;
+	return 0;
 }
 
 int rt_shm_free(unsigned long name)

Re: [Xenomai-core] [PATCH v3 6/9] rtai: Try to fix _shm_free

[Philippe Gerum]

On Tue, 2009-10-20 at 13:37 +0200, Jan Kiszka wrote:
> This is totally untested but should not make things worse than they
> already are.

I cowardly agree with this accurate analysis.

Looks ok anyway, and if not eventually, well, there are one or two -rc's
left, for the one or two users of this skin to check and validate those
changes.

> 
> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
> ---
> 
>  ksrc/skins/rtai/shm.c |   31 +++++++++++++++++++------------
>  1 files changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/ksrc/skins/rtai/shm.c b/ksrc/skins/rtai/shm.c
> index 4c56495..21c3b07 100644
> --- a/ksrc/skins/rtai/shm.c
> +++ b/ksrc/skins/rtai/shm.c
> @@ -260,19 +260,24 @@ void *rt_heap_open(unsigned long name, int size, int suprt)
>  	return _shm_alloc(name, size, suprt, 0, &opaque);
>  }
>  
> -#ifndef CONFIG_XENO_OPT_PERVASIVE
> +#ifdef CONFIG_XENO_OPT_PERVASIVE
> +static void __heap_flush_shared(xnheap_t *heap)
> +{
> +	xnheap_free(&kheap, heap);
> +}
> +#else /* !CONFIG_XENO_OPT_PERVASIVE */
>  static void __heap_flush_private(xnheap_t *heap,
>  				 void *heapmem, u_long heapsize, void *cookie)
>  {
>  	xnarch_free_host_mem(heapmem, heapsize);
>  }
> -#endif /* CONFIG_XENO_OPT_PERVASIVE */
> +#endif /* !CONFIG_XENO_OPT_PERVASIVE */
>  
>  static int _shm_free(unsigned long name)
>  {
> -	int ret = 0;
>  	xnholder_t *holder;
>  	xnshm_a_t *p;
> +	int ret;
>  	spl_t s;
>  
>  	xnlock_get_irqsave(&nklock, s);
> @@ -283,27 +288,29 @@ static int _shm_free(unsigned long name)
>  		p = link2shma(holder);
>  
>  		if (p->name == name && --p->ref == 0) {
> +			removeq(&xnshm_allocq, &p->link);
>  			if (p->handle)
>  				xnregistry_remove(p->handle);
> +
> +			xnlock_put_irqrestore(&nklock, s);
> +
>  			if (p->heap == &kheap)
>  				xnheap_free(&kheap, p->chunk);
>  			else {
> -				/* Should release lock here? 
> -				 * Can destroy_mapped suspend ?
> -				 * [YES!]
> -				 */
>  #ifdef CONFIG_XENO_OPT_PERVASIVE
> -				xnheap_destroy_mapped(p->heap, NULL, NULL);
> +				xnheap_destroy_mapped(p->heap,
> +						      __heap_flush_shared,
> +						      NULL);
>  #else /* !CONFIG_XENO_OPT_PERVASIVE */
>  				xnheap_destroy(p->heap,
>  					       &__heap_flush_private, NULL);
> -#endif /* !CONFIG_XENO_OPT_PERVASIVE */
>  				xnheap_free(&kheap, p->heap);
> +#endif /* !CONFIG_XENO_OPT_PERVASIVE */
>  			}
> -			removeq(&xnshm_allocq, &p->link);
>  			ret = p->size;
>  			xnheap_free(&kheap, p);
> -			break;
> +
> +			return ret;
>  		}
>  
>  		holder = nextq(&xnshm_allocq, holder);
> @@ -311,7 +318,7 @@ static int _shm_free(unsigned long name)
>  
>  	xnlock_put_irqrestore(&nklock, s);
>  
> -	return ret;
> +	return 0;
>  }
>  
>  int rt_shm_free(unsigned long name)
> 
-- 
Philippe.

[Xenomai-core] [PATCH v3 2/9] nucleus: Use Linux spin lock for heap list management

[Jan Kiszka]

No need for hard nklock protection of kheapq and the map counter, a
normal spin lock suffices as all users must run over the root thread
anyway.

Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
---

 ksrc/nucleus/heap.c |   26 ++++++++++++--------------
 1 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/ksrc/nucleus/heap.c b/ksrc/nucleus/heap.c
index 9ca2591..9ba5227 100644
--- a/ksrc/nucleus/heap.c
+++ b/ksrc/nucleus/heap.c
@@ -951,8 +951,10 @@ EXPORT_SYMBOL_GPL(xnheap_check_block);
 #include <linux/device.h>
 #include <linux/vmalloc.h>
 #include <linux/mm.h>
+#include <linux/spinlock.h>
 
 static DEFINE_XNQUEUE(kheapq);	/* Shared heap queue. */
+static DEFINE_SPINLOCK(kheapq_lock);
 
 static inline void *__alloc_and_reserve_heap(size_t size, int kmflags)
 {
@@ -1022,14 +1024,13 @@ static void __unreserve_and_free_heap(void *ptr, size_t size, int kmflags)
 static void xnheap_vmclose(struct vm_area_struct *vma)
 {
 	xnheap_t *heap = vma->vm_private_data;
-	spl_t s;
 
-	xnlock_get_irqsave(&nklock, s);
+	spin_lock(&kheapq_lock);
 
 	if (atomic_dec_and_test(&heap->archdep.numaps)) {
 		if (heap->archdep.release) {
 			removeq(&kheapq, &heap->link);
-			xnlock_put_irqrestore(&nklock, s);
+			spin_unlock(&kheapq_lock);
 			__unreserve_and_free_heap(heap->archdep.heapbase,
 						  xnheap_extentsize(heap),
 						  heap->archdep.kmflags);
@@ -1038,7 +1039,7 @@ static void xnheap_vmclose(struct vm_area_struct *vma)
 		}
 	}
 
-	xnlock_put_irqrestore(&nklock, s);
+	spin_unlock(&kheapq_lock);
 }
 
 static struct vm_operations_struct xnheap_vmops = {
@@ -1068,9 +1069,8 @@ static int xnheap_ioctl(struct inode *inode,
 {
 	xnheap_t *heap;
 	int err = 0;
-	spl_t s;
 
-	xnlock_get_irqsave(&nklock, s);
+	spin_lock(&kheapq_lock);
 
 	heap = __validate_heap_addr((void *)arg);
 
@@ -1083,7 +1083,7 @@ static int xnheap_ioctl(struct inode *inode,
 
       unlock_and_exit:
 
-	xnlock_put_irqrestore(&nklock, s);
+	spin_unlock(&kheapq_lock);
 
 	return err;
 }
@@ -1148,7 +1148,6 @@ static int xnheap_mmap(struct file *file, struct vm_area_struct *vma)
 int xnheap_init_mapped(xnheap_t *heap, u_long heapsize, int memflags)
 {
 	void *heapbase;
-	spl_t s;
 	int err;
 
 	/* Caller must have accounted for internal overhead. */
@@ -1172,9 +1171,9 @@ int xnheap_init_mapped(xnheap_t *heap, u_long heapsize, int memflags)
 	heap->archdep.heapbase = heapbase;
 	heap->archdep.release = NULL;
 
-	xnlock_get_irqsave(&nklock, s);
+	spin_lock(&kheapq_lock);
 	appendq(&kheapq, &heap->link);
-	xnlock_put_irqrestore(&nklock, s);
+	spin_unlock(&kheapq_lock);
 
 	return 0;
 }
@@ -1184,20 +1183,19 @@ int xnheap_destroy_mapped(xnheap_t *heap, void (*release)(struct xnheap *heap),
 {
 	int ret = 0, ccheck;
 	unsigned long len;
-	spl_t s;
 
 	ccheck = mapaddr ? 1 : 0;
 
-	xnlock_get_irqsave(&nklock, s);
+	spin_lock(&kheapq_lock);
 
 	if (atomic_read(&heap->archdep.numaps) > ccheck) {
 		heap->archdep.release = release;
-		xnlock_put_irqrestore(&nklock, s);
+		spin_unlock(&kheapq_lock);
 		return -EBUSY;
 	}
 
 	removeq(&kheapq, &heap->link); /* Prevent further mapping. */
-	xnlock_put_irqrestore(&nklock, s);
+	spin_unlock(&kheapq_lock);
 
 	len = xnheap_extentsize(heap);
 

[Xenomai-core] [PATCH v3 9/9] nucleus: Include all heaps in statistics

[Jan Kiszka]

This extends /proc/xenomai/heap with statistics about all currently used
heaps. It takes care to flush nklock while iterating of this potentially
long list.

Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
---

 include/nucleus/heap.h    |   12 +++-
 ksrc/drivers/ipc/iddp.c   |    3 +
 ksrc/drivers/ipc/xddp.c   |    6 ++
 ksrc/nucleus/heap.c       |  131 ++++++++++++++++++++++++++++++++++++++++-----
 ksrc/nucleus/module.c     |    2 -
 ksrc/nucleus/pod.c        |    5 +-
 ksrc/nucleus/shadow.c     |    5 +-
 ksrc/skins/native/heap.c  |    6 +-
 ksrc/skins/native/pipe.c  |    4 +
 ksrc/skins/native/queue.c |    6 +-
 ksrc/skins/posix/shm.c    |    4 +
 ksrc/skins/psos+/rn.c     |    6 +-
 ksrc/skins/rtai/shm.c     |    7 ++
 ksrc/skins/vrtx/heap.c    |    6 +-
 ksrc/skins/vrtx/syscall.c |    3 +
 15 files changed, 169 insertions(+), 37 deletions(-)

diff --git a/include/nucleus/heap.h b/include/nucleus/heap.h
index 44db738..f653cd7 100644
--- a/include/nucleus/heap.h
+++ b/include/nucleus/heap.h
@@ -115,6 +115,10 @@ typedef struct xnheap {
 
 	XNARCH_DECL_DISPLAY_CONTEXT();
 
+	xnholder_t stat_link;	/* Link in heapq */
+
+	char name[48];
+
 } xnheap_t;
 
 extern xnheap_t kheap;
@@ -202,7 +206,8 @@ void xnheap_cleanup_proc(void);
 
 int xnheap_init_mapped(xnheap_t *heap,
 		       u_long heapsize,
-		       int memflags);
+		       int memflags,
+		       const char *name, ...);
 
 void xnheap_destroy_mapped(xnheap_t *heap,
 			   void (*release)(struct xnheap *heap),
@@ -224,7 +229,10 @@ void xnheap_destroy_mapped(xnheap_t *heap,
 int xnheap_init(xnheap_t *heap,
 		void *heapaddr,
 		u_long heapsize,
-		u_long pagesize);
+		u_long pagesize,
+		const char *name, ...);
+
+void xnheap_set_name(xnheap_t *heap, const char *name, ...);
 
 void xnheap_destroy(xnheap_t *heap,
 		    void (*flushfn)(xnheap_t *heap,
diff --git a/ksrc/drivers/ipc/iddp.c b/ksrc/drivers/ipc/iddp.c
index a407946..b6382f1 100644
--- a/ksrc/drivers/ipc/iddp.c
+++ b/ksrc/drivers/ipc/iddp.c
@@ -559,7 +559,8 @@ static int __iddp_bind_socket(struct rtipc_private *priv,
 		}
 
 		ret = xnheap_init(&sk->privpool,
-				  poolmem, poolsz, XNHEAP_PAGE_SIZE);
+				  poolmem, poolsz, XNHEAP_PAGE_SIZE,
+				  "ippd: %d", port);
 		if (ret) {
 			xnarch_free_host_mem(poolmem, poolsz);
 			goto fail;
diff --git a/ksrc/drivers/ipc/xddp.c b/ksrc/drivers/ipc/xddp.c
index f62147a..a5dafef 100644
--- a/ksrc/drivers/ipc/xddp.c
+++ b/ksrc/drivers/ipc/xddp.c
@@ -703,7 +703,7 @@ static int __xddp_bind_socket(struct rtipc_private *priv,
 		}
 
 		ret = xnheap_init(&sk->privpool,
-				  poolmem, poolsz, XNHEAP_PAGE_SIZE);
+				  poolmem, poolsz, XNHEAP_PAGE_SIZE, "");
 		if (ret) {
 			xnarch_free_host_mem(poolmem, poolsz);
 			goto fail;
@@ -746,6 +746,10 @@ static int __xddp_bind_socket(struct rtipc_private *priv,
 	sk->minor = ret;
 	sa->sipc_port = ret;
 	sk->name = *sa;
+
+	if (poolsz > 0)
+		xnheap_set_name(sk->bufpool, "xddp: %d", sa->sipc_port);
+
 	/* Set default destination if unset at binding time. */
 	if (sk->peer.sipc_port < 0)
 		sk->peer = *sa;
diff --git a/ksrc/nucleus/heap.c b/ksrc/nucleus/heap.c
index 96c46f8..793d1c5 100644
--- a/ksrc/nucleus/heap.c
+++ b/ksrc/nucleus/heap.c
@@ -76,6 +76,9 @@ EXPORT_SYMBOL_GPL(kheap);
 xnheap_t kstacks;	/* Private stack pool */
 #endif
 
+static DEFINE_XNQUEUE(heapq);	/* Heap list for /proc reporting */
+static unsigned long heapq_rev;
+
 static void init_extent(xnheap_t *heap, xnextent_t *extent)
 {
 	caddr_t freepage;
@@ -108,7 +111,7 @@ static void init_extent(xnheap_t *heap, xnextent_t *extent)
  */
 
 /*!
- * \fn xnheap_init(xnheap_t *heap,void *heapaddr,u_long heapsize,u_long pagesize)
+ * \fn xnheap_init(xnheap_t *heap,void *heapaddr,u_long heapsize,u_long pagesize,const char *name,...)
  * \brief Initialize a memory heap.
  *
  * Initializes a memory heap suitable for time-bounded allocation
@@ -145,6 +148,10 @@ static void init_extent(xnheap_t *heap, xnextent_t *extent)
  * best one for your needs. In the current implementation, pagesize
  * must be a power of two in the range [ 8 .. 32768 ] inclusive.
  *
+ * @param name Name displayed in statistic outputs. This parameter can
+ * be a format string, in which case succeeding parameters will be used
+ * to resolve the final name.
+ *
  * @return 0 is returned upon success, or one of the following error
  * codes:
  *
@@ -161,12 +168,13 @@ static void init_extent(xnheap_t *heap, xnextent_t *extent)
  * Rescheduling: never.
  */
 
-int xnheap_init(xnheap_t *heap,
-		void *heapaddr, u_long heapsize, u_long pagesize)
+static int xnheap_init_va(xnheap_t *heap, void *heapaddr, u_long heapsize,
+			  u_long pagesize, const char *name, va_list args)
 {
 	unsigned cpu, nr_cpus = xnarch_num_online_cpus();
 	u_long hdrsize, shiftsize, pageshift;
 	xnextent_t *extent;
+	spl_t s;
 
 	/*
 	 * Perform some parametrical checks first.
@@ -232,12 +240,71 @@ int xnheap_init(xnheap_t *heap,
 
 	appendq(&heap->extents, &extent->link);
 
+	vsnprintf(heap->name, sizeof(heap->name), name, args);
+
+	xnlock_get_irqsave(&nklock, s);
+	appendq(&heapq, &heap->stat_link);
+	heapq_rev++;
+	xnlock_put_irqrestore(&nklock, s);
+
 	xnarch_init_display_context(heap);
 
 	return 0;
 }
+
+int xnheap_init(xnheap_t *heap, void *heapaddr, u_long heapsize,
+		u_long pagesize, const char *name, ...)
+{
+	va_list args;
+	int ret;
+
+	va_start(args, name);
+	ret = xnheap_init_va(heap, heapaddr, heapsize, pagesize, name, args);
+	va_end(args);
+
+	return ret;
+}
 EXPORT_SYMBOL_GPL(xnheap_init);
 
+/*!
+ * \fn xnheap_set_name(xnheap_t *heap,const char *name,...)
+ * \brief Overwrite the heap's name.
+ *
+ * Set the heap name that will be used in statistic outputs. This service
+ * is useful if the final name is not yet defined on xnheap_init().
+ *
+ * @param heap The address of a heap descriptor.
+ *
+ * @param name Name displayed in statistic outputs. This parameter can
+ * be a format string, in which case succeeding parameters will be used
+ * to resolve the final name.
+ *
+ * Environments:
+ *
+ * This service can be called from:
+ *
+ * - Kernel module initialization/cleanup code
+ * - Kernel-based task
+ * - User-space task
+ *
+ * Rescheduling: never.
+ */
+
+void xnheap_set_name(xnheap_t *heap, const char *name, ...)
+{
+	va_list args;
+	spl_t s;
+
+	va_start(args, name);
+
+	xnlock_get_irqsave(&nklock, s);
+	vsnprintf(heap->name, sizeof(heap->name), name, args);
+	xnlock_put_irqrestore(&nklock, s);
+
+	va_end(args);
+}
+EXPORT_SYMBOL_GPL(xnheap_set_name);
+
 /*! 
  * \fn void xnheap_destroy(xnheap_t *heap, void (*flushfn)(xnheap_t *heap, void *extaddr, u_long extsize, void *cookie), void *cookie)
  * \brief Destroys a memory heap.
@@ -273,6 +340,11 @@ void xnheap_destroy(xnheap_t *heap,
 	xnholder_t *holder;
 	spl_t s;
 
+	xnlock_get_irqsave(&nklock, s);
+	removeq(&heapq, &heap->stat_link);
+	heapq_rev++;
+	xnlock_put_irqrestore(&nklock, s);
+
 	if (!flushfn)
 		return;
 
@@ -1140,9 +1212,11 @@ deref_out:
 	return err;
 }
 
-int xnheap_init_mapped(xnheap_t *heap, u_long heapsize, int memflags)
+int xnheap_init_mapped(xnheap_t *heap, u_long heapsize, int memflags,
+		       const char *name, ...)
 {
 	void *heapbase;
+	va_list args;
 	int err;
 
 	/* Caller must have accounted for internal overhead. */
@@ -1156,7 +1230,9 @@ int xnheap_init_mapped(xnheap_t *heap, u_long heapsize, int memflags)
 	if (!heapbase)
 		return -ENOMEM;
 
-	err = xnheap_init(heap, heapbase, heapsize, PAGE_SIZE);
+	va_start(args, name);
+	err = xnheap_init_va(heap, heapbase, heapsize, PAGE_SIZE, name, args);
+	va_end(args);
 	if (err) {
 		__unreserve_and_free_heap(heapbase, heapsize, memflags);
 		return err;
@@ -1178,6 +1254,7 @@ void xnheap_destroy_mapped(xnheap_t *heap,
 			   void __user *mapaddr)
 {
 	unsigned long len;
+	spl_t s;
 
 	/*
 	 * Trying to unmap user memory without providing a release handler for
@@ -1185,6 +1262,11 @@ void xnheap_destroy_mapped(xnheap_t *heap,
 	 */
 	XENO_ASSERT(NUCLEUS, !mapaddr || release, /* nop */);
 
+	xnlock_get_irqsave(&nklock, s);
+	removeq(&heapq, &heap->stat_link);
+	heapq_rev++;
+	xnlock_put_irqrestore(&nklock, s);
+
 	spin_lock(&kheapq_lock);
 
 	removeq(&kheapq, &heap->link); /* Prevent further mapping. */
@@ -1285,22 +1367,41 @@ static int heap_read_proc(char *page,
 			  char **start,
 			  off_t off, int count, int *eof, void *data)
 {
+	unsigned long rev;
+	xnholder_t *entry;
+	xnheap_t *heap;
 	int len;
+	spl_t s;
 
 	if (!xnpod_active_p())
 		return -ESRCH;
 
-	len = sprintf(page, "size=%lu:used=%lu:pagesz=%lu  (main heap)\n",
-		      xnheap_usable_mem(&kheap),
-		      xnheap_used_mem(&kheap),
-		      xnheap_page_size(&kheap));
+	xnlock_get_irqsave(&nklock, s);
 
-#if CONFIG_XENO_OPT_SYS_STACKPOOLSZ > 0
-	len += sprintf(page + len, "size=%lu:used=%lu:pagesz=%lu  (stack pool)\n",
-		       xnheap_usable_mem(&kstacks),
-		       xnheap_used_mem(&kstacks),
-		       xnheap_page_size(&kstacks));
-#endif
+restart:
+	len = 0;
+
+	entry = getheadq(&heapq);
+	while (entry) {
+		heap = container_of(entry, xnheap_t, stat_link);
+		len += sprintf(page + len,
+			       "size=%lu:used=%lu:pagesz=%lu  (%s)\n",
+			       xnheap_usable_mem(heap),
+			       xnheap_used_mem(heap),
+			       xnheap_page_size(heap),
+			       heap->name);
+
+		rev = heapq_rev;
+
+		xnlock_sync_irq(&nklock, s);
+
+		if (heapq_rev != rev)
+			goto restart;
+
+		entry = nextq(&heapq, entry);
+	}
+
+	xnlock_put_irqrestore(&nklock, s);
 
 	len -= off;
 	if (len <= off + count)
diff --git a/ksrc/nucleus/module.c b/ksrc/nucleus/module.c
index 141276a..a0efc14 100644
--- a/ksrc/nucleus/module.c
+++ b/ksrc/nucleus/module.c
@@ -100,7 +100,7 @@ int __init __xeno_sys_init(void)
 #ifndef __XENO_SIM__
 	ret = xnheap_init_mapped(&__xnsys_global_ppd.sem_heap,
 				 CONFIG_XENO_OPT_GLOBAL_SEM_HEAPSZ * 1024,
-				 XNARCH_SHARED_HEAP_FLAGS);
+				 XNARCH_SHARED_HEAP_FLAGS, "global sem heap");
 	if (ret)
 		goto cleanup_arch;
 #endif
diff --git a/ksrc/nucleus/pod.c b/ksrc/nucleus/pod.c
index 370ad28..07fbcdb 100644
--- a/ksrc/nucleus/pod.c
+++ b/ksrc/nucleus/pod.c
@@ -384,9 +384,8 @@ int xnpod_init(void)
 	heapaddr = xnarch_alloc_host_mem(xnmod_sysheap_size);
 	if (heapaddr == NULL ||
 	    xnheap_init(&kheap, heapaddr, xnmod_sysheap_size,
-			XNHEAP_PAGE_SIZE) != 0) {
+			XNHEAP_PAGE_SIZE, "main heap") != 0)
 		return -ENOMEM;
-	}
 
 #if CONFIG_XENO_OPT_SYS_STACKPOOLSZ > 0
 	/*
@@ -404,7 +403,7 @@ int xnpod_init(void)
 	heapaddr = xnarch_alloc_stack_mem(CONFIG_XENO_OPT_SYS_STACKPOOLSZ * 1024);
 	if (heapaddr == NULL ||
 	    xnheap_init(&kstacks, heapaddr, CONFIG_XENO_OPT_SYS_STACKPOOLSZ * 1024,
-			XNHEAP_PAGE_SIZE) != 0) {
+			XNHEAP_PAGE_SIZE, "stack pool") != 0) {
 		xnheap_destroy(&kheap, &xnpod_flush_heap, NULL);
 		return -ENOMEM;
 	}
diff --git a/ksrc/nucleus/shadow.c b/ksrc/nucleus/shadow.c
index d6d1203..e8876d8 100644
--- a/ksrc/nucleus/shadow.c
+++ b/ksrc/nucleus/shadow.c
@@ -1886,7 +1886,9 @@ static void *xnshadow_sys_event(int event, void *data)
 
 		err = xnheap_init_mapped(&p->sem_heap,
 					 CONFIG_XENO_OPT_SEM_HEAPSZ * 1024,
-					 XNARCH_SHARED_HEAP_FLAGS);
+					 XNARCH_SHARED_HEAP_FLAGS,
+					 "private sem heap [%d]",
+					 current->pid);
 		if (err) {
 			xnarch_free_host_mem(p, sizeof(*p));
 			return ERR_PTR(err);
@@ -1896,6 +1898,7 @@ static void *xnshadow_sys_event(int event, void *data)
 
 	case XNSHADOW_CLIENT_DETACH:
 		p = ppd2sys((xnshadow_ppd_t *) data);
+
 		xnheap_destroy_mapped(&p->sem_heap, post_ppd_release, NULL);
 
 		return NULL;
diff --git a/ksrc/skins/native/heap.c b/ksrc/skins/native/heap.c
index 886758c..3072f99 100644
--- a/ksrc/skins/native/heap.c
+++ b/ksrc/skins/native/heap.c
@@ -266,7 +266,8 @@ int rt_heap_create(RT_HEAP *heap, const char *name, size_t heapsize, int mode)
 					 heapsize,
 					 ((mode & H_DMA) ? GFP_DMA : 0)
 					 | ((mode & H_NONCACHED) ?
-					    XNHEAP_GFP_NONCACHED : 0));
+					    XNHEAP_GFP_NONCACHED : 0),
+					 "rt_heap: %s", name);
 		if (err)
 			return err;
 
@@ -286,7 +287,8 @@ int rt_heap_create(RT_HEAP *heap, const char *name, size_t heapsize, int mode)
 		if (!heapmem)
 			return -ENOMEM;
 
-		err = xnheap_init(&heap->heap_base, heapmem, heapsize, XNHEAP_PAGE_SIZE);
+		err = xnheap_init(&heap->heap_base, heapmem, heapsize,
+				  XNHEAP_PAGE_SIZE, "rt_heap: %s", name);
 		if (err) {
 			xnarch_free_host_mem(heapmem, heapsize);
 			return err;
diff --git a/ksrc/skins/native/pipe.c b/ksrc/skins/native/pipe.c
index 672fa7a..18d9aa0 100644
--- a/ksrc/skins/native/pipe.c
+++ b/ksrc/skins/native/pipe.c
@@ -295,7 +295,9 @@ int rt_pipe_create(RT_PIPE *pipe, const char *name, int minor, size_t poolsize)
 			return -ENOMEM;
 
 		/* Use natural page size */
-		err = xnheap_init(&pipe->privpool, poolmem, poolsize, XNHEAP_PAGE_SIZE);
+		err = xnheap_init(&pipe->privpool, poolmem, poolsize,
+				  XNHEAP_PAGE_SIZE,
+				  "rt_pipe: %d / %s", minor, name);
 		if (err) {
 			xnarch_free_host_mem(poolmem, poolsize);
 			return err;
diff --git a/ksrc/skins/native/queue.c b/ksrc/skins/native/queue.c
index 249947a..becc0f0 100644
--- a/ksrc/skins/native/queue.c
+++ b/ksrc/skins/native/queue.c
@@ -229,7 +229,8 @@ int rt_queue_create(RT_QUEUE *q,
 		err = xnheap_init_mapped(&q->bufpool,
 					 poolsize,
 					 ((mode & Q_DMA) ? GFP_DMA 
-					  : XNARCH_SHARED_HEAP_FLAGS));
+					  : XNARCH_SHARED_HEAP_FLAGS),
+					 "rt_queue: %s", name);
 		if (err)
 			return err;
 
@@ -249,7 +250,8 @@ int rt_queue_create(RT_QUEUE *q,
 		if (!poolmem)
 			return -ENOMEM;
 
-		err = xnheap_init(&q->bufpool, poolmem, poolsize, XNHEAP_PAGE_SIZE);
+		err = xnheap_init(&q->bufpool, poolmem, poolsize,
+				  XNHEAP_PAGE_SIZE, "rt_queue: %s", name);
 		if (err) {
 			xnarch_free_host_mem(poolmem, poolsize);
 			return err;
diff --git a/ksrc/skins/posix/shm.c b/ksrc/skins/posix/shm.c
index c92096a..0f0bd1a 100644
--- a/ksrc/skins/posix/shm.c
+++ b/ksrc/skins/posix/shm.c
@@ -539,7 +539,9 @@ int ftruncate(int fd, off_t len)
 			int flags = XNARCH_SHARED_HEAP_FLAGS |
 				((desc_flags & O_DIRECT) ? GFP_DMA : 0);
 
-			err = -xnheap_init_mapped(&shm->heapbase, len, flags);
+			err = -xnheap_init_mapped(&shm->heapbase, len, flags,
+						  "posix shm: %s",
+						  shm->nodebase.name);
 			if (err)
 				goto err_up;
 
diff --git a/ksrc/skins/psos+/rn.c b/ksrc/skins/psos+/rn.c
index 98f4500..3eb3ab9 100644
--- a/ksrc/skins/psos+/rn.c
+++ b/ksrc/skins/psos+/rn.c
@@ -191,7 +191,8 @@ u_long rn_create(const char *name,
 
 		rnsize = xnheap_rounded_size(rnsize, PAGE_SIZE),
 		err = xnheap_init_mapped(&rn->heapbase, rnsize, 
-					 XNARCH_SHARED_HEAP_FLAGS);
+					 XNARCH_SHARED_HEAP_FLAGS,
+					 "psosrn: %s", name);
 
 		if (err)
 			return err;
@@ -206,7 +207,8 @@ u_long rn_create(const char *name,
 		 * Caller must have accounted for overhead and
 		 * alignment since it supplies the memory space.
 		 */
-		if (xnheap_init(&rn->heapbase, rnaddr, rnsize, XNHEAP_PAGE_SIZE) != 0)
+		if (xnheap_init(&rn->heapbase, rnaddr, rnsize,
+				XNHEAP_PAGE_SIZE, "psosrn: %s", name) != 0)
 			return ERR_TINYRN;
 
 	inith(&rn->link);
diff --git a/ksrc/skins/rtai/shm.c b/ksrc/skins/rtai/shm.c
index 21c3b07..0b33a57 100644
--- a/ksrc/skins/rtai/shm.c
+++ b/ksrc/skins/rtai/shm.c
@@ -151,7 +151,8 @@ static xnshm_a_t *create_new_heap(unsigned long name, int heapsize, int suprt)
 	err = xnheap_init_mapped(p->heap,
 				 heapsize,
 				 (suprt == USE_GFP_KERNEL ? GFP_KERNEL : 0)
-				 | XNARCH_SHARED_HEAP_FLAGS);
+				 | XNARCH_SHARED_HEAP_FLAGS,
+				 "rtai heap: 0x%lx", name);
 #else /* !CONFIG_XENO_OPT_PERVASIVE */
 	{
 		void *heapmem;
@@ -164,7 +165,9 @@ static xnshm_a_t *create_new_heap(unsigned long name, int heapsize, int suprt)
 			err = -ENOMEM;
 		} else {
 
-			err = xnheap_init(p->heap, heapmem, heapsize, XNHEAP_PAGE_SIZE);
+			err = xnheap_init(p->heap, heapmem, heapsize,
+					  XNHEAP_PAGE_SIZE,
+					  "rtai heap: 0x%lx", name);
 			if (err) {
 				xnarch_free_host_mem(heapmem, heapsize);
 			}
diff --git a/ksrc/skins/vrtx/heap.c b/ksrc/skins/vrtx/heap.c
index de60792..8f8a1d5 100644
--- a/ksrc/skins/vrtx/heap.c
+++ b/ksrc/skins/vrtx/heap.c
@@ -165,7 +165,8 @@ int sc_hcreate(char *heapaddr, u_long heapsize, unsigned log2psize, int *errp)
 #ifdef CONFIG_XENO_OPT_PERVASIVE
 		heapsize = xnheap_rounded_size(heapsize, PAGE_SIZE);
 		err = xnheap_init_mapped(&heap->sysheap, heapsize, 
-					 XNARCH_SHARED_HEAP_FLAGS);
+					 XNARCH_SHARED_HEAP_FLAGS,
+					 "vrtx sysheap");
 
 		if (err) {
 			*errp = ER_MEM;
@@ -184,7 +185,8 @@ int sc_hcreate(char *heapaddr, u_long heapsize, unsigned log2psize, int *errp)
 		 * Caller must have accounted for overhead and
 		 * alignment since it supplies the memory space.
 		 */
-		err = xnheap_init(&heap->sysheap, heapaddr, heapsize, pagesize);
+		err = xnheap_init(&heap->sysheap, heapaddr, heapsize, pagesize,
+				  "vrtx sysheap");
 
 		if (err) {
 			if (err == -EINVAL)
diff --git a/ksrc/skins/vrtx/syscall.c b/ksrc/skins/vrtx/syscall.c
index 1623066..e3d2e31 100644
--- a/ksrc/skins/vrtx/syscall.c
+++ b/ksrc/skins/vrtx/syscall.c
@@ -1052,7 +1052,8 @@ static int __sc_pcreate(struct pt_regs *regs)
 	/* Block size. */
 	bsize = __xn_reg_arg3(regs);
 
-	err = xnheap_init_mapped(ptheap, ptsize, XNARCH_SHARED_HEAP_FLAGS);
+	err = xnheap_init_mapped(ptheap, ptsize, XNARCH_SHARED_HEAP_FLAGS,
+				 "vrtx ptheap");
 
 	if (err)
 		goto free_heap;

Re: [Xenomai-core] [PATCH v3 9/9] nucleus: Include all heaps in statistics

[Philippe Gerum]

On Tue, 2009-10-20 at 13:37 +0200, Jan Kiszka wrote:
> This extends /proc/xenomai/heap with statistics about all currently used
> heaps. It takes care to flush nklock while iterating of this potentially
> long list.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
> ---
> 
>  include/nucleus/heap.h    |   12 +++-
>  ksrc/drivers/ipc/iddp.c   |    3 +
>  ksrc/drivers/ipc/xddp.c   |    6 ++
>  ksrc/nucleus/heap.c       |  131 ++++++++++++++++++++++++++++++++++++++++-----
>  ksrc/nucleus/module.c     |    2 -
>  ksrc/nucleus/pod.c        |    5 +-
>  ksrc/nucleus/shadow.c     |    5 +-
>  ksrc/skins/native/heap.c  |    6 +-
>  ksrc/skins/native/pipe.c  |    4 +
>  ksrc/skins/native/queue.c |    6 +-
>  ksrc/skins/posix/shm.c    |    4 +
>  ksrc/skins/psos+/rn.c     |    6 +-
>  ksrc/skins/rtai/shm.c     |    7 ++
>  ksrc/skins/vrtx/heap.c    |    6 +-
>  ksrc/skins/vrtx/syscall.c |    3 +
>  15 files changed, 169 insertions(+), 37 deletions(-)
> 
> diff --git a/include/nucleus/heap.h b/include/nucleus/heap.h
> index 44db738..f653cd7 100644
> --- a/include/nucleus/heap.h
> +++ b/include/nucleus/heap.h
> @@ -115,6 +115,10 @@ typedef struct xnheap {
>  
>  	XNARCH_DECL_DISPLAY_CONTEXT();
>  
> +	xnholder_t stat_link;	/* Link in heapq */
> +
> +	char name[48];

s,48,XNOBJECT_NAME_LEN

> +
>  } xnheap_t;
>  
>  extern xnheap_t kheap;
> @@ -202,7 +206,8 @@ void xnheap_cleanup_proc(void);
>  
>  int xnheap_init_mapped(xnheap_t *heap,
>  		       u_long heapsize,
> -		       int memflags);
> +		       int memflags,
> +		       const char *name, ...);
>  

The va_list is handy, but this breaks the common pattern used throughout
the rest of the nucleus, based on passing pre-formatted labels. So
either we make all creation calls use va_lists (but xnthread would need
more work), or we make xnheap_init_mapped use the not-so-handy current
form.

Actually, providing xnheap_set_name() and a name parameter/va_list to
xnheap_init* is one too many, this clutters an inner interface
uselessly. The latter should go away, assuming that anon heaps may still
exist.

>  void xnheap_destroy_mapped(xnheap_t *heap,
>  			   void (*release)(struct xnheap *heap),
> @@ -224,7 +229,10 @@ void xnheap_destroy_mapped(xnheap_t *heap,
>  int xnheap_init(xnheap_t *heap,
>  		void *heapaddr,
>  		u_long heapsize,
> -		u_long pagesize);
> +		u_long pagesize,
> +		const char *name, ...);
> +
> +void xnheap_set_name(xnheap_t *heap, const char *name, ...);
>  
>  void xnheap_destroy(xnheap_t *heap,
>  		    void (*flushfn)(xnheap_t *heap,
> diff --git a/ksrc/drivers/ipc/iddp.c b/ksrc/drivers/ipc/iddp.c
> index a407946..b6382f1 100644
> --- a/ksrc/drivers/ipc/iddp.c
> +++ b/ksrc/drivers/ipc/iddp.c
> @@ -559,7 +559,8 @@ static int __iddp_bind_socket(struct rtipc_private *priv,
>  		}
>  
>  		ret = xnheap_init(&sk->privpool,
> -				  poolmem, poolsz, XNHEAP_PAGE_SIZE);
> +				  poolmem, poolsz, XNHEAP_PAGE_SIZE,
> +				  "ippd: %d", port);
>  		if (ret) {
>  			xnarch_free_host_mem(poolmem, poolsz);
>  			goto fail;
> diff --git a/ksrc/drivers/ipc/xddp.c b/ksrc/drivers/ipc/xddp.c
> index f62147a..a5dafef 100644
> --- a/ksrc/drivers/ipc/xddp.c
> +++ b/ksrc/drivers/ipc/xddp.c
> @@ -703,7 +703,7 @@ static int __xddp_bind_socket(struct rtipc_private *priv,
>  		}
>  
>  		ret = xnheap_init(&sk->privpool,
> -				  poolmem, poolsz, XNHEAP_PAGE_SIZE);
> +				  poolmem, poolsz, XNHEAP_PAGE_SIZE, "");
>  		if (ret) {
>  			xnarch_free_host_mem(poolmem, poolsz);
>  			goto fail;
> @@ -746,6 +746,10 @@ static int __xddp_bind_socket(struct rtipc_private *priv,
>  	sk->minor = ret;
>  	sa->sipc_port = ret;
>  	sk->name = *sa;
> +
> +	if (poolsz > 0)
> +		xnheap_set_name(sk->bufpool, "xddp: %d", sa->sipc_port);
> +
>  	/* Set default destination if unset at binding time. */
>  	if (sk->peer.sipc_port < 0)
>  		sk->peer = *sa;
> diff --git a/ksrc/nucleus/heap.c b/ksrc/nucleus/heap.c
> index 96c46f8..793d1c5 100644
> --- a/ksrc/nucleus/heap.c
> +++ b/ksrc/nucleus/heap.c
> @@ -76,6 +76,9 @@ EXPORT_SYMBOL_GPL(kheap);
>  xnheap_t kstacks;	/* Private stack pool */
>  #endif
>  
> +static DEFINE_XNQUEUE(heapq);	/* Heap list for /proc reporting */
> +static unsigned long heapq_rev;
> +
>  static void init_extent(xnheap_t *heap, xnextent_t *extent)
>  {
>  	caddr_t freepage;
> @@ -108,7 +111,7 @@ static void init_extent(xnheap_t *heap, xnextent_t *extent)
>   */
>  
>  /*!
> - * \fn xnheap_init(xnheap_t *heap,void *heapaddr,u_long heapsize,u_long pagesize)
> + * \fn xnheap_init(xnheap_t *heap,void *heapaddr,u_long heapsize,u_long pagesize,const char *name,...)
>   * \brief Initialize a memory heap.
>   *
>   * Initializes a memory heap suitable for time-bounded allocation
> @@ -145,6 +148,10 @@ static void init_extent(xnheap_t *heap, xnextent_t *extent)
>   * best one for your needs. In the current implementation, pagesize
>   * must be a power of two in the range [ 8 .. 32768 ] inclusive.
>   *
> + * @param name Name displayed in statistic outputs. This parameter can
> + * be a format string, in which case succeeding parameters will be used
> + * to resolve the final name.
> + *
>   * @return 0 is returned upon success, or one of the following error
>   * codes:
>   *
> @@ -161,12 +168,13 @@ static void init_extent(xnheap_t *heap, xnextent_t *extent)
>   * Rescheduling: never.
>   */
>  
> -int xnheap_init(xnheap_t *heap,
> -		void *heapaddr, u_long heapsize, u_long pagesize)
> +static int xnheap_init_va(xnheap_t *heap, void *heapaddr, u_long heapsize,
> +			  u_long pagesize, const char *name, va_list args)
>  {
>  	unsigned cpu, nr_cpus = xnarch_num_online_cpus();
>  	u_long hdrsize, shiftsize, pageshift;
>  	xnextent_t *extent;
> +	spl_t s;
>  
>  	/*
>  	 * Perform some parametrical checks first.
> @@ -232,12 +240,71 @@ int xnheap_init(xnheap_t *heap,
>  
>  	appendq(&heap->extents, &extent->link);
>  
> +	vsnprintf(heap->name, sizeof(heap->name), name, args);
> +
> +	xnlock_get_irqsave(&nklock, s);
> +	appendq(&heapq, &heap->stat_link);
> +	heapq_rev++;
> +	xnlock_put_irqrestore(&nklock, s);
> +
>  	xnarch_init_display_context(heap);
>  
>  	return 0;
>  }
> +
> +int xnheap_init(xnheap_t *heap, void *heapaddr, u_long heapsize,
> +		u_long pagesize, const char *name, ...)
> +{
> +	va_list args;
> +	int ret;
> +
> +	va_start(args, name);
> +	ret = xnheap_init_va(heap, heapaddr, heapsize, pagesize, name, args);
> +	va_end(args);
> +
> +	return ret;
> +}
>  EXPORT_SYMBOL_GPL(xnheap_init);
>  
> +/*!
> + * \fn xnheap_set_name(xnheap_t *heap,const char *name,...)
> + * \brief Overwrite the heap's name.

At some point, there should be a common base struct containing
everything needed to manipulate named objects, we would include in
higher level containers. It seems we are over-specializing quite generic
work.

> + *
> + * Set the heap name that will be used in statistic outputs. This service
> + * is useful if the final name is not yet defined on xnheap_init().
> + *
> + * @param heap The address of a heap descriptor.
> + *
> + * @param name Name displayed in statistic outputs. This parameter can
> + * be a format string, in which case succeeding parameters will be used
> + * to resolve the final name.
> + *
> + * Environments:
> + *
> + * This service can be called from:
> + *
> + * - Kernel module initialization/cleanup code
> + * - Kernel-based task
> + * - User-space task
> + *
> + * Rescheduling: never.
> + */
> +
> +void xnheap_set_name(xnheap_t *heap, const char *name, ...)
> +{
> +	va_list args;
> +	spl_t s;
> +
> +	va_start(args, name);
> +
> +	xnlock_get_irqsave(&nklock, s);
> +	vsnprintf(heap->name, sizeof(heap->name), name, args);
> +	xnlock_put_irqrestore(&nklock, s);
> +
> +	va_end(args);
> +}
> +EXPORT_SYMBOL_GPL(xnheap_set_name);
> +
>  /*! 
>   * \fn void xnheap_destroy(xnheap_t *heap, void (*flushfn)(xnheap_t *heap, void *extaddr, u_long extsize, void *cookie), void *cookie)
>   * \brief Destroys a memory heap.
> @@ -273,6 +340,11 @@ void xnheap_destroy(xnheap_t *heap,
>  	xnholder_t *holder;
>  	spl_t s;
>  
> +	xnlock_get_irqsave(&nklock, s);
> +	removeq(&heapq, &heap->stat_link);
> +	heapq_rev++;
> +	xnlock_put_irqrestore(&nklock, s);
> +
>  	if (!flushfn)
>  		return;
>  
> @@ -1140,9 +1212,11 @@ deref_out:
>  	return err;
>  }
>  
> -int xnheap_init_mapped(xnheap_t *heap, u_long heapsize, int memflags)
> +int xnheap_init_mapped(xnheap_t *heap, u_long heapsize, int memflags,
> +		       const char *name, ...)
>  {
>  	void *heapbase;
> +	va_list args;
>  	int err;
>  
>  	/* Caller must have accounted for internal overhead. */
> @@ -1156,7 +1230,9 @@ int xnheap_init_mapped(xnheap_t *heap, u_long heapsize, int memflags)
>  	if (!heapbase)
>  		return -ENOMEM;
>  
> -	err = xnheap_init(heap, heapbase, heapsize, PAGE_SIZE);
> +	va_start(args, name);
> +	err = xnheap_init_va(heap, heapbase, heapsize, PAGE_SIZE, name, args);
> +	va_end(args);
>  	if (err) {
>  		__unreserve_and_free_heap(heapbase, heapsize, memflags);
>  		return err;
> @@ -1178,6 +1254,7 @@ void xnheap_destroy_mapped(xnheap_t *heap,
>  			   void __user *mapaddr)
>  {
>  	unsigned long len;
> +	spl_t s;
>  
>  	/*
>  	 * Trying to unmap user memory without providing a release handler for
> @@ -1185,6 +1262,11 @@ void xnheap_destroy_mapped(xnheap_t *heap,
>  	 */
>  	XENO_ASSERT(NUCLEUS, !mapaddr || release, /* nop */);
>  
> +	xnlock_get_irqsave(&nklock, s);
> +	removeq(&heapq, &heap->stat_link);
> +	heapq_rev++;
> +	xnlock_put_irqrestore(&nklock, s);
> +
>  	spin_lock(&kheapq_lock);
>  
>  	removeq(&kheapq, &heap->link); /* Prevent further mapping. */
> @@ -1285,22 +1367,41 @@ static int heap_read_proc(char *page,
>  			  char **start,
>  			  off_t off, int count, int *eof, void *data)
>  {
> +	unsigned long rev;
> +	xnholder_t *entry;
> +	xnheap_t *heap;
>  	int len;
> +	spl_t s;
>  
>  	if (!xnpod_active_p())
>  		return -ESRCH;
>  
> -	len = sprintf(page, "size=%lu:used=%lu:pagesz=%lu  (main heap)\n",
> -		      xnheap_usable_mem(&kheap),
> -		      xnheap_used_mem(&kheap),
> -		      xnheap_page_size(&kheap));
> +	xnlock_get_irqsave(&nklock, s);
>  
> -#if CONFIG_XENO_OPT_SYS_STACKPOOLSZ > 0
> -	len += sprintf(page + len, "size=%lu:used=%lu:pagesz=%lu  (stack pool)\n",
> -		       xnheap_usable_mem(&kstacks),
> -		       xnheap_used_mem(&kstacks),
> -		       xnheap_page_size(&kstacks));
> -#endif
> +restart:
> +	len = 0;
> +
> +	entry = getheadq(&heapq);
> +	while (entry) {
> +		heap = container_of(entry, xnheap_t, stat_link);
> +		len += sprintf(page + len,
> +			       "size=%lu:used=%lu:pagesz=%lu  (%s)\n",
> +			       xnheap_usable_mem(heap),
> +			       xnheap_used_mem(heap),
> +			       xnheap_page_size(heap),
> +			       heap->name);
> +
> +		rev = heapq_rev;
> +
> +		xnlock_sync_irq(&nklock, s);
> +
> +		if (heapq_rev != rev)
> +			goto restart;
> +
> +		entry = nextq(&heapq, entry);
> +	}
> +
> +	xnlock_put_irqrestore(&nklock, s);
>  
>  	len -= off;
>  	if (len <= off + count)
> diff --git a/ksrc/nucleus/module.c b/ksrc/nucleus/module.c
> index 141276a..a0efc14 100644
> --- a/ksrc/nucleus/module.c
> +++ b/ksrc/nucleus/module.c
> @@ -100,7 +100,7 @@ int __init __xeno_sys_init(void)
>  #ifndef __XENO_SIM__
>  	ret = xnheap_init_mapped(&__xnsys_global_ppd.sem_heap,
>  				 CONFIG_XENO_OPT_GLOBAL_SEM_HEAPSZ * 1024,
> -				 XNARCH_SHARED_HEAP_FLAGS);
> +				 XNARCH_SHARED_HEAP_FLAGS, "global sem heap");
>  	if (ret)
>  		goto cleanup_arch;
>  #endif
> diff --git a/ksrc/nucleus/pod.c b/ksrc/nucleus/pod.c
> index 370ad28..07fbcdb 100644
> --- a/ksrc/nucleus/pod.c
> +++ b/ksrc/nucleus/pod.c
> @@ -384,9 +384,8 @@ int xnpod_init(void)
>  	heapaddr = xnarch_alloc_host_mem(xnmod_sysheap_size);
>  	if (heapaddr == NULL ||
>  	    xnheap_init(&kheap, heapaddr, xnmod_sysheap_size,
> -			XNHEAP_PAGE_SIZE) != 0) {
> +			XNHEAP_PAGE_SIZE, "main heap") != 0)
>  		return -ENOMEM;
> -	}
>  
>  #if CONFIG_XENO_OPT_SYS_STACKPOOLSZ > 0
>  	/*
> @@ -404,7 +403,7 @@ int xnpod_init(void)
>  	heapaddr = xnarch_alloc_stack_mem(CONFIG_XENO_OPT_SYS_STACKPOOLSZ * 1024);
>  	if (heapaddr == NULL ||
>  	    xnheap_init(&kstacks, heapaddr, CONFIG_XENO_OPT_SYS_STACKPOOLSZ * 1024,
> -			XNHEAP_PAGE_SIZE) != 0) {
> +			XNHEAP_PAGE_SIZE, "stack pool") != 0) {
>  		xnheap_destroy(&kheap, &xnpod_flush_heap, NULL);
>  		return -ENOMEM;
>  	}
> diff --git a/ksrc/nucleus/shadow.c b/ksrc/nucleus/shadow.c
> index d6d1203..e8876d8 100644
> --- a/ksrc/nucleus/shadow.c
> +++ b/ksrc/nucleus/shadow.c
> @@ -1886,7 +1886,9 @@ static void *xnshadow_sys_event(int event, void *data)
>  
>  		err = xnheap_init_mapped(&p->sem_heap,
>  					 CONFIG_XENO_OPT_SEM_HEAPSZ * 1024,
> -					 XNARCH_SHARED_HEAP_FLAGS);
> +					 XNARCH_SHARED_HEAP_FLAGS,
> +					 "private sem heap [%d]",
> +					 current->pid);
>  		if (err) {
>  			xnarch_free_host_mem(p, sizeof(*p));
>  			return ERR_PTR(err);
> @@ -1896,6 +1898,7 @@ static void *xnshadow_sys_event(int event, void *data)
>  
>  	case XNSHADOW_CLIENT_DETACH:
>  		p = ppd2sys((xnshadow_ppd_t *) data);
> +
>  		xnheap_destroy_mapped(&p->sem_heap, post_ppd_release, NULL);
>  
>  		return NULL;
> diff --git a/ksrc/skins/native/heap.c b/ksrc/skins/native/heap.c
> index 886758c..3072f99 100644
> --- a/ksrc/skins/native/heap.c
> +++ b/ksrc/skins/native/heap.c
> @@ -266,7 +266,8 @@ int rt_heap_create(RT_HEAP *heap, const char *name, size_t heapsize, int mode)
>  					 heapsize,
>  					 ((mode & H_DMA) ? GFP_DMA : 0)
>  					 | ((mode & H_NONCACHED) ?
> -					    XNHEAP_GFP_NONCACHED : 0));
> +					    XNHEAP_GFP_NONCACHED : 0),
> +					 "rt_heap: %s", name);
>  		if (err)
>  			return err;
>  
> @@ -286,7 +287,8 @@ int rt_heap_create(RT_HEAP *heap, const char *name, size_t heapsize, int mode)
>  		if (!heapmem)
>  			return -ENOMEM;
>  
> -		err = xnheap_init(&heap->heap_base, heapmem, heapsize, XNHEAP_PAGE_SIZE);
> +		err = xnheap_init(&heap->heap_base, heapmem, heapsize,
> +				  XNHEAP_PAGE_SIZE, "rt_heap: %s", name);
>  		if (err) {
>  			xnarch_free_host_mem(heapmem, heapsize);
>  			return err;
> diff --git a/ksrc/skins/native/pipe.c b/ksrc/skins/native/pipe.c
> index 672fa7a..18d9aa0 100644
> --- a/ksrc/skins/native/pipe.c
> +++ b/ksrc/skins/native/pipe.c
> @@ -295,7 +295,9 @@ int rt_pipe_create(RT_PIPE *pipe, const char *name, int minor, size_t poolsize)
>  			return -ENOMEM;
>  
>  		/* Use natural page size */
> -		err = xnheap_init(&pipe->privpool, poolmem, poolsize, XNHEAP_PAGE_SIZE);
> +		err = xnheap_init(&pipe->privpool, poolmem, poolsize,
> +				  XNHEAP_PAGE_SIZE,
> +				  "rt_pipe: %d / %s", minor, name);
>  		if (err) {
>  			xnarch_free_host_mem(poolmem, poolsize);
>  			return err;
> diff --git a/ksrc/skins/native/queue.c b/ksrc/skins/native/queue.c
> index 249947a..becc0f0 100644
> --- a/ksrc/skins/native/queue.c
> +++ b/ksrc/skins/native/queue.c
> @@ -229,7 +229,8 @@ int rt_queue_create(RT_QUEUE *q,
>  		err = xnheap_init_mapped(&q->bufpool,
>  					 poolsize,
>  					 ((mode & Q_DMA) ? GFP_DMA 
> -					  : XNARCH_SHARED_HEAP_FLAGS));
> +					  : XNARCH_SHARED_HEAP_FLAGS),
> +					 "rt_queue: %s", name);
>  		if (err)
>  			return err;
>  
> @@ -249,7 +250,8 @@ int rt_queue_create(RT_QUEUE *q,
>  		if (!poolmem)
>  			return -ENOMEM;
>  
> -		err = xnheap_init(&q->bufpool, poolmem, poolsize, XNHEAP_PAGE_SIZE);
> +		err = xnheap_init(&q->bufpool, poolmem, poolsize,
> +				  XNHEAP_PAGE_SIZE, "rt_queue: %s", name);
>  		if (err) {
>  			xnarch_free_host_mem(poolmem, poolsize);
>  			return err;
> diff --git a/ksrc/skins/posix/shm.c b/ksrc/skins/posix/shm.c
> index c92096a..0f0bd1a 100644
> --- a/ksrc/skins/posix/shm.c
> +++ b/ksrc/skins/posix/shm.c
> @@ -539,7 +539,9 @@ int ftruncate(int fd, off_t len)
>  			int flags = XNARCH_SHARED_HEAP_FLAGS |
>  				((desc_flags & O_DIRECT) ? GFP_DMA : 0);
>  
> -			err = -xnheap_init_mapped(&shm->heapbase, len, flags);
> +			err = -xnheap_init_mapped(&shm->heapbase, len, flags,
> +						  "posix shm: %s",
> +						  shm->nodebase.name);
>  			if (err)
>  				goto err_up;
>  
> diff --git a/ksrc/skins/psos+/rn.c b/ksrc/skins/psos+/rn.c
> index 98f4500..3eb3ab9 100644
> --- a/ksrc/skins/psos+/rn.c
> +++ b/ksrc/skins/psos+/rn.c
> @@ -191,7 +191,8 @@ u_long rn_create(const char *name,
>  
>  		rnsize = xnheap_rounded_size(rnsize, PAGE_SIZE),
>  		err = xnheap_init_mapped(&rn->heapbase, rnsize, 
> -					 XNARCH_SHARED_HEAP_FLAGS);
> +					 XNARCH_SHARED_HEAP_FLAGS,
> +					 "psosrn: %s", name);
>  
>  		if (err)
>  			return err;
> @@ -206,7 +207,8 @@ u_long rn_create(const char *name,
>  		 * Caller must have accounted for overhead and
>  		 * alignment since it supplies the memory space.
>  		 */
> -		if (xnheap_init(&rn->heapbase, rnaddr, rnsize, XNHEAP_PAGE_SIZE) != 0)
> +		if (xnheap_init(&rn->heapbase, rnaddr, rnsize,
> +				XNHEAP_PAGE_SIZE, "psosrn: %s", name) != 0)
>  			return ERR_TINYRN;
>  
>  	inith(&rn->link);
> diff --git a/ksrc/skins/rtai/shm.c b/ksrc/skins/rtai/shm.c
> index 21c3b07..0b33a57 100644
> --- a/ksrc/skins/rtai/shm.c
> +++ b/ksrc/skins/rtai/shm.c
> @@ -151,7 +151,8 @@ static xnshm_a_t *create_new_heap(unsigned long name, int heapsize, int suprt)
>  	err = xnheap_init_mapped(p->heap,
>  				 heapsize,
>  				 (suprt == USE_GFP_KERNEL ? GFP_KERNEL : 0)
> -				 | XNARCH_SHARED_HEAP_FLAGS);
> +				 | XNARCH_SHARED_HEAP_FLAGS,
> +				 "rtai heap: 0x%lx", name);
>  #else /* !CONFIG_XENO_OPT_PERVASIVE */
>  	{
>  		void *heapmem;
> @@ -164,7 +165,9 @@ static xnshm_a_t *create_new_heap(unsigned long name, int heapsize, int suprt)
>  			err = -ENOMEM;
>  		} else {
>  
> -			err = xnheap_init(p->heap, heapmem, heapsize, XNHEAP_PAGE_SIZE);
> +			err = xnheap_init(p->heap, heapmem, heapsize,
> +					  XNHEAP_PAGE_SIZE,
> +					  "rtai heap: 0x%lx", name);
>  			if (err) {
>  				xnarch_free_host_mem(heapmem, heapsize);
>  			}
> diff --git a/ksrc/skins/vrtx/heap.c b/ksrc/skins/vrtx/heap.c
> index de60792..8f8a1d5 100644
> --- a/ksrc/skins/vrtx/heap.c
> +++ b/ksrc/skins/vrtx/heap.c
> @@ -165,7 +165,8 @@ int sc_hcreate(char *heapaddr, u_long heapsize, unsigned log2psize, int *errp)
>  #ifdef CONFIG_XENO_OPT_PERVASIVE
>  		heapsize = xnheap_rounded_size(heapsize, PAGE_SIZE);
>  		err = xnheap_init_mapped(&heap->sysheap, heapsize, 
> -					 XNARCH_SHARED_HEAP_FLAGS);
> +					 XNARCH_SHARED_HEAP_FLAGS,
> +					 "vrtx sysheap");
>  
>  		if (err) {
>  			*errp = ER_MEM;
> @@ -184,7 +185,8 @@ int sc_hcreate(char *heapaddr, u_long heapsize, unsigned log2psize, int *errp)
>  		 * Caller must have accounted for overhead and
>  		 * alignment since it supplies the memory space.
>  		 */
> -		err = xnheap_init(&heap->sysheap, heapaddr, heapsize, pagesize);
> +		err = xnheap_init(&heap->sysheap, heapaddr, heapsize, pagesize,
> +				  "vrtx sysheap");
>  
>  		if (err) {
>  			if (err == -EINVAL)
> diff --git a/ksrc/skins/vrtx/syscall.c b/ksrc/skins/vrtx/syscall.c
> index 1623066..e3d2e31 100644
> --- a/ksrc/skins/vrtx/syscall.c
> +++ b/ksrc/skins/vrtx/syscall.c
> @@ -1052,7 +1052,8 @@ static int __sc_pcreate(struct pt_regs *regs)
>  	/* Block size. */
>  	bsize = __xn_reg_arg3(regs);
>  
> -	err = xnheap_init_mapped(ptheap, ptsize, XNARCH_SHARED_HEAP_FLAGS);
> +	err = xnheap_init_mapped(ptheap, ptsize, XNARCH_SHARED_HEAP_FLAGS,
> +				 "vrtx ptheap");
>  
>  	if (err)
>  		goto free_heap;
> 
-- 
Philippe.

Re: [Xenomai-core] [PATCH v3 9/9] nucleus: Include all heaps in statistics

[Jan Kiszka]

Philippe Gerum wrote:
> On Tue, 2009-10-20 at 13:37 +0200, Jan Kiszka wrote:
>> This extends /proc/xenomai/heap with statistics about all currently used
>> heaps. It takes care to flush nklock while iterating of this potentially
>> long list.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>> ---
>>
>>  include/nucleus/heap.h    |   12 +++-
>>  ksrc/drivers/ipc/iddp.c   |    3 +
>>  ksrc/drivers/ipc/xddp.c   |    6 ++
>>  ksrc/nucleus/heap.c       |  131 ++++++++++++++++++++++++++++++++++++++++-----
>>  ksrc/nucleus/module.c     |    2 -
>>  ksrc/nucleus/pod.c        |    5 +-
>>  ksrc/nucleus/shadow.c     |    5 +-
>>  ksrc/skins/native/heap.c  |    6 +-
>>  ksrc/skins/native/pipe.c  |    4 +
>>  ksrc/skins/native/queue.c |    6 +-
>>  ksrc/skins/posix/shm.c    |    4 +
>>  ksrc/skins/psos+/rn.c     |    6 +-
>>  ksrc/skins/rtai/shm.c     |    7 ++
>>  ksrc/skins/vrtx/heap.c    |    6 +-
>>  ksrc/skins/vrtx/syscall.c |    3 +
>>  15 files changed, 169 insertions(+), 37 deletions(-)
>>
>> diff --git a/include/nucleus/heap.h b/include/nucleus/heap.h
>> index 44db738..f653cd7 100644
>> --- a/include/nucleus/heap.h
>> +++ b/include/nucleus/heap.h
>> @@ -115,6 +115,10 @@ typedef struct xnheap {
>>
>>       XNARCH_DECL_DISPLAY_CONTEXT();
>>
>> +     xnholder_t stat_link;   /* Link in heapq */
>> +
>> +     char name[48];
> 
> s,48,XNOBJECT_NAME_LEN

OK, but XNOBJECT_NAME_LEN+16 (due to class prefixes and additional
information like the minor ID).

> 
>> +
>>  } xnheap_t;
>>
>>  extern xnheap_t kheap;
>> @@ -202,7 +206,8 @@ void xnheap_cleanup_proc(void);
>>
>>  int xnheap_init_mapped(xnheap_t *heap,
>>                      u_long heapsize,
>> -                    int memflags);
>> +                    int memflags,
>> +                    const char *name, ...);
>>
> 
> The va_list is handy, but this breaks the common pattern used throughout
> the rest of the nucleus, based on passing pre-formatted labels. So
> either we make all creation calls use va_lists (but xnthread would need
> more work), or we make xnheap_init_mapped use the not-so-handy current
> form.
> 
> Actually, providing xnheap_set_name() and a name parameter/va_list to
> xnheap_init* is one too many, this clutters an inner interface
> uselessly. The latter should go away, assuming that anon heaps may still
> exist.

If we want to print all heaps, we should at least set a name indicating
their class. And therefore we need to pass a descriptive name along with
/every/ heap initialization. Forcing the majority of xnheap_init users
to additionally issue xnheap_set_name is the cluttering a wanted to
avoid. Only a minority needs this split-up, and therefore you see both
interfaces in my patch.

> 
>>  void xnheap_destroy_mapped(xnheap_t *heap,
>>                          void (*release)(struct xnheap *heap),
>> @@ -224,7 +229,10 @@ void xnheap_destroy_mapped(xnheap_t *heap,
>>  int xnheap_init(xnheap_t *heap,
>>               void *heapaddr,
>>               u_long heapsize,
>> -             u_long pagesize);
>> +             u_long pagesize,
>> +             const char *name, ...);
>> +
>> +void xnheap_set_name(xnheap_t *heap, const char *name, ...);
>>
>>  void xnheap_destroy(xnheap_t *heap,
>>                   void (*flushfn)(xnheap_t *heap,
>> diff --git a/ksrc/drivers/ipc/iddp.c b/ksrc/drivers/ipc/iddp.c
>> index a407946..b6382f1 100644
>> --- a/ksrc/drivers/ipc/iddp.c
>> +++ b/ksrc/drivers/ipc/iddp.c
>> @@ -559,7 +559,8 @@ static int __iddp_bind_socket(struct rtipc_private *priv,
>>               }
>>
>>               ret = xnheap_init(&sk->privpool,
>> -                               poolmem, poolsz, XNHEAP_PAGE_SIZE);
>> +                               poolmem, poolsz, XNHEAP_PAGE_SIZE,
>> +                               "ippd: %d", port);
>>               if (ret) {
>>                       xnarch_free_host_mem(poolmem, poolsz);
>>                       goto fail;
>> diff --git a/ksrc/drivers/ipc/xddp.c b/ksrc/drivers/ipc/xddp.c
>> index f62147a..a5dafef 100644
>> --- a/ksrc/drivers/ipc/xddp.c
>> +++ b/ksrc/drivers/ipc/xddp.c
>> @@ -703,7 +703,7 @@ static int __xddp_bind_socket(struct rtipc_private *priv,
>>               }
>>
>>               ret = xnheap_init(&sk->privpool,
>> -                               poolmem, poolsz, XNHEAP_PAGE_SIZE);
>> +                               poolmem, poolsz, XNHEAP_PAGE_SIZE, "");
>>               if (ret) {
>>                       xnarch_free_host_mem(poolmem, poolsz);
>>                       goto fail;
>> @@ -746,6 +746,10 @@ static int __xddp_bind_socket(struct rtipc_private *priv,
>>       sk->minor = ret;
>>       sa->sipc_port = ret;
>>       sk->name = *sa;
>> +
>> +     if (poolsz > 0)
>> +             xnheap_set_name(sk->bufpool, "xddp: %d", sa->sipc_port);
>> +
>>       /* Set default destination if unset at binding time. */
>>       if (sk->peer.sipc_port < 0)
>>               sk->peer = *sa;
>> diff --git a/ksrc/nucleus/heap.c b/ksrc/nucleus/heap.c
>> index 96c46f8..793d1c5 100644
>> --- a/ksrc/nucleus/heap.c
>> +++ b/ksrc/nucleus/heap.c
>> @@ -76,6 +76,9 @@ EXPORT_SYMBOL_GPL(kheap);
>>  xnheap_t kstacks;    /* Private stack pool */
>>  #endif
>>
>> +static DEFINE_XNQUEUE(heapq);        /* Heap list for /proc reporting */
>> +static unsigned long heapq_rev;
>> +
>>  static void init_extent(xnheap_t *heap, xnextent_t *extent)
>>  {
>>       caddr_t freepage;
>> @@ -108,7 +111,7 @@ static void init_extent(xnheap_t *heap, xnextent_t *extent)
>>   */
>>
>>  /*!
>> - * \fn xnheap_init(xnheap_t *heap,void *heapaddr,u_long heapsize,u_long pagesize)
>> + * \fn xnheap_init(xnheap_t *heap,void *heapaddr,u_long heapsize,u_long pagesize,const char *name,...)
>>   * \brief Initialize a memory heap.
>>   *
>>   * Initializes a memory heap suitable for time-bounded allocation
>> @@ -145,6 +148,10 @@ static void init_extent(xnheap_t *heap, xnextent_t *extent)
>>   * best one for your needs. In the current implementation, pagesize
>>   * must be a power of two in the range [ 8 .. 32768 ] inclusive.
>>   *
>> + * @param name Name displayed in statistic outputs. This parameter can
>> + * be a format string, in which case succeeding parameters will be used
>> + * to resolve the final name.
>> + *
>>   * @return 0 is returned upon success, or one of the following error
>>   * codes:
>>   *
>> @@ -161,12 +168,13 @@ static void init_extent(xnheap_t *heap, xnextent_t *extent)
>>   * Rescheduling: never.
>>   */
>>
>> -int xnheap_init(xnheap_t *heap,
>> -             void *heapaddr, u_long heapsize, u_long pagesize)
>> +static int xnheap_init_va(xnheap_t *heap, void *heapaddr, u_long heapsize,
>> +                       u_long pagesize, const char *name, va_list args)
>>  {
>>       unsigned cpu, nr_cpus = xnarch_num_online_cpus();
>>       u_long hdrsize, shiftsize, pageshift;
>>       xnextent_t *extent;
>> +     spl_t s;
>>
>>       /*
>>        * Perform some parametrical checks first.
>> @@ -232,12 +240,71 @@ int xnheap_init(xnheap_t *heap,
>>
>>       appendq(&heap->extents, &extent->link);
>>
>> +     vsnprintf(heap->name, sizeof(heap->name), name, args);
>> +
>> +     xnlock_get_irqsave(&nklock, s);
>> +     appendq(&heapq, &heap->stat_link);
>> +     heapq_rev++;
>> +     xnlock_put_irqrestore(&nklock, s);
>> +
>>       xnarch_init_display_context(heap);
>>
>>       return 0;
>>  }
>> +
>> +int xnheap_init(xnheap_t *heap, void *heapaddr, u_long heapsize,
>> +             u_long pagesize, const char *name, ...)
>> +{
>> +     va_list args;
>> +     int ret;
>> +
>> +     va_start(args, name);
>> +     ret = xnheap_init_va(heap, heapaddr, heapsize, pagesize, name, args);
>> +     va_end(args);
>> +
>> +     return ret;
>> +}
>>  EXPORT_SYMBOL_GPL(xnheap_init);
>>
>> +/*!
>> + * \fn xnheap_set_name(xnheap_t *heap,const char *name,...)
>> + * \brief Overwrite the heap's name.
> 
> At some point, there should be a common base struct containing
> everything needed to manipulate named objects, we would include in
> higher level containers. It seems we are over-specializing quite generic
> work.

Probably right (though this case remains special to some degree because
names may consist of numeric IDs that are formatted on visualization). I
think I should rename 'name' to 'display_name' or so to clarify that we
are not dealing with a generic object name here.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [PATCH v3 9/9] nucleus: Include all heaps in statistics

[Jan Kiszka]

Jan Kiszka wrote:
> Philippe Gerum wrote:
>> On Tue, 2009-10-20 at 13:37 +0200, Jan Kiszka wrote:
>>> This extends /proc/xenomai/heap with statistics about all currently used
>>> heaps. It takes care to flush nklock while iterating of this potentially
>>> long list.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>> ---
>>>
>>>  include/nucleus/heap.h    |   12 +++-
>>>  ksrc/drivers/ipc/iddp.c   |    3 +
>>>  ksrc/drivers/ipc/xddp.c   |    6 ++
>>>  ksrc/nucleus/heap.c       |  131 ++++++++++++++++++++++++++++++++++++++++-----
>>>  ksrc/nucleus/module.c     |    2 -
>>>  ksrc/nucleus/pod.c        |    5 +-
>>>  ksrc/nucleus/shadow.c     |    5 +-
>>>  ksrc/skins/native/heap.c  |    6 +-
>>>  ksrc/skins/native/pipe.c  |    4 +
>>>  ksrc/skins/native/queue.c |    6 +-
>>>  ksrc/skins/posix/shm.c    |    4 +
>>>  ksrc/skins/psos+/rn.c     |    6 +-
>>>  ksrc/skins/rtai/shm.c     |    7 ++
>>>  ksrc/skins/vrtx/heap.c    |    6 +-
>>>  ksrc/skins/vrtx/syscall.c |    3 +
>>>  15 files changed, 169 insertions(+), 37 deletions(-)
>>>
>>> diff --git a/include/nucleus/heap.h b/include/nucleus/heap.h
>>> index 44db738..f653cd7 100644
>>> --- a/include/nucleus/heap.h
>>> +++ b/include/nucleus/heap.h
>>> @@ -115,6 +115,10 @@ typedef struct xnheap {
>>>
>>>       XNARCH_DECL_DISPLAY_CONTEXT();
>>>
>>> +     xnholder_t stat_link;   /* Link in heapq */
>>> +
>>> +     char name[48];
>> s,48,XNOBJECT_NAME_LEN
> 
> OK, but XNOBJECT_NAME_LEN+16 (due to class prefixes and additional
> information like the minor ID).
> 
>>> +
>>>  } xnheap_t;
>>>
>>>  extern xnheap_t kheap;
>>> @@ -202,7 +206,8 @@ void xnheap_cleanup_proc(void);
>>>
>>>  int xnheap_init_mapped(xnheap_t *heap,
>>>                      u_long heapsize,
>>> -                    int memflags);
>>> +                    int memflags,
>>> +                    const char *name, ...);
>>>
>> The va_list is handy, but this breaks the common pattern used throughout
>> the rest of the nucleus, based on passing pre-formatted labels. So
>> either we make all creation calls use va_lists (but xnthread would need
>> more work), or we make xnheap_init_mapped use the not-so-handy current
>> form.
>>
>> Actually, providing xnheap_set_name() and a name parameter/va_list to
>> xnheap_init* is one too many, this clutters an inner interface
>> uselessly. The latter should go away, assuming that anon heaps may still
>> exist.
> 
> If we want to print all heaps, we should at least set a name indicating
> their class. And therefore we need to pass a descriptive name along with
> /every/ heap initialization. Forcing the majority of xnheap_init users
> to additionally issue xnheap_set_name is the cluttering a wanted to
> avoid. Only a minority needs this split-up, and therefore you see both
> interfaces in my patch.
> 
>>>  void xnheap_destroy_mapped(xnheap_t *heap,
>>>                          void (*release)(struct xnheap *heap),
>>> @@ -224,7 +229,10 @@ void xnheap_destroy_mapped(xnheap_t *heap,
>>>  int xnheap_init(xnheap_t *heap,
>>>               void *heapaddr,
>>>               u_long heapsize,
>>> -             u_long pagesize);
>>> +             u_long pagesize,
>>> +             const char *name, ...);
>>> +
>>> +void xnheap_set_name(xnheap_t *heap, const char *name, ...);
>>>
>>>  void xnheap_destroy(xnheap_t *heap,
>>>                   void (*flushfn)(xnheap_t *heap,
>>> diff --git a/ksrc/drivers/ipc/iddp.c b/ksrc/drivers/ipc/iddp.c
>>> index a407946..b6382f1 100644
>>> --- a/ksrc/drivers/ipc/iddp.c
>>> +++ b/ksrc/drivers/ipc/iddp.c
>>> @@ -559,7 +559,8 @@ static int __iddp_bind_socket(struct rtipc_private *priv,
>>>               }
>>>
>>>               ret = xnheap_init(&sk->privpool,
>>> -                               poolmem, poolsz, XNHEAP_PAGE_SIZE);
>>> +                               poolmem, poolsz, XNHEAP_PAGE_SIZE,
>>> +                               "ippd: %d", port);
>>>               if (ret) {
>>>                       xnarch_free_host_mem(poolmem, poolsz);
>>>                       goto fail;
>>> diff --git a/ksrc/drivers/ipc/xddp.c b/ksrc/drivers/ipc/xddp.c
>>> index f62147a..a5dafef 100644
>>> --- a/ksrc/drivers/ipc/xddp.c
>>> +++ b/ksrc/drivers/ipc/xddp.c
>>> @@ -703,7 +703,7 @@ static int __xddp_bind_socket(struct rtipc_private *priv,
>>>               }
>>>
>>>               ret = xnheap_init(&sk->privpool,
>>> -                               poolmem, poolsz, XNHEAP_PAGE_SIZE);
>>> +                               poolmem, poolsz, XNHEAP_PAGE_SIZE, "");
>>>               if (ret) {
>>>                       xnarch_free_host_mem(poolmem, poolsz);
>>>                       goto fail;
>>> @@ -746,6 +746,10 @@ static int __xddp_bind_socket(struct rtipc_private *priv,
>>>       sk->minor = ret;
>>>       sa->sipc_port = ret;
>>>       sk->name = *sa;
>>> +
>>> +     if (poolsz > 0)
>>> +             xnheap_set_name(sk->bufpool, "xddp: %d", sa->sipc_port);
>>> +
>>>       /* Set default destination if unset at binding time. */
>>>       if (sk->peer.sipc_port < 0)
>>>               sk->peer = *sa;
>>> diff --git a/ksrc/nucleus/heap.c b/ksrc/nucleus/heap.c
>>> index 96c46f8..793d1c5 100644
>>> --- a/ksrc/nucleus/heap.c
>>> +++ b/ksrc/nucleus/heap.c
>>> @@ -76,6 +76,9 @@ EXPORT_SYMBOL_GPL(kheap);
>>>  xnheap_t kstacks;    /* Private stack pool */
>>>  #endif
>>>
>>> +static DEFINE_XNQUEUE(heapq);        /* Heap list for /proc reporting */
>>> +static unsigned long heapq_rev;
>>> +
>>>  static void init_extent(xnheap_t *heap, xnextent_t *extent)
>>>  {
>>>       caddr_t freepage;
>>> @@ -108,7 +111,7 @@ static void init_extent(xnheap_t *heap, xnextent_t *extent)
>>>   */
>>>
>>>  /*!
>>> - * \fn xnheap_init(xnheap_t *heap,void *heapaddr,u_long heapsize,u_long pagesize)
>>> + * \fn xnheap_init(xnheap_t *heap,void *heapaddr,u_long heapsize,u_long pagesize,const char *name,...)
>>>   * \brief Initialize a memory heap.
>>>   *
>>>   * Initializes a memory heap suitable for time-bounded allocation
>>> @@ -145,6 +148,10 @@ static void init_extent(xnheap_t *heap, xnextent_t *extent)
>>>   * best one for your needs. In the current implementation, pagesize
>>>   * must be a power of two in the range [ 8 .. 32768 ] inclusive.
>>>   *
>>> + * @param name Name displayed in statistic outputs. This parameter can
>>> + * be a format string, in which case succeeding parameters will be used
>>> + * to resolve the final name.
>>> + *
>>>   * @return 0 is returned upon success, or one of the following error
>>>   * codes:
>>>   *
>>> @@ -161,12 +168,13 @@ static void init_extent(xnheap_t *heap, xnextent_t *extent)
>>>   * Rescheduling: never.
>>>   */
>>>
>>> -int xnheap_init(xnheap_t *heap,
>>> -             void *heapaddr, u_long heapsize, u_long pagesize)
>>> +static int xnheap_init_va(xnheap_t *heap, void *heapaddr, u_long heapsize,
>>> +                       u_long pagesize, const char *name, va_list args)
>>>  {
>>>       unsigned cpu, nr_cpus = xnarch_num_online_cpus();
>>>       u_long hdrsize, shiftsize, pageshift;
>>>       xnextent_t *extent;
>>> +     spl_t s;
>>>
>>>       /*
>>>        * Perform some parametrical checks first.
>>> @@ -232,12 +240,71 @@ int xnheap_init(xnheap_t *heap,
>>>
>>>       appendq(&heap->extents, &extent->link);
>>>
>>> +     vsnprintf(heap->name, sizeof(heap->name), name, args);
>>> +
>>> +     xnlock_get_irqsave(&nklock, s);
>>> +     appendq(&heapq, &heap->stat_link);
>>> +     heapq_rev++;
>>> +     xnlock_put_irqrestore(&nklock, s);
>>> +
>>>       xnarch_init_display_context(heap);
>>>
>>>       return 0;
>>>  }
>>> +
>>> +int xnheap_init(xnheap_t *heap, void *heapaddr, u_long heapsize,
>>> +             u_long pagesize, const char *name, ...)
>>> +{
>>> +     va_list args;
>>> +     int ret;
>>> +
>>> +     va_start(args, name);
>>> +     ret = xnheap_init_va(heap, heapaddr, heapsize, pagesize, name, args);
>>> +     va_end(args);
>>> +
>>> +     return ret;
>>> +}
>>>  EXPORT_SYMBOL_GPL(xnheap_init);
>>>
>>> +/*!
>>> + * \fn xnheap_set_name(xnheap_t *heap,const char *name,...)
>>> + * \brief Overwrite the heap's name.
>> At some point, there should be a common base struct containing
>> everything needed to manipulate named objects, we would include in
>> higher level containers. It seems we are over-specializing quite generic
>> work.
> 
> Probably right (though this case remains special to some degree because
> names may consist of numeric IDs that are formatted on visualization). I
> think I should rename 'name' to 'display_name' or so to clarify that we
> are not dealing with a generic object name here.
> 

There wasn't any further feedback regarding this. What shall be the
direction now? I would really like to see this analysis feature in final
2.5 so that people can check which heap overflew on ENOMEM.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [PATCH v3 9/9] nucleus: Include all heaps in statistics

[Philippe Gerum]

On Wed, 2009-11-11 at 13:59 +0100, Jan Kiszka wrote:
> Jan Kiszka wrote:
> > Philippe Gerum wrote:
> >> On Tue, 2009-10-20 at 13:37 +0200, Jan Kiszka wrote:
> >>> This extends /proc/xenomai/heap with statistics about all currently used
> >>> heaps. It takes care to flush nklock while iterating of this potentially
> >>> long list.
> >>>
> >>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
> >>> ---
> >>>
> >>>  include/nucleus/heap.h    |   12 +++-
> >>>  ksrc/drivers/ipc/iddp.c   |    3 +
> >>>  ksrc/drivers/ipc/xddp.c   |    6 ++
> >>>  ksrc/nucleus/heap.c       |  131 ++++++++++++++++++++++++++++++++++++++++-----
> >>>  ksrc/nucleus/module.c     |    2 -
> >>>  ksrc/nucleus/pod.c        |    5 +-
> >>>  ksrc/nucleus/shadow.c     |    5 +-
> >>>  ksrc/skins/native/heap.c  |    6 +-
> >>>  ksrc/skins/native/pipe.c  |    4 +
> >>>  ksrc/skins/native/queue.c |    6 +-
> >>>  ksrc/skins/posix/shm.c    |    4 +
> >>>  ksrc/skins/psos+/rn.c     |    6 +-
> >>>  ksrc/skins/rtai/shm.c     |    7 ++
> >>>  ksrc/skins/vrtx/heap.c    |    6 +-
> >>>  ksrc/skins/vrtx/syscall.c |    3 +
> >>>  15 files changed, 169 insertions(+), 37 deletions(-)
> >>>
> >>> diff --git a/include/nucleus/heap.h b/include/nucleus/heap.h
> >>> index 44db738..f653cd7 100644
> >>> --- a/include/nucleus/heap.h
> >>> +++ b/include/nucleus/heap.h
> >>> @@ -115,6 +115,10 @@ typedef struct xnheap {
> >>>
> >>>       XNARCH_DECL_DISPLAY_CONTEXT();
> >>>
> >>> +     xnholder_t stat_link;   /* Link in heapq */
> >>> +
> >>> +     char name[48];
> >> s,48,XNOBJECT_NAME_LEN
> > 
> > OK, but XNOBJECT_NAME_LEN+16 (due to class prefixes and additional
> > information like the minor ID).
> > 
> >>> +
> >>>  } xnheap_t;
> >>>
> >>>  extern xnheap_t kheap;
> >>> @@ -202,7 +206,8 @@ void xnheap_cleanup_proc(void);
> >>>
> >>>  int xnheap_init_mapped(xnheap_t *heap,
> >>>                      u_long heapsize,
> >>> -                    int memflags);
> >>> +                    int memflags,
> >>> +                    const char *name, ...);
> >>>
> >> The va_list is handy, but this breaks the common pattern used throughout
> >> the rest of the nucleus, based on passing pre-formatted labels. So
> >> either we make all creation calls use va_lists (but xnthread would need
> >> more work), or we make xnheap_init_mapped use the not-so-handy current
> >> form.
> >>
> >> Actually, providing xnheap_set_name() and a name parameter/va_list to
> >> xnheap_init* is one too many, this clutters an inner interface
> >> uselessly. The latter should go away, assuming that anon heaps may still
> >> exist.
> > 
> > If we want to print all heaps, we should at least set a name indicating
> > their class. And therefore we need to pass a descriptive name along with
> > /every/ heap initialization. Forcing the majority of xnheap_init users
> > to additionally issue xnheap_set_name is the cluttering a wanted to
> > avoid. Only a minority needs this split-up, and therefore you see both
> > interfaces in my patch.
> > 
> >>>  void xnheap_destroy_mapped(xnheap_t *heap,
> >>>                          void (*release)(struct xnheap *heap),
> >>> @@ -224,7 +229,10 @@ void xnheap_destroy_mapped(xnheap_t *heap,
> >>>  int xnheap_init(xnheap_t *heap,
> >>>               void *heapaddr,
> >>>               u_long heapsize,
> >>> -             u_long pagesize);
> >>> +             u_long pagesize,
> >>> +             const char *name, ...);
> >>> +
> >>> +void xnheap_set_name(xnheap_t *heap, const char *name, ...);
> >>>
> >>>  void xnheap_destroy(xnheap_t *heap,
> >>>                   void (*flushfn)(xnheap_t *heap,
> >>> diff --git a/ksrc/drivers/ipc/iddp.c b/ksrc/drivers/ipc/iddp.c
> >>> index a407946..b6382f1 100644
> >>> --- a/ksrc/drivers/ipc/iddp.c
> >>> +++ b/ksrc/drivers/ipc/iddp.c
> >>> @@ -559,7 +559,8 @@ static int __iddp_bind_socket(struct rtipc_private *priv,
> >>>               }
> >>>
> >>>               ret = xnheap_init(&sk->privpool,
> >>> -                               poolmem, poolsz, XNHEAP_PAGE_SIZE);
> >>> +                               poolmem, poolsz, XNHEAP_PAGE_SIZE,
> >>> +                               "ippd: %d", port);
> >>>               if (ret) {
> >>>                       xnarch_free_host_mem(poolmem, poolsz);
> >>>                       goto fail;
> >>> diff --git a/ksrc/drivers/ipc/xddp.c b/ksrc/drivers/ipc/xddp.c
> >>> index f62147a..a5dafef 100644
> >>> --- a/ksrc/drivers/ipc/xddp.c
> >>> +++ b/ksrc/drivers/ipc/xddp.c
> >>> @@ -703,7 +703,7 @@ static int __xddp_bind_socket(struct rtipc_private *priv,
> >>>               }
> >>>
> >>>               ret = xnheap_init(&sk->privpool,
> >>> -                               poolmem, poolsz, XNHEAP_PAGE_SIZE);
> >>> +                               poolmem, poolsz, XNHEAP_PAGE_SIZE, "");
> >>>               if (ret) {
> >>>                       xnarch_free_host_mem(poolmem, poolsz);
> >>>                       goto fail;
> >>> @@ -746,6 +746,10 @@ static int __xddp_bind_socket(struct rtipc_private *priv,
> >>>       sk->minor = ret;
> >>>       sa->sipc_port = ret;
> >>>       sk->name = *sa;
> >>> +
> >>> +     if (poolsz > 0)
> >>> +             xnheap_set_name(sk->bufpool, "xddp: %d", sa->sipc_port);
> >>> +
> >>>       /* Set default destination if unset at binding time. */
> >>>       if (sk->peer.sipc_port < 0)
> >>>               sk->peer = *sa;
> >>> diff --git a/ksrc/nucleus/heap.c b/ksrc/nucleus/heap.c
> >>> index 96c46f8..793d1c5 100644
> >>> --- a/ksrc/nucleus/heap.c
> >>> +++ b/ksrc/nucleus/heap.c
> >>> @@ -76,6 +76,9 @@ EXPORT_SYMBOL_GPL(kheap);
> >>>  xnheap_t kstacks;    /* Private stack pool */
> >>>  #endif
> >>>
> >>> +static DEFINE_XNQUEUE(heapq);        /* Heap list for /proc reporting */
> >>> +static unsigned long heapq_rev;
> >>> +
> >>>  static void init_extent(xnheap_t *heap, xnextent_t *extent)
> >>>  {
> >>>       caddr_t freepage;
> >>> @@ -108,7 +111,7 @@ static void init_extent(xnheap_t *heap, xnextent_t *extent)
> >>>   */
> >>>
> >>>  /*!
> >>> - * \fn xnheap_init(xnheap_t *heap,void *heapaddr,u_long heapsize,u_long pagesize)
> >>> + * \fn xnheap_init(xnheap_t *heap,void *heapaddr,u_long heapsize,u_long pagesize,const char *name,...)
> >>>   * \brief Initialize a memory heap.
> >>>   *
> >>>   * Initializes a memory heap suitable for time-bounded allocation
> >>> @@ -145,6 +148,10 @@ static void init_extent(xnheap_t *heap, xnextent_t *extent)
> >>>   * best one for your needs. In the current implementation, pagesize
> >>>   * must be a power of two in the range [ 8 .. 32768 ] inclusive.
> >>>   *
> >>> + * @param name Name displayed in statistic outputs. This parameter can
> >>> + * be a format string, in which case succeeding parameters will be used
> >>> + * to resolve the final name.
> >>> + *
> >>>   * @return 0 is returned upon success, or one of the following error
> >>>   * codes:
> >>>   *
> >>> @@ -161,12 +168,13 @@ static void init_extent(xnheap_t *heap, xnextent_t *extent)
> >>>   * Rescheduling: never.
> >>>   */
> >>>
> >>> -int xnheap_init(xnheap_t *heap,
> >>> -             void *heapaddr, u_long heapsize, u_long pagesize)
> >>> +static int xnheap_init_va(xnheap_t *heap, void *heapaddr, u_long heapsize,
> >>> +                       u_long pagesize, const char *name, va_list args)
> >>>  {
> >>>       unsigned cpu, nr_cpus = xnarch_num_online_cpus();
> >>>       u_long hdrsize, shiftsize, pageshift;
> >>>       xnextent_t *extent;
> >>> +     spl_t s;
> >>>
> >>>       /*
> >>>        * Perform some parametrical checks first.
> >>> @@ -232,12 +240,71 @@ int xnheap_init(xnheap_t *heap,
> >>>
> >>>       appendq(&heap->extents, &extent->link);
> >>>
> >>> +     vsnprintf(heap->name, sizeof(heap->name), name, args);
> >>> +
> >>> +     xnlock_get_irqsave(&nklock, s);
> >>> +     appendq(&heapq, &heap->stat_link);
> >>> +     heapq_rev++;
> >>> +     xnlock_put_irqrestore(&nklock, s);
> >>> +
> >>>       xnarch_init_display_context(heap);
> >>>
> >>>       return 0;
> >>>  }
> >>> +
> >>> +int xnheap_init(xnheap_t *heap, void *heapaddr, u_long heapsize,
> >>> +             u_long pagesize, const char *name, ...)
> >>> +{
> >>> +     va_list args;
> >>> +     int ret;
> >>> +
> >>> +     va_start(args, name);
> >>> +     ret = xnheap_init_va(heap, heapaddr, heapsize, pagesize, name, args);
> >>> +     va_end(args);
> >>> +
> >>> +     return ret;
> >>> +}
> >>>  EXPORT_SYMBOL_GPL(xnheap_init);
> >>>
> >>> +/*!
> >>> + * \fn xnheap_set_name(xnheap_t *heap,const char *name,...)
> >>> + * \brief Overwrite the heap's name.
> >> At some point, there should be a common base struct containing
> >> everything needed to manipulate named objects, we would include in
> >> higher level containers. It seems we are over-specializing quite generic
> >> work.
> > 
> > Probably right (though this case remains special to some degree because
> > names may consist of numeric IDs that are formatted on visualization). I
> > think I should rename 'name' to 'display_name' or so to clarify that we
> > are not dealing with a generic object name here.
> > 
> 
> There wasn't any further feedback regarding this. What shall be the
> direction now? I would really like to see this analysis feature in final
> 2.5 so that people can check which heap overflew on ENOMEM.
> 

Same answer as previously I'm afraid. Adding a string format arg list to
xnheap_init(), which already takes 4 args, would just clutter the
interface, way more than introducing a dedicated routine to attach a
label to a heap. E.g.

-		err = xnheap_init(&pipe->privpool, poolmem, poolsize, XNHEAP_PAGE_SIZE);
+		err = xnheap_init(&pipe->privpool, poolmem, poolsize,
+				  XNHEAP_PAGE_SIZE,
+				  "rt_pipe: %d / %s", minor, name);

introduces a 7-args call form, which is not that nice, especially since
I'm trying now to reduce those abuses throughout the code (see
xnpod_init_thread/start_thread). 

Additionally, we want any "name" property to be XNOBJECT_NAME_LEN long, 
and nothing less or more, because this is a common assumption everywhere
in the code, and I see no reason for xnheap to diverge from that. Not to
speak of the fact that such a name is a natural candidate for index key
for registration purpose, and the descriptive labels introduced would
not be valid keys.

I would rather pick your suggestion to introduce a display-oriented name
we could call a label instead, to make clear that we are only dealing
with pretty-printing in /proc.

I.e.

- xnheap_init() remains unchanged
- xnheap_set_label(fmt, args...) is introduced

> Jan
> 


-- 
Philippe.

Re: [Xenomai-core] [PATCH v3 9/9] nucleus: Include all heaps in statistics

[Jan Kiszka]

Philippe Gerum wrote:
> ...
> I would rather pick your suggestion to introduce a display-oriented name
> we could call a label instead, to make clear that we are only dealing
> with pretty-printing in /proc.
> 
> I.e.
> 
> - xnheap_init() remains unchanged
> - xnheap_set_label(fmt, args...) is introduced
> 

OK, will take this route!

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Xenomai-core] [PATCH v3 8/9] native: Fix memory leak on heap/queue auto-deletion

[Jan Kiszka]

We are currently leaking user space heap/queue objects when the owner
terminates without deleting them before. Fix it by releasing the objects
in the corresponding cleanup callbacks which are also called on owner
termination.

Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
---

 ksrc/skins/native/heap.c    |    5 +++++
 ksrc/skins/native/queue.c   |    5 +++++
 ksrc/skins/native/syscall.c |   25 ++++++-------------------
 3 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/ksrc/skins/native/heap.c b/ksrc/skins/native/heap.c
index f7411e8..886758c 100644
--- a/ksrc/skins/native/heap.c
+++ b/ksrc/skins/native/heap.c
@@ -341,6 +341,11 @@ static void __heap_post_release(struct xnheap *h)
 		xnpod_schedule();
 
 	xnlock_put_irqrestore(&nklock, s);
+
+#ifdef CONFIG_XENO_OPT_PERVASIVE
+	if (heap->cpid)
+		xnfree(heap);
+#endif
 }
 
 /**
diff --git a/ksrc/skins/native/queue.c b/ksrc/skins/native/queue.c
index 3592a4a..249947a 100644
--- a/ksrc/skins/native/queue.c
+++ b/ksrc/skins/native/queue.c
@@ -303,6 +303,11 @@ static void __queue_post_release(struct xnheap *heap)
 		xnpod_schedule();
 
 	xnlock_put_irqrestore(&nklock, s);
+
+#ifdef CONFIG_XENO_OPT_PERVASIVE
+	if (q->cpid)
+		xnfree(q);
+#endif
 }
 
 /**
diff --git a/ksrc/skins/native/syscall.c b/ksrc/skins/native/syscall.c
index 28c720e..cb9f075 100644
--- a/ksrc/skins/native/syscall.c
+++ b/ksrc/skins/native/syscall.c
@@ -2073,24 +2073,17 @@ static int __rt_queue_delete(struct pt_regs *regs)
 {
 	RT_QUEUE_PLACEHOLDER ph;
 	RT_QUEUE *q;
-	int err;
 
 	if (__xn_safe_copy_from_user(&ph, (void __user *)__xn_reg_arg1(regs),
 				     sizeof(ph)))
 		return -EFAULT;
 
 	q = (RT_QUEUE *)xnregistry_fetch(ph.opaque);
-
 	if (!q)
-		err = -ESRCH;
-	else {
-		/* Callee will check the queue descriptor for validity again. */
-		err = rt_queue_delete_inner(q, (void __user *)ph.mapbase);
-		if (!err && q->cpid)
-			xnfree(q);
-	}
+		return -ESRCH;
 
-	return err;
+	/* Callee will check the queue descriptor for validity again. */
+	return rt_queue_delete_inner(q, (void __user *)ph.mapbase);
 }
 
 /*
@@ -2604,7 +2597,6 @@ static int __rt_heap_delete(struct pt_regs *regs)
 {
 	RT_HEAP_PLACEHOLDER ph;
 	RT_HEAP *heap;
-	int err;
 
 	if (__xn_safe_copy_from_user(&ph, (void __user *)__xn_reg_arg1(regs),
 				     sizeof(ph)))
@@ -2613,15 +2605,10 @@ static int __rt_heap_delete(struct pt_regs *regs)
 	heap = (RT_HEAP *)xnregistry_fetch(ph.opaque);
 
 	if (!heap)
-		err = -ESRCH;
-	else {
-		/* Callee will check the heap descriptor for validity again. */
-		err = rt_heap_delete_inner(heap, (void __user *)ph.mapbase);
-		if (!err && heap->cpid)
-			xnfree(heap);
-	}
+		return -ESRCH;
 
-	return err;
+	/* Callee will check the heap descriptor for validity again. */
+	return rt_heap_delete_inner(heap, (void __user *)ph.mapbase);
 }
 
 /*

[Xenomai-core] [PATCH] native: Avoid double release on queue/heap auto-cleanup

[Jan Kiszka]

Jan Kiszka wrote:
> We are currently leaking user space heap/queue objects when the owner
> terminates without deleting them before. Fix it by releasing the objects
> in the corresponding cleanup callbacks which are also called on owner
> termination.

Just realized that we need this patch in addition:

------------>

Commit 3a7330b164 also requires this patch to avoid that the queue and
heap objects are released twice on automatic cleanup (via the xnheap
release handler and via the ppd object queue flush).

Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
---

 include/native/heap.h  |    2 +-
 include/native/queue.h |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/native/heap.h b/include/native/heap.h
index 5444a43..0c2a7a2 100644
--- a/include/native/heap.h
+++ b/include/native/heap.h
@@ -117,7 +117,7 @@ void __native_heap_pkg_cleanup(void);
 
 static inline void __native_heap_flush_rq(xnqueue_t *rq)
 {
-	xeno_flush_rq(RT_HEAP, rq, heap);
+	xeno_flush_rq_norelease(RT_HEAP, rq, heap);
 }
 
 int rt_heap_delete_inner(RT_HEAP *heap,
diff --git a/include/native/queue.h b/include/native/queue.h
index 77925c2..2951c42 100644
--- a/include/native/queue.h
+++ b/include/native/queue.h
@@ -129,7 +129,7 @@ void __native_queue_pkg_cleanup(void);
 
 static inline void __native_queue_flush_rq(xnqueue_t *rq)
 {
-	xeno_flush_rq(RT_QUEUE, rq, queue);
+	xeno_flush_rq_norelease(RT_QUEUE, rq, queue);
 }
 
 ssize_t rt_queue_receive_inner(RT_QUEUE *q, void **bufp,

[Xenomai-core] [PATCH v3 7/9] native: Do not requeue on auto-cleanup errors

[Jan Kiszka]

Migrating an object to the global queue in case of an error during
deletion is racy and may paper over potential bugs. Now that the main
reason for this approach is no longer existing (rt_heap/queue_delete
will not return EBUSY anymore), replace the requeueing with basic
consistency checks.

Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
---

 include/native/ppd.h |   16 ++++------------
 1 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/include/native/ppd.h b/include/native/ppd.h
index 3dbda6a..c6e7479 100644
--- a/include/native/ppd.h
+++ b/include/native/ppd.h
@@ -101,19 +101,11 @@ static inline xeno_rholder_t *xeno_get_rholder(void)
 			xnlock_put_irqrestore(&nklock, s);		\
 			obj = rlink2##__name(holder);			\
 			err = rt_##__name##_delete(obj);		\
+			XENO_ASSERT(NATIVE, !err || err == -EIDRM, );	\
 			__xeno_trace_release(#__name, obj, err);	\
-			if (unlikely(err)) {				\
-				if ((__rq) != &__native_global_rholder.__name##q) { \
-					xnlock_get_irqsave(&nklock, s);	\
-					nholder = popq((rq), holder);	\
-					appendq(&__native_global_rholder.__name##q, holder); \
-					obj->rqueue = &__native_global_rholder.__name##q; \
-				}					\
-			} else {					\
-				if (__release)				\
-					__xeno_release_obj(obj);	\
-				xnlock_get_irqsave(&nklock, s);		\
-			}						\
+			if (!err && __release)				\
+				__xeno_release_obj(obj);		\
+			xnlock_get_irqsave(&nklock, s);			\
 		}							\
 		xnlock_put_irqrestore(&nklock, s);			\
 	} while(0)