Misc improvements #1245
Open
Misc improvements #1245
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
On iiod_io_cancel(), io->write_token is protected by the iiod_responder lock while on iiod_io_wait_for_command_done() was using the iiod_io lock. That is surely not fine, so always use the iiod_responder lock to protect io->write_token. Also, make sure the NULL check in iiod_enqueue_command() is properly locked. Signed-off-by: Nuno Sá <nuno.sa@analog.com>
We need to completely synchronize handle_free_blocks() with operations that might enqueue them in another task (like handle_transfer_block()). Otherwise we can race in a way that a block is freed but it will still be enqueued in another task which will lead to a user after free. For doing this we add get_iio_block_unlocked() and lock around it so we can synchronize beyond getting the block. Note that this still allows for a block to be enqueued in a task and then freed while in use by that task. But it does not allow for a block to be freed __while__ being enqueued which will allow for a future fix where we can properly sync tokens before freeing the block. Signed-off-by: Nuno Sá <nuno.sa@analog.com>
We were getting the buffer lock in get_iio_block(), releasing it and then grabbing it again. It does not make sense so let's make use of get_iio_block_unlocked() and grab the lock for the whole time. Also, there's no point in going two times over the buffer list of blocks. Just pass the buffer_entry pointer inside get_iio_block_unlocked(). Signed-off-by: Nuno Sá <nuno.sa@analog.com>
By default, the struct iio_task_token token is destroyed after syncing the task. In preparation for adding tokens with no bounded lifetimes (meaning that outer callers can create and destroy tokens as needed), split iio_task_sync() so we can control if the token is automatically destroyed or not. Signed-off-by: Nuno Sá <nuno.sa@analog.com>
In preparation for adding support for creating and destroying struct iio_task_token tokens with full control of their lifetimes, add an helper function to find tokens in the task list as that code will latter be reused outside of iio_task_cancel(). Signed-off-by: Nuno Sá <nuno.sa@analog.com>
This change is also in preparation for adding support for creating and destroying struct iio_task_token tokens with full control of their lifetimes. Hence, iio_task_token_do_enqueue() is added so that we can reuse the code that adds a token to a task (and better control if it's an existing vs a new token). Signed-off-by: Nuno Sá <nuno.sa@analog.com>
This adds APIs to create, enqueue and destroy (destroying just turns the already existing function public) struct iio_task_token tokens. This will be important in fixing a race between freeing IIO blocks and enqueue/dequeueing them since with this, we will be able to properly sync on the tokens. Signed-off-by: Nuno Sá <nuno.sa@analog.com>
Normally, iio_task_cancel() is combined with __iio_task_sync(). Hence let's add an helper that combines these two calls. Note that it will automatically call __iio_task_sync() with token_destroy set to false. Signed-off-by: Nuno Sá <nuno.sa@analog.com>
IIOD was often crashing during client removal when buffering. The reason was that blocks could be enqueued in the (d)enqueue tasks, then freed and only then the tasks would process the already freed block. In order to fix this (and now that we have no possible race between handle_free_block() and handle_transfer_block()), we need to sync on the (d)enqueue block tokens before freeing it. Like this we can be sure that the block won't be used after it is freed. Signed-off-by: Nuno Sá <nuno.sa@analog.com>
Make sure to dequeue any possible in flight block before destroying it. Note that if the block is not enqueued, this returns an error but we really don't care about it. Making sure the block is dequeued makes for a much cleaner exit specially in cases where freeing the blocks is deferred to the default client. Signed-off-by: Nuno Sá <nuno.sa@analog.com>
When destroying the stream, we'll try to dequeue (as it improves things on iiod side) any possible pending block and if the buffer is already cancelled we'll just timeout and make exiting the app much slower. Hence, let's try a clean exit first and if we get blocked a second hit on ctrl-c will go and cancel the buffer as before. Signed-off-by: Nuno Sá <nuno.sa@analog.com>
nunojsa
requested review from
cristina-suteu,
dNechita and
mhennerich
as code owners
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Description
This improves iio_rwdev exit and is a combination of two things:
So this is really a combination of the above two points which improves things a lot when cancelling a buffer session with iio_rwdev.
Note this is only tested on linux so it would be great if someone could do a quick test on windows.
PR Type
PR Checklist