FreeRTOS Support Archive
The FreeRTOS support forum is used to obtain active support directly from Real
Time Engineers Ltd. In return for using our top quality software and services for
free, we request you play fair and do your bit to help others too! Sign up
to receive notifications of new support topics then help where you can.
This is a read only archive of threads posted to the FreeRTOS support forum.
The archive is updated every week, so will not always contain the very latest posts.
Use these archive pages to search previous posts. Use the Live FreeRTOS Forum
link to reply to a post, or start a new support thread.
[FreeRTOS Home] [Live FreeRTOS Forum] [FAQ] [Archive Top] [February 2015 Threads] Should prvNotifyQueueSetContainer check if the queue is locked before editing event lists?Posted by cman12345 on February 19, 2015 In queue.c, I noticed the following description about prvLock/UnlockQueue:
~~~~~
/*
* Unlocks a queue locked by a call to prvLockQueue. Locking a queue does not
* prevent an ISR from adding or removing items to the queue, but does prevent
* an ISR from removing tasks from the queue event lists. If an ISR finds a
* queue is locked it will instead increment the appropriate queue lock count
* to indicate that a task may require unblocking. When the queue in unlocked
* these lock counts are inspected, and the appropriate action taken.
*/
static void prvUnlockQueue( Queuet * const pxQueue ) PRIVILEGEDFUNCTION;
~~~~~
Does that mean it's dangerous to edit the queue event list while running from an ISR?
If so, I'm trying to understand why there is no check to see if the queue is locked in prvNotifyQueueSetContainer where the following path can be reached:
xQueueGenericSendFromISR() -> prvNotifyQueueSetContainer() -> xTaskRemoveFromEventList()
Thanks for any help!
Should prvNotifyQueueSetContainer check if the queue is locked before editing event lists?Posted by rtel on February 19, 2015 I've been looking at this for the last half hour, and on first inspection you could be onto something here. From the scenarios I have drawn out it does look like a race condition could occur. Also I'm running the coverage tests as I type and we don't seem to hit that condition. It will take some time to investigate further - but if there is an issue it can be corrected with a simple update that checks the queue lock before calling prvNotifyQueueSetContainer(), and does nothing but increment the lock if it is found that the lock is set.
Regards.
Should prvNotifyQueueSetContainer check if the queue is locked before editing event lists?Posted by rtel on February 19, 2015 It look to me like the code should be like the following (test markets and asserts removed for clarity). Do you agree:
EDIT - the formatting was unreadable so instead I have attached a small file that contains the update.
Regards.
Should prvNotifyQueueSetContainer check if the queue is locked before editing event lists?Posted by cman12345 on February 19, 2015 Yes, that aligns with what I would expect. Convention wise, I suppose a prvNotifyQueueSetContainerFromISR() routine that does this would line up with how the other code is organized.
Looking closer, I think this patch is definitely needed because otherwise you would have race conditions in xQueueGenericReceive if you are waiting on a queue set and xQueueGenericSendFromISR is called on a queue that is part of the queue set. In this case, the task list could get edited incorrectly.
Thanks for the response, much appreciated!
Should prvNotifyQueueSetContainer check if the queue is locked before editing event lists?Posted by rtel on February 19, 2015 Yes - that is the conclusion I came to too. The code has been updated
but is still running tests so not checked in yet. However the tests are
not hitting that line - so highlighting an issue in the tests.
Regards.
Copyright (C) Amazon Web Services, Inc. or its affiliates. All rights reserved.
|