Received-SPF: pass (google.com: domain of amsfield22@gmail.com designates 2607:f8b0:400e:c03::235 as permitted sender) client-ip=2607:f8b0:400e:c03::235;
Date: Fri, 12 Feb 2016 11:35:19 -0800
From: Alison Schofield <amsfield22@gmail.com>
To: Tejun Heo <tj@kernel.org>
Cc: outreachy-kernel@googlegroups.com
Subject: Re: [Outreachy kernel] [PATCH] staging: vt6656: replace kzalloc with
 kmalloc
Message-ID: <20160212193518.GA5221@d830.WORKGROUP>
References: <20160212063353.GA12041@d830.WORKGROUP>
 <20160212172045.GK3741@mtj.duckdns.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20160212172045.GK3741@mtj.duckdns.org>
User-Agent: Mutt/1.5.23 (2014-03-12)

On Fri, Feb 12, 2016 at 12:20:45PM -0500, Tejun Heo wrote:
> Hello,
> 
> On Thu, Feb 11, 2016 at 10:34:05PM -0800, Alison Schofield wrote:
> > All fields of allocated structure are filled in directly following
> > the allocate, so no need to initialize it to zero.
> > 
> > Use preferred kmalloc() sizeof style to silence:
> > CHECK: Prefer kmalloc(sizeof(*priv->rcb[ii])...)
> > 
> > Signed-off-by: Alison Schofield <amsfield22@gmail.com>
> > ---
> >  drivers/staging/vt6656/main_usb.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
> > index a2f23ae..74ff707 100644
> > --- a/drivers/staging/vt6656/main_usb.c
> > +++ b/drivers/staging/vt6656/main_usb.c
> > @@ -449,7 +449,7 @@ static bool vnt_alloc_bufs(struct vnt_private *priv)
> >  	}
> >  
> >  	for (ii = 0; ii < priv->num_rcb; ii++) {
> > -		priv->rcb[ii] = kzalloc(sizeof(struct vnt_rcb), GFP_KERNEL);
> > +		priv->rcb[ii] = kmalloc(sizeof(*priv->rcb[ii]), GFP_KERNEL);
> 
> I don't know.  It's on the init path which is extremely cold and
> zeroing followed by inits are extremely cheap as the inits end up
> hitting the same cachelines (sometimes even cheaper than not doing it
> as zeroing often doesn't even need to read in the cachelines, so it
> just becomes cacheline allocation).  It's nice to zero this type of
> structures as later field additions or code modifications may lead to
> surprises.
> 
> This doesn't mean kzalloc is always better than kmalloc or vice-versa
> and what to use when can be a surprisingly tricky question.  I'd
> suggest staying away from mechanical conversions between kmalloc and
> kzalloc.
> 
> Thanks.
> 
> -- 
> tejun
Thanks for the review and detailed comments. This caught my eye because
it differed from the other kmallocs in same function.  I found that it
had been changed to kZalloc a few years ago via a patch that replaced
kmalloc+memset with kzalloc. Unfortunately there was no mention of why
the zeroing was needed in the first place. I saw that to be a "mechanial
conversion" and so pursued further.

Each piece of this struct is initialized immediately after the alloc,
so usage before initialization is not the issue.  As far as being on a
cold path...that's where I lost the trail.  It was not clear to me that the
".start" op is only done at probe.  I guess I will drop it based on
that fact.  

(cachelines and protecting future changes were not on my radar)

Thanks,
alison