Re: [PATCH 4/4] xfs: Remove kmem_free()

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Mon, Nov 18, 2019 at 09:30:08AM +0100, Carlos Maiolino wrote:
> On Fri, Nov 15, 2019 at 09:23:22AM -0800, Darrick J. Wong wrote:
> > On Fri, Nov 15, 2019 at 03:20:55PM +0100, Carlos Maiolino wrote:
> > > On Thu, Nov 14, 2019 at 01:00:00PM -0800, Darrick J. Wong wrote:
> > > > On Thu, Nov 14, 2019 at 09:09:55PM +0100, Carlos Maiolino wrote:
> > > > > This can be replaced by direct calls to kfree() or kvfree() (whenever
> > > > > allocation is done via kmem_alloc_io() or kmem_alloc_large().
> > > > > 
> > > > > This patch has been partially scripted. I used the following sed to
> > > > > replace all kmem_free() calls by kfree()
> > > > > 
> > > > >  # find fs/xfs/ -type f -name '*.c' -o -name '*.h' | xargs sed -i \
> > > > >    's/kmem_free/kfree/g'
> > > > 
> > > > Coccinelle? ;)
> > > 
> > > /me Doesn't understand the reference but thinks Darrick is talking about
> > > Coccinelle fancy brand :P
> > > 
> > > /me is adept to conference-wear :D
> > 
> > http://coccinelle.lip6.fr/
> > 
> > The semantic patch thing, because understanding the weird spatch
> > language is slightly less infuriating than making tons of minor code
> > changes by hand. :P
> 
> Oh, I didn't know about this. Thanks. The name could be something different
> other than a fashion brand making googling for it easier :(
> 
> > > I can't really say we will have any benefits in segmenting it by using kvfree()
> > > only where kmem_alloc_{large, io} is used, so I just relied on the comments
> > > above kvfree(), and well, we have an extra function call and a few extra
> > > instructions using kvfree(). So, even though it might be 'slightly' faster, this
> > > might build up on hot paths when handling millions of kfree().
> > > 
> > > But, at the end, I'd be lying if I say I spotted any significant difference.
> > 
> > <nod> Though the way I see it, kfree vs. kvfree is another bookkeepping
> > detail that xfs developers will have to keep straight.  But maybe that's
> > fine for the dozen or so specialized users of _io and _large?  What do
> > others think?
> 
> Ok, if we decide to move everything to kvfree() I'll just send a V2 of this
> patch, which should apply cleanly on top of the other 3.
> 
> > 
> > > Btw, Dave mentioned in a not so far future, kmalloc() requests will be
> > > guaranteed to be aligned, so, I wonder if we will be able to replace both
> > > kmem_alloc_large() and kmem_alloc_io() by simple calls to kvmalloc() which does
> > > the job of falling back to vmalloc() if kmalloc() fails?!
> > 
> > Sure, but I'll believe that when I see it.  And given that Christoph
> > Lameter seems totally opposed to the idea, I think we should keep our
> > silly wrapper for a while to see if they don't accidentally revert it or
> > something.
> > 
> 
> Sure, I don't have any plans to do it now in this series or in a very near
> future, I just used the email to share the idea :P

Eh, well, FWIW I took a second look at all the kvfree/kfree and decided
that the usage was correct.  For future reference, please do the
straight change as one patch and straighten out the usages as a separate
patch.

In any case it seemed to test ok over the weekend (and still seems ok
with your series from today), so...
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> Thanks for the review.
> 
> -- 
> Carlos
>