Browse code

- shoot-yourself-in-the-foot bug reporting and workarround (time_del(self) in a timer handle) - added timer_allow_del() - use with care - updated timer docs - tm: uses timer_allow_del() in fr (as a safeguard)

Andrei Pelinescu-Onciul authored on 12/12/2005 20:27:16
Showing 4 changed files
... ...
@@ -114,7 +114,8 @@ WARNING: do not initialize/re-initialize a running timer!
114 114
 
115 115
 To remove a timer from the active timer list call timer_del(struct timer_ln*).
116 116
 timer_del is the slowest of all the timer functions. If you are trying to delete a timer whose timer is executing. timer_del will wait until it finishes.
117
-WARNING: if you have an one shot timer that frees its memory before exiting, make sure you don't call timer_del afterwards (e.g. use some reference counters and free the memory only if the counter is 0).
117
+WARNING: - avoid deleting your own timer from its timer handle (if you try it, you'll get a BUG message in the log). If you _must_ have this, you have a broken design. If you still want it, look at  timer_allow_del().
118
+         - if you have an one shot timer that frees its memory before exiting, make sure you don't call timer_del afterwards (e.g. use some reference counters and free the memory only if the counter is 0).
118 119
 
119 120
 Race example (using the struct foo defined above):
120 121
 
... ...
@@ -139,14 +140,22 @@ timer_del(&f->timer); /* if the timer is already expired => f is already
139 139
 
140 140
 The above timer_del/free_in_one_shot_timer race example is very simple, but consider that you can have much more complex scenarios, when timer_del can be called from different processes on some asynchronous events. If this looks like you're intended usage, make sure you use some  reference counters or some other protection mechanism to avoid the above race.
141 141
 
142
+4.5 timer_allow_del
143
+-------------------
142 144
 
143
-4.5 Getting the ticks value
145
+Marks a timer as "to be deleted when the handler ends", usefull when the timer handler knows it won't prolong the timer anymore (it will return 0) and will do some time consuming work. Calling this function will cause simultaneous timer_dels to return immediately (they won't  wait anymore for the timer handle to finish). It will also allow  self-deleting from the timer handle without logging a bug.
146
+ WARNING: - if you rely on timer_del to know when the timer handle execution finishes (e.g. to free resources used in the timer handle), don't use this function.
147
+          - call this function only if this is your last timer handle run (the timer handle will return 0)
148
+          - this function can be called only from a timer handle (in timer context), all other calls will have no effect and will log a bug message
149
+
150
+
151
+4.6 Getting the ticks value
144 152
 ----------------------------
145 153
 
146 154
 If you need the current ticks value you can get with get_ticks_raw().
147 155
 WARNING: don't use get_ticks(). get_ticks() returns the number of ticks converted to seconds and it was kept only for compatibility reasons with the existing code.
148 156
 
149
-4.6 Conversion
157
+4.7 Conversion
150 158
 ---------------
151 159
 
152 160
 To convert between ticks & time and viceversa, include timer_ticks.h and use
... ...
@@ -161,12 +170,20 @@ TICKS_TO_MS(t)  /* converts from ticks to milliseconds, can overflow for
161 161
 TICKS_TO_S(t)  /* converts from ticks to s, rounded down */
162 162
 
163 163
 
164
-4.7 Backward compatibility
164
+4.8 Backward compatibility
165 165
 --------------------------
166 166
 
167 167
 The old  register_timer and get_ticks() are still supported for backward compatibility. This means that you don't have to change your existing working code.
168 168
 
169 169
 
170
+5.0 Debugging
171
+-------------
172
+
173
+The timers have built-in debugging information. To activate it you only need to define TIMER_DEBUG (recompile ser with make CC_EXTRA_OPTS=-DTIMER_DEBUG all).
174
+The timer debug log level is by default L_WARN. [ FIXME: add it as script option]
175
+TIMER_DEBUG enables extra sanity checks and it will log a lot of information
176
+ (like the caller of timer_* functions, where a timer was added a.s.o).
177
+
170 178
 
171 179
 [Todo]:
172 180
 - SLOW, DRIFT, RESYNC, FREQUENCY
... ...
@@ -99,6 +99,8 @@
99 99
  *  2004-02-13  t->is_invite, t->local, t->noisy_ctimer replaced;
100 100
  *              timer_link.payload removed (bogdan)
101 101
  *  2005-10-03  almost completely rewritten to use the new timers (andrei)
102
+ *  2005-12-12  on final response marked the rb as removed to avoid deleting
103
+ *              it from the timer handle; timer_allow_del()  (andrei)
102 104
  */
103 105
 
104 106
 #include "defs.h"
... ...
@@ -423,6 +425,8 @@ ticks_t retr_buf_handler(ticks_t ticks, struct timer_ln* tl, void *p)
423 423
 							 (both timers disabled)
424 424
 							  a little race risk, but
425 425
 							  nothing bad would happen */
426
+		timer_allow_del(); /* [optional] allow timer_dels, since we're done
427
+							  and there is no race risk */
426 428
 		final_response_handler(rbuf, t);
427 429
 		return 0;
428 430
 	}else{
... ...
@@ -29,6 +29,8 @@
29 29
  *  2003-03-19  replaced all the mallocs/frees w/ pkg_malloc/pkg_free (andrei)
30 30
  *  2003-03-29  cleaning pkg_mallocs introduced (jiri)
31 31
  *  2005-07-27  complete re-design/re-implementation (andrei)
32
+ *  2005-12-12  workarround & bug reporting for timer_del(self) called from
33
+ *              a timer handle; added timer_allow_del()  (andrei)
32 34
  */
33 35
 
34 36
 
... ...
@@ -80,6 +82,9 @@ static int timer_id=0;
80 80
 
81 81
 static gen_lock_t* timer_lock=0;
82 82
 static struct timer_ln* volatile* running_timer=0;/* running timer handler */
83
+static int in_timer=0;
84
+
85
+#define IS_IN_TIMER() (in_timer)
83 86
 
84 87
 #define LOCK_TIMER_LIST()		lock_get(timer_lock)
85 88
 #define UNLOCK_TIMER_LIST()		lock_release(timer_lock)
... ...
@@ -111,6 +116,9 @@ static struct timer_ln* volatile* running_timer2=0; /* timer handler running
111 111
 													     in the "slow" timer */
112 112
 static sigset_t slow_timer_sset;
113 113
 pid_t slow_timer_pid;
114
+static int in_slow_timer=0;
115
+
116
+#define IS_IN_TIMER_SLOW() (in_slow_timer)
114 117
 #define SET_RUNNING_SLOW(t)		(*running_timer2=(t))
115 118
 #define IS_RUNNING_SLOW(t)		(*running_timer2==(t))
116 119
 #define UNSET_RUNNING_SLOW()	(*running_timer2=0)
... ...
@@ -564,6 +572,23 @@ again:
564 564
 			}
