https://osmocom.org/https://osmocom.org/favicon.ico?16647414092021-02-11T17:33:37ZOpen Source Mobile CommunicationsOsmoPCU - Bug #5020: osmo-pcu: Poll and SBA allocation improvements and fixeshttps://osmocom.org/issues/5020?journal_id=212662021-02-11T17:33:37Zpespin
<ul></ul><p>The last point (TBF only supporting 1 active poll FN) is worse than expected, because PollController seems to be timing out non-acked polls for a larger period of time than the FN+13 one:<br /><pre>
void bts_set_current_frame_number(struct gprs_rlcmac_bts *bts, int fn)
{
/* The UL frame numbers lag 3 behind the DL frames and the data
* indication is only sent after all 4 frames of the block have been
* received. Sometimes there is an idle frame between the end of one
* and start of another frame (every 3 blocks). So the timeout should
* definitely be there if we're more than 8 frames past poll_fn. Let's
* stay on the safe side and say 13 or more. An additional delay can
* happen due to the block processing time in the DSP, so the delay of
* decoded blocks relative to the timing clock can be much larger.
* Values up to 50 frames have been observed under load. */
const static int max_delay = 60;
bts->cur_fn = fn;
bts->pollController->expireTimedout(bts->cur_fn, max_delay);
}
</pre></p>
<p>For the ACKed one's is fine, since gprs_rlcmac_pdch::rcv_control_ack() calls:<br /><pre>
TBF_POLL_SCHED_UNSET(tbf);
</pre></p> OsmoPCU - Bug #5020: osmo-pcu: Poll and SBA allocation improvements and fixeshttps://osmocom.org/issues/5020?journal_id=213312021-02-17T19:01:11Zpespin
<ul><li><strong>Related to</strong> <i><a class="issue tracker-1 status-3 priority-2 priority-default closed" href="/issues/5033">Bug #5033</a>: N3101 potentially being updated only during RBBP poll but not during USF poll</i> added</li></ul> OsmoPCU - Bug #5020: osmo-pcu: Poll and SBA allocation improvements and fixeshttps://osmocom.org/issues/5020?journal_id=214762021-03-04T16:27:25Zpespin
<ul></ul><p>Some more facts after further investigation:</p>
<ul>
<li>bts->cur_fn holds the current/last MAC block received on the UL, so it drives the clock.</li>
<li>The bts->cur_fn ield is updated in 2 code paths:
<ul>
<li>bts_set_current_frame_number: the main way, triggered by PCUIF time_ind received on every start of MAC block FN from BTS (even when direct phy is used, TIME.IND in lower layers are only received in BTS)</li>
<li>bts_set_current_block_frame_number: used only by sysmo direct PHY (not used by other direct phy like oc2g) to correct the clock (bts->cur_fn) in case there was some drift detected between what comes from PCUIF info_ind and its own direct phy.</li>
</ul></li>
</ul>
<p>So, the idea here would be to:<br />1- Make sure generic osmo-pcu code supports receiving data_len=0 minimally: "pcu_rx_data() => pcu_rx_data_ind_pdtch() => pdch->rcv_block()" This should already be fine OK since mcs_get_by_size_ul() will return UNKNOWN=0 and early return printing an error. So this first step is done.<br />2- Make sure all phys always send data_ind even if no data received, or corrupted, by setting data_len = 0 (aka NOPE.ind)<br />3- In osmo-pcu, ignore received time_ind messages (at most cross-check the FN and print warning otherwise), and update the clock (bts_set_current_frame_number) during data_ind instead.<br />4- In osmo-pcu upper layers, stored scheduled USF for that BTS+TRX+TS+FN and if data_ind data_len=0 (NOPE.ind), then increment N3101 for that TBF.</p>
<p>Step 2 (backend sending NOPE.ind data_len=0) requires:<br />2.1- Allow direct phy osmo-pcu code to submit corrupt/empty data_ind. For example, for sysmo direct phy:<br /><pre>
handle_ph_data_ind (sysmo_l1_if.c)
pcu_rx_block_time
bts_set_current_block_frame_number <--- RX FN number is corrected here
bts->pollController->expireTimedout(fn, max_delay);
if (data_ind->msgUnitParam.u8Size == 0)
return -1; <--- NOPE.ind is discarded here
" /* drop incomplete UL block */ if (data_ind->msgUnitParam.u8Buffer[0] != GsmL1_PdtchPlType_Full) break; <--- NOPE.ind is discarded here
pcu_rx_data_ind_pdtch
</pre><br />2.1- Allow indrect phy from osmo-bts to send PCUIF INFO_IND with data_len=0 (osmo-bts.git/src/common/l1sap.c):<br /><pre>
l1sap_ph_data_ind:
if (ts_is_pdch(&trx->ts[tn])) {
...
/* don't send bad frames to PCU */
if (len == 0)
return -EINVAL; <--- NOPE.ind is discarded here
/* drop incomplete UL block */
if (pr_info != PRES_INFO_BOTH)
return 0; <--- NOPE.ind is discarded here
pcu_tx_data_ind(...) <----- This function contains a memcpy(x, y, len) which must be done conditonal on len>0 (undefined behavior of memcpy for len=0).
reutrn 0;
}
</pre></p> OsmoPCU - Bug #5020: osmo-pcu: Poll and SBA allocation improvements and fixeshttps://osmocom.org/issues/5020?journal_id=214942021-03-05T18:32:02Zpespin
<ul></ul><p>I updated osmo-bts-trx, osmo-pcu and TTCN3 PCUIF_Components to send all DATA.ind even if len=0. Related patches:</p>
osmo-bts.git:
<ul>
<li><a class="external" href="https://gerrit.osmocom.org/c/osmo-bts/+/23249">https://gerrit.osmocom.org/c/osmo-bts/+/23249</a> l1sap: Transmit pdtch invalid MAC blocks to PCU</li>
<li><a class="external" href="https://gerrit.osmocom.org/c/osmo-bts/+/23257">https://gerrit.osmocom.org/c/osmo-bts/+/23257</a> bts-trx: Always submit rx PDTCH DATA.ind to l1sap</li>
<li>TODO: implement change for lower layers of other bts types!</li>
</ul>
osmo-pcu.git:
<ul>
<li><a class="external" href="https://gerrit.osmocom.org/c/osmo-pcu/+/23260">https://gerrit.osmocom.org/c/osmo-pcu/+/23260</a> pdch: Silently ignore DATA.ind with len=0</li>
<li><a class="external" href="https://gerrit.osmocom.org/c/osmo-pcu/+/23261">https://gerrit.osmocom.org/c/osmo-pcu/+/23261</a> Track TDMA clock with DATA.ind instead of TIME.ind</li>
</ul>
osmo-ttcn3-hacks.git:
<ul>
<li><a class="external" href="https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/23256">https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/23256</a> pcu: transmit PCUIF DATA.ind with len=0 when no UL data to transmit</li>
</ul>
docker-plaground.git:
<ul>
<li><a class="external" href="https://gerrit.osmocom.org/c/docker-playground/+/23262">https://gerrit.osmocom.org/c/docker-playground/+/23262</a> ttcn3-pcu: Disable sending all DATA.ind on pcu -latest</li>
</ul>
<p>I did some manual tests with osmo-bts-trx and osmo-pcu patches applied and everything looks good.<br />I also checked that TTCN3 patches still work fine with older versions of osmo-pcu.</p>
<p>osmo-ttcn3-hacks + docker can already be merged. The first osmo-pcu patch silently ignorig data_len=0 too. Other ones I want to first implement it for all BTS types and are marked as WIP in gerrit.</p> OsmoPCU - Bug #5020: osmo-pcu: Poll and SBA allocation improvements and fixeshttps://osmocom.org/issues/5020?journal_id=215232021-03-08T12:28:30Zpespin
<ul></ul><p>pespin wrote:</p>
<blockquote>
osmo-bts.git:
<ul>
<li>TODO: implement change for lower layers of other bts types!</li>
</ul>
</blockquote>
<p>Actually the code in osmo-bts.git is OK and already letting pass of size=0 and alike, due to previous work we already did with TCH channels.<br />The missing changes are in osmo-pcu for the direct phy case. I submitted a patch here:<br /><a class="external" href="https://gerrit.osmocom.org/c/osmo-pcu/+/23268">https://gerrit.osmocom.org/c/osmo-pcu/+/23268</a> direct_phy: Support submitting DATA.ind with len=0 to upper layers</p> OsmoPCU - Bug #5020: osmo-pcu: Poll and SBA allocation improvements and fixeshttps://osmocom.org/issues/5020?journal_id=217352021-03-24T18:18:53Zpespin
<ul><li><strong>Status</strong> changed from <i>New</i> to <i>In Progress</i></li><li><strong>% Done</strong> changed from <i>0</i> to <i>60</i></li></ul><p>remote: <a class="external" href="https://gerrit.osmocom.org/c/osmo-pcu/+/23486">https://gerrit.osmocom.org/c/osmo-pcu/+/23486</a> Fix: left shift cannot be repesented in type int [NEW]<br />remote: <a class="external" href="https://gerrit.osmocom.org/c/osmo-pcu/+/23487">https://gerrit.osmocom.org/c/osmo-pcu/+/23487</a> sched: Fix scheduling UL TBF not matching conditions [NEW]<br />remote: <a class="external" href="https://gerrit.osmocom.org/c/osmo-pcu/+/23488">https://gerrit.osmocom.org/c/osmo-pcu/+/23488</a> sched: Simplify usf selection code [NEW]<br />remote: <a class="external" href="https://gerrit.osmocom.org/c/osmo-pcu/+/23489">https://gerrit.osmocom.org/c/osmo-pcu/+/23489</a> Set matching USF if available when polling a UL TBF [NEW]<br />remote: <a class="external" href="https://gerrit.osmocom.org/c/osmo-pcu/+/23490">https://gerrit.osmocom.org/c/osmo-pcu/+/23490</a> pdch: Add mising pdch_ulc_release_node in Rx Cell Change Notif [NEW]<br />remote: <a class="external" href="https://gerrit.osmocom.org/c/osmo-pcu/+/23491">https://gerrit.osmocom.org/c/osmo-pcu/+/23491</a> pdch_ulc: Create helper API pdch_ulc_release_node [NEW]<br />remote: <a class="external" href="https://gerrit.osmocom.org/c/osmo-pcu/+/23492">https://gerrit.osmocom.org/c/osmo-pcu/+/23492</a> Track scheduled UL blocks through USF [NEW]<br />remote: <a class="external" href="https://gerrit.osmocom.org/c/osmo-pcu/+/23493">https://gerrit.osmocom.org/c/osmo-pcu/+/23493</a> Properly implement N3101 [NEW]</p>
After this bunch of patches become merged, we already track all sort of UL blocks in a unified way. What's missing regarding this ticket:
<ul>
<li>Improve SBA scheduler to offer something else than N+52 (AGCH_START_OFFSET)</li>
<li>Improve SBA/POLL scheduler to offer something else than N+13</li>
</ul> OsmoPCU - Bug #5020: osmo-pcu: Poll and SBA allocation improvements and fixeshttps://osmocom.org/issues/5020?journal_id=217482021-03-26T13:07:57Zpespin
<ul></ul><p>So, as per SBA allocation (Imm Assignment on CCCH):</p>
<p>The scheduled FN for the SBA is currently hardcoded to 52 (sba.c):<br /><pre>
/* starting time for assigning single slot
* This offset must be a multiple of 13. */
#define AGCH_START_OFFSET 52
struct gprs_rlcmac_sba *sba_alloc(void *ctx, struct gprs_rlcmac_pdch *pdch, uint8_t ta)
{
struct gprs_rlcmac_sba *sba;
sba = talloc_zero(ctx, struct gprs_rlcmac_sba);
if (!sba)
return NULL;
sba->pdch = pdch;
sba->ta = ta;
/* TODO: request ULC for next available FN instead of hardcoded AGCH_START_OFFSET */
sba->fn = next_fn(pdch->last_rts_fn, AGCH_START_OFFSET);
pdch_ulc_reserve_sba(pdch->ulc, sba);
return sba;
}
</pre></p>
<p>Then, that FN is fed into Imm Assignment when encoding it (bts.cpp bts_rcv_rach):<br /><pre>
sba = bts_alloc_sba(bts, ta);
sb_fn = sba->fn;
plen = Encoding::write_immediate_assignment(
&bts->trx[trx_no].pdch[ts_no], tbf, bv,
false, rip->ra, Fn, ta, usf, false, sb_fn,
bts_get_ms_pwr_alpha(bts), bts->pcu->vty.gamma, -1,
rip->burst_type);
</pre></p>
<p>And sba_Fn is used here in Encoding::write_immediate_assignment as ref_fn to indicate the offset to the MS to start using it:<br /><pre>
bitvec_write_field(dest, &wp,(ref_fn / (26 * 51)) % 32,5); // T1'
bitvec_write_field(dest, &wp,ref_fn % 51,6); // T3
bitvec_write_field(dest, &wp,ref_fn % 26,5); // T2
</pre></p>
<p>Now, the question is why is 52 being used, and why it must be multiple of 13... Need to find that in the spec.</p>
<p>The symbol AGCH_START_OFFSET and the related comment about being multiple of 13 was added here, without much explanation:<br /><pre>
commit 07e97cf8a551b05d7f5f3f9583b68b2eff0f1c23
Author: Andreas Eversberg <jolly@eversberg.eu>
Date: Tue Aug 7 16:00:56 2012 +0200
Adding single block allocation
It is mandatory to support it because MS may request a single block.
In this case the network must assign a single block.
It is possible to force single block allocation for all uplink requests
on RACH. (VTY option)
</pre></p> OsmoPCU - Bug #5020: osmo-pcu: Poll and SBA allocation improvements and fixeshttps://osmocom.org/issues/5020?journal_id=217852021-03-29T12:12:25Zpespin
<ul></ul><p>After these patches, FN for SBA and RRBP poll is scheduled based on available/unreserved FNs in the UL scheduler:<br />remote: <a class="external" href="https://gerrit.osmocom.org/c/osmo-pcu/+/23520">https://gerrit.osmocom.org/c/osmo-pcu/+/23520</a> Pick unreserved UL FN when allocating an SBA<br />remote: <a class="external" href="https://gerrit.osmocom.org/c/osmo-pcu/+/23522">https://gerrit.osmocom.org/c/osmo-pcu/+/23522</a> pdch_ulc: Support picking RRBP other than N+13 [NEW]</p>
So, after those are merged, what's TODO:
<ul>
<li>modify SBA allocator offset in sba_alloc() based on AGCH queue load in the BTS</li>
<li>Update gprs_rlcmac_tbf poll_state/poll_fn/poll_ts implementation to support multiple concurrent polls allocated.</li>
</ul> OsmoPCU - Bug #5020: osmo-pcu: Poll and SBA allocation improvements and fixeshttps://osmocom.org/issues/5020?journal_id=217872021-03-29T17:32:01Zpespin
<ul><li><strong>% Done</strong> changed from <i>60</i> to <i>80</i></li></ul><p>I submitted a bunch of commits to get rid of old tbf's poll_fn/poll_ts infra which only supported 1 concurrent poll request per tbf here:<br />remote: <a class="external" href="https://gerrit.osmocom.org/c/osmo-pcu/+/23525">https://gerrit.osmocom.org/c/osmo-pcu/+/23525</a> pdch_ulc: Store TBF poll reason [NEW]<br />remote: <a class="external" href="https://gerrit.osmocom.org/c/osmo-pcu/+/23526">https://gerrit.osmocom.org/c/osmo-pcu/+/23526</a> tbf: Get rid of unneeded poll_scheduled() [NEW]<br />remote: <a class="external" href="https://gerrit.osmocom.org/c/osmo-pcu/+/23527">https://gerrit.osmocom.org/c/osmo-pcu/+/23527</a> tbf: Allow multiple concurrent polls [NEW]<br />remote: <a class="external" href="https://gerrit.osmocom.org/c/osmo-pcu/+/23528">https://gerrit.osmocom.org/c/osmo-pcu/+/23528</a> Remove unneeded poll_state check [NEW]<br />remote: <a class="external" href="https://gerrit.osmocom.org/c/osmo-pcu/+/23529">https://gerrit.osmocom.org/c/osmo-pcu/+/23529</a> tbf: get rid of poll_state completely [NEW]<br />remote: <a class="external" href="https://gerrit.osmocom.org/c/osmo-pcu/+/23530">https://gerrit.osmocom.org/c/osmo-pcu/+/23530</a> Get rid of param 'poll' with constant value [NEW]<br />remote: <a class="external" href="https://gerrit.osmocom.org/c/osmo-pcu/+/23531">https://gerrit.osmocom.org/c/osmo-pcu/+/23531</a> tbf: Get rid of attribute poll_fn [NEW]<br />remote: <a class="external" href="https://gerrit.osmocom.org/c/osmo-pcu/+/23532">https://gerrit.osmocom.org/c/osmo-pcu/+/23532</a> tbf: Get rid of attribute poll_ts [NEW]</p> OsmoPCU - Bug #5020: osmo-pcu: Poll and SBA allocation improvements and fixeshttps://osmocom.org/issues/5020?journal_id=219392021-04-20T12:52:38Zpespin
<ul><li><strong>Related to</strong> <i><a class="issue tracker-2 status-1 priority-2 priority-default" href="/issues/5122">Feature #5122</a>: Choose SBA allocation offset based on AGCH queue load in the BTS</i> added</li></ul> OsmoPCU - Bug #5020: osmo-pcu: Poll and SBA allocation improvements and fixeshttps://osmocom.org/issues/5020?journal_id=219412021-04-20T12:53:45Zpespin
<ul><li><strong>Status</strong> changed from <i>In Progress</i> to <i>Resolved</i></li><li><strong>% Done</strong> changed from <i>80</i> to <i>100</i></li></ul><p>Patches were merged.</p>
<p>I created a separate ticket to track the SBA allocation fixed delay here:<br /><a class="external" href="https://osmocom.org/issues/5122">https://osmocom.org/issues/5122</a></p>
<p>Hence, closing this ticket. Other detailed ticket can be created in the future to track specific issues/improvements.</p>