Project

General

Profile

Feature #4539

review timer numbers: use (negative) X-timers instead of non-existing T-timers (like T23001), and avoid collisions of X-timer numbers across osmocom programs

Added by neels 11 months ago. Updated 6 months ago.

Status:
Resolved
Priority:
Normal
Assignee:
Target version:
-
Start date:
05/09/2020
Due date:
% Done:

100%

Spec Reference:

Description

the 3GPP specs define various timers like T3001, which we made configurable via vty using the osmo_tdef API.
Later on, we introduced the use of negative timer numbers to indicate timers that are not specified by 3GPP, and called them X-timers, like X1, X2,...
At that point, various invented timer numbers have already been in use, see for example timeslot_fsm.c and lchan_fsm.c in osmo-bsc: T23001 etc.

Timer numbers can be re-used in different contexts, so per se we could use X1 any number of times in different contexts.
However, since we have a huge number space available for X-timers, there is no need to create "collisions" between osmocom programs.
It is much clearer to just pick unused numbers for each program, sort of like out Port_Numbers.

Create one common overview of all X-timers (and while at it, why not also all T-timers) available in osmocom programs.
Replace osmocom-invented T-timers with X-timers,
avoid X-timer collisions between osmocom programs.

Not sure whether we should provide a backwards compat shim to still allow configuring old timer numbers, so that setting the old timer numbers actually adjusts the new ones?
Or maybe just make sure that configuring the new timer number causes an error message on program start indicating the matching new X-timer to use?


Related issues

Related to OsmoBSC - Bug #4538: timeslot_fsm.c: make the PDCH act/deact timeout configurableNew05/09/2020

Associated revisions

Revision 1eaa821e (diff)
Added by Neels Hofmeyr 7 months ago

add timer.vty

Before transitioning some unspecified T timers to X timers, and to introduce
timer groups, first add this timer VTY test. Changes here will illustrate that
the legacy commands will still work and redirect to new timer definitions.

Related: OS#4539
Change-Id: Ie1bc635e16dc9a4040d063e1d9a51cdc76d9d1f2

Revision aad76684 (diff)
Added by Neels Hofmeyr 7 months ago

clean up timer definitions: introduce groups, move some T to X

Backwards compatibly, introduce timer groups in OsmoBSC, and move some
non-specified T timers to new X timers:

T993111 -> X3111
T993210 -> X3210
T999 -> X4

Why X4? because there already is an X3 used elsewhere in Osmocom, and I find
it less confusing if X-numbers don't repeat across programs. See
https://osmocom.org/projects/cellular-infrastructure/wiki/List_of_Timer_numbers

Drop unused timers from g_mgw_tdefs. Only X2427 has an actual effect.
(libosmo-mgcp-client recently moved T2427001 to X2427.)

Put libosmo-mgcp-client related timers to the 'mgw' group, like in osmo-msc.
This makes the MGCP timeout configurable for the first time.

Keep previous timer commands as DEFUN_HIDDEN, and also translate the moved T
timers to X timers on-the-fly. All previous VTY commands still work, and new
'timer [(net|mgw)] ...' commands are added. timer.vty shows this.

Remove the "_OPTIONAL" from the legacy "timer" and "show timer" commands, so
that they don't ambiguously overload the new "timer [(net|mgw)] ..." commands.

Related: OS#4539
Related: If097f52701fd81f29bcca1d252f4fb4fca8a04f7 (osmo-mgw)
Change-Id: I4beec47502afa193dee343869c4be55dc6a4b536

Revision 638eb992 (diff)
Added by Neels Hofmeyr 7 months ago

clean up timer definitions: introduce groups, move some T to X

Backwards compatibly, introduce timer groups in OsmoBSC, and move some
non-specified T timers to new X timers:

T993111 -> X3111
T993210 -> X3210
T999 -> X4

Why X4? because there already is an X3 used elsewhere in Osmocom, and I find
it less confusing if X-numbers don't repeat across programs. See
https://osmocom.org/projects/cellular-infrastructure/wiki/List_of_Timer_numbers

Drop unused timers from g_mgw_tdefs. Only X2427 has an actual effect.
(libosmo-mgcp-client recently moved T2427001 to X2427.)

Put libosmo-mgcp-client related timers to the 'mgw' group, like in osmo-msc.
This makes the MGCP timeout configurable for the first time.

Keep previous timer commands as DEFUN_HIDDEN, and also translate the moved T
timers to X timers on-the-fly. All previous VTY commands still work, and new
'timer [(net|mgw)] ...' commands are added. timer.vty shows this.

Remove the "_OPTIONAL" from the legacy "timer" and "show timer" commands, so
that they don't ambiguously overload the new "timer [(net|mgw)] ..." commands.

Related: OS#4539
Related: If097f52701fd81f29bcca1d252f4fb4fca8a04f7 (osmo-mgw)
Change-Id: I4beec47502afa193dee343869c4be55dc6a4b536

