hi,
these 7 patches have been tested with sysmobts and osmo-bts (trx interface). it works with calls and gprs. before applying any of these patches, i suggest to double check it with a different test setup.
regards,
andreas
On Mon, Jul 15, 2013 at 12:07:58PM +0200, Andreas Eversberg wrote:
hi,
these 7 patches have been tested with sysmobts and osmo-bts (trx interface). it works with calls and gprs. before applying any of these patches, i suggest to double check it with a different test setup.
Hi,
I don't have much time to do QA in spare my time. So this is very brief:
* Doing the stress-testing I described on 4th and 6th of May. A compiled sysmobts binary with your changes is crashing on a talloc_free. zecke/hacks/stress-testing, executing allocate-all-channels will crash the code. I am confident that this is not due a binary incompatible change to the header files (we need to bump the so version of libosmocore in the next cycle)
* You are undoing all the lchan->s changes we did based on my review we did from 12th of March to 15th of March. To be honest I find this quite offensive.
* The extra msgb_alloc/memcpy to go from primitive to native is wasteful. One should re-write the header around the payload.
* The change to introduce the primitives is too big. Otherwise the above two issues could have been spotted with review. My proposal is to introduce primitives inside the sysmoBTS code piece by piece in one primitive per patch.
E.g. introduce a 'native' primitive and the l1sapup, then start converting that to other primitives until there are no native primitives left. Avoid creating a thousand lines switch/case statement. Modern compilers will nicely inline small static methods.
The benefit is that each change can be easily reviewed and stress tested (e.g. compare it with the changeset I did for the sapi queuing), and one avoids such things like undoing lchan->s changes that are burried somewhere in the patch.
cheers holger
Holger Hans Peter Freyther wrote:
Doing the stress-testing I described on 4th and 6th of May. A compiled sysmobts binary with your changes is crashing on a talloc_free. zecke/hacks/stress-testing, executing allocate-all-channels will crash the code. I am confident that this is not due a binary incompatible change to the header files (we need to bump the so version of libosmocore in the next cycle)
You are undoing all the lchan->s changes we did based on my review we did from 12th of March to 15th of March. To be honest I find this quite offensive.
The extra msgb_alloc/memcpy to go from primitive to native is wasteful. One should re-write the header around the payload.
The change to introduce the primitives is too big. Otherwise the above two issues could have been spotted with review. My proposal is to introduce primitives inside the sysmoBTS code piece by piece in one primitive per patch.
E.g. introduce a 'native' primitive and the l1sapup, then start converting that to other primitives until there are no native primitives left. Avoid creating a thousand lines switch/case statement. Modern compilers will nicely inline small static methods.
The benefit is that each change can be easily reviewed and stress tested (e.g. compare it with the changeset I did for the sapi queuing), and one avoids such things like undoing lchan->s changes that are burried somewhere in the patch.
hi holger,
i am currently out of time, but i will hopefully find the time to take a look at it and provide reworked patches.
regards,
andreas
Holger Hans Peter Freyther wrote:
The extra msgb_alloc/memcpy to go from primitive to native is wasteful. One should re-write the header around the payload.
The change to introduce the primitives is too big. Otherwise the above two issues could have been spotted with review. My proposal is to introduce primitives inside the sysmoBTS code piece by piece in one primitive per patch.
hi holger,
i started splitting the l1sap implementation into parts. the first part is just cover the BCCH RTS/REQ messages. these are moved from osmo-bts-sysmo code to common code. i pushed a new branch where i will commit each part. see last commit of jolly/l1sap_parts branch.
in this commit i found a solution to convert a l1sap message to l1p (sysmo-bts) message without memcpy: the l1p structure is not just a header+payload, but a structure that includes the payload as fixed element. i needed to expand the headroom and tail of a message (allocated at l1sap.c), so the l1p structure can be casted over the existing message. the payload will then be at the right spot inside the l1p structure. the structure's data around the payload will be cleared.
before the actual casting of structure is done, the l1sap header is pulled (data points to payload) and the length is trimmed to 0.
push/clear part of the l1p structure that is in front of the actual payload:
temp = l1p->u.phDataReq.msgUnitParam.u8Buffer; msgb_push(msg, temp - (uint8_t *)l1p); memset(msg->data, 0, msg->len);
put the actual payload that is already inside message:
msgb_put(msg, len); NOTE: 'len' is the actual lenght of payload
put/clear the rest of of the structure that is behind the actual payload.
memset(msg->tail, 0, sizeof(*l1p) - msg->len); msgb_put(msg, sizeof(*l1p) - msg->len); msg->l1h = msg->data;
regards,
andreas
On Sun, Jul 28, 2013 at 01:01:21PM +0200, Andreas Eversberg wrote:
i started splitting the l1sap implementation into parts. the first part is just cover the BCCH RTS/REQ messages. these are moved from osmo-bts-sysmo code to common code. i pushed a new branch where i will commit each part. see last commit of jolly/l1sap_parts branch.
it is still way too big...
* e.g... it adds handling for TCH.. when you are just starting with the BCCH...
primitives left. Avoid creating a thousand lines switch/case statement. Modern compilers will nicely inline small static methods.
* you really don't need to put everything into a single method. Try to put unrelated things into separate methods. Nobody can understand a method that has 1000 lines of codes and tons of if/else in it.
Holger Hans Peter Freyther wrote:
it is still way too big...
- e.g... it adds handling for TCH.. when you are just starting with the BCCH...
- you really don't need to put everything into a single method. Try to put unrelated things into separate methods. Nobody can understand a method that has 1000 lines of codes and tons of if/else in it.
i did some seperation:
- first i committed the l1sap interface header, because it includes all macros that are relevant for for later primitives. - then the actual BCCH implementation, which includes the base parts of the l1sap interface. - last the gsmtap support for the l1sap message primitives. the function is now splitted.
i think that later this gsmtap support should be placed at the end of all primitives patches.
what do you think?
-
Andreas Eversberg -
Holger Hans Peter Freyther