When mailbox controller provides two or more channels and
they are actively used by mailbox client(s) it's very easy
to trigger the warning in hrtimer_forward():
[ 247.853060] WARNING: CPU: 6 PID: 0 at kernel/time/hrtimer.c:805 hrtimer_forward+0x88/0xd8
[ 247.853549] Modules linked in:
[ 247.853907] CPU: 6 PID: 0 Comm: swapper/6 Tainted: G W 4.11.0-rc2-00362-g93afaa4513bb-dirty #13
[ 247.854472] Hardware name: linux,dummy-virt (DT)
[ 247.854699] task: ffff80001d89d780 task.stack: ffff80001d8c4000
[ 247.854999] PC is at hrtimer_forward+0x88/0xd8
[ 247.855280] LR is at txdone_hrtimer+0xd4/0xf8
[ 247.855551] pc : [<ffff0000081039f0>] lr : [<ffff00000881b874>] pstate: 200001c5
[ 247.855857] sp : ffff80001efbdeb0
[ 247.856072] x29: ffff80001efbdeb0 x28: ffff80001efc3140
[ 247.856358] x27: ffff00000881b7a0 x26: 00000039ac93e8b6
[ 247.856604] x25: ffff000008e756be x24: ffff80001c4a1348
[ 247.856882] x23: 0000000000000001 x22: 00000000000000f8
[ 247.857189] x21: ffff80001c4a1318 x20: ffff80001d327110
[ 247.857509] x19: 00000000000f4240 x18: 0000000000000030
[ 247.857808] x17: 0000ffffaecdf370 x16: ffff0000081ccc80
[ 247.858000] x15: 0000000000000010 x14: 00000000fffffff0
[ 247.858186] x13: ffff000008f488e0 x12: 000000000002e3eb
[ 247.858381] x11: ffff000008979690 x10: 0000000000000000
[ 247.858573] x9 : 0000000000000001 x8 : ffff80001efc66e0
[ 247.858758] x7 : ffff80001efc6708 x6 : 00000005be7732f2
[ 247.858943] x5 : 0000000000000001 x4 : ffff80001c4a1348
[ 247.859130] x3 : 00000039ac94952a x2 : 00000000000f4240
[ 247.859315] x1 : 00000039ac98243c x0 : 0000000000038f12
[ 247.859582] ---[ end trace d61812426ec3c30b ]---
To fix this current patch migrates hr timers to be per-channel
instead of using only one timer per-controller.
The racy reading of chan->active_req is removed from timer callback
since it's not done under spinlock and it seems that timer-based
polling logic shouldn't rely on this. Timer is started on the channel
when new message is submitted to controller and timer will continue
to reschedule itself until it detects that controller acked a message
by using ->last_tx_done(), after acknowledge from controller timer
callback will trigger mailbox tx state machine.
Signed-off-by: Alexey Klimov <[email protected]>
---
Hi Jassi and Sudeep,
could you please take a look at this?
The only thing that I don't know how to fix here is that
if controller reports timers-based polling and client supports
acknowledgement of message transfer then the scenario looks like
it theoretically possible to call tx_tick() from two
points: from timer callback and from a client. This is setup
in mbox_request_channel() in such lines:
if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone)
chan->txdone_method |= TXDONE_BY_ACK;
Thanks,
Alexey
drivers/mailbox/mailbox.c | 45 ++++++++++++++++++--------------------
include/linux/mailbox_controller.h | 2 +-
2 files changed, 22 insertions(+), 25 deletions(-)
diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index 4671f8a12872..124d2a64de83 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -87,7 +87,7 @@ static void msg_submit(struct mbox_chan *chan)
if (!err && (chan->txdone_method & TXDONE_BY_POLL))
/* kick start the timer immediately to avoid delays */
- hrtimer_start(&chan->mbox->poll_hrt, 0, HRTIMER_MODE_REL);
+ hrtimer_start(&chan->poll_hrt, 0, HRTIMER_MODE_REL);
}
static void tx_tick(struct mbox_chan *chan, int r)
@@ -113,25 +113,21 @@ static void tx_tick(struct mbox_chan *chan, int r)
static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer)
{
- struct mbox_controller *mbox =
- container_of(hrtimer, struct mbox_controller, poll_hrt);
+ struct mbox_chan *chan =
+ container_of(hrtimer, struct mbox_chan, poll_hrt);
bool txdone, resched = false;
- int i;
-
- for (i = 0; i < mbox->num_chans; i++) {
- struct mbox_chan *chan = &mbox->chans[i];
- if (chan->active_req && chan->cl) {
- txdone = chan->mbox->ops->last_tx_done(chan);
- if (txdone)
- tx_tick(chan, 0);
- else
- resched = true;
- }
+ if (chan->cl) {
+ txdone = chan->mbox->ops->last_tx_done(chan);
+ if (txdone)
+ tx_tick(chan, 0);
+ else
+ resched = true;
}
if (resched) {
- hrtimer_forward_now(hrtimer, ms_to_ktime(mbox->txpoll_period));
+ hrtimer_forward_now(hrtimer,
+ ms_to_ktime(chan->mbox->txpoll_period));
return HRTIMER_RESTART;
}
return HRTIMER_NORESTART;
@@ -350,6 +346,12 @@ struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index)
if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone)
chan->txdone_method |= TXDONE_BY_ACK;
+ if (chan->txdone_method == TXDONE_BY_POLL) {
+ hrtimer_init(&chan->poll_hrt, CLOCK_MONOTONIC,
+ HRTIMER_MODE_REL);
+ chan->poll_hrt.function = txdone_hrtimer;
+ }
+
spin_unlock_irqrestore(&chan->lock, flags);
ret = chan->mbox->ops->startup(chan);
@@ -411,6 +413,10 @@ void mbox_free_channel(struct mbox_chan *chan)
spin_lock_irqsave(&chan->lock, flags);
chan->cl = NULL;
chan->active_req = NULL;
+
+ if (chan->txdone_method == TXDONE_BY_POLL)
+ hrtimer_cancel(&chan->poll_hrt);
+
if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK))
chan->txdone_method = TXDONE_BY_POLL;
@@ -452,12 +458,6 @@ int mbox_controller_register(struct mbox_controller *mbox)
else /* It has to be ACK then */
txdone = TXDONE_BY_ACK;
- if (txdone == TXDONE_BY_POLL) {
- hrtimer_init(&mbox->poll_hrt, CLOCK_MONOTONIC,
- HRTIMER_MODE_REL);
- mbox->poll_hrt.function = txdone_hrtimer;
- }
-
for (i = 0; i < mbox->num_chans; i++) {
struct mbox_chan *chan = &mbox->chans[i];
@@ -496,9 +496,6 @@ void mbox_controller_unregister(struct mbox_controller *mbox)
for (i = 0; i < mbox->num_chans; i++)
mbox_free_channel(&mbox->chans[i]);
- if (mbox->txdone_poll)
- hrtimer_cancel(&mbox->poll_hrt);
-
mutex_unlock(&con_mutex);
}
EXPORT_SYMBOL_GPL(mbox_controller_unregister);
diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h
index 74deadb42d76..f5e5a6e8ccfc 100644
--- a/include/linux/mailbox_controller.h
+++ b/include/linux/mailbox_controller.h
@@ -82,7 +82,6 @@ struct mbox_controller {
struct mbox_chan *(*of_xlate)(struct mbox_controller *mbox,
const struct of_phandle_args *sp);
/* Internal to API */
- struct hrtimer poll_hrt;
struct list_head node;
};
@@ -123,6 +122,7 @@ struct mbox_chan {
unsigned msg_count, msg_free;
void *msg_data[MBOX_TX_QUEUE_LEN];
spinlock_t lock; /* Serialise access to the channel */
+ struct hrtimer poll_hrt;
void *con_priv;
};
--
2.11.0
On Thu, Apr 6, 2017 at 11:01 PM, Alexey Klimov <[email protected]> wrote:
> When mailbox controller provides two or more channels and
> they are actively used by mailbox client(s) it's very easy
> to trigger the warning in hrtimer_forward():
>
> [ 247.853060] WARNING: CPU: 6 PID: 0 at kernel/time/hrtimer.c:805 hrtimer_forward+0x88/0xd8
> [ 247.853549] Modules linked in:
> [ 247.853907] CPU: 6 PID: 0 Comm: swapper/6 Tainted: G W 4.11.0-rc2-00362-g93afaa4513bb-dirty #13
> [ 247.854472] Hardware name: linux,dummy-virt (DT)
> [ 247.854699] task: ffff80001d89d780 task.stack: ffff80001d8c4000
> [ 247.854999] PC is at hrtimer_forward+0x88/0xd8
> [ 247.855280] LR is at txdone_hrtimer+0xd4/0xf8
> [ 247.855551] pc : [<ffff0000081039f0>] lr : [<ffff00000881b874>] pstate: 200001c5
> [ 247.855857] sp : ffff80001efbdeb0
> [ 247.856072] x29: ffff80001efbdeb0 x28: ffff80001efc3140
> [ 247.856358] x27: ffff00000881b7a0 x26: 00000039ac93e8b6
> [ 247.856604] x25: ffff000008e756be x24: ffff80001c4a1348
> [ 247.856882] x23: 0000000000000001 x22: 00000000000000f8
> [ 247.857189] x21: ffff80001c4a1318 x20: ffff80001d327110
> [ 247.857509] x19: 00000000000f4240 x18: 0000000000000030
> [ 247.857808] x17: 0000ffffaecdf370 x16: ffff0000081ccc80
> [ 247.858000] x15: 0000000000000010 x14: 00000000fffffff0
> [ 247.858186] x13: ffff000008f488e0 x12: 000000000002e3eb
> [ 247.858381] x11: ffff000008979690 x10: 0000000000000000
> [ 247.858573] x9 : 0000000000000001 x8 : ffff80001efc66e0
> [ 247.858758] x7 : ffff80001efc6708 x6 : 00000005be7732f2
> [ 247.858943] x5 : 0000000000000001 x4 : ffff80001c4a1348
> [ 247.859130] x3 : 00000039ac94952a x2 : 00000000000f4240
> [ 247.859315] x1 : 00000039ac98243c x0 : 0000000000038f12
> [ 247.859582] ---[ end trace d61812426ec3c30b ]---
>
> To fix this current patch migrates hr timers to be per-channel
> instead of using only one timer per-controller.
>
I think we can do by just checking if hrtimer_active() returns false
before we do hrtimer_start() in msg_submit() ?
Thanks.
On Fri, Apr 07, 2017 at 08:39:35PM +0530, Jassi Brar wrote:
> On Thu, Apr 6, 2017 at 11:01 PM, Alexey Klimov <[email protected]> wrote:
> > When mailbox controller provides two or more channels and
> > they are actively used by mailbox client(s) it's very easy
> > to trigger the warning in hrtimer_forward():
> >
> > [ 247.853060] WARNING: CPU: 6 PID: 0 at kernel/time/hrtimer.c:805 hrtimer_forward+0x88/0xd8
> > [ 247.853549] Modules linked in:
> > [ 247.853907] CPU: 6 PID: 0 Comm: swapper/6 Tainted: G W 4.11.0-rc2-00362-g93afaa4513bb-dirty #13
> > [ 247.854472] Hardware name: linux,dummy-virt (DT)
> > [ 247.854699] task: ffff80001d89d780 task.stack: ffff80001d8c4000
> > [ 247.854999] PC is at hrtimer_forward+0x88/0xd8
> > [ 247.855280] LR is at txdone_hrtimer+0xd4/0xf8
> > [ 247.855551] pc : [<ffff0000081039f0>] lr : [<ffff00000881b874>] pstate: 200001c5
> > [ 247.855857] sp : ffff80001efbdeb0
> > [ 247.856072] x29: ffff80001efbdeb0 x28: ffff80001efc3140
> > [ 247.856358] x27: ffff00000881b7a0 x26: 00000039ac93e8b6
> > [ 247.856604] x25: ffff000008e756be x24: ffff80001c4a1348
> > [ 247.856882] x23: 0000000000000001 x22: 00000000000000f8
> > [ 247.857189] x21: ffff80001c4a1318 x20: ffff80001d327110
> > [ 247.857509] x19: 00000000000f4240 x18: 0000000000000030
> > [ 247.857808] x17: 0000ffffaecdf370 x16: ffff0000081ccc80
> > [ 247.858000] x15: 0000000000000010 x14: 00000000fffffff0
> > [ 247.858186] x13: ffff000008f488e0 x12: 000000000002e3eb
> > [ 247.858381] x11: ffff000008979690 x10: 0000000000000000
> > [ 247.858573] x9 : 0000000000000001 x8 : ffff80001efc66e0
> > [ 247.858758] x7 : ffff80001efc6708 x6 : 00000005be7732f2
> > [ 247.858943] x5 : 0000000000000001 x4 : ffff80001c4a1348
> > [ 247.859130] x3 : 00000039ac94952a x2 : 00000000000f4240
> > [ 247.859315] x1 : 00000039ac98243c x0 : 0000000000038f12
> > [ 247.859582] ---[ end trace d61812426ec3c30b ]---
> >
> > To fix this current patch migrates hr timers to be per-channel
> > instead of using only one timer per-controller.
> >
> I think we can do by just checking if hrtimer_active() returns false
> before we do hrtimer_start() in msg_submit() ?
It looks like it can be easily broken:
1) let's say first thread executes timer callback and already checked last_tx_done
on channel 0;
2) second thread submits a message to the controller, say, on channel 0 and with
help of hrtimer_active() observes that the timer is active (because timer callback
is running) and decides not to (re-)start timer;
After this first thread decides not to restart the timer and finishes callback.
The thing that first thread executes tx_tick isn't helpful: for example first
thread may have no messages to submit on any channel and therefore is not going
to deal with timer.
Finally, mailbox state machine is stalled. Second thread thinks that timer is
active while it's not.
One of the main questions is that there is only one timer per few channels
in current code.
Thanks,
Alexey.
On 11 April 2017 at 18:04, Alexey Klimov <[email protected]> wrote:
> On Fri, Apr 07, 2017 at 08:39:35PM +0530, Jassi Brar wrote:
>> On Thu, Apr 6, 2017 at 11:01 PM, Alexey Klimov <[email protected]> wrote:
>> > When mailbox controller provides two or more channels and
>> > they are actively used by mailbox client(s) it's very easy
>> > to trigger the warning in hrtimer_forward():
>> >
>> > [ 247.853060] WARNING: CPU: 6 PID: 0 at kernel/time/hrtimer.c:805 hrtimer_forward+0x88/0xd8
>> > [ 247.853549] Modules linked in:
>> > [ 247.853907] CPU: 6 PID: 0 Comm: swapper/6 Tainted: G W 4.11.0-rc2-00362-g93afaa4513bb-dirty #13
>> > [ 247.854472] Hardware name: linux,dummy-virt (DT)
>> > [ 247.854699] task: ffff80001d89d780 task.stack: ffff80001d8c4000
>> > [ 247.854999] PC is at hrtimer_forward+0x88/0xd8
>> > [ 247.855280] LR is at txdone_hrtimer+0xd4/0xf8
>> > [ 247.855551] pc : [<ffff0000081039f0>] lr : [<ffff00000881b874>] pstate: 200001c5
>> > [ 247.855857] sp : ffff80001efbdeb0
>> > [ 247.856072] x29: ffff80001efbdeb0 x28: ffff80001efc3140
>> > [ 247.856358] x27: ffff00000881b7a0 x26: 00000039ac93e8b6
>> > [ 247.856604] x25: ffff000008e756be x24: ffff80001c4a1348
>> > [ 247.856882] x23: 0000000000000001 x22: 00000000000000f8
>> > [ 247.857189] x21: ffff80001c4a1318 x20: ffff80001d327110
>> > [ 247.857509] x19: 00000000000f4240 x18: 0000000000000030
>> > [ 247.857808] x17: 0000ffffaecdf370 x16: ffff0000081ccc80
>> > [ 247.858000] x15: 0000000000000010 x14: 00000000fffffff0
>> > [ 247.858186] x13: ffff000008f488e0 x12: 000000000002e3eb
>> > [ 247.858381] x11: ffff000008979690 x10: 0000000000000000
>> > [ 247.858573] x9 : 0000000000000001 x8 : ffff80001efc66e0
>> > [ 247.858758] x7 : ffff80001efc6708 x6 : 00000005be7732f2
>> > [ 247.858943] x5 : 0000000000000001 x4 : ffff80001c4a1348
>> > [ 247.859130] x3 : 00000039ac94952a x2 : 00000000000f4240
>> > [ 247.859315] x1 : 00000039ac98243c x0 : 0000000000038f12
>> > [ 247.859582] ---[ end trace d61812426ec3c30b ]---
>> >
>> > To fix this current patch migrates hr timers to be per-channel
>> > instead of using only one timer per-controller.
>> >
>> I think we can do by just checking if hrtimer_active() returns false
>> before we do hrtimer_start() in msg_submit() ?
>
> It looks like it can be easily broken:
>
> 1) let's say first thread executes timer callback and already checked last_tx_done
> on channel 0;
> 2) second thread submits a message to the controller, say, on channel 0 and with
> help of hrtimer_active() observes that the timer is active (because timer callback
> is running) and decides not to (re-)start timer;
>
> After this first thread decides not to restart the timer and finishes callback.
> The thing that first thread executes tx_tick isn't helpful: for example first
> thread may have no messages to submit on any channel and therefore is not going
> to deal with timer.
>
> Finally, mailbox state machine is stalled. Second thread thinks that timer is
> active while it's not.
>
... you mean race :) and we have locks for that. You want me to send
in a patch?
> One of the main questions is that there is only one timer per few channels
> in current code.
>
I see that as a good thing because
a) Polling anyway doesn't provide 'hard' guarantee even if we have one
timer per channel
b) The poll period remains same for every channel, so functionality
wise you only increase timer callbacks.
thanks