Re: [PATCH 0/2] rcu: Transform kfree_rcu() into kvfree_rcu()

[Paul E. McKenney]

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()

[Paul E. McKenney]

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()

[Paul E. McKenney]

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()

[Paul E. McKenney]

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()

[Paul E. McKenney]

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()

[Paul E. McKenney]

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()

[Paul E. McKenney]

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()

[Paul E. McKenney]

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()

[Steven Rostedt]

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()

[Steven Rostedt]

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()

[Matthew Wilcox]

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()

[Matthew Wilcox]

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()

[Christopher Lameter]

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()

[Christopher Lameter]

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()

[Steven Rostedt]

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()

[Steven Rostedt]

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()

[Matthew Wilcox]

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()

[Matthew Wilcox]

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()

[Steven Rostedt]

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()

[Steven Rostedt]

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()

[Christopher Lameter]

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()

[Christopher Lameter]

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()

[Jesper Dangaard Brouer]

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()

[Jesper Dangaard Brouer]

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()

[Steven Rostedt]

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()

[Steven Rostedt]

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()

[Matthew Wilcox]

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()

[Matthew Wilcox]

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()

[Christopher Lameter]

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()

[Christopher Lameter]

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()

[Steven Rostedt]

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()

[Steven Rostedt]

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()

[Paul E. McKenney]

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()

[Paul E. McKenney]

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()

[Paul E. McKenney]

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()

[Paul E. McKenney]

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()

[Kirill Tkhai]

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()

[Kirill Tkhai]

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()

[Josh Triplett]

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()

[Josh Triplett]

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()

[Paul E. McKenney]

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()

[Paul E. McKenney]

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()

[Matthew Wilcox]

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()

[Matthew Wilcox]

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()

[Paul E. McKenney]

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 
> 

Re: [PATCH 0/2] rcu: Transform kfree_rcu() into kvfree_rcu()

[Paul E. McKenney]

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 
>