[2/2] Get rid of duplicated timer code
diff mbox

Message ID 1453737688-27611-2-git-send-email-suraev@alumni.ntnu.no
State New
Headers show

Commit Message

Suraev Jan. 25, 2016, 4:01 p.m. UTC
From: Max <msuraev@sysmocom.de>

Sponsored-by: On-Waves ehf
---
 src/Makefile.am   |   2 -
 src/gprs_rlcmac.h |   1 -
 src/gsm_timer.cpp | 238 ------------------------------------------------------
 src/gsm_timer.h   |  84 -------------------
 src/pcu_main.cpp  |   8 +-
 src/tbf.h         |   2 +-
 6 files changed, 5 insertions(+), 330 deletions(-)
 delete mode 100644 src/gsm_timer.cpp
 delete mode 100644 src/gsm_timer.h

Comments

Holger Freyther Jan. 26, 2016, 10:12 a.m. UTC | #1
> On 25 Jan 2016, at 17:01, suraev@alumni.ntnu.no wrote:
> 
> 	while (!quit) {
> -		osmo_gsm_timers_check();
> -		osmo_gsm_timers_prepare();
> -		osmo_gsm_timers_update();
> +		osmo_timers_check();
> +		osmo_timers_prepare();
> +		osmo_timers_update();
> 
> 		osmo_select_main(0);

timers_check,_prepare and _update are called from osmo_select_main
already so they are not needed.

The general idea to work in frame numbers makes a lot of sense, the
question is why we are not working with it anymore.

a.) Our fn's are not monotonic and we can work in time then we do not
need this code
b.) We should do things based on the fn (e.g. the T200 issues we found
in LAPDm show that it can be a good idea).

holger

Patch
diff mbox

diff --git a/src/Makefile.am b/src/Makefile.am
index 6428bef..e345f26 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -40,7 +40,6 @@  libgprs_la_SOURCES = \
 	gprs_rlcmac_ts_alloc.cpp \
 	gprs_ms.cpp \
 	gprs_ms_storage.cpp \
-	gsm_timer.cpp \
 	bitvector.cpp \
 	pcu_l1_if.cpp \
 	pcu_vty.c \
@@ -79,7 +78,6 @@  noinst_HEADERS = \
 	gprs_ms_storage.h \
 	pcuif_proto.h \
 	pcu_l1_if.h \
-	gsm_timer.h \
 	bitvector.h \
 	pcu_vty.h \
 	pcu_vty_functions.h \
diff --git a/src/gprs_rlcmac.h b/src/gprs_rlcmac.h
index f193dfa..ed1e529 100644
--- a/src/gprs_rlcmac.h
+++ b/src/gprs_rlcmac.h
@@ -24,7 +24,6 @@ 
 #ifdef __cplusplus
 #include <bitvector.h>
 #include <gsm_rlcmac.h>
