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.kis...@siemens.com>
> >>> ---
> >>>
> >>> 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.
_______________________________________________
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core
