Re: [PATCH 0/2] rcu: Transform kfree_rcu() into kvfree_rcu()
On Wed, Feb 07, 2018 at 10:10:55AM -0800, Matthew Wilcox wrote: > On Wed, Feb 07, 2018 at 05:45:13PM +0100, Jesper Dangaard Brouer wrote: > > On Wed, 7 Feb 2018 08:57:00 -0500 > > Steven Rostedt wrote: > > > To me kvfree() is a special case and should not be used by RCU as a > > > generic function. That would make RCU and MM much more coupled than > > > necessary. > > > > For the record, I fully agree with Steve here. > > > > And being a performance "fanatic" I don't like to have the extra branch > > (and compares) in the free code path... but it's a MM-decision (and > > sometimes you should not listen to "fanatics" ;-)) > > While free_rcu() is not withut its performance requirements, I think it's > currently dominated by cache misses and not by branches. By the time RCU > gets to run callbacks, memory is certainly L1/L2 cache-cold and probably > L3 cache-cold. Also calling the callback functions is utterly impossible > for the branch predictor. This seems to have fallen by the wayside. To get things going again, I suggest starting out by simply replacing the kfree() in __rcu_reclaim() with kvfree(). If desired, a kvfree_rcu() can also be defined as a synonym for kfree_rcu(). This gets us a very simple and small patch which provides the ability to dispose of kvmalloc() memory after a grace period. Thanx, Paul
Re: [PATCH 0/2] rcu: Transform kfree_rcu() into kvfree_rcu()
On Wed, Feb 07, 2018 at 01:26:19PM -0500, Steven Rostedt wrote: > On Wed, 7 Feb 2018 10:10:55 -0800 > Matthew Wilcox wrote: > > > > For the record, I fully agree with Steve here. > > Thanks, but... > > > > > > > And being a performance "fanatic" I don't like to have the extra branch > > > (and compares) in the free code path... but it's a MM-decision (and > > > sometimes you should not listen to "fanatics" ;-)) > > > > While free_rcu() is not withut its performance requirements, I think it's > > currently dominated by cache misses and not by branches. By the time RCU > > gets to run callbacks, memory is certainly L1/L2 cache-cold and probably > > L3 cache-cold. Also calling the callback functions is utterly impossible > > for the branch predictor. > > I agree with Matthew. > > This is far from any fast path. A few extra branches isn't going to > hurt anything here as it's mostly just garbage collection. With or > without the Spectre fixes. What Steve said! Thanx, Paul
Re: [PATCH 0/2] rcu: Transform kfree_rcu() into kvfree_rcu()
On Wed, Feb 07, 2018 at 08:55:47AM -0600, Christopher Lameter wrote: > On Tue, 6 Feb 2018, Paul E. McKenney wrote: > > > So it is OK to kvmalloc() something and pass it to either kfree() or > > kvfree(), and it had better be OK to kvmalloc() something and pass it > > to kvfree(). > > kvfree() is fine but not kfree(). Ah, even more fun, then! ;-) Thanx, Paul
Re: [PATCH 0/2] rcu: Transform kfree_rcu() into kvfree_rcu()
On Wed, Feb 07, 2018 at 08:57:00AM -0500, Steven Rostedt wrote: > On Wed, 7 Feb 2018 00:31:04 -0800 > "Paul E. McKenney" wrote: > > > I see problems. We would then have two different names for exactly the > > same thing. > > > > Seems like it would be a lot easier to simply document the existing > > kfree_rcu() behavior, especially given that it apparently already works. > > The really doesn't seem to me to be worth a name change. > > Honestly, I don't believe this is an RCU sub-system decision. This is a > memory management decision. I couldn't agree more! To that end, what are your thoughts on this patch? https://lkml.kernel.org/r/1513895570-28640-1-git-send-email-rao.sho...@oracle.com Advantages include the ability to optimize based on sl[aou]b state, getting rid of the 4K offset hack in __is_kfree_rcu_offset(), better cache localite, and, as you say, putting the naming responsibility in the memory-management domain. > If we have kmalloc(), vmalloc(), kfree(), vfree() and kvfree(), and we > want kmalloc() to be freed with kfree(), and vmalloc() to be freed with > vfree(), and for strange reasons, we don't know how the data was > allocated we have kvfree(). That's an mm decision not an rcu one. We > should have kfree_rcu(), vfree_rcu() and kvfree_rcu(), and honestly, > they should not depend on kvfree() doing the same thing for everything. > Each should call the corresponding member that they represent. Which > would change this patch set. > > Why? Too much coupling between RCU and MM. What if in the future > something changes and kvfree() goes away or changes drastically. We > would then have to go through all the users of RCU to change them too. > > To me kvfree() is a special case and should not be used by RCU as a > generic function. That would make RCU and MM much more coupled than > necessary. And that is one reason I am viewing the name-change patch with great suspicion, especially given that there seems to be some controversy among the memory-management folks as to exactly what the names should be. Thanx, Paul
Re: [PATCH 0/2] rcu: Transform kfree_rcu() into kvfree_rcu()
On Wed, 7 Feb 2018 10:10:55 -0800 Matthew Wilcox wrote: > > For the record, I fully agree with Steve here. Thanks, but... > > > > And being a performance "fanatic" I don't like to have the extra branch > > (and compares) in the free code path... but it's a MM-decision (and > > sometimes you should not listen to "fanatics" ;-)) > > While free_rcu() is not withut its performance requirements, I think it's > currently dominated by cache misses and not by branches. By the time RCU > gets to run callbacks, memory is certainly L1/L2 cache-cold and probably > L3 cache-cold. Also calling the callback functions is utterly impossible > for the branch predictor. I agree with Matthew. This is far from any fast path. A few extra branches isn't going to hurt anything here as it's mostly just garbage collection. With or without the Spectre fixes. -- Steve
Re: [PATCH 0/2] rcu: Transform kfree_rcu() into kvfree_rcu()
On Wed, Feb 07, 2018 at 05:45:13PM +0100, Jesper Dangaard Brouer wrote: > On Wed, 7 Feb 2018 08:57:00 -0500 > Steven Rostedt wrote: > > To me kvfree() is a special case and should not be used by RCU as a > > generic function. That would make RCU and MM much more coupled than > > necessary. > > For the record, I fully agree with Steve here. > > And being a performance "fanatic" I don't like to have the extra branch > (and compares) in the free code path... but it's a MM-decision (and > sometimes you should not listen to "fanatics" ;-)) While free_rcu() is not withut its performance requirements, I think it's currently dominated by cache misses and not by branches. By the time RCU gets to run callbacks, memory is certainly L1/L2 cache-cold and probably L3 cache-cold. Also calling the callback functions is utterly impossible for the branch predictor.
Re: [PATCH 0/2] rcu: Transform kfree_rcu() into kvfree_rcu()
On Wed, 7 Feb 2018, Steven Rostedt wrote: > But a generic "malloc" or "free" that does things differently depending > on the size is a different story. They would not be used for cases with special requirements but for the throwaway allows where noone cares about these details. Its just a convenience for the developers that do not need to be bothered with too much detail because they are not dealing with codepaths that have special requirements.
Re: [PATCH 0/2] rcu: Transform kfree_rcu() into kvfree_rcu()
On Wed, 7 Feb 2018 09:19:36 -0800 Matthew Wilcox wrote: > > Please no, I hate subtle internal decisions like this. It makes > > debugging much more difficult, when allocating dynamic sized variables. > > When something works at one size but not the other. > > You know we already have kvmalloc()? Yes, and the name suggests exactly what it does. It has both "k" and "v" which tells me that if I use it it could be one or the other. But a generic "malloc" or "free" that does things differently depending on the size is a different story. -- Steve
Re: [PATCH 0/2] rcu: Transform kfree_rcu() into kvfree_rcu()
On Wed, Feb 07, 2018 at 12:09:49PM -0500, Steven Rostedt wrote: > > Maybe lets implement malloc(), free() and realloc() in the kernel to be > > consistent with user space use as possible? Only use the others > > allocation variants for special cases. > > They would need to drop the GFP part and default to GFP_KERNEL. Yes, exactly. > > So malloc would check allocation sizes and if < 2* PAGE_SIZE use kmalloc() > > otherwise vmalloc(). > > Please no, I hate subtle internal decisions like this. It makes > debugging much more difficult, when allocating dynamic sized variables. > When something works at one size but not the other. You know we already have kvmalloc()?
Re: [PATCH 0/2] rcu: Transform kfree_rcu() into kvfree_rcu()
On Wed, 7 Feb 2018 10:47:02 -0600 (CST) Christopher Lameter wrote: > On Tue, 6 Feb 2018, Matthew Wilcox wrote: > > > Personally, I would like us to rename kvfree() to just free(), and have > > malloc(x) be an alias to kvmalloc(x, GFP_KERNEL), but I haven't won that > > fight yet. > > Maybe lets implement malloc(), free() and realloc() in the kernel to be > consistent with user space use as possible? Only use the others > allocation variants for special cases. They would need to drop the GFP part and default to GFP_KERNEL. > > So malloc would check allocation sizes and if < 2* PAGE_SIZE use kmalloc() > otherwise vmalloc(). Please no, I hate subtle internal decisions like this. It makes debugging much more difficult, when allocating dynamic sized variables. When something works at one size but not the other. -- Steve > > free() would free anything you give it.
Re: [PATCH 0/2] rcu: Transform kfree_rcu() into kvfree_rcu()
On Tue, 6 Feb 2018, Matthew Wilcox wrote: > Personally, I would like us to rename kvfree() to just free(), and have > malloc(x) be an alias to kvmalloc(x, GFP_KERNEL), but I haven't won that > fight yet. Maybe lets implement malloc(), free() and realloc() in the kernel to be consistent with user space use as possible? Only use the others allocation variants for special cases. So malloc would check allocation sizes and if < 2* PAGE_SIZE use kmalloc() otherwise vmalloc(). free() would free anything you give it.
Re: [PATCH 0/2] rcu: Transform kfree_rcu() into kvfree_rcu()
On Wed, 7 Feb 2018 08:57:00 -0500 Steven Rostedt wrote: > On Wed, 7 Feb 2018 00:31:04 -0800 > "Paul E. McKenney" wrote: > > > I see problems. We would then have two different names for exactly the > > same thing. > > > > Seems like it would be a lot easier to simply document the existing > > kfree_rcu() behavior, especially given that it apparently already works. > > The really doesn't seem to me to be worth a name change. > > Honestly, I don't believe this is an RCU sub-system decision. This is a > memory management decision. > > If we have kmalloc(), vmalloc(), kfree(), vfree() and kvfree(), and we > want kmalloc() to be freed with kfree(), and vmalloc() to be freed with > vfree(), and for strange reasons, we don't know how the data was > allocated we have kvfree(). That's an mm decision not an rcu one. We > should have kfree_rcu(), vfree_rcu() and kvfree_rcu(), and honestly, > they should not depend on kvfree() doing the same thing for everything. > Each should call the corresponding member that they represent. Which > would change this patch set. > > Why? Too much coupling between RCU and MM. What if in the future > something changes and kvfree() goes away or changes drastically. We > would then have to go through all the users of RCU to change them too. > > To me kvfree() is a special case and should not be used by RCU as a > generic function. That would make RCU and MM much more coupled than > necessary. For the record, I fully agree with Steve here. And being a performance "fanatic" I don't like to have the extra branch (and compares) in the free code path... but it's a MM-decision (and sometimes you should not listen to "fanatics" ;-)) void kvfree(const void *addr) { if (is_vmalloc_addr(addr)) vfree(addr); else kfree(addr); } EXPORT_SYMBOL(kvfree); -- Best regards, Jesper Dangaard Brouer MSc.CS, Principal Kernel Engineer at Red Hat LinkedIn: http://www.linkedin.com/in/brouer static inline bool is_vmalloc_addr(const void *x) { #ifdef CONFIG_MMU unsigned long addr = (unsigned long)x; return addr >= VMALLOC_START && addr < VMALLOC_END; #else return false; #endif }
Re: [PATCH 0/2] rcu: Transform kfree_rcu() into kvfree_rcu()
On Wed, 7 Feb 2018 08:18:46 -0800 Matthew Wilcox wrote: > Do we need to be able to free any of those objects in order to rename > kfree_rcu() to just free_rcu()? I'm just nervous about tightly coupling free_rcu() with all the *free() from the memory management system. I've been burnt in the past by doing such things. What's the down side of having a way of matching *free_rcu() with all the *free()s? I think it's easier to understand, and rcu doesn't need to worry about changes of *free() code. To me: kfree_rcu(x); is just a quick way of doing 'kfree(x)' after a synchronize_rcu() call. But a "free_rcu(x)", is something I have to think about, because I don't know from the name exactly what it is doing. I know this may sound a bit bike shedding, but the less I need to think about how other sub systems work, the easier it is to concentrate on my own sub system. -- Steve
Re: [PATCH 0/2] rcu: Transform kfree_rcu() into kvfree_rcu()
On Wed, Feb 07, 2018 at 08:57:00AM -0500, Steven Rostedt wrote: > On Wed, 7 Feb 2018 00:31:04 -0800 > "Paul E. McKenney" wrote: > > > I see problems. We would then have two different names for exactly the > > same thing. > > > > Seems like it would be a lot easier to simply document the existing > > kfree_rcu() behavior, especially given that it apparently already works. > > The really doesn't seem to me to be worth a name change. > > Honestly, I don't believe this is an RCU sub-system decision. This is a > memory management decision. > > If we have kmalloc(), vmalloc(), kfree(), vfree() and kvfree(), and we You missed kvmalloc() ... > want kmalloc() to be freed with kfree(), and vmalloc() to be freed with > vfree(), and for strange reasons, we don't know how the data was > allocated we have kvfree(). That's an mm decision not an rcu one. We > should have kfree_rcu(), vfree_rcu() and kvfree_rcu(), and honestly, > they should not depend on kvfree() doing the same thing for everything. > Each should call the corresponding member that they represent. Which > would change this patch set. > > Why? Too much coupling between RCU and MM. What if in the future > something changes and kvfree() goes away or changes drastically. We > would then have to go through all the users of RCU to change them too. > > To me kvfree() is a special case and should not be used by RCU as a > generic function. That would make RCU and MM much more coupled than > necessary. I'd still like it to be called free_rcu() ... so let's turn it around. What memory can you allocate and then *not* free by calling kvfree()? kvfree() can free memory allocated by kmalloc(), vmalloc(), any slab allocation (is that guaranteed, or just something that happens to work?) I think it can't free per-cpu allocations, bootmem, DMA allocations, or alloc_page/get_free_page. Do we need to be able to free any of those objects in order to rename kfree_rcu() to just free_rcu()?
Re: [PATCH 0/2] rcu: Transform kfree_rcu() into kvfree_rcu()
On Tue, 6 Feb 2018, Paul E. McKenney wrote: > So it is OK to kvmalloc() something and pass it to either kfree() or > kvfree(), and it had better be OK to kvmalloc() something and pass it > to kvfree(). kvfree() is fine but not kfree().
Re: [PATCH 0/2] rcu: Transform kfree_rcu() into kvfree_rcu()
On Wed, 7 Feb 2018 00:31:04 -0800 "Paul E. McKenney" wrote: > I see problems. We would then have two different names for exactly the > same thing. > > Seems like it would be a lot easier to simply document the existing > kfree_rcu() behavior, especially given that it apparently already works. > The really doesn't seem to me to be worth a name change. Honestly, I don't believe this is an RCU sub-system decision. This is a memory management decision. If we have kmalloc(), vmalloc(), kfree(), vfree() and kvfree(), and we want kmalloc() to be freed with kfree(), and vmalloc() to be freed with vfree(), and for strange reasons, we don't know how the data was allocated we have kvfree(). That's an mm decision not an rcu one. We should have kfree_rcu(), vfree_rcu() and kvfree_rcu(), and honestly, they should not depend on kvfree() doing the same thing for everything. Each should call the corresponding member that they represent. Which would change this patch set. Why? Too much coupling between RCU and MM. What if in the future something changes and kvfree() goes away or changes drastically. We would then have to go through all the users of RCU to change them too. To me kvfree() is a special case and should not be used by RCU as a generic function. That would make RCU and MM much more coupled than necessary. -- Steve
Re: [PATCH 0/2] rcu: Transform kfree_rcu() into kvfree_rcu()
On Wed, Feb 07, 2018 at 10:57:28AM +0300, Kirill Tkhai wrote: > On 07.02.2018 08:02, Paul E. McKenney wrote: > > On Tue, Feb 06, 2018 at 08:23:34PM -0800, Matthew Wilcox wrote: > >> On Tue, Feb 06, 2018 at 06:17:03PM -0800, Paul E. McKenney wrote: > >>> So it is OK to kvmalloc() something and pass it to either kfree() or > >>> kvfree(), and it had better be OK to kvmalloc() something and pass it > >>> to kvfree(). > >>> > >>> Is it OK to kmalloc() something and pass it to kvfree()? > >> > >> Yes, it absolutely is. > >> > >> void kvfree(const void *addr) > >> { > >> if (is_vmalloc_addr(addr)) > >> vfree(addr); > >> else > >> kfree(addr); > >> } > >> > >>> If so, is it really useful to have two different names here, that is, > >>> both kfree_rcu() and kvfree_rcu()? > >> > >> I think it's handy to have all three of kvfree_rcu(), kfree_rcu() and > >> vfree_rcu() available in the API for the symmetry of calling kmalloc() > >> / kfree_rcu(). > >> > >> Personally, I would like us to rename kvfree() to just free(), and have > >> malloc(x) be an alias to kvmalloc(x, GFP_KERNEL), but I haven't won that > >> fight yet. > > > > But why not just have the existing kfree_rcu() API cover both kmalloc() > > and kvmalloc()? Perhaps I am not in the right forums, but I am not hearing > > anyone arguing that the RCU API has too few members. ;-) > > People, far from RCU internals, consider kfree_rcu() like an extension > of kfree(). And it's not clear it's need to dive into kfree_rcu() comments, > when someone is looking a primitive to free vmalloc'ed memory. Seems like a relatively simple lesson to teach. > Also, construction like > > obj = kvmalloc(); > kfree_rcu(obj); > > makes me think it's legitimately to use plain kfree() as pair bracket to > kvmalloc(). So it all works as is, then. > So the significant change of kfree_rcu() behavior will complicate stable > backporters > life, because they will need to keep in mind such differences between > different > kernel versions. If I understood your construction above, that significant change in kfree_rcu() behavior has already happened. > It seems if we are going to use the single primitive for both kmalloc() > and kvmalloc() memory, it has to have another name. But I don't see problems > with having both kfree_rcu() and kvfree_rcu(). I see problems. We would then have two different names for exactly the same thing. Seems like it would be a lot easier to simply document the existing kfree_rcu() behavior, especially given that it apparently already works. The really doesn't seem to me to be worth a name change. Thanx, Paul
Re: [PATCH 0/2] rcu: Transform kfree_rcu() into kvfree_rcu()
On Tue, Feb 06, 2018 at 11:54:09PM -0800, Josh Triplett wrote: > On Tue, Feb 06, 2018 at 09:02:00PM -0800, Paul E. McKenney wrote: > > On Tue, Feb 06, 2018 at 08:23:34PM -0800, Matthew Wilcox wrote: > > > On Tue, Feb 06, 2018 at 06:17:03PM -0800, Paul E. McKenney wrote: > > > > So it is OK to kvmalloc() something and pass it to either kfree() or > > > > kvfree(), and it had better be OK to kvmalloc() something and pass it > > > > to kvfree(). > > > > > > > > Is it OK to kmalloc() something and pass it to kvfree()? > > > > > > Yes, it absolutely is. > > > > > > void kvfree(const void *addr) > > > { > > > if (is_vmalloc_addr(addr)) > > > vfree(addr); > > > else > > > kfree(addr); > > > } > > > > > > > If so, is it really useful to have two different names here, that is, > > > > both kfree_rcu() and kvfree_rcu()? > > > > > > I think it's handy to have all three of kvfree_rcu(), kfree_rcu() and > > > vfree_rcu() available in the API for the symmetry of calling kmalloc() > > > / kfree_rcu(). > > > > > > Personally, I would like us to rename kvfree() to just free(), and have > > > malloc(x) be an alias to kvmalloc(x, GFP_KERNEL), but I haven't won that > > > fight yet. > > > > But why not just have the existing kfree_rcu() API cover both kmalloc() > > and kvmalloc()? Perhaps I am not in the right forums, but I am not hearing > > anyone arguing that the RCU API has too few members. ;-) > > I don't have any problem with having just `kvfree_rcu`, but having just > `kfree_rcu` seems confusingly asymmetric. I don't understand why kmalloc()/kvfree_rcu() would seem any less asymmetric than kvmalloc()/kfree_rcu(). > (Also, count me in favor of having just one "free" function, too.) We agree on that much, anyway. ;-) Thanx, Paul
Re: [PATCH 0/2] rcu: Transform kfree_rcu() into kvfree_rcu()
On 07.02.2018 08:02, Paul E. McKenney wrote: > On Tue, Feb 06, 2018 at 08:23:34PM -0800, Matthew Wilcox wrote: >> On Tue, Feb 06, 2018 at 06:17:03PM -0800, Paul E. McKenney wrote: >>> So it is OK to kvmalloc() something and pass it to either kfree() or >>> kvfree(), and it had better be OK to kvmalloc() something and pass it >>> to kvfree(). >>> >>> Is it OK to kmalloc() something and pass it to kvfree()? >> >> Yes, it absolutely is. >> >> void kvfree(const void *addr) >> { >> if (is_vmalloc_addr(addr)) >> vfree(addr); >> else >> kfree(addr); >> } >> >>> If so, is it really useful to have two different names here, that is, >>> both kfree_rcu() and kvfree_rcu()? >> >> I think it's handy to have all three of kvfree_rcu(), kfree_rcu() and >> vfree_rcu() available in the API for the symmetry of calling kmalloc() >> / kfree_rcu(). >> >> Personally, I would like us to rename kvfree() to just free(), and have >> malloc(x) be an alias to kvmalloc(x, GFP_KERNEL), but I haven't won that >> fight yet. > > But why not just have the existing kfree_rcu() API cover both kmalloc() > and kvmalloc()? Perhaps I am not in the right forums, but I am not hearing > anyone arguing that the RCU API has too few members. ;-) People, far from RCU internals, consider kfree_rcu() like an extension of kfree(). And it's not clear it's need to dive into kfree_rcu() comments, when someone is looking a primitive to free vmalloc'ed memory. Also, construction like obj = kvmalloc(); kfree_rcu(obj); makes me think it's legitimately to use plain kfree() as pair bracket to kvmalloc(). So the significant change of kfree_rcu() behavior will complicate stable backporters life, because they will need to keep in mind such differences between different kernel versions. It seems if we are going to use the single primitive for both kmalloc() and kvmalloc() memory, it has to have another name. But I don't see problems with having both kfree_rcu() and kvfree_rcu(). Kirill
Re: [PATCH 0/2] rcu: Transform kfree_rcu() into kvfree_rcu()
On Tue, Feb 06, 2018 at 09:02:00PM -0800, Paul E. McKenney wrote: > On Tue, Feb 06, 2018 at 08:23:34PM -0800, Matthew Wilcox wrote: > > On Tue, Feb 06, 2018 at 06:17:03PM -0800, Paul E. McKenney wrote: > > > So it is OK to kvmalloc() something and pass it to either kfree() or > > > kvfree(), and it had better be OK to kvmalloc() something and pass it > > > to kvfree(). > > > > > > Is it OK to kmalloc() something and pass it to kvfree()? > > > > Yes, it absolutely is. > > > > void kvfree(const void *addr) > > { > > if (is_vmalloc_addr(addr)) > > vfree(addr); > > else > > kfree(addr); > > } > > > > > If so, is it really useful to have two different names here, that is, > > > both kfree_rcu() and kvfree_rcu()? > > > > I think it's handy to have all three of kvfree_rcu(), kfree_rcu() and > > vfree_rcu() available in the API for the symmetry of calling kmalloc() > > / kfree_rcu(). > > > > Personally, I would like us to rename kvfree() to just free(), and have > > malloc(x) be an alias to kvmalloc(x, GFP_KERNEL), but I haven't won that > > fight yet. > > But why not just have the existing kfree_rcu() API cover both kmalloc() > and kvmalloc()? Perhaps I am not in the right forums, but I am not hearing > anyone arguing that the RCU API has too few members. ;-) I don't have any problem with having just `kvfree_rcu`, but having just `kfree_rcu` seems confusingly asymmetric. (Also, count me in favor of having just one "free" function, too.)
Re: [PATCH 0/2] rcu: Transform kfree_rcu() into kvfree_rcu()
On Tue, Feb 06, 2018 at 08:23:34PM -0800, Matthew Wilcox wrote: > On Tue, Feb 06, 2018 at 06:17:03PM -0800, Paul E. McKenney wrote: > > So it is OK to kvmalloc() something and pass it to either kfree() or > > kvfree(), and it had better be OK to kvmalloc() something and pass it > > to kvfree(). > > > > Is it OK to kmalloc() something and pass it to kvfree()? > > Yes, it absolutely is. > > void kvfree(const void *addr) > { > if (is_vmalloc_addr(addr)) > vfree(addr); > else > kfree(addr); > } > > > If so, is it really useful to have two different names here, that is, > > both kfree_rcu() and kvfree_rcu()? > > I think it's handy to have all three of kvfree_rcu(), kfree_rcu() and > vfree_rcu() available in the API for the symmetry of calling kmalloc() > / kfree_rcu(). > > Personally, I would like us to rename kvfree() to just free(), and have > malloc(x) be an alias to kvmalloc(x, GFP_KERNEL), but I haven't won that > fight yet. But why not just have the existing kfree_rcu() API cover both kmalloc() and kvmalloc()? Perhaps I am not in the right forums, but I am not hearing anyone arguing that the RCU API has too few members. ;-) Thanx, Paul
Re: [PATCH 0/2] rcu: Transform kfree_rcu() into kvfree_rcu()
On Tue, Feb 06, 2018 at 06:17:03PM -0800, Paul E. McKenney wrote: > So it is OK to kvmalloc() something and pass it to either kfree() or > kvfree(), and it had better be OK to kvmalloc() something and pass it > to kvfree(). > > Is it OK to kmalloc() something and pass it to kvfree()? Yes, it absolutely is. void kvfree(const void *addr) { if (is_vmalloc_addr(addr)) vfree(addr); else kfree(addr); } > If so, is it really useful to have two different names here, that is, > both kfree_rcu() and kvfree_rcu()? I think it's handy to have all three of kvfree_rcu(), kfree_rcu() and vfree_rcu() available in the API for the symmetry of calling kmalloc() / kfree_rcu(). Personally, I would like us to rename kvfree() to just free(), and have malloc(x) be an alias to kvmalloc(x, GFP_KERNEL), but I haven't won that fight yet.
Re: [PATCH 0/2] rcu: Transform kfree_rcu() into kvfree_rcu()
On Tue, Feb 06, 2018 at 01:19:29PM +0300, Kirill Tkhai wrote: > Recent times kvmalloc() begun widely be used in kernel. > Some of such memory allocations have to be freed after > rcu grace period, and this patchset introduces a generic > primitive for doing this. > > Actually, everything is made in [1/2]. Patch [2/2] is just > added to make new kvfree_rcu() have the first user. > > The patch [1/2] transforms kfree_rcu(), its sub definitions > and its sub functions into kvfree_rcu() form. The most > significant change is in __rcu_reclaim(), where kvfree() > is used instead of kfree(). Since kvfree() is able to > have a deal with memory allocated via kmalloc(), vmalloc() > and kvmalloc(); kfree_rcu() and vfree_rcu() may simply > be defined through this new kvfree_rcu(). Interesting. So it is OK to kvmalloc() something and pass it to either kfree() or kvfree(), and it had better be OK to kvmalloc() something and pass it to kvfree(). Is it OK to kmalloc() something and pass it to kvfree()? If so, is it really useful to have two different names here, that is, both kfree_rcu() and kvfree_rcu()? Also adding Jesper and Rao on CC for their awareness. Thanx, Paul > --- > > Kirill Tkhai (2): > rcu: Transform kfree_rcu() into kvfree_rcu() > mm: Use kvfree_rcu() in update_memcg_params() > > > include/linux/rcupdate.h | 31 +-- > include/linux/rcutiny.h|4 ++-- > include/linux/rcutree.h|2 +- > include/trace/events/rcu.h | 12 ++-- > kernel/rcu/rcu.h |8 > kernel/rcu/tree.c | 14 +++--- > kernel/rcu/tree_plugin.h | 10 +- > mm/slab_common.c | 10 +- > 8 files changed, 43 insertions(+), 48 deletions(-) > > -- > Signed-off-by: Kirill Tkhai >