-#include <gsm_timer.h>
 
 extern "C" {
 #include <osmocom/core/linuxlist.h>
diff --git a/src/gsm_timer.cpp b/src/gsm_timer.cpp
deleted file mode 100644
index d3c59cb..0000000
--- a/src/gsm_timer.cpp
+++ /dev/null
@@ -1,238 +0,0 @@ 
-/* gsm_timer.cpp
- *
- * Copyright (C) 2012 Ivan Klyuchnikov
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License
- * as published by the Free Software Foundation; either version 2
- * of the License, or (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.
- */
- 
-/* These store the amount of frame number that we wait until next timer expires. */
-static int nearest;
-static int *nearest_p;
-
-/*! \addtogroup gsm_timer
- *  @{
- */
-
-/*! \file gsm_timer.cpp
- */
-
-#include <assert.h>
-#include <string.h>
-#include <limits.h>
-#include <gsm_timer.h>
-#include <pcu_l1_if.h>
-#include <bts.h>
-
-
-static struct rb_root timer_root = RB_ROOT;
-
-/*
- * TODO: make this depend on the BTS. This means that
- * all time functions schedule based on the BTS they
- * are scheduled on.
- */
-static int get_current_fn()
-{
-	return BTS::main_bts()->current_frame_number();
-}
-
-static void __add_gsm_timer(struct osmo_gsm_timer_list *timer)
-{
-	struct rb_node **new_node = &(timer_root.rb_node);
-	struct rb_node *parent = NULL;
-
-	while (*new_node) {
-		struct osmo_gsm_timer_list *this_timer;
-
-		this_timer = container_of(*new_node, struct osmo_gsm_timer_list, node);
-
-		parent = *new_node;
-		if (timer->fn < this_timer->fn)
-			new_node = &((*new_node)->rb_left);
-		else
-			new_node = &((*new_node)->rb_right);
-		}
-
-		rb_link_node(&timer->node, parent, new_node);
-		rb_insert_color(&timer->node, &timer_root);
-}
-
-/*! \brief add a new timer to the timer management
- *  \param[in] timer the timer that should be added
- */
-void osmo_gsm_timer_add(struct osmo_gsm_timer_list *timer)
-{
-	osmo_gsm_timer_del(timer);
-	timer->active = 1;
-	INIT_LLIST_HEAD(&timer->list);
-	__add_gsm_timer(timer);
-}
-
-/*! \brief schedule a gsm timer at a given future relative time
- *  \param[in] timer the to-be-added timer
- *  \param[in] number of frames from now
- *
- * This function can be used to (re-)schedule a given timer at a
- * specified number of frames in the future.  It will
- * internally add it to the timer management data structures, thus
- * osmo_timer_add() is automatically called.
- */
-void
-osmo_gsm_timer_schedule(struct osmo_gsm_timer_list *timer, int fn)
-{
-	int current_fn;
-
-	current_fn = get_current_fn();
-	timer->fn = current_fn + fn;
-	osmo_gsm_timer_add(timer);
-}
-
-/*! \brief delete a gsm timer from timer management
- *  \param[in] timer the to-be-deleted timer
- *
- * This function can be used to delete a previously added/scheduled
- * timer from the timer management code.
- */
-void osmo_gsm_timer_del(struct osmo_gsm_timer_list *timer)
-{
-	if (timer->active) {
-		timer->active = 0;
-		rb_erase(&timer->node, &timer_root);
-		/* make sure this is not already scheduled for removal. */
-		if (!llist_empty(&timer->list))
-			llist_del_init(&timer->list);
-	}
-}
-
-/*! \brief check if given timer is still pending
- *  \param[in] timer the to-be-checked timer
- *  \return 1 if pending, 0 otherwise
- *
- * This function can be used to determine whether a given timer
- * has alredy expired (returns 0) or is still pending (returns 1)
- */
-int osmo_gsm_timer_pending(struct osmo_gsm_timer_list *timer)
-{
-	return timer->active;
-}
-
-/*
- * if we have a nearest frame number return the delta between the current
- * FN and the FN of the nearest timer.
- * If the nearest timer timed out return NULL and then we will
- * dispatch everything after the select
- */
-int *osmo_gsm_timers_nearest(void)
-{
-	/* nearest_p is exactly what we need already: NULL if nothing is
-	 * waiting, {0,0} if we must dispatch immediately, and the correct
-	 * delay if we need to wait */
-	return nearest_p;
-}
-
-static void update_nearest(int *cand, int *current)
-{
-	if (*cand != LONG_MAX) {
-		if (*cand > *current)
-			nearest = *cand - *current;
-		else {
-			/* loop again inmediately */
-			nearest = 0;
-		}
-		nearest_p = &nearest;
-	} else {
-		nearest_p = NULL;
-	}
-}
-
-/*
- * Find the nearest FN and update s_nearest_time
- */
-void osmo_gsm_timers_prepare(void)
-{
-	struct rb_node *node;
-	int current_fn;
-
-	current_fn = get_current_fn();
-
-	node = rb_first(&timer_root);
-	if (node) {
-		struct osmo_gsm_timer_list *this_timer;
-		this_timer = container_of(node, struct osmo_gsm_timer_list, node);
-		update_nearest(&this_timer->fn, &current_fn);
-	} else {
-		nearest_p = NULL;
-	}
-}
-
-/*
- * fire all timers... and remove them
- */
-int osmo_gsm_timers_update(void)
-{
-	int current_fn;
-	struct rb_node *node;
-	struct llist_head timer_eviction_list;
-	struct osmo_gsm_timer_list *this_timer;
-	int work = 0;
-
-	current_fn = get_current_fn();
-
-	INIT_LLIST_HEAD(&timer_eviction_list);
-	for (node = rb_first(&timer_root); node; node = rb_next(node)) {
-		this_timer = container_of(node, struct osmo_gsm_timer_list, node);
-
-		if (this_timer->fn > current_fn)
-			break;
-
-		llist_add(&this_timer->list, &timer_eviction_list);
-	}
-
-	/*
-	 * The callbacks might mess with our list and in this case
-	 * even llist_for_each_entry_safe is not safe to use. To allow
-	 * osmo_gsm_timer_del to be called from within the callback we need
-	 * to restart the iteration for each element scheduled for removal.
-	 *
-	 * The problematic scenario is the following: Given two timers A
-	 * and B that have expired at the same time. Thus, they are both
-	 * in the eviction list in this order: A, then B. If we remove
-	 * timer B from the A's callback, we continue with B in the next
-	 * iteration step, leading to an access-after-release.
-	 */
-restart:
-	llist_for_each_entry(this_timer, &timer_eviction_list, list) {
-		osmo_gsm_timer_del(this_timer);
-		this_timer->cb(this_timer->data);
-		work = 1;
-		goto restart;
-	}
-
-	return work;
-}
-
-int osmo_gsm_timers_check(void)
-{
-	struct rb_node *node;
-	int i = 0;
-
-	for (node = rb_first(&timer_root); node; node = rb_next(node)) {
-		i++;
-	}
-	return i;
-}
-
-/*! }@ */
-
diff --git a/src/gsm_timer.h b/src/gsm_timer.h
deleted file mode 100644
index fc42caf..0000000
--- a/src/gsm_timer.h
+++ /dev/null
@@ -1,84 +0,0 @@ 
-/* gsm_timer.h
- *
- * Copyright (C) 2012 Ivan Klyuchnikov
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License
- * as published by the Free Software Foundation; either version 2
- * of the License, or (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.
- */
-
-/*! \defgroup timer GSM timers
- *  @{
- */
-
-/*! \file gsm_timer.h
- *  \brief GSM timer handling routines
- */
-#ifndef GSM_TIMER_H
-#define GSM_TIMER_H
-
-extern "C" {
-#include <osmocom/core/linuxlist.h>
-#include <osmocom/core/linuxrbtree.h>
-}
-/**
- * Timer management:
- *      - Create a struct osmo_gsm_timer_list
- *      - Fill out timeout and use add_gsm_timer or
- *        use schedule_gsm_timer to schedule a timer in
- *        x frames from now...
- *      - Use del_gsm_timer to remove the timer
- *
- *  Internally:
- *      - We hook into select.c to give a frame number of the
- *        nearest timer. On already passed timers we give
- *        it a 0 to immediately fire after the select.
- *      - update_gsm_timers will call the callbacks and remove
- *        the timers.
- *
- */
-/*! \brief A structure representing a single instance of a gsm timer */
-struct osmo_gsm_timer_list {
-	struct rb_node node;	  /*!< \brief rb-tree node header */
-	struct llist_head list;   /*!< \brief internal list header */
-	int fn;                   /*!< \brief expiration frame number */
-	unsigned int active  : 1; /*!< \brief is it active? */
-
-	void (*cb)(void*);	  /*!< \brief call-back called at timeout */
-	void *data;		  /*!< \brief user data for callback */
-};
-
-/**
- * timer management
- */
-
-void osmo_gsm_timer_add(struct osmo_gsm_timer_list *timer);
-
-void osmo_gsm_timer_schedule(struct osmo_gsm_timer_list *timer, int fn);
-
-void osmo_gsm_timer_del(struct osmo_gsm_timer_list *timer);
-
-int osmo_gsm_timer_pending(struct osmo_gsm_timer_list *timer);
-
-
-/*
- * internal timer list management
- */
-int *osmo_gsm_timers_nearest(void);
-void osmo_gsm_timers_prepare(void);
-int osmo_gsm_timers_update(void);
-int osmo_gsm_timers_check(void);
-
-/*! }@ */
-
-#endif // GSM_TIMER_H
diff --git a/src/pcu_main.cpp b/src/pcu_main.cpp
index 77e1082..a9f9df1 100644
--- a/src/pcu_main.cpp
+++ b/src/pcu_main.cpp
@@ -21,7 +21,6 @@ 
 #include <arpa/inet.h>
 #include <pcu_l1_if.h>
 #include <gprs_rlcmac.h>
-#include <gsm_timer.h>
 #include <gprs_debug.h>
 #include <unistd.h>
 #include <getopt.h>
@@ -33,6 +32,7 @@  extern "C" {
 #include <osmocom/vty/telnet_interface.h>
 #include <osmocom/vty/logging.h>
 #include <osmocom/core/stats.h>
+#include <osmocom/core/timer.h>
 }
 
 extern struct gprs_nsvc *nsvc;
@@ -256,9 +256,9 @@  int main(int argc, char *argv[])
 	}
 
 	while (!quit) {
-		osmo_gsm_timers_check();
-		osmo_gsm_timers_prepare();
-		osmo_gsm_timers_update();
+		osmo_timers_check();
+		osmo_timers_prepare();
+		osmo_timers_update();
 
 		osmo_select_main(0);
 	}
diff --git a/src/tbf.h b/src/tbf.h
index a3a2eeb..411d113 100644
--- a/src/tbf.h
+++ b/src/tbf.h
@@ -202,7 +202,7 @@  struct gprs_rlcmac_tbf {
 	unsigned int T; /* Txxxx number */
 	unsigned int num_T_exp; /* number of consecutive T expirations */
 	
-	struct osmo_gsm_timer_list	gsm_timer;
+	struct osmo_timer_list	gsm_timer;
 	unsigned int fT; /* fTxxxx number */
 	unsigned int num_fT_exp; /* number of consecutive fT expirations */