Re: [PATCH v3 09/16] sl[au]b: always get the cache from its page in kfree
On Fri, Sep 21, 2012 at 11:14:45PM +0300, Pekka Enberg wrote: > On Fri, Sep 21, 2012 at 11:07 PM, Tejun Heo wrote: > > Not necessarily disagreeing, but I don't think it's helpful to set the > > bar impossibly high. Even static_key doesn't have "exactly *zero*" > > impact. Let's stick to as minimal as possible when not in use and > > reasonable in use. > > For !CONFIG_MEMCG_KMEM, it should be exactly zero. No need to play > games with static_key. Ah, misread, though you were talking about MEMCG_KMEM && not in use. Yeap, if turned off in Kconfig, it shouldn't have any performance overhead. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 09/16] sl[au]b: always get the cache from its page in kfree
On Fri, Sep 21, 2012 at 11:07 PM, Tejun Heo wrote: > Not necessarily disagreeing, but I don't think it's helpful to set the > bar impossibly high. Even static_key doesn't have "exactly *zero*" > impact. Let's stick to as minimal as possible when not in use and > reasonable in use. For !CONFIG_MEMCG_KMEM, it should be exactly zero. No need to play games with static_key. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 09/16] sl[au]b: always get the cache from its page in kfree
Hello, On Fri, Sep 21, 2012 at 12:41:52PM +0300, Pekka Enberg wrote: > > I am already using static keys extensively in this patchset, and that is > > how I intend to handle this particular case. > > Cool. > > The key point here is that !CONFIG_MEMCG_KMEM should have exactly *zero* > performance impact and CONFIG_MEMCG_KMEM disabled at runtime should have > absolute minimal impact. Not necessarily disagreeing, but I don't think it's helpful to set the bar impossibly high. Even static_key doesn't have "exactly *zero*" impact. Let's stick to as minimal as possible when not in use and reasonable in use. And, yeah, this one can be easily solved by using static_key. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 09/16] sl[au]b: always get the cache from its page in kfree
On Fri, 21 Sep 2012, Glauber Costa wrote: > > We should assume that most distributions enable CONFIG_MEMCG_KMEM, > > right? Therfore, any performance impact should be dependent on whether > > or not kmem memcg is *enabled* at runtime or not. > > > > Can we use the "static key" thingy introduced by tracing folks for this? > > Yes. > > I am already using static keys extensively in this patchset, and that is > how I intend to handle this particular case. Cool. The key point here is that !CONFIG_MEMCG_KMEM should have exactly *zero* performance impact and CONFIG_MEMCG_KMEM disabled at runtime should have absolute minimal impact. Pekka -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 09/16] sl[au]b: always get the cache from its page in kfree
On 09/21/2012 01:33 PM, Pekka Enberg wrote:
> On Wed, Sep 19, 2012 at 10:42 AM, Glauber Costa wrote:
index f2d760c..18de3f6 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3938,9 +3938,12 @@ EXPORT_SYMBOL(__kmalloc);
* Free an object which was previously allocated from this
* cache.
*/
-void kmem_cache_free(struct kmem_cache *cachep, void *objp)
+void kmem_cache_free(struct kmem_cache *s, void *objp)
{
unsigned long flags;
+struct kmem_cache *cachep = virt_to_cache(objp);
+
+VM_BUG_ON(!slab_equal_or_parent(cachep, s));
>>>
>>> This is an extremely hot path of the kernel and you are adding significant
>>> processing. Check how the benchmarks are influenced by this change.
>>> virt_to_cache can be a bit expensive.
>>
>> Would it be enough for you to have a separate code path for
>> !CONFIG_MEMCG_KMEM?
>>
>> I don't really see another way to do it, aside from deriving the cache
>> from the object in our case. I am open to suggestions if you do.
>
> We should assume that most distributions enable CONFIG_MEMCG_KMEM,
> right? Therfore, any performance impact should be dependent on whether
> or not kmem memcg is *enabled* at runtime or not.
>
> Can we use the "static key" thingy introduced by tracing folks for this?
>
Yes.
I am already using static keys extensively in this patchset, and that is
how I intend to handle this particular case.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 09/16] sl[au]b: always get the cache from its page in kfree
On Wed, Sep 19, 2012 at 10:42 AM, Glauber Costa wrote:
>>> index f2d760c..18de3f6 100644
>>> --- a/mm/slab.c
>>> +++ b/mm/slab.c
>>> @@ -3938,9 +3938,12 @@ EXPORT_SYMBOL(__kmalloc);
>>> * Free an object which was previously allocated from this
>>> * cache.
>>> */
>>> -void kmem_cache_free(struct kmem_cache *cachep, void *objp)
>>> +void kmem_cache_free(struct kmem_cache *s, void *objp)
>>> {
>>> unsigned long flags;
>>> +struct kmem_cache *cachep = virt_to_cache(objp);
>>> +
>>> +VM_BUG_ON(!slab_equal_or_parent(cachep, s));
>>
>> This is an extremely hot path of the kernel and you are adding significant
>> processing. Check how the benchmarks are influenced by this change.
>> virt_to_cache can be a bit expensive.
>
> Would it be enough for you to have a separate code path for
> !CONFIG_MEMCG_KMEM?
>
> I don't really see another way to do it, aside from deriving the cache
> from the object in our case. I am open to suggestions if you do.
We should assume that most distributions enable CONFIG_MEMCG_KMEM,
right? Therfore, any performance impact should be dependent on whether
or not kmem memcg is *enabled* at runtime or not.
Can we use the "static key" thingy introduced by tracing folks for this?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 09/16] sl[au]b: always get the cache from its page in kfree
On Wed, 19 Sep 2012, Glauber Costa wrote: > > This is an extremely hot path of the kernel and you are adding significant > > processing. Check how the benchmarks are influenced by this change. > > virt_to_cache can be a bit expensive. > Would it be enough for you to have a separate code path for > !CONFIG_MEMCG_KMEM? Yes, at least add an #ifdef around this. > I don't really see another way to do it, aside from deriving the cache > from the object in our case. I am open to suggestions if you do. Rethink the whole memcg approach and find some other way to do it? This whole scheme is very intrusive and is likely to increase instability in the VM given the explosion of lru lists, duplication of slab caches and significantly more complex processing throughout the VM. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 09/16] sl[au]b: always get the cache from its page in kfree
On 09/18/2012 07:28 PM, Christoph Lameter wrote:
> On Tue, 18 Sep 2012, Glauber Costa wrote:
>
>> index f2d760c..18de3f6 100644
>> --- a/mm/slab.c
>> +++ b/mm/slab.c
>> @@ -3938,9 +3938,12 @@ EXPORT_SYMBOL(__kmalloc);
>> * Free an object which was previously allocated from this
>> * cache.
>> */
>> -void kmem_cache_free(struct kmem_cache *cachep, void *objp)
>> +void kmem_cache_free(struct kmem_cache *s, void *objp)
>> {
>> unsigned long flags;
>> +struct kmem_cache *cachep = virt_to_cache(objp);
>> +
>> +VM_BUG_ON(!slab_equal_or_parent(cachep, s));
>>
>
> This is an extremely hot path of the kernel and you are adding significant
> processing. Check how the benchmarks are influenced by this change.
> virt_to_cache can be a bit expensive.
>
Would it be enough for you to have a separate code path for
!CONFIG_MEMCG_KMEM?
I don't really see another way to do it, aside from deriving the cache
from the object in our case. I am open to suggestions if you do.
>> diff --git a/mm/slub.c b/mm/slub.c
>> index 4778548..a045dfc 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -2604,7 +2604,9 @@ void kmem_cache_free(struct kmem_cache *s, void *x)
>>
>> page = virt_to_head_page(x);
>>
>> -slab_free(s, page, x, _RET_IP_);
>> +VM_BUG_ON(!slab_equal_or_parent(page->slab, s));
>> +
>> +slab_free(page->slab, page, x, _RET_IP_);
>>
>
> Less of a problem here but you are eroding one advantage that slab has had
> in the past over slub in terms of freeing objects.
>
likewise.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 09/16] sl[au]b: always get the cache from its page in kfree
On Tue, 18 Sep 2012, Glauber Costa wrote:
> index f2d760c..18de3f6 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -3938,9 +3938,12 @@ EXPORT_SYMBOL(__kmalloc);
> * Free an object which was previously allocated from this
> * cache.
> */
> -void kmem_cache_free(struct kmem_cache *cachep, void *objp)
> +void kmem_cache_free(struct kmem_cache *s, void *objp)
> {
> unsigned long flags;
> + struct kmem_cache *cachep = virt_to_cache(objp);
> +
> + VM_BUG_ON(!slab_equal_or_parent(cachep, s));
>
This is an extremely hot path of the kernel and you are adding significant
processing. Check how the benchmarks are influenced by this change.
virt_to_cache can be a bit expensive.
> diff --git a/mm/slub.c b/mm/slub.c
> index 4778548..a045dfc 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -2604,7 +2604,9 @@ void kmem_cache_free(struct kmem_cache *s, void *x)
>
> page = virt_to_head_page(x);
>
> - slab_free(s, page, x, _RET_IP_);
> + VM_BUG_ON(!slab_equal_or_parent(page->slab, s));
> +
> + slab_free(page->slab, page, x, _RET_IP_);
>
Less of a problem here but you are eroding one advantage that slab has had
in the past over slub in terms of freeing objects.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
