[asterisk-dev] ringinuse seems to include busy; also performance of available queue member search

[Dave WOOLLEY]

Whilst looking at the performance of the search for an available queue member I've come across code that handles the ringinuse flag that seems to count busy an inuse as the same, that doesn't make sense to me.  I've also come across very long standing code that means ringinuse disables state checks entirely, allowing the code ot fall through to the, very expensive, compare_weights code.  Finally, the compare_weights code seems to waste a lot of time trying to match a member in queues that don't have higher weights.

Looking at the current trunk version, revision 325483 introduced additional "case" lines into, what is now, is_member_available, which treat busy and inuse the same:

2310                       case AST_DEVICE_INUSE:
2311                       case AST_DEVICE_BUSY:
2312                       case AST_DEVICE_RINGING:
2313                       case AST_DEVICE_RINGINUSE:
2314                       case AST_DEVICE_ONHOLD:
2315                                       if (!mem->ringinuse) {
2316                                                       break;
2317                                       }

Whilst this postdates the code we are using, so doesn't directly affect us, this seems wrong to me; I thought the whole subtlety behind calling it ringinuse, not ringbusy, is that there is a difference between busy and in use.  Could someone explain?

Next,  in can_ring_entry, ringinuse set completely bypasses the device state check:

4169       if (!call->member->ringinuse && !member_status_available(call->member->status)) {
4170                       ast_debug(1, "%s not available, can't receive call\n", call->interface);
4171                       return 0;
4172       }
The consequence of this is that, if ringinuse is set, even if the member is unavailable (e.g. agent logged out), or definitively busy, the code falls through to:

4182       if (use_weight && compare_weight(qe->parent, call->member)) {
4183                       ast_debug(1, "Priority queue delaying call to %s:%s\n",
4184                                       qe->parent->name, call->interface);
4185                       return 0;
4186       }
compare_weight is very expensive, because it iterates over all queues and all members of them. Also, at least in the version we are using, although I haven't checked this in trunk, there is a lock held on the queue of interest and locks are taken, in enumeration sequence, on all the other queues.  This looks to me that it will cause serialisation of access to that function.

So, finally, looking at compare_weight, this line:

4080                                                       if (q->weight > rq->weight && q->count >= num_available_members(q)) {
Is run after this line:

4078                                       if ((mem = ao2_find(q->members, member, OBJ_POINTER))) {

Whilst the second term is expensive, involving a scan of all the members, the first term is cheap.  In our case, there are lots of members in common between lots of queues, a lot of which are not at high priority.  It seems to me that doing the cheap test before the search for the member, would quickly eliminate queues that could never pre-empt the agent..  Even if ao2_find hashes, it is still going to be expensive compared with a compare of two, one level indirect, scalars.

One other observation, is that compare_weight doesn't account for members that are unavailable to the conflicting queue because there is an even higher priority queue.  NB the member in question need not be the same member as is being considered for the call.  (Without a more efficient algorithm, recursing the check would add additional multipliers to the computational complexity, so could well be unworkable.)


BTS Holdings PLC - Registered office: BTS House, Manor Road, Wallington, SM6 0DD - Registered in England: 1517630
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20141120/504aab52/attachment.html>