[ofa-general] Re: [PATCH draft, untested] ehca srq emulation (for IPoIB CM)

[Pradeep Satyanarayana]

Pradeep Satyanarayana wrote:
> Roland Dreier wrote:
>>  > In the absence of any further discussions about the IPoIB CM 
>> without SRQ
>>  > patches, I will incorporate Sean Hefty's comments and plan to resubmit
>>  > the patches, unless I hear something soon.
>>
>> Sorry for not devoting enough time to this, but something always seems
>> to come up, and I really want to be able to focus a concentrated chunk
>> of time on this, and I never seem to be able to.  Anyway, I would
>> prefer to find a solution that everyone can agree on, without me
>> having to rule by decree.
>>
>> I think updating the patch is a good idea.  Although I didn't get a
>> chance to review it carefully there were a number of obvious messy
>> parts that should be cleaned up.
>>
>> I am beginning to think that your basic approach is probably right,
>> but I also still think it should be possible to handle both SRQ and
>> non-SRQ without any overhead on the fast path.  I don't understand the
>> "maintainability" argument against doing this.  Can you expand on your
>> position a little?
>>
> 
> I will try to illustrate with an example:
> 
> One of the ways to do this is to completely split SRQ and non-SRQ
> processing starting in ipoib_poll(). This would eliminate most of
> the if (srq) kind of branches. However, there would be a lot of code
> duplication. If a bug is discovered in one path, then one needs to
> fix that in the other path too.
> 
> One way to mitigate this situation is to alter the current SRQ code
> to use common code (between SRQ and non-SRQ). However, one might not 
> want to factor off a few lines of common code into a new function. There
> may be several such occurrences of this resulting in code bloat.
> 
> If you look back, several weeks ago ipoib_drain_cq() did not exist. This
> is another function that calls ipoib_cm_handle_rx_wc(). We would need
> to alter this function too to accommodate SRQ and non-SRQ split. In
> effect, we have propagated the SRQ and non-SRQ code to functions
> outside ipoiob_cm.c. In the future, if IPoIB CM would support UC mode
> this might mean additional functions handling the split.
> 
> On the other hand, in V6 (and previous versions) of the patch
> ipoib_cm_handle_rx_wc() handles the SRQ and non-SRQ paths. Both SRQ and
> non-SRQ functionality is contained within ipoib_cm.c. What we now have
> is probably one extra branch in the packet handling path than the
> minimum (desired) with a lot of common code.
> 
> Pradeep
> 

Roland,

Since the merge window will close in the next few days, do you have a
few suggestions that you would like me to incorporate into the patch?

Pradeep