[ofa-general] [PATCH RFC] rds: add iwarp support

[Jon Mason]

On Fri, Jul 04, 2008 at 12:59:08AM +0300, Or Gerlitz wrote:
> On 7/4/08, Jon Mason <jon at opengridcomputing.com> wrote:
> 
> >  The bulk of the code is a copy of the RDS IB with the changes necessary
> >  to get iWARP working.
> 
> So the intension is to have two code bases "RDS/IB" and "RDS/iWARP"?
> 
> if yes, why? the rds code was enhanced to support credit management
> and my understanding is that this paves the way to have joint code, as
> done for any other RDMA ULP that was ported to iWARP (rNFS, Open-MPI,
> Lustre, etc)

The plan was to create an iWARP specific module and once it has been
verified to work well then push it into the IB module (and verify that
it does not impact IB performance).  I was erring on the side of caution
and keeping it completely separate in this version and then merging as
much common code as possible in a future version.


> >  The logic in the RDS connection setup to determine which method to use,
> >  requires a new function in the core IB logic to translate a given IP
> >  Address to the IB/iWARP device associated with it.  The only place this
> >  can be determined is in the pre-existing structs in cma.c.
> 
> .laddr_check is not used in this patchset, and I could not understand
> its role from this description.

Perhaps I misunderstand your comment, but I do believe laddr_check is
being used in this patch for iWARP.  See lines 323 and 354 of the patch.
If you are asking what it is doing, it is using a new function I created
in the infiniband core logic to determine what iWARP/IB device is tied
to the given IP address and then checks to see if it is iWARP or IB.

> >  Olaf, if you would like to pull this into your personal tree, I can
> >  provide you with delta patches until it is accepted into the OFED 1.4
> >  tree (assuming that you have no major problem with the code below).
> 
> As its an RFC, lets have it go a little review before pulling it,
> unless the intension is to put it in an experimental branch and let
> people play with. Anyway, structured set of delta patches is always
> prefered, for review, acceptance, debugging and maintainace reasons.

Reviews are great!

> >  --- a/drivers/infiniband/core/cma.c
> >  +++ b/drivers/infiniband/core/cma.c
> 
> >  +struct ib_device *ipaddr_to_ibdev(u32 addr)
> 
> is there a reason why rdma_resolve_addr(NULL, addr, timeout) wouldn't
> do what ipaddr_to_ibdev does?

There could very well be a different/better way of doing this.  I could
not find one at the time, but I will investigate your suggestion and see
if I can get rid of this call.

> Also, from quick looking I understand that this patchset adds some
> sort of fmr pool for the new fmrs within the rds code, can this pool
> be generalized and added to the core module such that other ULPs would
> be able to enjoy it?

The fast_reg_mr pool that is being created is very similar to the fmr
pool that exists in the RDS IB module.  I am not opposed to
extrapolating this out, but are there any other users that would benefit
from this?

Thanks for the review :)

Jon

> 
> Or.