Hi!
I'm getting to a point where we need to associate more and more state and/or references with every individual msgb.
In the circuit switched BSC case, every message (msgb) is associated with a logical channel (lchan) through which it is sent.
However, in the packet switched case, this is not the case. Rather, a msgb is received on a particular NS-VC, inside a particular BSSGP-VC, inside a LLC session, etc.
When we pass a msgb through the protocol layers and back, we somehow need to identify where to send the response to. Once we actually reach the 04.08/04.11 layer, things become a bit less difficult. In the end, all messages are at least somehow associated with a 'subscriber'.
If a message passes from the UDP socket into the NS protocol and into BSSGP, we need to know the BSVCI and NSVCI if we want to send a response back. One option would be to pass those values as explicit function call arguments, which is partially what my current code is doing. But the more layers we traverse, the longer the function arguments get.
Furthermore, there are parts of our protocol (04.08 and 04.11 particularly) that can either be transported on top of 08.58 (RSL) or LLC/BSSGP/NS. Those upper layers shouldn't really care what the underlying transport looks like.
One possible option is to somehow identify the NS-VC and BSSGP-VC by putting an identifier or pointer into a 'struct msgb' member. This would require libosmocore changes every time we add some bits here or there.
Another option is to introduce something similar to the skb->cb in the linux kernel: A 'control buffer' area of unspecified content provide by the core networking code as part of every sk_buff, which gets type-casted to the IPCB or TCB-CB when the msgb enters the respective protocol layer.
The advantage is that we could have one msgb structure definition with the typecasting functions/macros being part of the application (e.g. openbsc)
The problem with this is: Its scope is limited to whatever is the current layer of the protocol stack. The IPCB is overwritten with the TCBCB once the message enters TCP.
In our case, we would actually want to have the associated data to persist even while the msgb passes from one layer into another. This would mean that there is a more-or-less static structure containing all the various identifiers, which is then typecasted to the control buffer.
While this might not be ideal, it definitely keeps the details hidden away from libosmocore, which is good.
Any comments are welcome, Harald
Hi Harald,
In our case, we would actually want to have the associated data to persist even while the msgb passes from one layer into another. This would mean that there is a more-or-less static structure containing all the various identifiers, which is then typecasted to the control buffer.
While this might not be ideal, it definitely keeps the details hidden away from libosmocore, which is good.
FWIW, I think it's a good idea. When I had a first look at the gprs branch a few months ago, all those new fields required in msgb definitely were something I wasn't thrilled to see.
As I see it, it might not be perfect but : - It does the job - It's a whole lot better than the current situation - It's simple to understand
One question that pops to mind is : - Would allocation / freeing of those be handled by msgb routines ? I would think so since that'd avoid error and I don't see any loss of 'genericity' in doing that, but just asking.
Also, would you include the various header pointer in this cb struct or splitted ?
Personaly, I'd still see them splitted but more something like "uint8_t *h[8];" and let the application define its own macros msgb_ns_hdr(), msgb_rsl_hdr(), msgb_gmm_hdr() that map to the unique 'h' pointer rather than things like
union { unsigned char *smsh; unsigned char *llch; unsigned char *l4h; };
This way the msgb wouldn't show even any 'gsm' aspect and would just be a generic message buffer ...
Cheers,
Sylvain
Hi Sylvain,
On Fri, Apr 30, 2010 at 12:19:33AM +0200, Sylvain Munaut wrote:
While this might not be ideal, it definitely keeps the details hidden away from libosmocore, which is good.
FWIW, I think it's a good idea. When I had a first look at the gprs branch a few months ago, all those new fields required in msgb definitely were something I wasn't thrilled to see.
I also didn't find them particularly exciting.
As I see it, it might not be perfect but :
- It does the job
- It's a whole lot better than the current situation
- It's simple to understand
One question that pops to mind is :
- Would allocation / freeing of those be handled by msgb routines ? I
would think so since that'd avoid error and I don't see any loss of 'genericity' in doing that, but just asking.
As you can see from the current code in libosmocore, there is no separate allocation but simply a small data array in the msgb itself, just like the sk_buff.cb member.
Also, would you include the various header pointer in this cb struct or splitted ?
Personaly, I'd still see them splitted but more something like "uint8_t *h[8];" and let the application define its own macros msgb_ns_hdr(), msgb_rsl_hdr(), msgb_gmm_hdr() that map to the unique 'h' pointer rather than things like
union { unsigned char *smsh; unsigned char *llch; unsigned char *l4h; };
Yes, I agree with that, too. We can do a gradual migration, i.e. first introduce the accessor macros (while still pointing to the named msgb members) fields) and later removing those named msgb members.
Using accessor macros would also remove the need for those stupid typecasts all over the code (like "struct gsm48_hdr *gh = (struct gsm48_hdr *) msgb->l3h;"). It would simply be "struct gsm48_hdr *gh = msgb_gsm48hdr(msg);" instead.
Due to time constraints I will not be working on this and rather work on completing gprs functionality. However, if somebody submits patches, I'm happy to merge them.
Another stupidity that I also dislike about the current gsm_04_08_gprs.c code is that it uses a different header (msgb->gmmh, now msgb_gmmh(msg)) than the gsm_04_08.c code (msg->l3h). The reason for this is that the layer stacking in GPRS is deeper than in the GSM case.
I plan to rework it in a way that all 04.08 code (gsm or gprs) uses msgb_gsm48h(msg) whihc resolves to msg->l3h;
The additional layers introduced by NS/BSSGP/LLC stacking will be hidden in the cb[]. This way we can have code that works on any gsm48 msgb without knowing if its on GSM or GPRS. This will become important e.g. for SMS later.
This way the msgb wouldn't show even any 'gsm' aspect and would just be a generic message buffer ...
yes, that is definitely one of the goals...
Cheers, Harald
-
Harald Welte -
Sylvain Munaut