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@xxxxxxxxxx> --D > Thanks for the review. > > -- > Carlos >
