[openib-general][PATCH][kdapl]: evd upcall policy implementation

[Guy German]

Hi,

James Lentini <mailto:jlentini at netapp.com> wrote:
> How about changing the comment to say that DAT_UPCALL_MANY is not
> supported and leave it at that?

OK. It was a very long debate over a remark... :)

>> That leaves me with a question: what did happened to
>> DAT_CONNECTION_EVENT_BROKEN?
> I'm not sure.

I think we need to add it back, at some point, even though the consumers can 
handle it themselves, for now...

>>> You changed the order in which the CQ upcall is enabled and the
>>> kDAPL upcall is made. 
> kDAPL doesn't dequeue the CQ events, so this shouldn't be an issue.

I think we agreed on that issue, on another thread, that I shall leave it:
>  call kDAPL upcall
>  enable CQ upcall
right ?

> I disagree that this is in violation of the DAPL spec. The spec.
> describes DAPL upcall policies and leaves the interaction with lower
> layers (IB verbs) unspecified.
> In any event, we should fix any of the performance problems you've
> found. 
Agreed.

> Currently the IB upcall is initially enabled, but there are checks on
> the upcall path to determine if the EVD upcall is enabled. See
> dapl_evd.c line 827: 
> 
>         if (state == DAPL_EVD_STATE_OPEN &&
>             evd->upcall_policy != DAT_UPCALL_DISABLE) {
> 
> which you've replaced with
> 
>         if (state == DAPL_EVD_STATE_OPEN) {

Yes, because I believe the correct place to check it is inside 
dapl_evd_upcall_trigger, after the lock and before the actual upcall.

>> You've only decrease the window in which that scenario could occur,
>> not eliminated it. If a DTO completion occurred after you count the
>> number of pending events but before you enable the CQ callback, a
>> completion will be missed. 
>> 
>> gg:
>> I don't think so. That is what the spin_lock_irqsave is for.
> 
> What if there are multiple processors in the system?

AFAIK, spin_lock_irqsave does the trick. am I wrong ?

>> Also, the pending_event_queue is only used for kDAPL generated
>> software events. This queue can be empty when there are events on the
>> CQ, so your would need to be expanded your check to cover that.

Actually, even though, I agreed before, I tend to disagree now. 
The consumer will still get the DTO events as soon as the CQ 
upcall is triggered (enabled), so only problem is with the pending
events list.


Thanks,
Guy