> From: Jason Gunthorpe [mailto:jgunthorpe@xxxxxxxxxxxxxxxxxxxx] > Sent: Thursday, June 11, 2015 3:52 PM > To: Wan, Kaike > Cc: linux-rdma@xxxxxxxxxxxxxxx; Fleck, John; Weiny, Ira > Subject: Re: [PATCH v5 1/4] IB/netlink: Add defines for local service requests > through netlink > > On Thu, Jun 11, 2015 at 07:21:11PM +0000, Wan, Kaike wrote: > > > Recommend a 'QP Type' with options of: > > > - RC QP: Return up to the full 6 path tuple with full APM data > > > - UD QP: Return a single non-reversible path > > > - GMP QP: Return up to two reversible GMP paths. > > > > > > If it is hard to get accurate information then you can sketch this > > > in and infer RC QP and GMP QP by the request having the reversible > > > set bit. But ideally RDMA CM and SRP would request RC QP, IPoIB would > request UD QP. > > > > It's quite indirect here for this implementation. > > (ib_sa_path_rec_get()) > > > > Caller reversible numb_path QP Type > > cma 1 1 GMP > > ipoib Not set 1 (?) UD > > srp Not set 1 UD > > CMA (mostly) and SRP use RC paths. IPoIB uses UD and RC. The IPoIB path > record query looks wrong for RC mode because it does not request reversible, > that should be fixed independently. > > The indirection is because you don't want to modify the callers to provide > their additional information, which is probably a mistake. Add a netlink fill > callback and do it there? Apparently, to achieve this goal, we need the following changes: 1. Caller (cma, ipoib, srp): - Add a new callback to handle multiple pathrecords returned in the response. - Change the calling function to supply additional context info. The problem is that currently only one copy of pathrecord is required for these callers and we need to find the use of other pathrecords in these callers. Otherwise, we simply are overdesigning. 2. ib_sa: - Add a function to take additional context info; - Code this addition context into new attributes. - New callback function to return multiple pathrecords to the caller. 3. User space application: Apparently, ibacm can only return one copy of pathrecord and we need to find something different to test the code. And the current implementation of ibacm can't make use of the supplied context info (QP type or whatever). I think that these changes run well beyond the original goal of this patch series to speed up the pathrecord query performance and may warranty a subsequent patch series. For this patch series, how about limiting the change to combining reversible and numb_path into reversible_numb_path attribute? > > > In addition, on the user side, we need to convert QP type back into > > reversible_numpath again, which may not interpret the original request > > correctly. Why should we do all the translation? > > The goal is to carry enough context information so that user space can do > something smart. We have alot more information available to drive policy > than is present in the simple IBA Path Record MAD. > > In this case, the kernel does not actually care about numb paths or reversible, > except in the context of how it intends to use the path(s). > > For instance, having CM ask for reversible makes no sense, since in the > general case it wants the 3 path tuple of > GMP(reversible=1),FORWARD(reversible=0),RETURN(reversible=0). Similarly, > it doesn't make sense to ask for numbPath=1 because in the general case it > wants the 6 path APM set. > > Being more specific here allows userspace to implement more detailed policy > beyond what the IB Path Record scheme allows. And for that we need the > context information from the requestor. > > acm won't use it, it will always request numbPaths=1 and derive the > reversible bit from this netlink field. (or always set reversible=1) > > > Is QP type definition already exported to user space? > > QP Type could be a bad name, open to ideas > > > I see IBV_QPT_RC/UC/UD/RAW_PACKET/XRC_SEND/XRC_RECV, but not > GMP in verbs.h. > > GMP is a special case of UD. > > > > > +/* Local Service ServiceID attribute */ struct > > > > +rdma_nla_ls_service_id { > > > > + __be64 service_id; > > > > +}; > > > > > > Do not expose BE to userspace, everything should be in cpu order. > > > > If we use cpu order, we need to do two conversions: from BE to cpu > > order in kernel and from cpu order to BE in user space. Struct > > ib_user_path_rec contains many __be32 fields. > > I don't see a problem with the extra conversion. That's fine. I can do the conversion on both sides. In the future, if the callers do not do the conversion, there will be only one conversion on the user space side: caller (cpu byte order) ->ib_sa (cpu byte order) ->user space (cpu byte order ->network byte order). Kaike -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
