Re: Adding capture support to score
Hi, On Tue, Jul 8, 2014 at 3:19 PM, Jennifer Averett jennifer.aver...@oarcorp.com wrote: The attached patches are a starting point for discussions for adding capture support to core objects. We started to write notes based on our discussions but the text was harder to follow than just writing some code and commenting on it. It is hard to see the impact of word changes versus real changes. Is this related to capture engine? if not, there needs to be some better terminology to avoid confusion. I find it easier to comment on patches that are sent in-line, see instructions on the wiki Git page about using git-send-email. The initial cut has one method call per Score Handler with an enumerated parameter indicating points of interest. Most take a thread pointer and an object id as arguments which can be interpreted based on the event indicated by the enumerated first argument. Why have separate event sets (types) for mutex and semaphore? (I will say event set to mean the points of interest associated with a class of objects. Thus, your first patch includes 3 event sets, one for generic Object, one for Mutex, and one for Semaphore.) Most all locks will have the same kinds of abstract events that affect them. Some of the items we would like to discuss: 1) Should the number of methods be expanded to one method per point of interest? This might make sense, since probably the approach taken requires the one function handler per event set to determine the source of the event anyways, so they'll all probably just be implemented as a giant switch statement. It makes sense to me that there is a specific function invoked for each kind of event that occurs, and it could help with debugging since a backtrace makes more sense, and inlining can be done more aggressively with finer-grained functions. gcc can inline function pointers under certain conditions, although I forget what they are off-hand. I would choose a generic template for the function pointer though, something like... typedef void (*_Capture_Event_handler)(_Capture_Event e, Objects_Id Object, Thread_Control *thread); Or something similar, if it can be made generic enough to cover the parameters needed at each point of interest. Having one method per event or doing it this way is a no-win choice. The callout table will grow larger with more entries if we go to one function per event. More methods will be in the capture engine. But if we stick this way, there is a smaller callout table, fewer capture methods, but the capture methods likely will have some decode logic. Perhaps, or you just need to be fastidious in how many events you include in an event set. You could also have a catch-all function, e.g. _Capture_Event_handler default; that can be invoked for when there isn't a specific handler for an event at some point of interest. 2) This design focuses on pass paths for points of interest and ignores failure paths, is this the direction we wish to follow? There are potentially LOTS of failure points and the errors will be in the supercore level. Worse, a try lock which doesn't get the mutex could end up in a busy loop swamping the capture engine. You may consider reporting only one failure at each point of interest. For debugging purposes, it probably helps to see failures? 3) We think the callout table should be completely populated. The table is either full of methods or not. It is an error not to install a handler for every event. This simplified checks at run-time. And if I'm only interested in tracking Object allocate/free? I still need to provide stubs (and suffer overhead) for locks or whatever other events there are? 4) We discussed having the Score own a table with stubs so the calls are always present but that could negatively impact minimum footprint. This only puts a single pointer and code to call. This point is arguable if we stick to the small number of handlers approach. Eliminating the if table installed check and always making the subroutine call to a stub MIGHT be better. But we thought avoiding a subroutine call in critical paths was best. If you can figure out how to make gcc inline the function pointer for this case, it can be practicable since there should be zero-overhead after optimizations. That's all I have for now. -Gedare So this is basically some code using one approach for discussion purposes. It Is only known to compile and this may not be enough for you to compile it. :) We want to discuss the design approach and see where to take this. Jennifer Averett On-Line Applications Research ___ 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
Re: Misc on RBTree Thread Queue Priority Discipline Changes
On 7/9/2014 10:34 AM, Gedare Bloom wrote: On Tue, Jul 8, 2014 at 5:37 PM, Joel Sherrill joel.sherr...@oarcorp.com wrote: Hi If you take the patches in their entirety, most of the tests appear to be about 500 bytes smaller on the erc32. What is the change in wkspace size? Basically you add 3 pointers + enum to each TCB / thd proxy, but remove some space for the control node I guess. I would guess the code savings is worth it. Good question. I was watching code space. :) Before: (gdb) p sizeof(Thread_queue_Control) $1 = 64 (gdb) p sizeof(Thread_Control) $2 = 360 After: (gdb) p sizeof(Thread_queue_Control) $1 = 40 (gdb) p sizeof(Thread_Control) $2 = 376 Summary: -24 Thread_queue_Control +16 Thread_Control Since thread queues are used in blocking objects, I assume there will be more of them in a system and this is a net win. Technically, they could be used in the Scheduler implementations. When a thread is not blocked, the RBTree_Node is unused. None of the tmtests do priority based blocking so I can't report any changes there. There was historically a subroutine in the threadq calls for a discipline specific routine. Using the RBTree, this resulted in only 3-5 lines of code unique to the discipline in those discipline files. The other 20+ lines was duplicated. The last patch folds those subroutines into the main methods. There is still some room for clean up since the code that is in threadblockingoperationcancel.c is open coded there and three other places. So there are a total of four copies of this code in the tree. + rtems/src/eventsurrender.c + score/src/threadqdequeue.c + score/src/threadqextract.c + score/src/threadblockingoperationcancel.c I do not see discipline used in threadblockingoperationcancel.c what do you mean? All four of those methods do exactly the same thing AFTER they deal with the event or threadq specific discipline code. I have a patch pending with this change. Two of those need the debug check code and two do not. Also the method name isn't right for all four cases. It indicates its use on the resource released in ISR usage not the normal clean up from blocking case. Suggestions on a new name and whether this should be a real subroutine or a static inline is appreciated. Then I can rework and reduce the code duplication. Propose the code; I'm not sure what you mean to refactor into the new function. I found a solution and will pose a patch. Two methods. The one used for undoing on a sync state trigger will be a wrapper for the one used for normal unblocking. -Gedare -- Joel Sherrill, Ph.D. Director of Research Development joel.sherr...@oarcorp.comOn-Line Applications Research Ask me about RTEMS: a free RTOS Huntsville AL 35805 Support Available(256) 722-9985 ___ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel -- Joel Sherrill, Ph.D. Director of Research Development joel.sherr...@oarcorp.comOn-Line Applications Research Ask me about RTEMS: a free RTOS Huntsville AL 35805 Support Available(256) 722-9985 ___ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
[PATCH 6/6] Use Shared Method for Thread Unblock Cleanup
When a thread is removed from a thread queue or is unblocked
by receiving an event, the same actions are required.
+ timeout watchdog canceled,
+ thread must be unblocked, and
+ (MP only) proxy cleaned up
This patch makes sure there is only one copy of this code.
---
cpukit/score/include/rtems/score/threadimpl.h| 18 ++
cpukit/score/src/threadblockingoperationcancel.c | 65 --
cpukit/score/src/threadqdequeue.c| 18 +-
cpukit/score/src/threadqextract.c| 22 ++-
4 files changed, 63 insertions(+), 60 deletions(-)
diff --git a/cpukit/score/include/rtems/score/threadimpl.h
b/cpukit/score/include/rtems/score/threadimpl.h
index 4971e9d..5327e29 100644
--- a/cpukit/score/include/rtems/score/threadimpl.h
+++ b/cpukit/score/include/rtems/score/threadimpl.h
@@ -403,6 +403,24 @@ void _Thread_blocking_operation_Cancel(
ISR_Level level
);
+/**
+ * @brief Finalize a blocking operation.
+ *
+ * This method is used to finalize a blocking operation that was
+ * satisfied. It may be used with thread queues or any other synchronization
+ * object that uses the blocking states and watchdog times for timeout.
+ *
+ * This method will restore the previous ISR disable level during the cancel
+ * operation. Thus it is an implicit _ISR_Enable().
+ *
+ * @param[in] the_thread is the thread whose blocking is canceled
+ * @param[in] level is the previous ISR disable level
+ */
+void _Thread_blocking_operation_Finalize(
+ Thread_Control *the_thread,
+ ISR_Level level
+);
+
RTEMS_INLINE_ROUTINE Per_CPU_Control *_Thread_Get_CPU(
const Thread_Control *thread
)
diff --git a/cpukit/score/src/threadblockingoperationcancel.c
b/cpukit/score/src/threadblockingoperationcancel.c
index 127d852..b496796 100644
--- a/cpukit/score/src/threadblockingoperationcancel.c
+++ b/cpukit/score/src/threadblockingoperationcancel.c
@@ -24,6 +24,41 @@
#endif
#include rtems/score/watchdogimpl.h
+void _Thread_blocking_operation_Finalize(
+ Thread_Control *the_thread,
+ ISR_Level level
+)
+{
+ /*
+ * The thread is not waiting on anything after this completes.
+ */
+ the_thread-Wait.queue = NULL;
+
+ /*
+ * If the sync state is timed out, this is very likely not needed.
+ * But better safe than sorry when it comes to critical sections.
+ */
+ if ( _Watchdog_Is_active( the_thread-Timer ) ) {
+_Watchdog_Deactivate( the_thread-Timer );
+_ISR_Enable( level );
+(void) _Watchdog_Remove( the_thread-Timer );
+ } else
+_ISR_Enable( level );
+
+ /*
+ * Global objects with thread queue's should not be operated on from an
+ * ISR. But the sync code still must allow short timeouts to be processed
+ * correctly.
+ */
+
+ _Thread_Unblock( the_thread );
+
+#if defined(RTEMS_MULTIPROCESSING)
+ if ( !_Objects_Is_local_id( the_thread-Object.id ) )
+_Thread_MP_Free_proxy( the_thread );
+#endif
+}
+
void _Thread_blocking_operation_Cancel(
#if defined(RTEMS_DEBUG)
Thread_blocking_operation_States sync_state,
@@ -59,33 +94,5 @@ void _Thread_blocking_operation_Cancel(
}
#endif
- /*
- * The thread is not waiting on anything after this completes.
- */
- the_thread-Wait.queue = NULL;
-
- /*
- * If the sync state is timed out, this is very likely not needed.
- * But better safe than sorry when it comes to critical sections.
- */
- if ( _Watchdog_Is_active( the_thread-Timer ) ) {
-_Watchdog_Deactivate( the_thread-Timer );
-_ISR_Enable( level );
-(void) _Watchdog_Remove( the_thread-Timer );
- } else
-_ISR_Enable( level );
-
- /*
- * Global objects with thread queue's should not be operated on from an
- * ISR. But the sync code still must allow short timeouts to be processed
- * correctly.
- */
-
- _Thread_Unblock( the_thread );
-
-#if defined(RTEMS_MULTIPROCESSING)
- if ( !_Objects_Is_local_id( the_thread-Object.id ) )
-_Thread_MP_Free_proxy( the_thread );
-#endif
-
+ _Thread_blocking_operation_Finalize( the_thread, level );
}
diff --git a/cpukit/score/src/threadqdequeue.c
b/cpukit/score/src/threadqdequeue.c
index 3b55e52..d745ef2 100644
--- a/cpukit/score/src/threadqdequeue.c
+++ b/cpukit/score/src/threadqdequeue.c
@@ -70,22 +70,10 @@ Thread_Control *_Thread_queue_Dequeue(
/*
* We found a thread to unblock.
+ *
+ * NOTE: This is invoked with interrupts still disabled.
*/
- the_thread-Wait.queue = NULL;
- if ( !_Watchdog_Is_active( the_thread-Timer ) ) {
-_ISR_Enable( level );
- } else {
-_Watchdog_Deactivate( the_thread-Timer );
-_ISR_Enable( level );
-(void) _Watchdog_Remove( the_thread-Timer );
- }
-
- _Thread_Unblock( the_thread );
-
-#if defined(RTEMS_MULTIPROCESSING)
- if ( !_Objects_Is_local_id( the_thread-Object.id ) )
-_Thread_MP_Free_proxy( the_thread );
-#endif
+ _Thread_blocking_operation_Finalize(
