Fwd: Re: [PATCH] IB/core: Make device counter infrastructure dynamic

Fwd: Re: [PATCH] IB/core: Make device counter infrastructure dynamic

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 2513 bytes --]

Reply-all didn't include the list for some reason :-/


-------- Forwarded Message --------
Subject: Re: [PATCH] IB/core: Make device counter infrastructure dynamic
Date: Thu, 19 May 2016 18:13:22 -0400
From: Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Organization: Red Hat, Inc.
To: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
CC: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Christoph Lameter <cl-vYTEC60ixJUAvxtiuMwx3w@public.gmane.org>, Mark
Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

On 05/19/2016 05:50 PM, Jason Gunthorpe wrote:
> On Thu, May 19, 2016 at 05:35:13PM -0400, Doug Ledford wrote:
> 
>> +	return sprintf(buf, "%d msecs (rounded from jiffies), 0-10000 allowed "
>> +		       "range\n", msecs);
> 
> sysfs files should not have usage commentary like that, just use the
> %d

I had that originally, and I'll pull this out when the stuff is
documented somewhere else (either in the code or Documentation).

>> +char *names[] = {
> 
> static const char * const names[] = {
> 
> For Kees

Fixed (in all places)

>> +	[NR_COUNTERS] = NULL
> 
> Don't really need this, just use ARRAY_SIZE(names) in place of
> NR_COUTNERS

The original code required a NULL terminated array.  Given that the attr
array requires a NULL terminated array, and these are used to set that
up, I'm not sure I mind burning an extra NULL for the semantic match,
even though it's no longer necessary.

>> +       stats = kzalloc(sizeof(*stats) + NR_COUNTERS * sizeof(u64), GFP_KERNEL);
> 
> People have been switching the above pattern to use kcalloc

Read the above again.  It allocates one copy of the stats struct and
then NR_COUNTERS elements of u64.  Unless I'm misreading kcalloc, it
doesn't allow this (not without math to fudge the size).

>> +	[I40IW_HW_STAT_INDEX_MAX_64 + I40IW_HW_STAT_INDEX_MAX_32] = ""
> 
> Why an empty string?

Oops, vi command p overrun.

> Trailing comments are not the kdoc standard for structure members,
> IIRC.

That falls in the same category as the lifespan sysfs file.  I tend to
document things in place while I'm writing, and then erase the
documentation when the final version is complete and the real docs are
in place.


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD




-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: Fwd: Re: [PATCH] IB/core: Make device counter infrastructure dynamic

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 795 bytes --]

On 05/19/2016 11:15 PM, Doug Ledford wrote:
> On 05/19/2016 05:50 PM, Jason Gunthorpe wrote:
>> On Thu, May 19, 2016 at 05:35:13PM -0400, Doug Ledford wrote:
>>> +	[NR_COUNTERS] = NULL
>>
>> Don't really need this, just use ARRAY_SIZE(names) in place of
>> NR_COUTNERS
> 
> The original code required a NULL terminated array.  Given that the attr
> array requires a NULL terminated array, and these are used to set that
> up, I'm not sure I mind burning an extra NULL for the semantic match,
> even though it's no longer necessary.

As a second issue, this is what allowed us to remove half of the
BUILD_BUG_ON statements.  So, removing it means putting those back in.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: Fwd: Re: [PATCH] IB/core: Make device counter infrastructure dynamic

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2585 bytes --]

On Thu, May 19, 2016 at 11:15:45PM -0400, Doug Ledford wrote:
> Reply-all didn't include the list for some reason :-/
> 
> 
> -------- Forwarded Message --------
> Subject: Re: [PATCH] IB/core: Make device counter infrastructure dynamic
> Date: Thu, 19 May 2016 18:13:22 -0400
> From: Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Organization: Red Hat, Inc.
> To: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> CC: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Christoph Lameter <cl-vYTEC60ixJUAvxtiuMwx3w@public.gmane.org>, Mark
> Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> On 05/19/2016 05:50 PM, Jason Gunthorpe wrote:
> > On Thu, May 19, 2016 at 05:35:13PM -0400, Doug Ledford wrote:
> > 
> >> +	return sprintf(buf, "%d msecs (rounded from jiffies), 0-10000 allowed "
> >> +		       "range\n", msecs);
> > 
> > sysfs files should not have usage commentary like that, just use the
> > %d
> 
> I had that originally, and I'll pull this out when the stuff is
> documented somewhere else (either in the code or Documentation).

There is Documentation/infiniband/sysfs.txt [1], which looks as a good
candidate for this.

[1] http://lxr.free-electrons.com/source/Documentation/infiniband/sysfs.txt

> 
> >> +char *names[] = {
> > 
> > static const char * const names[] = {
> > 
> > For Kees
> 
> Fixed (in all places)
> 
> >> +	[NR_COUNTERS] = NULL
> > 
> > Don't really need this, just use ARRAY_SIZE(names) in place of
> > NR_COUTNERS
> 
> The original code required a NULL terminated array.  Given that the attr
> array requires a NULL terminated array, and these are used to set that
> up, I'm not sure I mind burning an extra NULL for the semantic match,
> even though it's no longer necessary.

I disagree with your statement that removing BUILD_BUG_ON() checks is
a positive thing. In this case, we are talking about constant array
which is not going to be changed till new code will be added and compiled
again.

The questions which can be asked, do we need so much BUILD_BUG_ON checks?
Can we minimize the number of them?

> 
> > Trailing comments are not the kdoc standard for structure members,
> > IIRC.
> 
> That falls in the same category as the lifespan sysfs file.  I tend to
> document things in place while I'm writing, and then erase the
> documentation when the final version is complete and the real docs are
> in place.

In such case, it is better to send as RFC to present that this patch is
not in its final stage yet.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]