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.
Jan
--
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux
_______________________________________________
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core
