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

[Mark Michelson]

On 11/20/2014 07:31 AM, Dave WOOLLEY wrote:
>
> 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?
>

Yeah that looks wrong to me. When I was last involved in writing queue 
behavior code,  the queue was supposed to never attempt to send calls to 
busy members, ever. The ringinuse option should only have an effect on 
members who are in use. It should have no effect on members who are 
busy. It's possible that the code moved in its current direction 
"organically" through community patches, or it could just be a mistake.

> 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.
>

This is a case where patches are certainly welcome. The q->members 
container should be a hash table for all queue strategies aside from the 
linear strategy. That requires the use of a linked list. Assuming the 
worst case at all times, you're right that the search for a specific 
member is going to be more expensive than the check of the queue weight. 
This could probably be refactored to compare weights first, then find 
the specified member, and then finally check the number of available 
members in the queue.

> 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/646298aa/attachment-0001.html>