While working on the talloc context patches, I was wondering if we should spend a bit of time to further improve libosmocore and collect something like a wishlist.
I would currently identify the following areas:
1) initialization of the various sub-systems is too complex, there are too many functions an application has to call. I would like to move more to a global "application initialization", where an application registers some large struct [of structs, ...] at start-up and tells the library the log configuration, the copyright statement, the VTY IP/port, the config file name, ... (some of those can of course be NULL and hence not used)
2) have some kind of extensible command line options/arguments parser It would be useful to have common/library parts register some common command line arguments (like config file, logging, daemonization, ..) while the actual appliacation extending that with only its application-specific options. I don't think this is possible with how getopt() works, so it would require some new/different infrastructure how applications would register their arguments
3) move global select() state into some kind of structure. This would mean that there could be multiple lists of file descriptors rather than the one implicit global one. Alternatively, turn the state into thread-local storage, so each thread would have its own set of registered file descriptors, which probably makes most sense. Not sure if one would have diffeent 'sets' of registered file descriptors in a single thread. The same would apply for timers: Have a list of timers for each thread; timeouts would then also always execute on the same thread. This would put talloc context, select and timers all in the same concept: Have one set of each on each thread, used automatically.
Any other wishlist items?
Is this for big refactoring wishes only?
If not, I would like to have ^L working in telnet VTY connections like in a typical terminal: make it clear the buffer.
On 3/20/19 11:13 AM, Harald Welte wrote:
While working on the talloc context patches, I was wondering if we should spend a bit of time to further improve libosmocore and collect something like a wishlist.
I would currently identify the following areas:
initialization of the various sub-systems is too complex, there are too many functions an application has to call. I would like to move more to a global "application initialization", where an application registers some large struct [of structs, ...] at start-up and tells the library the log configuration, the copyright statement, the VTY IP/port, the config file name, ... (some of those can of course be NULL and hence not used)
have some kind of extensible command line options/arguments parser It would be useful to have common/library parts register some common command line arguments (like config file, logging, daemonization, ..) while the actual appliacation extending that with only its application-specific options. I don't think this is possible with how getopt() works, so it would require some new/different infrastructure how applications would register their arguments
move global select() state into some kind of structure. This would mean that there could be multiple lists of file descriptors rather than the one implicit global one. Alternatively, turn the state into thread-local storage, so each thread would have its own set of registered file descriptors, which probably makes most sense. Not sure if one would have diffeent 'sets' of registered file descriptors in a single thread. The same would apply for timers: Have a list of timers for each thread; timeouts would then also always execute on the same thread. This would put talloc context, select and timers all in the same concept: Have one set of each on each thread, used automatically.
Any other wishlist items?
On Wed, Mar 20, 2019 at 12:28:14PM +0100, Oliver Smith wrote:
If not, I would like to have ^L working in telnet VTY connections like in a typical terminal: make it clear the buffer.
that should be rather trivial to add, I guess. I was more thinking of more fundamental changes :)
Hi,
On 3/20/19 11:13 AM, Harald Welte wrote:
While working on the talloc context patches, I was wondering if we should spend a bit of time to further improve libosmocore and collect something like a wishlist.
I would currently identify the following areas:
initialization of the various sub-systems is too complex, there are too many functions an application has to call. I would like to move more to a global "application initialization", where an application registers some large struct [of structs, ...] at start-up and tells the library the log configuration, the copyright statement, the VTY IP/port, the config file name, ... (some of those can of course be NULL and hence not used)
have some kind of extensible command line options/arguments parser It would be useful to have common/library parts register some common command line arguments (like config file, logging, daemonization, ..) while the actual appliacation extending that with only its application-specific options. I don't think this is possible with how getopt() works, so it would require some new/different infrastructure how applications would register their arguments
1 and 2 not really worth for me. I'm not against it, but I'm happy enough with current state.
- move global select() state into some kind of structure. This would mean that there could be multiple lists of file descriptors rather than the one implicit global one. Alternatively, turn the state into thread-local storage, so each thread would have its own set of registered file descriptors, which probably makes most sense. Not sure if one would have diffeent 'sets' of registered file descriptors in a single thread. The same would apply for timers: Have a list of timers for each thread; timeouts would then also always execute on the same thread. This would put talloc context, select and timers all in the same concept: Have one set of each on each thread, used automatically.
Turn sate into thread-local storage makes sense. No need for different sets per thread imho. Loosely related: It may be good to refactor code to allow for other polling systems (like epoll()), which may be more efficient if we have a long set of file descriptors but only a few are triggered every loop step.
Regarding some other thread you wrote recently, I don't see much issue in having VTY library code not supporting multithread,since you can always force polling it from same thread and then if your process is multithread you take care of message passing between threads yourself in the VTY app-specific code.
For logging code we may want to add some callback which provides the application with some way to lock/unlock a mutex or something similar. If cb is NULL, then there's no performance penalty during logging.
Regards, Pau
Hi Pau,
On Wed, Mar 20, 2019 at 01:04:04PM +0100, Pau Espin Pedrol wrote:
- initialization of the various sub-systems is too complex, there are too many functions an application has to call. I would like to move more to a global "application initialization", where an application registers some large struct [of structs, ...] at start-up and tells the library the log configuration, the copyright statement, the VTY IP/port, the config file name, ... (some of those can of course be NULL and hence not used)
One addition here is also vty initialization. It sucks that everyone has to manually install the VTY commands for talloc, etc. - that should just happen automagically in some way.
- have some kind of extensible command line options/arguments parser It would be useful to have common/library parts register some common command line arguments (like config file, logging, daemonization, ..) while the actual appliacation extending that with only its application-specific options. I don't think this is possible with how getopt() works, so it would require some new/different infrastructure how applications would register their arguments
Another topic that I forgot to list is signal handling. I would think it makes sense to move the SIGUSR1/SIGUSR2 handling into libosmocore now, particularly as we have the root talloc context[s] in libosmocore with my related patches. It just sucks to have to have a SIGUSR1->talloc_dump code snippet in each and every of our programs.
Turn state into thread-local storage makes sense. No need for different sets per thread imho. Loosely related: It may be good to refactor code to allow for other polling systems (like epoll()), which may be more efficient if we have a long set of file descriptors but only a few are triggered every loop step.
That's a good point, indeed. It's been on the wishlist for a long time, but nobody appeared to have been reporting any performance issues as of yet. Ideally we wouldn't have to change our 'struct osmo_fd' at all but simply call a different "osmo_select_main()" function which would then call epoll instead of select.
Regarding some other thread you wrote recently, I don't see much issue in having VTY library code not supporting multithread,since you can always force polling it from same thread and then if your process is multithread you take care of message passing between threads yourself in the VTY app-specific code.
I agree we shouldn't tackle this problem now. However, we have many sub-systems that e.g. add a "show ..." VTY command. That command iterates over some dynamically created structures, like e.g. subscriber connections. So if we were in a multi-threaded environment where external events/messages were processed in other threads than the main thread that runs the VTY, then we can no longer simply iterate over any of those lists, but we'd have to have mutexes/rwlocks/RCU/... to synchronize.
That's out of scope for now, as we don't have any big plans for multi-threading in a big way just yet. We can look at it if we ever get to that point.
For logging code we may want to add some callback which provides the application with some way to lock/unlock a mutex or something similar. If cb is NULL, then there's no performance penalty during logging.
Not sure I understand you here. The problem starts if we use multiple printf() calls to write a signle line, especially with LOGPC/DEBUGPC.
This introduces problems if multiple threads writing to the same log output then garble / intersperse each others output at points that are not the end of a lot line.
Especially with the new _buf() and even the _c() variants of our various stringify functions it should be possible to convert all callers of log functions to hand in a full line in one call and remove the LOGPC/DEBUGPC for continuation.
Once we reach that point, at least log stdout/stderr/file will be ok, as glibc guarantees that a single write/printf is "atomic". Not sure about our VTY log backend, though.
Regards, Harald
Hi,
On 3/20/19 5:43 PM, Harald Welte wrote:
Hi Pau,
On Wed, Mar 20, 2019 at 01:04:04PM +0100, Pau Espin Pedrol wrote:
- initialization of the various sub-systems is too complex, there are too many functions an application has to call. I would like to move more to a global "application initialization", where an application registers some large struct [of structs, ...] at start-up and tells the library the log configuration, the copyright statement, the VTY IP/port, the config file name, ... (some of those can of course be NULL and hence not used)
One addition here is also vty initialization. It sucks that everyone has to manually install the VTY commands for talloc, etc. - that should just happen automagically in some way.
Fine with that. Anyway that's done at initialization, before multiple threads/workers are started, so no issue there with multithread.
Another topic that I forgot to list is signal handling. I would think it makes sense to move the SIGUSR1/SIGUSR2 handling into libosmocore now, particularly as we have the root talloc context[s] in libosmocore with my related patches. It just sucks to have to have a SIGUSR1->talloc_dump code snippet in each and every of our programs.
Please don't. Let's allow apps decide how they handle user-defined signals and not try doing everything in a generic library. Some app using libosmocore may want to do something else under SIGUSR1/SIGUSR2. I really prefer having this kind of stuff being done in the app and not in the library. If still you want to do it, at least document clearly what libosmocore does and how to let apps overwrite the signals (so it becomes a "public" feature and not some hidden-implementation requirement).
Turn state into thread-local storage makes sense. No need for different sets per thread imho. Loosely related: It may be good to refactor code to allow for other polling systems (like epoll()), which may be more efficient if we have a long set of file descriptors but only a few are triggered every loop step.
That's a good point, indeed. It's been on the wishlist for a long time, but nobody appeared to have been reporting any performance issues as of yet. Ideally we wouldn't have to change our 'struct osmo_fd' at all but simply call a different "osmo_select_main()" function which would then call epoll instead of select.
Maybe is as easy as having a compile flag which builds osmo_select_main against epoll() if existent, and use select() as fallback.
Regarding some other thread you wrote recently, I don't see much issue in having VTY library code not supporting multithread,since you can always force polling it from same thread and then if your process is multithread you take care of message passing between threads yourself in the VTY app-specific code.
I agree we shouldn't tackle this problem now. However, we have many sub-systems that e.g. add a "show ..." VTY command. That command iterates over some dynamically created structures, like e.g. subscriber connections. So if we were in a multi-threaded environment where external events/messages were processed in other threads than the main thread that runs the VTY, then we can no longer simply iterate over any of those lists, but we'd have to have mutexes/rwlocks/RCU/... to synchronize.
That's out of scope for now, as we don't have any big plans for multi-threading in a big way just yet. We can look at it if we ever get to that point.
Sure, I'm not expecting multi-threaded for current apps so far (perhaps osmo-mgw in the future?). IF we add multi-thread to those we'll need this kind of changes. I was howerver thinking about new apps, which will already have this kind of tools/architecture in place since time 0.
For logging code we may want to add some callback which provides the application with some way to lock/unlock a mutex or something similar. If cb is NULL, then there's no performance penalty during logging.
Not sure I understand you here. The problem starts if we use multiple printf() calls to write a signle line, especially with LOGPC/DEBUGPC
This introduces problems if multiple threads writing to the same log output then garble / intersperse each others output at points that are not the end of a lot line.
Especially with the new _buf() and even the _c() variants of our various stringify functions it should be possible to convert all callers of log functions to hand in a full line in one call and remove the LOGPC/DEBUGPC for continuation.
Once we reach that point, at least log stdout/stderr/file will be ok, as glibc guarantees that a single write/printf is "atomic". Not sure about our VTY log backend, though.
Indeed, I was thinking about LOGPC/DEBUGPC and other backends like VTY here.
Regards, Pau
On Wed, Mar 20, 2019 at 06:12:30PM +0100, Pau Espin Pedrol wrote:
Another topic that I forgot to list is signal handling. I would think it makes sense to move the SIGUSR1/SIGUSR2 handling into libosmocore now, particularly as we have the root talloc context[s] in libosmocore with my related patches. It just sucks to have to have a SIGUSR1->talloc_dump code snippet in each and every of our programs.
Please don't. Let's allow apps decide how they handle user-defined signals and not try doing everything in a generic library.
I think it's better to avoid the code dup in each application, have well-defined standard handlers once.
Thinking in terms of one global libosmocore subsystem config struct, this could be switch-off-able.
If it were implicit any application can easily put other signal handlers in place after the fact.
~N
Hi all,
Thinking in terms of one global libosmocore subsystem config struct, this could be switch-off-able.
FYI, we already have something similar in layer23 applications of OsmocomBB:
https://git.osmocom.org/osmocom-bb/tree/src/host/layer23/src/misc/app_cell_l... https://git.osmocom.org/osmocom-bb/tree/src/host/layer23/src/misc/app_cell_l...
With best regards, Vadim Yanitskiy.
On Wed, Mar 20, 2019 at 05:43:51PM +0100, Harald Welte wrote:
it should be possible to convert all callers of log functions to hand in a full line in one call and remove the LOGPC/DEBUGPC for continuation.
+1 for removing LOGPC.
I already avoid using them, and write patches to remove them, because 1) they end up in messy multiple GSMTAP-log packets, 2) they check each time over whether a log category is enabled and invoke the log system N times for the same line, 3) often cause with buggy logging, forgetting an \n or with intermediate other logging happening in some cases.
~N
On 20. Mar 2019, at 10:13, Harald Welte laforge@gnumonks.org wrote:
While working on the talloc context patches, I was wondering if we should spend a bit of time to further improve libosmocore and collect something like a wishlist.
I would currently identify the following areas:
- initialization of the various sub-systems is too complex, there are too
many functions an application has to call. I would like to move more to a global "application initialization", where an application registers some large struct [of structs, ...] at start-up and tells the library the log configuration, the copyright statement, the VTY IP/port, the config file name, ... (some of those can of course be NULL and hence not used)
ack. One big struct for options? But how would this work across libosmocore and libosmonetif/libosmo-abis?
In Go there is a "pattern" to pass an options struct into the method.
- have some kind of extensible command line options/arguments parser
It would be useful to have common/library parts register some common command line arguments (like config file, logging, daemonization, ..) while the actual appliacation extending that with only its application-specific options. I don't think this is possible with how getopt() works, so it would require some new/different infrastructure how applications would register their arguments
I started to like the absl flags infrastructure (but we need to make sure to not have an excessive amount of them):
https://abseil.io/docs/python/guides/flags
flags.DEFINE_integer('age', None, 'Your age in years.', lower_bound=0)
The same concept exsists for C++, Java, Go, Python and Bash.
- move global select() state into some kind of structure. This would mean
that there could be multiple lists of file descriptors rather than the one implicit global one. Alternatively, turn the state into thread-local storage, so each thread would have its own set of registered file descriptors, which probably makes most sense. Not sure if one would have diffeent 'sets' of registered file descriptors in a single thread. The same would apply for timers: Have a list of timers for each thread; timeouts would then also always execute on the same thread. This would put talloc context, select and timers all in the same concept: Have one set of each on each thread, used automatically.
Do we plan to have threads? On the low-end we could have an EventServer that runs one epoll_wait per thread. But then we are in the game of scheduling across the threads, work stealing, etc. Maybe something already exists we can use?
On the high-end I wondered if we could have something like "fibers" and FSMs and CSP as first class citizens?
* When creating a new fsm it gets scheduled on the least busy worker thread * When creating a child it stays on the same thread. * Components communicate strictly using a CSP like primitive. * We can scale up/scale down worker threads based on load.
4) Adopt/build an RPC mechanism (maybe evolve GSUP to it). I underestimated the "network" effect of every binary offering the same RPC interface. Suddenly sending a SMS, placing a call.. becomes..
the_rpc_cli endpoint service.method < arguments
And to inspect a service
the_rpc_cli endpoint ls [service.method]
5) Plan for seamless/cooperative upgrades. E.g. by passing fds somewhere else. E.g. leave existing TCP connections in the old process and accept new ones in the new version. The difficulty is how to deal with the VTY and other services. We probably need a meta server.. and meta server upgrades.
Or this might be the time to break from VTY. Give up on runtime reconfiguration (we never had a solid model for it) and see how plain rpc can save our day?
-
Harald Welte -
Holger Freyther -
Neels Hofmeyr -
Oliver Smith -
Pau Espin Pedrol -
Vadim Yanitskiy