Queue tracing

Kernel
1

Hi!

Thank you for the amazing software. I have been using FreeRTOS for many years.

For a course I am developing I was playing around with the tracing hooks, and after spending some time with it I don’t see a ‘proper’ way of correctly tracing the fill state of a queue with the given tracing hooks.

Specifically, the hooks don’t seem to fully cover queue overrides and resets.

First, overrides:

The traceQUEUE_SEND hook is called on any sucesfull call to xQueueGenericSend, including if xCopyPosition == queueOVERWRITE:

BaseType_t xQueueGenericSend( QueueHandle_t xQueue,
                              const void * const pvItemToQueue,
                              TickType_t xTicksToWait,
                              const BaseType_t xCopyPosition )
{
    BaseType_t xEntryTimeSet = pdFALSE, xYieldRequired;
    TimeOut_t xTimeOut;
    Queue_t * const pxQueue = xQueue;
    traceENTER_xQueueGenericSend( xQueue, pvItemToQueue, xTicksToWait, xCopyPosition );
    // --- snip asserts ---
    for( ; ; )
    {
        taskENTER_CRITICAL();
        {
            // --- snip comment ---
            if( ( pxQueue->uxMessagesWaiting < pxQueue->uxLength ) || ( xCopyPosition == queueOVERWRITE ) )
            {
                traceQUEUE_SEND( pxQueue );
...

After pull request #47, the above traceQUEUE_SEND is the only place that this hook is called, so in theory it would be possible to access xCopyPosition from within the hook, but this seems hacky and fragile. But without this information, my tracer cannot determine if a traceQUEUE_SEND event increased or decreased the number of items in the queue.

I could rely on the traceENTER_xQueueGenericSend hook, but it would seem desirable to write a full tracer without having to rely on these, which would require significant logic in the
trace event decoding and create many more tracing events.

The same issue exists for the traceQUEUE_SEND_ISR hook of course.

Secondly, xQueueGenericReset does not have any tracing hooks, except for the ENTER and RETURN ones, which on their own don’t given any infromation about the sucess of the reset and would again require multiple events and tracer logic to properly decode.

First, is this assessment correct? I have looked around for other issues and solutions but have not found anything. Of course I don’t want to rule out that I have missed something obvious or that there are many other problems to tracing queue fill levels that I missed :slight_smile:

If so, is there any interest in this getting updated? I would be happy to put together a PR. It would seem fairly simple (and backwards compatible) to include a new traceQUEUE_RESET(..) hook.

The issues with traceQUEUE_SEND I am not immediately sure how to address in a backwards-compatible manner, as adding a xCopyPostion parameter would be a breaking change. Is there some preference/precedent to how this should be handled? Maybe adding a second, redundant tracing hook?

// --snip--
traceQUEUE_SEND( pxQueue );
traceQUEUE_SEND_EXT( pxQueue, xCopyPosition );
// --snip--

As mentioned, I am happy to implement this and create a PR, but thought I would first ask for feedback if there is interest and preference for how this should be done, to avoid redundant work :slight_smile:

Thank you again!
Philipp

2

I think a send cannot decrease the number of items in a queue. I guess you want to know the number of items in the queue after the send succeeds. If we move the traceQUEUE_SEND to after the data has been copied to the queue i.e. here and here, then your tracer can determine the number of items in the queue by calling uxQueueMessagesWaiting API. The same can be done for traceQUEUE_SEND_FROM_ISR.

Does the xReturn in traceRETURN_xQueueGenericReset( xReturn ); not provide this reset status?

3

Hi!

Thanks for your reply.

Sorry - that was a typo. I mean to say “tracer cannot determine if the queue size increased or remained the same after a send”.

Yes that would expose more information. If done, it would make sense to do the same for the traceQUEUE_RECEIVE (plus ISR) calls to keep it symmetrical.

It is not optimal thought, as it increases the size of the tracing messages: The tracer has no way of knowing the previous size of the queue without on-device book keeping, so it cannot directly determine if the send increased the queue size or not. Instead it would have to transmit the queue length with every queue send trace call because it may or may not have changed. If the tracer were to have access to the value of xCopyPosition it could instead emit either a queue send or queue overwrite event which only has the queue identifier as metadata, and only differ in trace event ID.

Maybe it would also simply be an acceptable solution to explicitly allow accessing xCopyPosition in the trace macro description, as is done for pxCurrentTCB in, amongst others, traceTASK_SWITCHED_IN:

Called after a task has been selected to run. pxCurrentTCB holds a pointer to the task control block of the selected task.

This would not change anything about the current tracing behavior to ensure backwards compatibility, but (I presume) allow tracers to expect this behavior to be kept going forwards and rely on it.

The challenge here would be that this documentation change does not immediately work for the traceQUEUE_SEND_FROM_ISR macro, as it is called from both xQueueGenericSendFromISR, which features an xCopyPosition variable and from xQueueGiveFromISR which does not. This could still be addressed by providing a static xCopyPosition in xQueueGiveFromISR.

Alternatively, it could make sense to introduce a separate traceQUEUE_OVERWRITE hook, which could fall back on traceQUEUE_SEND if not defined - as is done for the traceQUEUE_SET_SEND hook.

It may be useful to implement some combination of the above things.

It does, but this does not (explicitly) expose which queue is being reset, again requiring the tracer to track state. That is further complicated, because all these trace macros are called outside of a critical section, so it is not even guaranteed that the calls to traceENTER_xQueueGenericReset( xQueue, xNewQueue ) (which provides which queue is being reset) and configASSERT( xReturn != pdFAIL ); (which provides if the reset actually happened) are adjacent to each other in the trace message log.

Providing a traceQUEUE_RESET(xQueue) would be a backwards-compatible change that provides this information in a simple fashion.

I have all the above modifications locally, so if there is any interest I am happy to open one or more PRs for discussion :slight_smile:

Thank you again for you feedback!

Philipp

4

Thank you for explaining your use case. I think we can add traceQUEUE_SEND_EXT as per your proposal. The following should make it backward compatible:

#ifndef traceQUEUE_SEND_EXT
    #define traceQUEUE_SEND_EXT( pxQueue, xCopyPosition )    traceQUEUE_SEND( pxQueue )
#endif

The call to traceQUEUE_SEND( pxQueue ); can be replaced with traceQUEUE_SEND_EXT( pxQueue, xCopyPosition ); in queue.c.

Sounds good. Please raise a PR.

5

Done. See PR #1062.

Thank you for your support!