Re: [PATCH v3 rdma-next 5/5] iw_cxgb4: dump detailed provider-specific QP information

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux