Hi,
While updating libosmocore in osmocom-bb, a problem arised: 100%CPU usage and hanging. Turns out 'mobile' sometimes call osmo_timer_schedule for an already scheduled timer.
Obviously this used to work and now it doesn't. The question is : Was there a defined semantics for this case previously ? Or is it supposed to be unsupported ? What would you expect it to do ?
Cheers,
Sylvain
Hi all,
On Fri, Nov 11, 2011 at 03:56:21PM +0100, Sylvain Munaut wrote:
While updating libosmocore in osmocom-bb, a problem arised: 100%CPU usage and hanging. Turns out 'mobile' sometimes call osmo_timer_schedule for an already scheduled timer.
Obviously this used to work and now it doesn't. The question is : Was there a defined semantics for this case previously ? Or is it supposed to be unsupported ? What would you expect it to do ?
I think a second/follow-up timer_schedule should re-schedule the existing timer safely.
Regards, Harald
On Fri, Nov 11, 2011 at 06:50:31PM +0100, Harald Welte wrote:
Hi all,
On Fri, Nov 11, 2011 at 03:56:21PM +0100, Sylvain Munaut wrote:
While updating libosmocore in osmocom-bb, a problem arised: 100%CPU usage and hanging. Turns out 'mobile' sometimes call osmo_timer_schedule for an already scheduled timer.
Obviously this used to work and now it doesn't. The question is : Was there a defined semantics for this case previously ? Or is it supposed to be unsupported ? What would you expect it to do ?
I think a second/follow-up timer_schedule should re-schedule the existing timer safely.
The patch attached should recover the former behaviour, sorry for missing this case.
The patch attached should recover the former behaviour, sorry for missing this case.
Oops, sorry, we actually continued on IRC.
Look at the first few commits of http://cgit.osmocom.org/cgit/libosmocore/log/?h=sylvain/for_jolly
They bring in some fixes/cleanups from the kernel for rbtree then something that fixes the timer. This patch keeps the list head intact and just remove the timer from the tree and re-adds it at the right place.
Cheers,
Sylvain
On Sat, Nov 12, 2011 at 11:07:03PM +0100, Sylvain Munaut wrote:
The patch attached should recover the former behaviour, sorry for missing this case.
Oops, sorry, we actually continued on IRC.
Look at the first few commits of http://cgit.osmocom.org/cgit/libosmocore/log/?h=sylvain/for_jolly
They bring in some fixes/cleanups from the kernel for rbtree then something that fixes the timer. This patch keeps the list head intact and just remove the timer from the tree and re-adds it at the right place.
Hm, I think I found one weird scenario which is not handled fine with your patch.
Say you have two timers A and B. Assume both expire on the same time (and they are inserted in the eviction list in this order, first A, then B). Then, A re-schedules B in its callback. B is reinserted in the tree, however, B is still in the eviction list.
With aeeb7070f84437aa608a3d843346b1efa916d175, since we don't touch the list head. B will be removed from the tree, but I think it should remain there as it has been rescheduled.
Yes, this scenario is strange, but I think we try to cover all possible situations. So I think my patch should be applied instead.
Thanks for collecting the patches that went into rbtree.c in the linux kernel, btw.
Hm, I think I found one weird scenario which is not handled fine with your patch.
Say you have two timers A and B. Assume both expire on the same time (and they are inserted in the eviction list in this order, first A, then B). Then, A re-schedules B in its callback. B is reinserted in the tree, however, B is still in the eviction list.
With aeeb7070f84437aa608a3d843346b1efa916d175, since we don't touch the list head. B will be removed from the tree, but I think it should remain there as it has been rescheduled.
Yes, this scenario is strange, but I think we try to cover all possible situations. So I think my patch should be applied instead.
You're right. Even worse, with my patch during the scan of the eviction list, we'll call the new B callback with the new B data at the old time.
Thanks for collecting the patches that went into rbtree.c in the linux kernel, btw.
fyi, I skipped the few last ones that add a new "augmented rb tree" functionality that we don't need (yet?).
Cheers,
Sylvain
Hi Sylvain,
On Sun, Nov 13, 2011 at 10:07:58AM +0100, Sylvain Munaut wrote: [...]
Thanks for collecting the patches that went into rbtree.c in the linux kernel, btw.
fyi, I skipped the few last ones that add a new "augmented rb tree" functionality that we don't need (yet?).
That skipping is fine to me, we can get it if we ever need it.
-
Harald Welte -
Pablo Neira Ayuso -
Sylvain Munaut