[openib-general] [PATCH] ib_sync_cq ( was Re: RFC: ib_set_comp_handler)

[Roland Dreier]

    James> The purpose of this function would be more obvious if you
    James> included the new comp_handler and cq_contex in the function
    James> signature. A different name would help as well. I would
    James> suggest: void ib_modify_cq(struct ib_cq *cq, void
    James> (*event_handler)(struct ib_event *, void *), void
    James> *cq_context);

    Sean> I think that this makes more sense.  It keeps the
    Sean> synchronization internal to the verbs layer, and prevents
    Sean> the user from overwriting the event_handler at the same time
    Sean> that it may be read by the hca driver.  Can we rely on a
    Sean> write to the cq->event_handler being atomic wrt a read of
    Sean> the same value?

I'm not sure that writes to function pointers are always atomic.  For
example the ppc64 ABI does some crazy stuff with function descriptors.

On the other hand I'm not sure I like wrapping things up in an
ib_modify_cq() function.  Calling ib_modify_cq() from a completion
event handler will deadlock, and if we allow clearing the CQ event
handler, it seems legitimate thing to do so from a CQ event handler.

In any case I'm not sure I buy the motivation behind adding this
function.  It saves a conditional branch in SDP's completion handler,
but on the other hand, that branch is going to be a test of a value in
a cache line that already gets used, and it's almost always going to
be predicted correctly.  So I'm not sure the performance gain even exists.

 - R.