Skip to content

Commit

Permalink
dmatest: don't use set_freezable_with_signal()
Browse files Browse the repository at this point in the history
Commit 981ed70 (dmatest: make dmatest threads freezable) made
dmatest kthread use set_freezable_with_signal(); however, the
interface is scheduled to be removed in the next merge window.

The problem is that unlike userland tasks there's no default place
which handles signal pending state and it isn't clear who owns and/or
is responsible for clearing TIF_SIGPENDING.  For example, in the
current code, try_to_freeze() clears TIF_SIGPENDING but it isn't sure
whether it actually owns the TIF_SIGPENDING nor is it race-free -
ie. the task may continue to run with TIF_SIGPENDING set after the
freezable section.

Unfortunately, we don't have wait_for_completion_freezable_timeout().
This patch open codes it and uses wait_event_freezable_timeout()
instead and removes timeout reloading - wait_event_freezable_timeout()
won't return across freezing events (currently racy but fix scheduled)
and timer doesn't decrement while the task is in freezer.  Although
this does lose timer-reset-over-freezing, given that timeout is
supposed to be long enough and failure to finish inside is considered
irrecoverable, I don't think this is worth the complexity.

While at it, move completion to outer scope and explain that we're
ignoring dangling pointer problem after timeout.  This should give
slightly better chance at avoiding oops after timeout.

Signed-off-by: Tejun Heo <[email protected]>
Acked-by: Dan Williams <[email protected]>
Cc: Guennadi Liakhovetski <[email protected]>
Cc: Nicolas Ferre <[email protected]>
  • Loading branch information
htejun committed Nov 23, 2011
1 parent ec01247 commit adfa543
Showing 1 changed file with 27 additions and 19 deletions.
46 changes: 27 additions & 19 deletions drivers/dma/dmatest.c
Original file line number Diff line number Diff line change
Expand Up @@ -214,9 +214,18 @@ static unsigned int dmatest_verify(u8 **bufs, unsigned int start,
return error_count;
}

static void dmatest_callback(void *completion)
/* poor man's completion - we want to use wait_event_freezable() on it */
struct dmatest_done {
bool done;
wait_queue_head_t *wait;
};

static void dmatest_callback(void *arg)
{
complete(completion);
struct dmatest_done *done = arg;

done->done = true;
wake_up_all(done->wait);
}

/*
Expand All @@ -235,7 +244,9 @@ static void dmatest_callback(void *completion)
*/
static int dmatest_func(void *data)
{
DECLARE_WAIT_QUEUE_HEAD_ONSTACK(done_wait);
struct dmatest_thread *thread = data;
struct dmatest_done done = { .wait = &done_wait };
struct dma_chan *chan;
const char *thread_name;
unsigned int src_off, dst_off, len;
Expand All @@ -252,7 +263,7 @@ static int dmatest_func(void *data)
int i;

thread_name = current->comm;
set_freezable_with_signal();
set_freezable();

ret = -ENOMEM;

Expand Down Expand Up @@ -306,9 +317,6 @@ static int dmatest_func(void *data)
struct dma_async_tx_descriptor *tx = NULL;
dma_addr_t dma_srcs[src_cnt];
dma_addr_t dma_dsts[dst_cnt];
struct completion cmp;
unsigned long start, tmo, end = 0 /* compiler... */;
bool reload = true;
u8 align = 0;

total_tests++;
Expand Down Expand Up @@ -391,9 +399,9 @@ static int dmatest_func(void *data)
continue;
}

init_completion(&cmp);
done.done = false;
tx->callback = dmatest_callback;
tx->callback_param = &cmp;
tx->callback_param = &done;
cookie = tx->tx_submit(tx);

if (dma_submit_error(cookie)) {
Expand All @@ -407,20 +415,20 @@ static int dmatest_func(void *data)
}
dma_async_issue_pending(chan);

do {
start = jiffies;
if (reload)
end = start + msecs_to_jiffies(timeout);
else if (end <= start)
end = start + 1;
tmo = wait_for_completion_interruptible_timeout(&cmp,
end - start);
reload = try_to_freeze();
} while (tmo == -ERESTARTSYS);
wait_event_freezable_timeout(done_wait, done.done,
msecs_to_jiffies(timeout));

status = dma_async_is_tx_complete(chan, cookie, NULL, NULL);

if (tmo == 0) {
if (!done.done) {
/*
* We're leaving the timed out dma operation with
* dangling pointer to done_wait. To make this
* correct, we'll need to allocate wait_done for
* each test iteration and perform "who's gonna
* free it this time?" dancing. For now, just
* leave it dangling.
*/
pr_warning("%s: #%u: test timed out\n",
thread_name, total_tests - 1);
failed_tests++;
Expand Down

0 comments on commit adfa543

Please sign in to comment.