Addressing all the comments. About the scoped blocks I used the style that existed in cpuusage already. That section of code is modified code from rtems_cpu_usage_report_with_plugin.
Jennifer > -----Original Message----- > From: devel [mailto:devel-boun...@rtems.org] On Behalf Of Gedare Bloom > Sent: Thursday, October 30, 2014 9:54 AM > To: Jennifer Averett > Cc: rtems-de...@rtems.org > Subject: Re: [rtems commit] libmisc: Add top to cpuusage. > > I missed reviewing this patch earlier, but there are some style problems with > it, please fix them on the head. > > On Tue, Oct 28, 2014 at 11:17 AM, Jennifer Averett <jenni...@rtems.org> > wrote: > > Module: rtems > > Branch: master > > Commit: 6031da438d219c6ec5d9d48f1df2aef91710cce3 > > Changeset: > > > http://git.rtems.org/rtems/commit/?id=6031da438d219c6ec5d9d48f1df2aef9 > > 1710cce3 > > > > Author: Jennifer Averett <jennifer.aver...@oarcorp.com> > > Date: Mon Sep 29 10:20:27 2014 -0500 > > > > libmisc: Add top to cpuusage. > > > > --- > > > > cpukit/libmisc/Makefile.am | 2 +- > > cpukit/libmisc/cpuuse/cpuusagetop.c | 337 > +++++++++++++++++++++++++++++++++++ > > cpukit/libmisc/cpuuse/cpuuse.h | 19 ++ > > 3 files changed, 357 insertions(+), 1 deletions(-) > > > > diff --git a/cpukit/libmisc/Makefile.am b/cpukit/libmisc/Makefile.am > > index 2f41ffa..d26c484 100644 > > --- a/cpukit/libmisc/Makefile.am > > +++ b/cpukit/libmisc/Makefile.am > > @@ -27,7 +27,7 @@ EXTRA_DIST += cpuuse/README > > > > noinst_LIBRARIES += libcpuuse.a > > libcpuuse_a_SOURCES = cpuuse/cpuusagereport.c > cpuuse/cpuusagereset.c \ > > - cpuuse/cpuuse.h cpuuse/cpuusagedata.c > > + cpuuse/cpuuse.h cpuuse/cpuusagedata.c cpuuse/cpuusagetop.c > > > > ## devnull > > noinst_LIBRARIES += libdevnull.a > > diff --git a/cpukit/libmisc/cpuuse/cpuusagetop.c > > b/cpukit/libmisc/cpuuse/cpuusagetop.c > > new file mode 100644 > > index 0000000..7e7348a > > --- /dev/null > > +++ b/cpukit/libmisc/cpuuse/cpuusagetop.c > > @@ -0,0 +1,337 @@ > > +/** > > + * @file > > + * > > + * @brief CPU Usage Top > > + * @ingroup libmisc_cpuuse CPU Usage > > + */ > > + > > +/* > > + * COPYRIGHT (c) 2014. > > + * On-Line Applications Research Corporation (OAR). > > + * > > + * The license and distribution terms for this file may be > > + * found in the file LICENSE in this distribution or at > > + * http://www.rtems.org/license/LICENSE. > > + */ > > + > > +#ifdef HAVE_CONFIG_H > > +#include "config.h" > > +#endif > > + > > +#include <string.h> > > +#include <stdlib.h> > > +#include <stdio.h> > > +#include <ctype.h> > > +#include <inttypes.h> > > + > > +#include <rtems/cpuuse.h> > > +#include <rtems/score/objectimpl.h> > > +#include <rtems/score/threadimpl.h> > > +#include <rtems/score/todimpl.h> > > +#include <rtems/score/watchdogimpl.h> > > + > > + > > +/* > > + * Common variable to sync the load monitor task. > > + */ > > +static volatile int rtems_cpuusage_top_thread_active; > > + > > + > remove multiple blank lines > > > +typedef struct { > > + void *context; > > + rtems_printk_plugin_t print; > > +}rtems_cpu_usage_plugin_t; > > + > > +#define RTEMS_CPUUSAGE_TOP_MAX_LOAD_TASKS (20) > > + > > +/* > > + * rtems_cpuusage_top_thread > > + * > > + * DESCRIPTION: > > + * > > + * This function displays the load of the tasks on an ANSI terminal. > > + * > Get rid of "DESCRIPTION", prefer to use minimalist comment style on static > functions. > > > + */ > > + > > +static void > > +rtems_cpuusage_top_thread (rtems_task_argument arg) { > > + uint32_t api_index; > > + Thread_Control* the_thread; > > + int i; > > + int j; > > + int k; > > + Objects_Information* information; > > + char name[13]; > > + int task_count = 0; > > + uint32_t seconds, nanoseconds; > > + rtems_cpu_usage_plugin_t* plugin = (rtems_cpu_usage_plugin_t*)arg; > > + Thread_Control* > load_tasks[RTEMS_CPUUSAGE_TOP_MAX_LOAD_TASKS + 1]; > > + unsigned long long > load[RTEMS_CPUUSAGE_TOP_MAX_LOAD_TASKS + 1]; > > + > > + while (true) > > + { > Opening brace should go on whille() line. > > > + #ifndef __RTEMS_USE_TICKS_FOR_STATISTICS__ > > + Timestamp_Control uptime, total, ran, uptime_at_last_reset; > > + #else > > + uint32_t total_units = 0; > > + #endif > > + > > + rtems_cpuusage_top_thread_active = 1; > > + > > + memset (load_tasks, 0, sizeof (load_tasks)); > > + memset (load, 0, sizeof (load)); > > + > > + /* > > + * Iterate over the tasks and sort the highest load tasks > > + * into our local arrays. We only handle a limited number of > > + * tasks. > > + */ > > + for ( api_index = 1 ; api_index <= OBJECTS_APIS_LAST ; api_index++ ) { > > + #if !defined(RTEMS_POSIX_API) || defined(RTEMS_DEBUG) > > + if ( !_Objects_Information_table[ api_index ] ) > > + continue; > > + #endif > > + > > + information = _Objects_Information_table[ api_index ][ 1 ]; > > + if ( information ) { > > + for ( k=1 ; k <= information->maximum ; k++ ) { > I prefer that nested iterators should be used in increasing order, e.g. i > first. > > > + the_thread = (Thread_Control *)information->local_table[ k ]; > > + if ( the_thread ) { > > + > No blank space after the opening brace. > > > + Thread_CPU_usage_t l = the_thread->cpu_time_used; > Avoid using l and I as variable names.They're hard to differentiate from each > other and from the digit 1. This should be a more useful variable name > anyway. A one-letter variable should really only be used for iterators. We > ought to have a rule about this if we don't. > > > + > > + /* > > + * When not using nanosecond CPU usage resolution, we have to > count > > + * the number of "ticks" we gave credit for to give the user > > a rough > > + * guideline as to what each number means proportionally. > > + */ > > + #ifdef __RTEMS_USE_TICKS_FOR_STATISTICS__ > > + total_units += l; > > + #endif > > + > > + /* Count the number of tasks and sort this load value */ > > + task_count++; > > + for (i = 0; i < RTEMS_CPUUSAGE_TOP_MAX_LOAD_TASKS; i++) { > > + if (load_tasks[i]) { > > + if ((l == 0) || (l < load[i])) > > + continue; > > + for (j = (RTEMS_CPUUSAGE_TOP_MAX_LOAD_TASKS - 1); j >= i; > > j- > -){ > > + load_tasks[j + 1] = load_tasks[j]; > > + load[j + 1] = load[j]; > > + } > > + } > > + load_tasks[i] = the_thread; > > + load[i] = l; > > + break; > > + } > > + } > > + } > > + } > > + } > > + > > + #ifndef __RTEMS_USE_TICKS_FOR_STATISTICS__ > > + _Timestamp_Set_to_zero( &total ); > > + uptime_at_last_reset = CPU_usage_Uptime_at_last_reset; > > + #endif > > + > > + _TOD_Get_uptime( &uptime ); > > + seconds = _Timestamp_Get_seconds( &uptime ); > > + nanoseconds = _Timestamp_Get_nanoseconds( &uptime ) / > > + TOD_NANOSECONDS_PER_MICROSECOND; > > + (*plugin->print)(plugin->context, "\x1b[H\x1b[J Press ENTER to > exit.\n\n"); > > + (*plugin->print)(plugin->context, "uptime: "); > > + (*plugin->print)(plugin->context, > > + "%7" PRIu32 ".%06" PRIu32 "\n", seconds, nanoseconds > > + ); > > + > > + (*plugin->print)( > > + plugin->context, > > + > > "-------------------------------------------------------------------------------\n" > > + " CPU USAGE BY THREAD\n" > > + > > "------------+---------------------+---------------+---------------+------------ > \n" > > + #ifndef __RTEMS_USE_TICKS_FOR_STATISTICS__ > > + " ID | NAME | RPRI | CPRI | SECONDS > > | PERCENT\n" > > + #else > > + " ID | NAME | RPRI | CPRI | TICKS > > | PERCENT\n" > > + #endif > > + > > "------------+---------------------+---------------+---------------+------------ > \n" > > + ); > > + > > + for (i = 0; i < RTEMS_CPUUSAGE_TOP_MAX_LOAD_TASKS; i++) > > + { > > + > Fix style here. > > > + if (!load_tasks[i]) > > + break; > > + > > + /* > > + * If this is the currently executing thread, account for time > > + * since the last context switch. > > + */ > > + the_thread = load_tasks[i]; > > + > > + rtems_object_get_name( the_thread->Object.id, sizeof(name), name > ); > > + (*plugin->print)( > > + plugin->context, > > + " 0x%08" PRIx32 " | %-19s | %3" PRId32 " | %3" PRId32 " |", > > + the_thread->Object.id, > > + name, > > + the_thread->real_priority, > > + the_thread->current_priority > > + ); > > + > > + #ifndef __RTEMS_USE_TICKS_FOR_STATISTICS__ > > + { > I don't know how to feel about artifically adding scoped blocks. > Anyone else weigh in? > > > + Timestamp_Control last; > > + uint32_t ival, fval; > > + > > + /* > > + * If this is the currently executing thread, account for time > > + * since the last context switch. > > + */ > > + ran = load[i]; > > + if ( _Thread_Get_time_of_last_context_switch( the_thread, &last ) ) > { > > + Timestamp_Control used; > > + _TOD_Get_uptime( &uptime ); > > + _Timestamp_Subtract( &last, &uptime, &used ); > > + _Timestamp_Add_to( &ran, &used ); > > + } else { > > + _TOD_Get_uptime( &uptime ); > > + } > > + _Timestamp_Subtract( &uptime_at_last_reset, &uptime, &total ); > > + _Timestamp_Divide( &ran, &total, &ival, &fval ); > > + > > + /* > > + * Print the information > > + */ > > + > > + seconds = _Timestamp_Get_seconds( &ran ); > > + nanoseconds = _Timestamp_Get_nanoseconds( &ran ) / > > + TOD_NANOSECONDS_PER_MICROSECOND; > > + (*plugin->print)( plugin->context, > > + "%7" PRIu32 ".%06" PRIu32 " |%4" PRIu32 ".%03" PRIu32 "\n", > > + seconds, nanoseconds, > > + ival, fval > > + ); > > + } > > + #else > > + if (total_units) { > > + uint64_t ival_64; > > + > > + ival_64 = load[i]; > > + ival_64 *= 100000; > > + ival = ival_64 / total_units; > > + } else { > > + ival = 0; > > + } > > + > > + fval = ival % 1000; > > + ival /= 1000; > > + (*plugin->print)( plugin->context, > > + "%14" PRIu32 " |%4" PRIu32 ".%03" PRIu32 "\n", > > + load[i], > > + ival, > > + fval > > + ); > > + #endif > > + } > > + > > + if (task_count < RTEMS_CPUUSAGE_TOP_MAX_LOAD_TASKS) > > + { > > + j = RTEMS_CPUUSAGE_TOP_MAX_LOAD_TASKS - task_count; > > + while (j > 0) > > + { > > + (*plugin->print)( plugin->context, "\x1b[K\n"); > > + j--; > > + } > > + } > > + > > + rtems_cpuusage_top_thread_active = 0; > > + > > + rtems_task_wake_after (RTEMS_MICROSECONDS_TO_TICKS > (5000000)); > > + } > > +} > > + > > +void rtems_cpu_usage_top_with_plugin( > > + void *context, > > + rtems_printk_plugin_t print > > +) > > +{ > > + rtems_status_code sc; > > + rtems_task_priority priority; > > + rtems_name name; > > + rtems_id id; > > + rtems_cpu_usage_plugin_t plugin; > > + > > + if ( !print ) > > + return; > > + > > + plugin.context = context; > > + plugin.print = print; > > + > > + sc = rtems_task_set_priority (RTEMS_SELF, RTEMS_CURRENT_PRIORITY, > > + &priority); > > + > > + if (sc != RTEMS_SUCCESSFUL) > > + { > > + (*print)( > > + context, > > + "error: cannot obtain the current priority: %s\n", > > + rtems_status_text (sc) > > + ); > > + return; > > + } > > + > > + name = rtems_build_name('C', 'P', 'l', 't'); > > + > > + sc = rtems_task_create (name, priority, 4 * 1024, > > + RTEMS_NO_FLOATING_POINT | RTEMS_LOCAL, > > + RTEMS_PREEMPT | RTEMS_TIMESLICE | RTEMS_NO_ASR, > > + &id); > > + > > + if (sc != RTEMS_SUCCESSFUL) > > + { > > + (*print)( > > + context, > > + "error: cannot create helper thread: %s\n", > > + rtems_status_text (sc) > > + ); > > + return; > > + } > > + > > + sc = rtems_task_start ( > > + id, rtems_cpuusage_top_thread, (rtems_task_argument)&plugin ); > > + if (sc != RTEMS_SUCCESSFUL) { > > + (*print)( > > + context, > > + "error: cannot start helper thread: %s\n", > > + rtems_status_text (sc) > > + ); > > + rtems_task_delete (id); > > + return; > > + } > > + > > + for (;;) > > + { > > + int c = getchar (); > > + > > + if ((c == '\r') || (c == '\n')) > > + { > > + int loops = 20; > > + > > + while (loops && rtems_cpuusage_top_thread_active) > > + rtems_task_wake_after (RTEMS_MICROSECONDS_TO_TICKS > (100000)); > > + > > + rtems_task_delete (id); > > + > > + (*print)(context, "load monitoring stopped.\n"); > > + return; > > + } > > + } > > +} > > + > > +void rtems_cpu_usage_top( void ) > > +{ > > + rtems_cpu_usage_top_with_plugin( NULL, printk_plugin ); } > > diff --git a/cpukit/libmisc/cpuuse/cpuuse.h > > b/cpukit/libmisc/cpuuse/cpuuse.h index 1aee275..662d905 100644 > > --- a/cpukit/libmisc/cpuuse/cpuuse.h > > +++ b/cpukit/libmisc/cpuuse/cpuuse.h > > @@ -63,6 +63,25 @@ void rtems_cpu_usage_report_with_plugin( > > void rtems_cpu_usage_report( void ); > > > > /** > > + * @brief CPU usage Top plugin > > + * > > + * Report CPU Usage in top format to > > + * to a print plugin. > > + */ > > +void rtems_cpu_usage_top_with_plugin( > > + void *context, > > + rtems_printk_plugin_t print > > +); > > + > > +/** > > + * @brief CPU usage top. > > + * > > + * CPU Usage top > > + */ > > + > > +void rtems_cpu_usage_top( void ); > > + > > +/** > > * @brief Reset CPU usage. > > * > > * CPU Usage Reporter > > > > _______________________________________________ > > vc mailing list > > v...@rtems.org > > http://lists.rtems.org/mailman/listinfo/vc > _______________________________________________ > devel mailing list > devel@rtems.org > http://lists.rtems.org/mailman/listinfo/devel _______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