History

#1 Updated by neels 11 months ago

  • Related to Bug #4538: timeslot_fsm.c: make the PDCH act/deact timeout configurable added

#2 Updated by neels 11 months ago

IMO we should not use numbers like X23001 but start with lower numbers.
We could think of either creating a number space for each program, like osmo-bts has X1000-X1999, osmo-bsc X2000-X2999 etc.,
or just start with X1 and assign the next X2, X3 etc, whichever comes along next, like we did in Port_Numbers.

#3 Updated by osmith 11 months ago

Thanks for creating the issue!

neels wrote:

IMO we should not use numbers like X23001 but start with lower numbers.
We could think of either creating a number space for each program, like osmo-bts has X1000-X1999, osmo-bsc X2000-X2999 etc.,
or just start with X1 and assign the next X2, X3 etc, whichever comes along next, like we did in Port_Numbers.

I'd prefer the number space, so it's clear to which program a timer belongs.

Do we need more opinions on this (laforge, fixeria, pespin?), or can I create such a wiki page and start using these numbers in my open related osmo-bsc patch?

#4 Updated by fixeria 11 months ago

Do we need more opinions on this (laforge, fixeria, pespin?)

I definitely like the idea of having unique 'X' timer numbers. But still, it's unclean to me how do we ensure backwards compatibility, i.e. what should we do with the existing timer numbers that are used in more than one project (e.g. X1)?

can I create such a wiki page and start using these numbers in my open related osmo-bsc patch?

It probably makes sense to have a header file in libosmocore listing all used timers, so then we could automatically generate the wiki page from it. This way it's less likely that somebody forgets to reserve a port number, and somebody else starts to use it in another osmo-* program.

#5 Updated by pespin 11 months ago

I'm fine with having a unique list in a wiki page listing all X timers.

Regarding starting timers from X1 and so: I don't think it's generally a good idea (though it can be for some cases). What I mean is: Sometimes afair some of our X timers are highly related to a standard timer T, in which cases it makes sense to keep either Xn for Tn, or some value around n, specially since sometimes in standard some related timers have similar numbering.

#6 Updated by laforge 11 months ago

I don't have any comments on the detailed allocation

Having some systematic approach with per program number ranges and wanted associated wiki page looks like a good idea.

#7 Updated by neels 11 months ago

I've been pondering the thought of fixing X timer numbers.
It is easiest to just change the number, hence the config changes and probably fails, saying "timer X23 does not exist" all of a sudden.
But ideally, when we change a timer number, we would keep some backwards compatibility in the configuration.
We are currently missing any approach to provide backwards compat for old timer numbers.
Most (new) timers are managed by tdef_vty.h which makes no provision for backwards compat.
It might be possible to implement separate vty functions, without the need for a generic approach, like:

DEFUN(deprecated_timers, deprecated_timers_cmd,
"timer (T23|T42|X99) .IGNORED") {
vty_out("Timer numbers have changed, please adjust your config: T23->X17 T42->X18 X99->X19");
return CMD_WARNING;
}

could possibly be advanced to actually redirecting the legacy number to a new proper number, maybe like:

DEFUN(deprecated_timer_T23, deprecated_timer_T23_cmd,
"timer T23 .ARGS") {
vty_out("Timer T23 is now X17, please adjust your config");
argv[0] = "X17"; // somehow something like this
return cfg_timer_cmd(vty, argc, argv); // meaning the tdef_vty.c cfg_timer_cmd
}

#8 Updated by neels 7 months ago

  • Status changed from New to Resolved
  • Assignee set to neels
  • % Done changed from 0 to 100

After https://gerrit.osmocom.org/c/osmo-bsc/+/17973 was assigned to me, I figured now is as good a time as ever to clean up the timers.

I collected all osmo_tdef timers I could find in:
https://osmocom.org/projects/cellular-infrastructure/wiki/List_of_Timer_numbers
(the wiki page reflects the status after merging pending patches https://gerrit.osmocom.org/q/topic:%22timers%22+(status:open%20OR%20status:merged) )

From that it became apparent that only OsmoBSC exposes T timers that are not specified by 3GPP.
Adding timer groups to OsmoBSC like in OsmoMSC (backwards compatibly) was a nice fit to move those to X timers.
https://gerrit.osmocom.org/c/osmo-bsc/+/20155

Also fixed an mgw timer in osmo-msc https://gerrit.osmocom.org/c/osmo-msc/+/20156

#9 Updated by neels 7 months ago

  • Status changed from Resolved to In Progress
  • % Done changed from 100 to 90

wait for merge of course

#10 Updated by neels 6 months ago

  • Status changed from In Progress to Resolved
  • % Done changed from 90 to 100

Also available in: Atom PDF

Add picture from clipboard (Maximum size: 48.8 MB)