Change in osmo-bsc[master]: abis_rsl: prioritize emergency calls over regular calls

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

laforge gerrit-no-reply at lists.osmocom.org
Mon Aug 24 07:29:50 UTC 2020


laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/19793 )

Change subject: abis_rsl: prioritize emergency calls over regular calls
......................................................................


Patch Set 1:

(8 comments)

In terms of the general logic of this patch, can you please explan why you decided to make all channel request processin asynchronous?  When working with the related feature request some months ago, I would have expected it to be sufficient to only queue/delay any channel requests if we face contention, i.e. if we try to allocate a channel but none is available anymore.  So on an unloaded system, things would proceed in-line without a queue.  Only if resource exhaustion is hit, and we have an emergency call incoming, we trigger release of an existing lchan and sit on a queue until the lchan release happens.  The queue would then only enqueue
emergency calls; normal RACH requests arriving concurrently would be rejected.

As mentioned in the detailed comments below, I don't like introducing additional delays by means of a timer.  It can be done, but I don't think it's very elegant.

After all, entries in the queue are waiting for lchans to become available.  So when a lchan is fully released, the queue coulds be triggered without a timer.  This way we would continue immediately at a point in time when a lchan is available - also, at this point we are guaranteed that no other concurrent request might be grabbing that lchan (due to synchronous dispatch of the 'lchan release complete' event)

At that point, as we *know* which exact lchan was released, we could actually avoid doing any allocation via the channel allocator, but directly use the just-released lchan.

To make things even more "tight", we could introudce the concept of 'reserving' a channel, i.e. when an emergency RACH starts release of the lchan, we could store a back-pointr within that lchan to the specific 'struct chan_req'.  So once the lchan is released, we know exactly for which emergency call it was reserved,...

Those are all just some ideas. In the end its up to you to figure out which way you'd prefer.

https://gerrit.osmocom.org/c/osmo-bsc/+/19793/1/src/osmo-bsc/abis_rsl.c 
File src/osmo-bsc/abis_rsl.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/19793/1/src/osmo-bsc/abis_rsl.c@1331 
PS1, Line 1331: 	bo
I think all other members are self-explanatory, but this one maybe could deserver a one-line comment.


https://gerrit.osmocom.org/c/osmo-bsc/+/19793/1/src/osmo-bsc/abis_rsl.c@1446 
PS1, Line 1446:  
documentation of return value meaning


https://gerrit.osmocom.org/c/osmo-bsc/+/19793/1/src/osmo-bsc/abis_rsl.c@1451 
PS1, Line 1451: 	if(rqd->reason != GSM_CHREQ_REASON_EMERG)
ws


https://gerrit.osmocom.org/c/osmo-bsc/+/19793/1/src/osmo-bsc/abis_rsl.c@1491 
PS1, Line 1491: skipping
should we not attempt the next lchan in this case?  What happens to the emergency call here?


https://gerrit.osmocom.org/c/osmo-bsc/+/19793/1/src/osmo-bsc/abis_rsl.c@1519 
PS1, Line 1519: if (llist_empty(&bts->chan_rqd_queue)) {
              : 		return 0;
              : 	}
              : 	lh_rqd = bts->chan_rqd_queue.next;
              : 	if(!lh_rqd)
              : 		return 0;
please use the llist_first_entry_or_null() macro of linuxlist.h instead of re-implementing it inline here.


https://gerrit.osmocom.org/c/osmo-bsc/+/19793/1/src/osmo-bsc/abis_rsl.c@1557 
PS1, Line 1557: 
              : 	lchan = lchan_select_by_type(bts, GSM_LCHAN_TCH_F);
              : 
              : 	/* If an emergency call is incoming, the preemption logic already
              : 	 * has made sure that there is at least one TCH/F or TCH/H available,
              : 	 * so we refrain from assigning an SDCCH first, assigning the TCH
              : 	 * dirrectly is faster and safer in this case */
              : 	if (rqd->reason == GSM_CHREQ_REASON_EMERG)
              : 		lchan = NULL;
              : 	else
this looks odd. We first unconditionally select a TCH_F channel, and then set lchan to NULL again?  And in the non-emergency case, we select a SDCCH despite already selecting a TCH_F?

To me it would make more sense to either select a TCH_F (emergency) or SDCCH (non-emergency)?


https://gerrit.osmocom.org/c/osmo-bsc/+/19793/1/src/osmo-bsc/bts.c 
File src/osmo-bsc/bts.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/19793/1/src/osmo-bsc/bts.c@35 
PS1, Line 35: /* timer call-back to poll CHAN RQD queue */
            : static void chan_rqd_poll_cb(void *data)
            : {
            : 	struct gsm_bts *bts = (struct gsm_bts *)data;
            : 	abis_rsl_chan_rqd_queue_poll(bts);
            : 	osmo_timer_schedule(&bts->chan_rqd_queue_timer, 0, 100000);
            : }
I don't really like the idea of the timer.  If I understand it correctly, it will delay every channel request by 0.1 seconds, right?  I don't have a complete solution in my mind yet, but somehow I think it must be possible without such a timer.  

Also, if we do have a timer that is always running and which should always fire in fixed periods (as opposed to a reconfigurable timer that's not running all the time, and at different intervals), osmo_timerfd might be the better choice.  You start it once and it keeps firing at periodic intervals.


https://gerrit.osmocom.org/c/osmo-bsc/+/19793/1/src/osmo-bsc/lchan_select.c 
File src/osmo-bsc/lchan_select.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/19793/1/src/osmo-bsc/lchan_select.c@254 
PS1, Line 254: lchan_select_avail
naming.  We are not selecting a channel, as we are not returning it, or performing any action on it.  We are determining if a lchan of given type is available.  Something like 'lchan_is_available' would be more logical, IMHO.



-- 
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/19793
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: If8651265928797dbda9f528b544931dcfa4a0b36
Gerrit-Change-Number: 19793
Gerrit-PatchSet: 1
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: laforge <laforge at osmocom.org>
Gerrit-Comment-Date: Mon, 24 Aug 2020 07:29:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20200824/45aeb637/attachment.htm>


More information about the gerrit-log mailing list