On Fri, Jun 12, 2015 at 10:06:06AM -0400, [email protected] wrote:
> This patch routes a SA pathrecord query to netlink first and processes the
> response appropriately. If a failure is returned, the request will be sent
> through IB. The decision whether to route the request to netlink first is
> determined by the presence of a listener for the local service netlink
> multicast group. If the user-space local service netlink multicast group
> listener is not present, the request will be sent through IB, just like
> what is currently being done.
I'd really like to test this out here, I was hoping to get to that
this week, but not yet.. And I was still hoping to read through
carefully.
Anyhow, brief check seemed like things make sense
> +struct ib_nl_attr_info {
> + u16 len; /* Total attr len: header + payload + padding */
> + ib_sa_comp_mask comp_mask;
> + void *input;
> + void (*set_attrs)(struct sk_buff *skb, struct ib_nl_attr_info
> *info);
I don't really get what this is for, set_attrs is only ever equal to
ib_nl_set_path_rec_attrs - is this whole indirection left over from
something else? Drop it?
This code does wander quite a bit, lots of functions are called from
only one place, not necessarily bad, but at least topo sort them so the
one-place called function is before the only caller... Makes it easier
to read
> + struct ib_sa_path_rec *sa_rec = info->input;
> + __u8 val1;
> + __u16 val2;
> + __u64 val3;
At least use val8, val16, val64 for names and IIRC, the __ is not used
inside the kernel proper
> +
> + 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)
The kernel never sets numb path, I would just never set _ALL for now
and leave it as a placeholder.
> + val1 = LS_NLA_PATH_USE_ALL;
> + else
> + val1 = LS_NLA_PATH_USE_GMP;
> + } else {
> + val1 = LS_NLA_PATH_USE_UNIDIRECTIONAL;
> + }
> + nla_put(skb, RDMA_NLA_F_MANDATORY | LS_NLA_TYPE_PATH_USE, sizeof(val1),
> + &val1);
This uses a u8 but other places are not using that:
+ case LS_NLA_TYPE_PATH_USE:
+ use = (struct rdma_nla_ls_path_use *) NLA_DATA(attr);
> + if (curr->nla_type == LS_NLA_TYPE_PATH_RECORD) {
> + rec = nla_data(curr);
> + if (rec->flags && IB_PATH_PRIMARY) {
&& is the wrong operator isn't it?
IB_PATH_PRIMARY is the wrong test, I think I covered this already..
I still feel like the netlink specific stuff should live in its own
file, not be jammed into sa_query.c - unless it is really hard to
disentangle, but it doesn't look too bad at first glance..
> +static int ib_nl_get_path_rec_attrs_len(struct ib_nl_attr_info *info)
> +{
> + if (len > 0)
> + len += nla_total_size(sizeof(struct rdma_nla_ls_path_use));
Seems a bit goofy, I don't think we should ever generate an empty
netlink query. That feels like a WARN_ON scenario.
> + skb = nlmsg_new(attrs->len, GFP_KERNEL);
> + if (!skb) {
> + pr_err("alloc failed ret=%d\n", ret);
Doesn't alloc failure already print enough?
> + if (ret != -ESRCH)
> + pr_err("ibnl_multicast failed l=%d, r=%d\n",
> + attrs->len,ret);
What is this print about? Seem strange?
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
