Good Morning,
sched_poll is doing:
llist_for_each_entry(tbf, &gprs_rlcmac_ul_tbfs, list) { /* this trx, this ts */ if (tbf->trx_no != trx || tbf->control_ts != ts) continue; /* polling for next uplink block */ if (tbf->poll_state == GPRS_RLCMAC_POLL_SCHED && tbf->poll_fn == poll_fn) *poll_tbf = tbf; if (tbf->ul_ack_state == GPRS_RLCMAC_UL_ACK_SEND_ACK) *ul_ack_tbf = tbf; if (tbf->dl_ass_state == GPRS_RLCMAC_DL_ASS_SEND_ASS) *dl_ass_tbf = tbf; if (tbf->ul_ass_state == GPRS_RLCMAC_UL_ASS_SEND_ASS) *ul_ass_tbf = tbf; } llist_for_each_entry(tbf, &gprs_rlcmac_dl_tbfs, list) { /* this trx, this ts */ if (tbf->trx_no != trx || tbf->control_ts != ts) continue; /* polling for next uplink block */ if (tbf->poll_state == GPRS_RLCMAC_POLL_SCHED && tbf->poll_fn == poll_fn) *poll_tbf = tbf; if (tbf->dl_ass_state == GPRS_RLCMAC_DL_ASS_SEND_ASS) *dl_ass_tbf = tbf; if (tbf->ul_ass_state == GPRS_RLCMAC_UL_ASS_SEND_ASS) *ul_ass_tbf = tbf; }
New tbf's will be added to the front (llist_add). Is it on purpose that the dl_tbfs are preferred over ul_tbfs and that the last of each tbf's will be found? What is the reasoning for this? What will collect/catch poll_fn's that are in the past?
holger
Holger Hans Peter Freyther wrote:
Good Morning,
sched_poll is doing:
llist_for_each_entry(tbf, &gprs_rlcmac_ul_tbfs, list) { /* this trx, this ts */ if (tbf->trx_no != trx || tbf->control_ts != ts) continue; /* polling for next uplink block */ if (tbf->poll_state == GPRS_RLCMAC_POLL_SCHED && tbf->poll_fn == poll_fn) *poll_tbf = tbf; if (tbf->ul_ack_state == GPRS_RLCMAC_UL_ACK_SEND_ACK) *ul_ack_tbf = tbf; if (tbf->dl_ass_state == GPRS_RLCMAC_DL_ASS_SEND_ASS) *dl_ass_tbf = tbf; if (tbf->ul_ass_state == GPRS_RLCMAC_UL_ASS_SEND_ASS) *ul_ass_tbf = tbf; } llist_for_each_entry(tbf, &gprs_rlcmac_dl_tbfs, list) { /* this trx, this ts */ if (tbf->trx_no != trx || tbf->control_ts != ts) continue; /* polling for next uplink block */ if (tbf->poll_state == GPRS_RLCMAC_POLL_SCHED && tbf->poll_fn == poll_fn) *poll_tbf = tbf; if (tbf->dl_ass_state == GPRS_RLCMAC_DL_ASS_SEND_ASS) *dl_ass_tbf = tbf; if (tbf->ul_ass_state == GPRS_RLCMAC_UL_ASS_SEND_ASS) *ul_ass_tbf = tbf; }New tbf's will be added to the front (llist_add). Is it on purpose that the dl_tbfs are preferred over ul_tbfs and that the last of each tbf's will be found? What is the reasoning for this? What will collect/catch poll_fn's that are in the past?
holger
the function sched_poll() only refers to control messages and polling. for every frame number (fn) there is only one mobile that can be polled. polling is requested 13 frames earlier in a downlink RLC/MAC control block. because each downlink RLC/MAC control block addresses only one TBF, no more than one TBF will have the poll state set to GPRS_RLCMAC_POLL_SCHED for the current fn, and so there is no prio in the function above.
generally: the uplink blocks that are polled have prio over normal uplink data blocks. (uplink traffic can wait, polled control messages not) the control messages (uplink ack, downlin/uplink assignment) have prio over normal downlink data blocks. (downlink traffic can wait, control messages not)
assignment messges have prio over uplink ack messages. i did not implement any fairness for assignment control messages of each TBF, because the number of assignment control messages are always much lower than the number of data blocks, so there is no congestion, and i think there is no reason for any round-robin scheduling.
if a poll_fn is in the past (time skew or no response), pcu_rx_time_ind() will discover that.
On Thu, Oct 17, 2013 at 01:42:51PM +0200, Andreas Eversberg wrote:
if a poll_fn is in the past (time skew or no response), pcu_rx_time_ind() will discover that.
I stumbled on this code earlier today. From a software engineering point of view this code (and the other PCU code) is really problematic.
The concern of the pcu_rx_time_ind is to convert from low-level hardware primtive to inform other parts of the system. It should not be responsible for TBF/SBA timeout handling. It is the application of information hiding (encapsulation) and this concept is quite old.
If one manages to apply information hiding and create proper modules the complexity of the code is reduced (e.g. no unexpected side effects, modifications to internal state of a tbf) and one can start to test the parts of the application.
In terms of software engineering one should always ask the question of what is the secret of this class/module/function...
holger
Holger Hans Peter Freyther wrote:
The concern of the pcu_rx_time_ind is to convert from low-level hardware primtive to inform other parts of the system. It should not be responsible for TBF/SBA timeout handling. It is the application of information hiding (encapsulation) and this concept is quite old.
it is some kind of a hack. since we don't get bad frame indications from bts for sure, i had to find a way to detect when frames have not been received. normally the pcu_rx_data_ind function should call the timeout function in case of a bad frame. in this case the timeout function should actually be renamed to some kind like gprs_rlcmac_bfi(). if there is a way to fix bad frame indications from sysmobts, i would change the osmo-bts-trx code too, so both will send bad frame indications and we can get rid of this hack.
On Thu, Oct 17, 2013 at 03:17:17PM +0200, Andreas Eversberg wrote:
it is some kind of a hack. since we don't get bad frame indications from bts for sure, i had to find a way to detect when frames have not been received. normally the pcu_rx_data_ind function should call the timeout function in case of a bad frame. in this case the timeout function should actually be renamed to some kind like gprs_rlcmac_bfi(). if there is a way to fix bad frame indications from sysmobts, i would change the osmo-bts-trx code too, so both will send bad frame indications and we can get rid of this hack.
Andreas,
the point is not about if polling should be used but how the code is structured. E.g. why does the polling belong into the part of pcu_rx_data_ind?
For me the responsibility of pcu_l1if.c is to communicate with the BTS (dispatch indications, send primitives, handle the socket). I would have never expected this code to be in a function with that name.
Mixing code like this is problematic because:
* It is increasing the depdency between the pcu_l1if and the rest of the code. This means understanding the PCU is more complicated and modification is more difficult/dangerous
* It makes it more difficult to test the different parts. E.g. a very basic test for pcu_rx_time_ind would be that the current fn is updated. Currently this can not be tested.
osmocom-net-gprs@lists.osmocom.org
-
Andreas Eversberg -
Holger Hans Peter Freyther