<p style="white-space: pre-wrap; word-wrap: break-word;">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<br>emergency calls; normal RACH requests arriving concurrently would be rejected.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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)</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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,...</p><p style="white-space: pre-wrap; word-wrap: break-word;">Those are all just some ideas. In the end its up to you to figure out which way you'd prefer.</p><p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19793">View Change</a></p><p>8 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19793/1/src/osmo-bsc/abis_rsl.c">File src/osmo-bsc/abis_rsl.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19793/1/src/osmo-bsc/abis_rsl.c@1331">Patch Set #1, Line 1331:</a> <code style="font-family:monospace,monospace"> bo</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think all other members are self-explanatory, but this one maybe could deserver a one-line comment.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19793/1/src/osmo-bsc/abis_rsl.c@1446">Patch Set #1, Line 1446:</a> <code style="font-family:monospace,monospace"> </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">documentation of return value meaning</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19793/1/src/osmo-bsc/abis_rsl.c@1451">Patch Set #1, Line 1451:</a> <code style="font-family:monospace,monospace"> if(rqd->reason != GSM_CHREQ_REASON_EMERG)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ws</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19793/1/src/osmo-bsc/abis_rsl.c@1491">Patch Set #1, Line 1491:</a> <code style="font-family:monospace,monospace">skipping</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">should we not attempt the next lchan in this case? What happens to the emergency call here?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19793/1/src/osmo-bsc/abis_rsl.c@1519">Patch Set #1, Line 1519:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">if (llist_empty(&bts->chan_rqd_queue)) {<br> return 0;<br> }<br> lh_rqd = bts->chan_rqd_queue.next;<br> if(!lh_rqd)<br> return 0;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">please use the llist_first_entry_or_null() macro of linuxlist.h instead of re-implementing it inline here.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19793/1/src/osmo-bsc/abis_rsl.c@1557">Patch Set #1, Line 1557:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><br> lchan = lchan_select_by_type(bts, GSM_LCHAN_TCH_F);<br><br> /* If an emergency call is incoming, the preemption logic already<br> * has made sure that there is at least one TCH/F or TCH/H available,<br> * so we refrain from assigning an SDCCH first, assigning the TCH<br> * dirrectly is faster and safer in this case */<br> if (rqd->reason == GSM_CHREQ_REASON_EMERG)<br> lchan = NULL;<br> else<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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?</p><p style="white-space: pre-wrap; word-wrap: break-word;">To me it would make more sense to either select a TCH_F (emergency) or SDCCH (non-emergency)?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19793/1/src/osmo-bsc/bts.c">File src/osmo-bsc/bts.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19793/1/src/osmo-bsc/bts.c@35">Patch Set #1, Line 35:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">/* timer call-back to poll CHAN RQD queue */<br>static void chan_rqd_poll_cb(void *data)<br>{<br> struct gsm_bts *bts = (struct gsm_bts *)data;<br> abis_rsl_chan_rqd_queue_poll(bts);<br> osmo_timer_schedule(&bts->chan_rqd_queue_timer, 0, 100000);<br>}<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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. </p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19793/1/src/osmo-bsc/lchan_select.c">File src/osmo-bsc/lchan_select.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19793/1/src/osmo-bsc/lchan_select.c@254">Patch Set #1, Line 254:</a> <code style="font-family:monospace,monospace">lchan_select_avail</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19793">change 19793</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/osmo-bsc/+/19793"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-bsc </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: If8651265928797dbda9f528b544931dcfa4a0b36 </div>
<div style="display:none"> Gerrit-Change-Number: 19793 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-CC: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 24 Aug 2020 07:29:50 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Gerrit-MessageType: comment </div>