On Tue, Jun 30, 2015 at 09:45:55AM -0400, kaike....@intel.com wrote: I still think this is long and rambly, but that is mostly a style preference, I think you should look at it and remove some of the left over stuff, like we really don't need a rough in for other responses at this point.
> +#define IB_SA_ENABLE_LOCAL_SERVICE 0x00000001 > +#define IB_SA_CANCEL 0x00000002 > + > +#define IB_SA_LOCAL_SVC_ENABLED(query) \ > + ((query)->flags & IB_SA_ENABLE_LOCAL_SERVICE) > +#define IB_SA_ENABLE_LOCAL_SVC(query) \ > + ((query)->flags |= IB_SA_ENABLE_LOCAL_SERVICE) > +#define IB_SA_DISABLE_LOCAL_SVC(query) \ > + ((query)->flags &= ~IB_SA_ENABLE_LOCAL_SERVICE) > + > +#define IB_SA_QUERY_CANCELLED(query) \ > + ((query)->flags & IB_SA_CANCEL) > +#define IB_SA_CANCEL_QUERY(query) \ > + ((query)->flags |= IB_SA_CANCEL) This whole bit is really strange style - if you really want to keep this then at least use static inline functions not macros. > +static struct ib_nl_request_info * > +ib_nl_alloc_request(struct ib_sa_query *query) > +{ > + struct ib_nl_request_info *rinfo; > + > + rinfo = kzalloc(sizeof(*rinfo), GFP_ATOMIC); > + if (rinfo == NULL) > + return ERR_PTR(-ENOMEM); There really seem to be alot of kallocs going on for what is supposed to be a performance path. I would probably work to fold this into the ib_sa_query allocation, it is just a few bytes we can waste that ram if we are not using netlink. > + if ((info->comp_mask & IB_SA_PATH_REC_REVERSIBLE) && > + sa_rec->reversible != 0) { > + if ((info->comp_mask & IB_SA_PATH_REC_NUMB_PATH) && > + sa_rec->numb_path > 1) > + val8 = LS_NLA_PATH_USE_ALL; > + else > + val8 = LS_NLA_PATH_USE_GMP; Drop the num_paths stuff, just set USE_GMP, it is confusing. I thought I mentioned this already. > + } else { > + val8 = LS_NLA_PATH_USE_UNIDIRECTIONAL; > + } > + nla_put(skb, RDMA_NLA_F_MANDATORY | LS_NLA_TYPE_PATH_USE, sizeof(val8), > + &val8); Non-optional attributes should probably go in a non-nested header, possibly along with the portGUID/portnum/whatever. So the structure would be: nlmsghdr struct rdma_ls_resolve_path { u64 port_guid; // whatever u32 path_use; } nlattr[IB_SA_PATH_REC_PKEY,...]* This is standard layout for netlink messages > +static int ib_nl_get_path_rec_attrs_len(struct ib_nl_attr_info *info) > +{ > + /* > + * We need path use attribute no matter reversible or numb_path is > + * set or not, as long as some other fields get set > + */ > + if (WARN_ON(len == 0)) > + return len; The comment is obsolete, and it shouldn't exit without reserving space for the mandatory fields. > +static int ib_nl_send_request(struct ib_nl_request_info *rinfo) > +{ > + struct ib_nl_attr_info info; > + int opcode; > + struct ib_sa_mad *mad; > + unsigned long flags; > + unsigned long delay; > + int ret; > + > + mad = rinfo->query->mad_buf->mad; > + switch (mad->mad_hdr.attr_id) { > + case cpu_to_be16(IB_SA_ATTR_PATH_REC): > + opcode = RDMA_NL_LS_OP_RESOLVE; > + mad = rinfo->query->mad_buf->mad; > + info.comp_mask = mad->sa_hdr.comp_mask; > + info.input = rinfo->query->mad_buf->context[1]; > + rinfo->query->mad_buf->context[1] = NULL; > + info.len = ib_nl_get_path_rec_attrs_len(&info); > + info.set_attrs = ib_nl_set_path_rec_attrs; > + break; So now we put a bunch of stuff in yet another structure and call through a function pointer. Rambly, I'd streamline that.. > + struct ib_mad_send_wc mad_send_wc; > + struct ib_sa_mad *mad = NULL; > + const struct nlattr *head, *curr; > + struct ib_path_rec_data *rec; > + int len, rem; > + > + if (query->callback) { > + head = (const struct nlattr *) nlmsg_data(nlh); > + len = nlmsg_len(nlh); > + nla_for_each_attr(curr, head, len, rem) { > + if (curr->nla_type == LS_NLA_TYPE_PATH_RECORD) { > + rec = nla_data(curr); > + if (rec->flags && IB_PATH_PRIMARY) { This is still wrong. I looked very closely, and it turns out the record you want to find depends on the path use that was asked for. LS_NLA_PATH_USE_ALL: IB_PATH_PRIMARY | IB_PATH_GMP | IB_PATH_BIDIRECTIONAL LS_NLA_PATH_USE_GMP: IB_PATH_PRIMARY | IB_PATH_GMP | IB_PATH_BIDIRECTIONAL LS_NLA_PATH_USE_UNIDIRECTIONAL IB_PATH_PRIMARY | IB_PATH_OUTBOUND > +static inline int ib_nl_is_good_resolve_resp(const struct nlmsghdr *nlh) > +{ > + const struct nlattr *head, *curr; > + int len, rem; > + > + if (nlh->nlmsg_flags & RDMA_NL_LS_F_ERR) > + return 0; > + > + if (!(nlh->nlmsg_flags & RDMA_NL_LS_F_OK)) > + return 0; > + > + if (nlmsg_len(nlh) < nla_attr_size(sizeof(*rec))) > + return 0; > + > + head = (const struct nlattr *) nlmsg_data(nlh); > + len = nlmsg_len(nlh); > + nla_for_each_attr(curr, head, len, rem) { > + if (curr->nla_type == LS_NLA_TYPE_PATH_RECORD) > + return 1; > + } As discussed already, this needs to use nla_parse_nested, which should eliminate all of this. Do not do nla_for_each_attr here, just look for ERR. > +static int ib_nl_handle_set_timeout(struct sk_buff *skb, > + struct netlink_callback *cb) > +{ > + const struct nlmsghdr *nlh = (struct nlmsghdr *)cb->nlh; > + int timeout, delta, abs_delta; > + const struct nlattr *attr; > + struct rdma_nla_ls_timeout *to_attr; > + unsigned long flags; > + struct ib_nl_request_info *rinfo; > + long delay = 0; > + > + if (nlmsg_len(nlh) < nla_attr_size(sizeof(*to_attr))) > + goto settimeout_out; All this should be driven by nla_parse > + attr = (const struct nlattr *) nlmsg_data(nlh); > + if (attr->nla_type != LS_NLA_TYPE_TIMEOUT || > + nla_len(attr) != sizeof(*to_attr)) > + goto settimeout_out; No nested attr, as discussed There is something weird here, IIRC in netlink a SET should return back exactly the same message with the up to date values. (Probably should confirm, I'm not 100% on that) And I don't think this should be a dump, again, not 100% sure. I didn't check the locking or a few otherthings, but I did test it out a bit with a custom cache userspace client, would like to try again when the protocol is fixed up. Thanks, Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html