ib_destroy_cm_id() versus cm callback race ?

DISCUSSION Bart Van Assche 2 participants 7 messages Apr 27, 2012 – May 1, 2012 6 messages not in the archive InfiniBand

lore

Share & tools

lore

Bart Van Assche

author

2012-04-27 15:57 UTC #0 lore raw

Hello,

If I interpret the source code in drivers/infiniband/core/cm.c correctly ib_destroy_cm_id() can return before an ongoing cm_id callback has finished. Is this on purpose ? If not, isn't there a flush_workqueue(cm.wq) call missing in cm_destroy_id() ?

Thanks,

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

Hefty, Sean

2012-04-27 17:18 UTC #2 lore raw

Quoted 4 lines

If I interpret the source code in drivers/infiniband/core/cm.c correctly ib_destroy_cm_id() can return before an ongoing cm_id callback has finished. Is this on purpose ? If not, isn't there a flush_workqueue(cm.wq) call missing in cm_destroy_id() ?

ib_destroy_cm_id() will block while there's an outstanding reference held on the id. We'll hold a reference on an id if we have any MAD outstanding against it (which can result in a send callback) or during the processing of a receive.

- Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 1828884A29C6694DAF28B7E6B8A82373469D891F-P5GAC/[email protected]

Bart Van Assche

author

2012-04-28 09:56 UTC #4 lore raw

Quoted 10 lines · Sean

Quoted 4 lines
If I interpret the source code in drivers/infiniband/core/cm.c correctly ib_destroy_cm_id() can return before an ongoing cm_id callback has finished. Is this on purpose ? If not, isn't there a flush_workqueue(cm.wq) call missing in cm_destroy_id() ?
ib_destroy_cm_id() will block while there's an outstanding reference
held on the id. We'll hold a reference on an id if we have any MAD outstanding against it (which can result in a send callback) or during the processing of a receive.

Hello Sean,

Thanks for the quick reply. Something else: I've noticed that the IB CM work queue is created as follows:

    cm.wq = create_workqueue("ib_cm");

That makes me wonder how it is prevented that two CM callbacks for the same CM ID run concurrently on different CPUs ?

Thanks,

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

Hefty, Sean

2012-04-30 18:29 UTC #6 lore raw

PiBUaGF0IG1ha2VzIG1lIHdvbmRlciBob3cgaXQgaXMgcHJldmVudGVkIHRoYXQgdHdvIENNIGNh bGxiYWNrcyBmb3IgdGhlDQo+IHNhbWUgQ00gSUQgcnVuIGNvbmN1cnJlbnRseSBvbiBkaWZmZXJl bnQgQ1BVcyA/DQoNClRoZSBjYWxsYmFjayBjb2RlIGVuZHMgdXAgbG9va2luZyBsaWtlIHRoaXM6 DQoNCiAgICAgICAgcmV0ID0gYXRvbWljX2luY19hbmRfdGVzdCgmY21faWRfcHJpdi0+d29ya19j b3VudCk7DQogICAgICAgIGlmICghcmV0KQ0KICAgICAgICAgICAgICAgIGxpc3RfYWRkX3RhaWwo JndvcmstPmxpc3QsICZjbV9pZF9wcml2LT53b3JrX2xpc3QpOw0KICAgICAgICBzcGluX3VubG9j a19pcnEoJmNtX2lkX3ByaXYtPmxvY2spOw0KDQogICAgICAgIGlmIChyZXQpDQogICAgICAgICAg ICAgICAgY21fcHJvY2Vzc193b3JrKGNtX2lkX3ByaXYsIHdvcmspOw0KDQpPbmx5IDEgdGhyZWFk IHdpbGwgZW5kIHVwIGludm9raW5nIGNhbGxiYWNrcyB0byB0aGUgdXNlci4gIE90aGVyIGV2ZW50 cyBlbmQgdXAgYmVpbmcgcXVldWVkIG9uIHRoZSB3b3JrX2xpc3QgZm9yIGEgZ2l2ZW4gaWQuDQoN Ci0gU2Vhbg0K
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 1828884A29C6694DAF28B7E6B8A82373469FED01-P5GAC/[email protected]

Bart Van Assche

author

2012-04-30 19:04 UTC #8 lore raw

