On Fri, Apr 20, 2018 at 01:32:32PM -0500, Steve Wise wrote:
> > > +static int fill_sq(struct sk_buff *msg, struct t4_wq *wq)
> > > +{
> > > + struct nlattr *entry_attr;
> > > +
> > > + /* WQ+SQ */
> > > + entry_attr = nla_nest_start(msg,
> > RDMA_NLDEV_ATTR_PROVIDER_ENTRY);
> > > + if (!entry_attr)
> > > + goto err;
> >
> > Er, you want to nest ? But how will anything figure out what this
> > RDMA_NLDEV_ATTR_PROVIDER_ENTRY is connected to? Seems like
> > something
> > is missing here.
> >
> > I think I'd rather see no nesting and have you prefix the strings with
> > something to group them??
> >
>
> The nesting allowed for the rdma tool to take that as a time for a new line.
> So you'd get the qp-global stuff for this qp on one line, then the
> sq-specific on the next line, and then rq-specific.
Oh gross.. no..
> > > + if (rdma_nl_put_provider_u32(msg, " idx", idx))
> >
> > Why leading spaces? No.. Saw this a couple of times.
>
> Again, this made for nicer formatting on the display side. It is kind of
> tricky to get decent formatting/readability with these generic provider
> attributes.
Also gross, no..
The kernel shouldn't be involved in formatting like this.
> > > + table_attr = nla_nest_start(msg, RDMA_NLDEV_ATTR_PROVIDER);
> > > + if (!table_attr)
> > > + goto err;
> >
> > I think the core code should setup this nest and handle the closing of
> > the nest.. The driver should not be able to stuff arbitary things
> > outside its assigned scope.
>
> Leon had some requirement to support provider attributes for shared objects.
> So each provider could add its nested attributes.
I don't know what this means
Jason
--
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
