Looks OK. Need to apply to releases?
On Thu, Nov 27, 2014 at 8:43 AM, Sebastian Huber
<sebastian.hu...@embedded-brains.de> wrote:
> Bug report by Oleg Kravtsov:
>
> In rtems_bdbuf_swapout_processing() function there is the following
> lines:
>
> if (bdbuf_cache.sync_active && !transfered_buffers)
> {
>
> rtems_id sync_requester;
> rtems_bdbuf_lock_cache ();
> ...
>
> }
>
> Here access to bdbuf_cache.sync_active is not protected with anything.
> Imagine the following test case:
>
> 1. Task1 releases buffer(s) with bdbuf_release_modified() calls;
>
> 2. After a while swapout task starts and flushes all buffers;
>
> 3. In the end of that swapout flush we are before that part of code, and
> assume there is task switching (just before "if (bdbuf_cache.sync_active
> && !transfered_buffers)");
>
> 4. Some other task (with higher priority) does bdbuf_release_modified
> and rtems_bdbuf_syncdev().
>
> This task successfully gets both locks sync and pool (in
> rtems_bdbuf_syncdev() function), sets sync_active to true and starts
> waiting for RTEMS_BDBUF_TRANSFER_SYNC event with only sync lock got.
>
> 5. Task switching happens again and we are again before "if
> (bdbuf_cache.sync_active && !transfered_buffers)".
>
> As the result we check sync_active and we come inside that "if"
> statement.
>
> 6. The result is that we send RTEMS_BDBUF_TRANSFER_SYNC event! Though
> ALL modified messages of that task are not flushed yet!
>
> close #1485
> ---
> cpukit/libblock/src/bdbuf.c | 16 +++++++++++-----
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/cpukit/libblock/src/bdbuf.c b/cpukit/libblock/src/bdbuf.c
> index bae57e2..8ea8cba 100644
> --- a/cpukit/libblock/src/bdbuf.c
> +++ b/cpukit/libblock/src/bdbuf.c
> @@ -2749,10 +2749,16 @@ rtems_bdbuf_swapout_processing (unsigned long
> timer_delta,
> {
> rtems_bdbuf_swapout_worker* worker;
> bool transfered_buffers = false;
> + bool sync_active;
>
> rtems_bdbuf_lock_cache ();
>
> /*
> + * To set this to true you need the cache and the sync lock.
> + */
> + sync_active = bdbuf_cache.sync_active;
> +
> + /*
> * If a sync is active do not use a worker because the current code does
> not
> * cleaning up after. We need to know the buffers have been written when
> * syncing to release sync lock and currently worker threads do not return
> to
> @@ -2761,7 +2767,7 @@ rtems_bdbuf_swapout_processing (unsigned long
> timer_delta,
> * lock. The simplest solution is to get the main swap out task perform all
> * sync operations.
> */
> - if (bdbuf_cache.sync_active)
> + if (sync_active)
> worker = NULL;
> else
> {
> @@ -2773,14 +2779,14 @@ rtems_bdbuf_swapout_processing (unsigned long
> timer_delta,
>
> rtems_chain_initialize_empty (&transfer->bds);
> transfer->dd = BDBUF_INVALID_DEV;
> - transfer->syncing = bdbuf_cache.sync_active;
> + transfer->syncing = sync_active;
>
> /*
> * When the sync is for a device limit the sync to that device. If the sync
> * is for a buffer handle process the devices in the order on the sync
> * list. This means the dev is BDBUF_INVALID_DEV.
> */
> - if (bdbuf_cache.sync_active)
> + if (sync_active)
> transfer->dd = bdbuf_cache.sync_device;
>
> /*
> @@ -2799,7 +2805,7 @@ rtems_bdbuf_swapout_processing (unsigned long
> timer_delta,
> rtems_bdbuf_swapout_modified_processing (&transfer->dd,
> &bdbuf_cache.modified,
> &transfer->bds,
> - bdbuf_cache.sync_active,
> + sync_active,
> update_timers,
> timer_delta);
>
> @@ -2830,7 +2836,7 @@ rtems_bdbuf_swapout_processing (unsigned long
> timer_delta,
> transfered_buffers = true;
> }
>
> - if (bdbuf_cache.sync_active && !transfered_buffers)
> + if (sync_active && !transfered_buffers)
> {
> rtems_id sync_requester;
> rtems_bdbuf_lock_cache ();
> --
> 1.8.4.5
>
> _______________________________________________
> 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