Quoted 13 lines · Sean

Quoted 2 lines
That makes me wonder how it is prevented that two CM callbacks for the same CM ID run concurrently on different CPUs ?
The callback code ends up looking like this:
        ret = atomic_inc_and_test(&cm_id_priv->work_count);
        if (!ret)
                list_add_tail(&work->list, &cm_id_priv->work_list);
        spin_unlock_irq(&cm_id_priv->lock);

        if (ret)
                cm_process_work(cm_id_priv, work);

Quoted 3 lines

Only 1 thread will end up invoking callbacks to the user. Other events
end up being queued on the work_list for a given id.

Are you sure that only one thread at a time will invoke a CM callback ? As far as I can see cm_recv_handler() queues work without checking whether any other work is ongoing. From drivers/infiniband/core/cm.c:

static void cm_recv_handler(...)

{
        [ ... ]

        work = kmalloc(sizeof *work + sizeof(struct ib_sa_path_rec) * paths,
                       GFP_KERNEL);
        if (!work) {
                ib_free_recv_mad(mad_recv_wc);
                return;
        }

        INIT_DELAYED_WORK(&work->work, cm_work_handler);
        work->cm_event.event = event;
        work->mad_recv_wc = mad_recv_wc;
        work->port = port;
        queue_delayed_work(cm.wq, &work->work, 0);
}

What I have noticed could be explained by the following sequence of events:
* IB CM core receives a connection request and invokes the callback for event IB_CM_REQ_RECEIVED.
* That callback adds connection information to a global list (and keeps running).
* User requests shutdown and hence from another thread ib_send_cm_dreq() is invoked.
* IB CM core receives a DREP message and invokes the callback for event IB_CM_DREP_RECEIVED. That callback function gets confused because of the concurrent connection state manipulations by the IB_CM_REQ_RECEIVED handler (which is still running).

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

Hefty, Sean

2012-04-30 19:27 UTC #10 lore raw