565 565
 			if (IS_RUNNING_SLOW(tl)){
566 566
 				UNLOCK_SLOW_TIMER_LIST();
567
+				if (IS_IN_TIMER_SLOW()){
568
+					/* if somebody tries to shoot himself in the foot,
569
+					 * warn him and ignore the delete */
570
+					LOG(L_CRIT, "BUG: timer handle %p (s) tried to delete"
571
+							" itself\n", tl);
572
+#ifdef TIMER_DEBUG
573
+					LOG(timerlog, "WARN: -timer_del-: called from %s(%s):%d\n",
574
+									func, file, line);
575
+					LOG(timerlog, "WARN: -timer_del-: added %d times"
576
+						", last from: %s(%s):%d, deleted %d times"
577
+						", last from: %s(%s):%d, init %d times, expired %d \n",
578
+						tl->add_calls, tl->add_func, tl->add_file,
579
+						tl->add_line, tl->del_calls, tl->del_func, 
580
+						tl->del_file, tl->del_line, tl->init, tl->expires_no);
581
+#endif
582
+					return; /* do nothing */
583
+				}
567 584
 				sched_yield(); /* wait for it to complete */
568 585
 				goto again;
569 586
 			}
... ...
@@ -613,6 +638,23 @@ again:
613 613
 #endif
