Idle PEs compete with comm thread for node queue lock

jcphill commented 8 years ago

Original issue: https://charm.cs.illinois.edu/redmine/issues/1035

We've observed via projections that, in SMP mode when most of the PEs on a node are idle, the communication thread is very slow at adding incoming messages to the node queue. I assume this is because there is a single lock on the node queue, which the PEs are constantly fighting over, preventing the comm thread from adding work. I would suggest a two-level locking approach, adding a consumer lock which PEs must obtain before taking the queue lock to receive messages. In this way idle PEs compete only with each other (which is fine). PEs should try the consumer lock but not wait for it to become available, to not delay checking their private queue. Adding to the node queue should only require the main lock.

jcphill commented 5 years ago

Original date: 2016-04-14 14:26:08

This appears to be the node queue receive function in src/arch/util/machine-common-core.c:

 void *CmiGetNonLocalNodeQ(void) {
    CmiState cs = CmiGetState();
    char *result = 0;
    CmiIdleLock_checkMessage(&cs->idle);
    if (!CMIQueueEmpty(CsvAccess(NodeState).NodeRecv)) {
        MACHSTATE1(3,"CmiGetNonLocalNodeQ begin %d {", CmiMyPe());
        CmiLock(CsvAccess(NodeState).CmiNodeRecvLock);
        result = (char *) CMIQueuePop(CsvAccess(NodeState).NodeRecv);
        CmiUnlock(CsvAccess(NodeState).CmiNodeRecvLock);
        MACHSTATE1(3,"} CmiGetNonLocalNodeQ end %d ", CmiMyPe());
    }
    return result;
 }

Rather than checking that the queue is non-empty and then calling CmiLock, it may be better to call CmiTryLock, e.g.:

 void *CmiGetNonLocalNodeQ(void) {
    CmiState cs = CmiGetState();
    char *result = 0;
    CmiIdleLock_checkMessage(&cs->idle);
    while (!CMIQueueEmpty(CsvAccess(NodeState).NodeRecv)) {
        if (CmiTryLock(CsvAccess(NodeState).CmiNodeRecvLock)) continue;
        MACHSTATE1(3,"CmiGetNonLocalNodeQ begin %d {", CmiMyPe());
        result = (char *) CMIQueuePop(CsvAccess(NodeState).NodeRecv);
        CmiUnlock(CsvAccess(NodeState).CmiNodeRecvLock);
        MACHSTATE1(3,"} CmiGetNonLocalNodeQ end %d ", CmiMyPe());
        break;
    }
    return result;
 }

What is confusing is that PCQueuePop appears to already be thread safe, in which case the real question is why do CmiPushNode() and CmiSendNodeSelf() need to take CmiNodeRecvLock before calling CMIQueuePush?

harshithamenon commented 5 years ago

Original date: 2016-05-12 19:17:56

Jim, Bilge mentioned that you had implemented this and sent a patch to Antti-Pekka but it did not show benefit. Can you update this issue with the details.

Thank you

jcphill commented 5 years ago

Original date: 2016-05-13 16:33:21

I tried the above variant, as well as adding a lock to restrict node queue access to a single consumer at a time. Neither showed improvement in the observed performance.