PiBBcmUgeW91IHN1cmUgdGhhdCBvbmx5IG9uZSB0aHJlYWQgYXQgYSB0aW1lIHdpbGwgaW52b2tl IGEgQ00gY2FsbGJhY2sgPyBBcw0KPiBmYXIgYXMgSSBjYW4gc2VlIGNtX3JlY3ZfaGFuZGxlcigp IHF1ZXVlcyB3b3JrIHdpdGhvdXQgY2hlY2tpbmcgd2hldGhlcg0KPiBhbnkgb3RoZXIgd29yayBp cyBvbmdvaW5nLiBGcm9tIGRyaXZlcnMvaW5maW5pYmFuZC9jb3JlL2NtLmM6DQoNCkFsbCBjYWxs YmFja3MgZm9yIGEgc2luZ2xlIElEIHNob3VsZCBiZSBzZXJpYWxpemVkLiAgKEkgdGhpbmsgdGhl IGxpc3RlbiBJRCBpcyBhbiBleGNlcHRpb24sIHdoaWNoIHdvdWxkIGFsbG93IHJlcG9ydGluZyBt dWx0aXBsZSBjb25uZWN0aW9uIHJlcXVlc3RzLikNCg0KY21fd29ya19oYW5kbGVyIGRpc3BhdGNo ZXMgd29yayB0byBzcGVjaWZpYyBoYW5kbGVycyBiYXNlZCBvbiB0aGUgdHlwZSBvZiBDTSBtZXNz YWdlLiAgVGhlIHNlcmlhbGl6YXRpb24gZG9lc24ndCBvY2N1ciB1bnRpbCB3ZSBrbm93IHdoaWNo IElEIHdlJ3JlIGRlYWxpbmcgd2l0aCBhbmQgaXRzIHN0YXRlLiAgU2VlIHRoZSBjbV8qX2hhbmRs ZXIod29yaykgY2FsbHMuDQogDQo+IFdoYXQgSSBoYXZlIG5vdGljZWQgY291bGQgYmUgZXhwbGFp bmVkIGJ5IHRoZSBmb2xsb3dpbmcgc2VxdWVuY2Ugb2YgZXZlbnRzOg0KPiAqIElCIENNIGNvcmUg cmVjZWl2ZXMgYSBjb25uZWN0aW9uIHJlcXVlc3QgYW5kIGludm9rZXMgdGhlIGNhbGxiYWNrIGZv ciBldmVudA0KPiAgIElCX0NNX1JFUV9SRUNFSVZFRC4NCg0KVGhlIGZsb3cgc2hvdWxkIGJlOiBj bV93b3JrX2hhbmRsZXIoKSAtPiBjbV9yZXFfaGFuZGxlcigpIC0+IGNtX21hdGNoX3JlcSgpLCB3 aGljaCBpbmNyZW1lbnRzIHdvcmtfY291bnQgKHRvIDEgaW4gdGhpcyBjYXNlKS4NCg0KPiAqIFRo YXQgY2FsbGJhY2sgYWRkcyBjb25uZWN0aW9uIGluZm9ybWF0aW9uIHRvIGEgZ2xvYmFsIGxpc3Qg KGFuZCBrZWVwcw0KPiBydW5uaW5nKS4NCg0KRG8geW91IG1lYW4gdGhhdCB0aGUgY2FsbGJhY2sg ZG9lc24ndCByZXR1cm4gdG8gdGhlIENNPw0KDQo+ICogVXNlciByZXF1ZXN0cyBzaHV0ZG93biBh bmQgaGVuY2UgZnJvbSBhbm90aGVyIHRocmVhZCBpYl9zZW5kX2NtX2RyZXEoKSBpcw0KPiBpbnZv a2VkLg0KDQppYl9zZW5kX2NtX2RyZXEoKSBhdCB0aGlzIHBvaW50IHNob3VsZCBmYWlsIHdpdGgg RUlOVkFMLCBhcyB0aGUgY29ubmVjdGlvbiBzdGF0ZSBpcyBub3QgeWV0IGVzdGFibGlzaGVkLiAg U28sIEkgZG9uJ3QgdGhpbmsgSSdtIHF1aXRlIGZvbGxvd2luZyB0aGUgb3BlcmF0aW9uIHlldC4N Cg0KPiAqIElCIENNIGNvcmUgcmVjZWl2ZXMgYSBEUkVQIG1lc3NhZ2UgYW5kIGludm9rZXMgdGhl IGNhbGxiYWNrIGZvciBldmVudA0KPiAgIElCX0NNX0RSRVBfUkVDRUlWRUQuIFRoYXQgY2FsbGJh Y2sgZnVuY3Rpb24gZ2V0cyBjb25mdXNlZCBiZWNhdXNlIG9mIHRoZQ0KPiAgIGNvbmN1cnJlbnQg Y29ubmVjdGlvbiBzdGF0ZSBtYW5pcHVsYXRpb25zIGJ5IHRoZSBJQl9DTV9SRVFfUkVDRUlWRUQg aGFuZGxlcg0KPiAgICh3aGljaCBpcyBzdGlsbCBydW5uaW5nKS4NCg0KVGhlIGNtX2RyZXBfaGFu ZGxlcigpIGNvZGUgaW5jcmVtZW50cyB3b3JrX2NvdW50IGFuZCBjaGVja3MgdGhlIHJlc3VsdC4g IElmIHdvcmtfY291bnQgd2FzIG5vbi16ZXJvLCB0aGVuIHRoZSBEUkVQIGV2ZW50IHNob3VsZCBi ZSBxdWV1ZWQgb24gdGhlIGlkJ3Mgd29ya19saXN0LiAgVGhlIHRocmVhZCB3aGljaCBzZXQgd29y a19jb3VudCB0byAxLCB3aWxsIGV2ZW50dWFsbHkgaW52b2tlIHRoZSBjYWxsYmFjayBmb3IgdGhh dCBldmVudC4NCg0KLSBTZWFuDQoNCg==
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 1828884A29C6694DAF28B7E6B8A8237346A00D4A-P5GAC/[email protected]

Bart Van Assche

author

2012-05-01 07:19 UTC #12 lore raw

Quoted 6 lines · Sean

Quoted 2 lines
* User requests shutdown and hence from another thread ib_send_cm_dreq() is invoked.
ib_send_cm_dreq() at this point should fail with EINVAL, as the
connection state is not yet established.

You are right, that didn't make sense. What I have noticed could be explained by reception of a DREQ message while the REQ handler was still running.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html