614 614
 			if (IS_RUNNING(tl)){
615 615
 				UNLOCK_TIMER_LIST();
616
+				if (IS_IN_TIMER()){
617
+					/* if somebody tries to shoot himself in the foot,
618
+					 * warn him and ignore the delete */
619
+					LOG(L_CRIT, "BUG: timer handle %p tried to delete"
620
+							" itself\n", tl);
621
+#ifdef TIMER_DEBUG
622
+					LOG(timerlog, "WARN: -timer_del-: called from %s(%s):%d\n",
623
+									func, file, line);
624
+					LOG(timerlog, "WARN: -timer_del-: added %d times"
625
+						", last from: %s(%s):%d, deleted %d times"
626
+						", last from: %s(%s):%d, init %d times, expired %d \n",
627
+						tl->add_calls, tl->add_func, tl->add_file,
628
+						tl->add_line, tl->del_calls, tl->del_func, 
629
+						tl->del_file, tl->del_line, tl->init, tl->expires_no);
630
+#endif
631
+					return; /* do nothing */
632
+				}
616 633
 				sched_yield(); /* wait for it to complete */
617 634
 				goto again;
618 635
 			}
... ...
@@ -634,7 +676,7 @@ again:
634 634
 					"{ %p, %p, %d, %d, %p, %p, %04x, -}\n", get_ticks_raw(), 
635 635
 					tl,  tl->next, tl->prev, tl->expire, tl->initial_timeout,
636 636
 					tl->data, tl->f, tl->flags);
637
-				LOG(timerlog, "WANT: -timer_del-; called from %s(%s):%d\n",
637
+				LOG(timerlog, "WARN: -timer_del-; called from %s(%s):%d\n",
638 638
 						func, file, line);
639 639
 				LOG(timerlog, "WARN: -timer_del-: added %d times"
640 640
 						", last from: %s(%s):%d, deleted %d times"
... ...
@@ -658,6 +700,33 @@ again:
658 658
 
659 659
 
660 660
 
661
+/* marks a timer as "to be deleted when the handler ends", usefull when
662
+ * the timer handler knows it won't prolong the timer anymore (it will 
663
+ * return 0) and will do some time consuming work. Calling this function
664
+ * will cause simultaneous timer_dels to return immediately (they won't 
665
+ * wait anymore for the timer handle to finish). It will also allow 
666
+ * self-deleting from the timer handle without bug reports.
667
+ * WARNING: - if you rely on timer_del to know when the timer handle execution
668
+ *            finishes (e.g. to free resources used in the timer handle), don't
669
+ *            use this function.
670
+ *          - this function can be called only from a timer handle (in timer
671
+ *            context), all other calls will have no effect and will log a
672
+ *            bug message
673
+ */
674
+void timer_allow_del()
675
+{
676
+	if (IS_IN_TIMER() ){
677
+			UNSET_RUNNING();
678
+	}else
679
+#ifdef USE_SLOW_TIMER
680
+	if (IS_IN_TIMER_SLOW()){
681
+			UNSET_RUNNING_SLOW();
682
+	}else 
683
+#endif
684
+		LOG(L_CRIT, "BUG: timer_allow_del called outside a timer handle\n");
685
+}
686
+
687
+
661 688
 /* called from timer_handle, must be called with the timer lock held */
662 689
 inline static void timer_list_expire(ticks_t t, struct timer_head* h
663 690
 #ifdef USE_SLOW_TIMER
... ...
@@ -813,6 +882,7 @@ static void timer_handler()
813 813
 /* main timer function, never exists */
814 814
 void timer_main()
815 815
 {
816
+	in_timer=1; /* mark this process as the fast timer */
816 817
 	while(1){
817 818
 		if (run_timer){
818 819
 			timer_handler();
... ...
@@ -915,6 +985,7 @@ void slow_timer_main()
915 915
 	struct timer_ln* tl;
916 916
 	unsigned short i;
917 917
 	
918
+	in_slow_timer=1; /* mark this process as the slow timer */
918 919
 	while(1){
919 920
 		n=sigwaitinfo(&slow_timer_sset, 0);
920 921
 		if (n==-1){
... ...
@@ -175,6 +175,8 @@ void timer_del_safe(struct timer_ln *tl);
175 175
 #define timer_del timer_del_safe
176 176
 #endif
177 177
 
178
+void timer_allow_del();
179
+
178 180
 /* old timer compatibility functions & structure */
179 181
 
180 182
 struct sr_timer{