Re: RTEMS | Draft: microblaze/cpukit/cpuuse: add max cpu / bt support (!843)
Merge request https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843
was reviewed by Gedare Bloom
--
Gedare Bloom started a new discussion on
cpukit/libmisc/cpuuse/cpuuse_context_pc_aarch64.c:
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843#note_142013
> +#include
> +
> +uintptr_t rtems_cpuuse_context_to_pc( const Context_Control *context )
I would suggest making this part of each CPU port definitions in
`score/cpu/${arch}/include/rtems/score/cpu.h` and provide this as an score
interface to the `Context_Control` or the `CPU_Exception_frame`. If it needs to
be exported to users, then you could add an `rtems_...` interface to it
somewhere suitable, but I think this could be kept internal.
--
Gedare Bloom started a new discussion on cpukit/include/rtems/cpuuse.h:
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843#note_142014
> + uint32_t fval;
> +
> + char task_name[38];
why `38`?
--
Gedare Bloom started a new discussion on cpukit/include/rtems/cpuuse.h:
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843#note_142015
> + * @brief Fractional part of the percentage.
> + */
> + uint32_t fval;
this could be an `rtems_task_cpu_usage_info`? This appears to be a kind of
inheritance.
--
Gedare Bloom started a new discussion on cpukit/include/rtems/cpuuse.h:
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843#note_142016
> +rtems_status_code rtems_cpu_max_usage_extended(
> + rtems_max_task_cpu_usage_info *usage,
> + struct _Thread_Control **thread
should not be leaking internal data types. use an opaque type (e.g., `rtems_id`)
--
Gedare Bloom started a new discussion on
cpukit/include/rtems/cpuuse_backtrace.h:
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843#note_142017
> + * @retval RTEMS_INTERNAL_ERROR Stack walking encountered an error.
> + */
> +rtems_status_code rtems_cpu_usage_backtrace(
This seems to be not related to `cpuuse`, I think the API should be placed
somewhere else.
--
Gedare Bloom started a new discussion on
cpukit/include/rtems/cpuuse_backtrace.h:
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843#note_142018
> + */
> +rtems_status_code rtems_cpu_usage_backtrace(
> + Thread_Control *thread,
use opaque types
--
Gedare Bloom started a new discussion on
cpukit/include/rtems/cpuuse_backtrace.h:
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843#note_142019
> + */
> + uintptr_t stack_pointer;
> +} rtems_backtrace_frame_t;
avoid defining typedefs with `_t`. This is reserved by POSIX.
--
Gedare Bloom started a new discussion on
cpukit/include/rtems/cpuuse_backtrace.h:
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843#note_142020
> + const rtems_printer *printer,
> + const rtems_backtrace_t *backtrace,
> + bool show_details
same here, this is a configuration-time decision.
--
Gedare Bloom started a new discussion on
cpukit/include/rtems/cpuuse_backtrace.h:
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843#note_142021
> + * @retval false Backtrace is not supported on this architecture.
> + */
> +bool rtems_backtrace_supported(void);
same here, this is a configuration-time decision.
--
Gedare Bloom started a new discussion on
cpukit/include/rtems/cpuuse_backtrace.h:
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843#note_142022
> + Thread_Control *thread,
> + rtems_backtrace_t *backtrace,
> + int max_frames
I'm guessing this is a compile-time decision. I would consider making it part
of the configuration.
--
Gedare Bloom started a new discussion on
cpukit/include/rtems/cpuuse_backtrace.h:
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843#note_142023
> + * @brief Status of backtrace generation.
> + */
> + rtems_status_code status;
we usually return the status from functions, so it can be checked directly.
--
Gedare Bloom started a new discussion on
cpukit/include/rtems/cpuuse_backtrace.h:
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843#note_142024
> +
> + /**
> + * @brief RTL object containing the symbol.
RTL?
--
Gedare Bloom started a new discussion on cpukit/include/rtems/cpuuseimpl.h:
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843#note_142025
> + * held. Callers should copy any required string data under the lock.
> + */
> +bool rtems_cpuuse_find_nearest_symbol(
If these faciltiies exist in the RTL, maybe it should be relied on for the
backtrace functionality also?
--
View it on GitLab:
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843
You're receiving this email because of your account on gitlab.rtems.org.
___
bugs mailing list
[email protected]
http://lists.rtems.org/mailman/listinfo/bugs
Re: RTEMS | Draft: microblaze/cpukit/cpuuse: add max cpu / bt support (!843)
Gedare Bloom commented: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843#note_142026 This backtrace stuff appears to be unrelated to cpuuse, I think you should separate this into two MRs, one for max use and one for bt. -- View it on GitLab: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843#note_142026 You're receiving this email because of your account on gitlab.rtems.org. ___ bugs mailing list [email protected] http://lists.rtems.org/mailman/listinfo/bugs
Re: RTEMS | Draft: microblaze/cpukit/cpuuse: add max cpu / bt support (!843)
Chris Johns commented on a discussion on
cpukit/include/rtems/cpuuse_backtrace.h:
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843#note_136895
> + */
> +typedef struct {
> + /**
> + * @brief Frame address (program counter).
> + */
> + uintptr_t address;
> +
> + /**
> + * @brief Resolved symbol name (32 bytes, null-terminated).
> + */
> + char symbol_name[32];
> +
> + /**
> + * @brief Object/module name containing the symbol (32 bytes,
> null-terminated).
> + */
> + char object_name[32];
If the module has been deleted this symbol would not be valid. And if a new
module is loaded with a different symbol would there be a clash.
I suppose the nature of this data is determined by the life time of the struct.
If it is transient then maybe copies are OK? This however comes back to the
original issue of length and very long symbols.
--
View it on GitLab:
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843#note_136895
You're receiving this email because of your account on gitlab.rtems.org.
___
bugs mailing list
[email protected]
http://lists.rtems.org/mailman/listinfo/bugs
Re: RTEMS | Draft: microblaze/cpukit/cpuuse: add max cpu / bt support (!843)
Sam Price started a new discussion on cpukit/libmisc/cpuuse/cpuuse_symbols.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843#note_136888 > +/* SPDX-License-Identifier: BSD-2-Clause */ > + > +#ifdef HAVE_CONFIG_H > +#include "config.h" > +#endif > + > +#include > +#include > + > +bool rtems_cpuuse_find_nearest_symbol( @amar This might be related to !679 -- View it on GitLab: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843#note_136888 You're receiving this email because of your account on gitlab.rtems.org. ___ bugs mailing list [email protected] http://lists.rtems.org/mailman/listinfo/bugs
Re: RTEMS | Draft: microblaze/cpukit/cpuuse: add max cpu / bt support (!843)
Sam Price pushed new commits to merge request !843 Merge request URL: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843 * 1ca75843 - Refactor CPU usage PC helpers -- View it on GitLab: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843 You're receiving this email because of your account on gitlab.rtems.org. ___ bugs mailing list [email protected] http://lists.rtems.org/mailman/listinfo/bugs
Re: RTEMS | Draft: microblaze/cpukit/cpuuse: add max cpu / bt support (!843)
Joel Sherrill commented: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843#note_136482 Fold the three patches into one. -- View it on GitLab: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843#note_136482 You're receiving this email because of your account on gitlab.rtems.org. ___ bugs mailing list [email protected] http://lists.rtems.org/mailman/listinfo/bugs
Re: RTEMS | Draft: microblaze/cpukit/cpuuse: add max cpu / bt support (!843)
Merge request https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843
was reviewed by Joel Sherrill
--
Joel Sherrill started a new discussion on
cpukit/libmisc/cpuuse/cpuuse_backtrace.c:
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843#note_136479
> +)
> +{
> + if (thread == NULL) {
Can this happen in a non-debug build? If for safety, use the RTEMS _Assert()
which is turned on for debug.
Otherwise, this becomes untestable code.
--
Joel Sherrill started a new discussion on cpukit/libmisc/cpuuse/cpuusagemax.c:
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843#note_136480
> + * @brief Extract program counter from context for supported architectures
> + */
> +static uintptr_t RtemsContextToPC(Context_Control *context)
I think this file should be split into multiple per architecture files like
libdl and libdebugger are done. Otherwise, this is going to become huge and
unwieldy.
--
View it on GitLab:
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843
You're receiving this email because of your account on gitlab.rtems.org.
___
bugs mailing list
[email protected]
http://lists.rtems.org/mailman/listinfo/bugs
Re: RTEMS | Draft: microblaze/cpukit/cpuuse: add max cpu / bt support (!843)
Joel Sherrill started a new discussion on
cpukit/libmisc/cpuuse/cpuusagefortask.c:
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843#note_136478
> + uint32_t ival;
> + uint32_t fval;
> + Timestamp_Control uptime;
> + Timestamp_Control uptime_at_last_reset;
> + Timestamp_Control used;
> + Timestamp_Control total;
> +
> + uptime_at_last_reset = CPU_usage_Uptime_at_last_reset;
> +
> + the_thread = _Thread_Get( task_id, &lock_context );
> +
> + if ( the_thread == NULL ) {
> +return RTEMS_INVALID_ID;
> + }
> +
> + used = _Thread_Get_CPU_time_used_after_last_reset( the_thread );
A comment above this block would be helpful.
--
View it on GitLab:
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843#note_136478
You're receiving this email because of your account on gitlab.rtems.org.
___
bugs mailing list
[email protected]
http://lists.rtems.org/mailman/listinfo/bugs
Re: RTEMS | Draft: microblaze/cpukit/cpuuse: add max cpu / bt support (!843)
Merge request https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843 was reviewed by Joel Sherrill -- Joel Sherrill commented on a discussion on cpukit/include/rtems/cpuuse_backtrace.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843#note_136476 > + * @brief Architecture-specific error information. > + */ > + uint32_t arch_error; Yes. No point in keeping an unused variable. -- View it on GitLab: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843 You're receiving this email because of your account on gitlab.rtems.org. ___ bugs mailing list [email protected] http://lists.rtems.org/mailman/listinfo/bugs
Re: RTEMS | Draft: microblaze/cpukit/cpuuse: add max cpu / bt support (!843)
Sam Price started a new discussion on cpukit/include/rtems/cpuuse_backtrace.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843#note_136475 > + int frame_count; > + > + /** > + * @brief Array of stack frames. > + */ > + rtems_backtrace_frame_t frames[RTEMS_CPUUSE_BACKTRACE_MAX_FRAMES]; > + > + /** > + * @brief Status of backtrace generation. > + */ > + rtems_status_code status; > + > + /** > + * @brief Architecture-specific error information. > + */ > + uint32_t arch_error; This isnt used remove? -- View it on GitLab: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/843#note_136475 You're receiving this email because of your account on gitlab.rtems.org. ___ bugs mailing list [email protected] http://lists.rtems.org/mailman/listinfo/bugs
