Dear Andreas,
I went through the multislot allocation algorithm and cleaned and structured the code in testable parts that take over parts of the allocation and I have added a unit test for some of the allocation cases (UL first, then DL.. e.g. due a RACH burst and then DL single, UL and DL update).
Today morning I added a testcase that tests all PDCH combinations and all MS Classes and verifies that an allocation takes place and that the DL and UL first_common_ts do match. For (xxOxOOxO) and MS_Class=5 this assert is wrong. The alloc/AllocTest of the sysmocom/allocation-cleanups branch should show you this condition.
While reading the code I noticed the following things. Could you please explain why these are problems or no problems.
* select_first_ts (it exists after the refactoring). It initializes i and compares it but it will never increment it. This means that the code can look at PDCHs _outside_ of the tx_range.
* first_common_ts handling. When assigning the DL tbf we pick a first_common_ts but when the actual UL assignment happens there might not be a free USF on the Uplink and at that point the phone might not listen on the TS we think.
* Sum for Rx+Tx is not used. I see that in update_rx_win_max you modify the window to make some room.
* select_ul_slots. "i" is not incremented in all cases which could potentially lead to using slots outside of the tx_range. For the MS Type == 1 handling you could introduce a different variable that counts how many slots were used?
it would be nice if we could fix that during the 30C3.
holger
Holger Hans Peter Freyther wrote:
select_first_ts (it exists after the refactoring). It initializes i and compares it but it will never increment it. This means that the code can look at PDCHs _outside_ of the tx_range.
first_common_ts handling. When assigning the DL tbf we pick a first_common_ts but when the actual UL assignment happens there might not be a free USF on the Uplink and at that point the phone might not listen on the TS we think.
Sum for Rx+Tx is not used. I see that in update_rx_win_max you modify the window to make some room.
select_ul_slots. "i" is not incremented in all cases which could potentially lead to using slots outside of the tx_range. For the MS Type == 1 handling you could introduce a different variable that counts how many slots were used?
dear holger,
i added serveral fixes that showed up with your test code. i have pushed them to the jolly/allocation-fixes branch. in also includes a fix for the missing incrementation of 'i' in select_first_ts() and select_ul_slots().
the Sum variable is not used, because the algorithm assigns only one uplink time slot. the supports RX slots plus 1 TX slot never exceeds the Sum.
i have no fix for the USF problem. if the first_common_ts on concurrent TBFs is different, we should reject the TBF by sending a Packet Access Reject, but this message is also not implemented.
regards,
andreas
On Sat, Jan 04, 2014 at 04:19:34PM +0100, Andreas Eversberg wrote:
dear holger,
dear andreas,
i added serveral fixes that showed up with your test code. i have pushed them to the jolly/allocation-fixes branch. in also includes a fix for the missing incrementation of 'i' in select_first_ts() and select_ul_slots().
I merged your changes and the crazy test that tested all combinations started to work so I went ahead an merged your change. What Daniel (and the jenkins) noticed is that I didn't update the test result. I asked him to update the test result but he pointed out that there appears to be a genuine regression:
@@ -409,7 +409,6 @@ PDCH[5] is first common for DL PDCH[5] is used for UL PDCH[6] is used for UL -PDCH[7] is used for UL PDCH[5] is control_ts for UL PDCH[5] is first common for UL PDCH[5] is used for DL @@ -420,7 +419,6 @@ Testing jolly example PDCH[1] is used for UL PDCH[2] is used for UL -PDCH[3] is used for UL PDCH[1] is control_ts for UL PDCH[1] is first common for UL PDCH[1] is used for DL
so it appears that the multislot algorithm started to not assign/use the last timeslot. Could you please have a look at that?
i have no fix for the USF problem. if the first_common_ts on concurrent TBFs is different, we should reject the TBF by sending a Packet Access Reject, but this message is also not implemented.
I will add a todo to the code.
Holger Hans Peter Freyther wrote:
hi holger,
thanx for merging the patches.
@@ -409,7 +409,6 @@ PDCH[5] is first common for DL PDCH[5] is used for UL PDCH[6] is used for UL -PDCH[7] is used for UL PDCH[5] is control_ts for UL PDCH[5] is first common for UL PDCH[5] is used for DL @@ -420,7 +419,6 @@ Testing jolly example PDCH[1] is used for UL PDCH[2] is used for UL -PDCH[3] is used for UL PDCH[1] is control_ts for UL PDCH[1] is first common for UL PDCH[1] is used for DL
so it appears that the multislot algorithm started to not assign/use the last timeslot. Could you please have a look at that?
If you look at patch "alloc_algorithm_b: Increment 'i', so allocated TS will not exceed tx_range", i added the missing "i++" in for-loops. inside one loop there is an incrementation (already), which needs to be removed:
- if (++i == ms_max_txslots) { + if (i+1 == ms_max_txslots) {
I compared the output of the AllocTest with the expected output. Now they match.
regards,
andreas
On Wed, Jan 15, 2014 at 01:58:27PM +0100, Andreas Eversberg wrote:
I compared the output of the AllocTest with the expected output. Now they match.
applied. osmo-pcu is back to green/blue on the CI.
osmocom-net-gprs@lists.osmocom.org