[patch][rfc] acpi: do not use kmem caches

[patch][rfc] acpi: do not use kmem caches

[Nick Piggin]

Hi,
What does everyone think about this patch?

--

ACPI subsystem creates a handful of kmem caches that are not particularly
appropriate. Most of them seem to be empty or nearly empty most of the time,
and the others don't have too many objects. In this situation, kmem caches
can actually have more overhead than they save.

Just use ACPI's generic code for its acpi_cache_t type.
---
 drivers/acpi/osl.c              |   85 ----------------------------------------
 include/acpi/acmacros.h         |    2 
 include/acpi/platform/aclinux.h |    9 ----
 3 files changed, 3 insertions(+), 93 deletions(-)

Index: linux-2.6/include/acpi/acmacros.h
===================================================================
--- linux-2.6.orig/include/acpi/acmacros.h
+++ linux-2.6/include/acpi/acmacros.h
@@ -670,7 +670,7 @@ struct acpi_integer_overlay {
 #define ACPI_ALLOCATE_ZEROED(a)     acpi_ut_allocate_zeroed((acpi_size)(a), ACPI_MEM_PARAMETERS)
 #endif
 #ifndef ACPI_FREE
-#define ACPI_FREE(a)                acpio_os_free(a)
+#define ACPI_FREE(a)                acpi_os_free(a)
 #endif
 #define ACPI_MEM_TRACKING(a)
 
Index: linux-2.6/include/acpi/platform/aclinux.h
===================================================================
--- linux-2.6.orig/include/acpi/platform/aclinux.h
+++ linux-2.6/include/acpi/platform/aclinux.h
@@ -65,7 +65,6 @@
 /* Host-dependent types and defines */
 
 #define ACPI_MACHINE_WIDTH          BITS_PER_LONG
-#define acpi_cache_t                        struct kmem_cache
 #define acpi_spinlock                   spinlock_t *
 #define ACPI_EXPORT_SYMBOL(symbol)  EXPORT_SYMBOL(symbol);
 #define strtoul                     simple_strtoul
@@ -73,6 +72,8 @@
 /* Full namespace pathname length limit - arbitrary */
 #define ACPI_PATHNAME_MAX              256
 
+#define ACPI_USE_LOCAL_CACHE
+
 #else				/* !__KERNEL__ */
 
 #include <stdarg.h>
@@ -128,12 +129,6 @@ static inline void *acpi_os_allocate_zer
 	return kzalloc(size, irqs_disabled()? GFP_ATOMIC : GFP_KERNEL);
 }
 
-static inline void *acpi_os_acquire_object(acpi_cache_t * cache)
-{
-	return kmem_cache_zalloc(cache,
-				 irqs_disabled()? GFP_ATOMIC : GFP_KERNEL);
-}
-
 #define ACPI_ALLOCATE(a)	acpi_os_allocate(a)
 #define ACPI_ALLOCATE_ZEROED(a)	acpi_os_allocate_zeroed(a)
 #define ACPI_FREE(a)		kfree(a)
Index: linux-2.6/drivers/acpi/osl.c
===================================================================
--- linux-2.6.orig/drivers/acpi/osl.c
+++ linux-2.6/drivers/acpi/osl.c
@@ -1212,91 +1212,6 @@ void acpi_os_release_lock(acpi_spinlock
 	spin_unlock_irqrestore(lockp, flags);
 }
 
-#ifndef ACPI_USE_LOCAL_CACHE
-
-/*******************************************************************************
- *
- * FUNCTION:    acpi_os_create_cache
- *
- * PARAMETERS:  name      - Ascii name for the cache
- *              size      - Size of each cached object
- *              depth     - Maximum depth of the cache (in objects) <ignored>
- *              cache     - Where the new cache object is returned
- *
- * RETURN:      status
- *
- * DESCRIPTION: Create a cache object
- *
- ******************************************************************************/
-
-acpi_status
-acpi_os_create_cache(char *name, u16 size, u16 depth, acpi_cache_t ** cache)
-{
-	*cache = kmem_cache_create(name, size, 0, 0, NULL);
-	if (*cache == NULL)
-		return AE_ERROR;
-	else
-		return AE_OK;
-}
-
-/*******************************************************************************
- *
- * FUNCTION:    acpi_os_purge_cache
- *
- * PARAMETERS:  Cache           - Handle to cache object
- *
- * RETURN:      Status
- *
- * DESCRIPTION: Free all objects within the requested cache.
- *
- ******************************************************************************/
-
-acpi_status acpi_os_purge_cache(acpi_cache_t * cache)
-{
-	kmem_cache_shrink(cache);
-	return (AE_OK);
-}
-
-/*******************************************************************************
- *
- * FUNCTION:    acpi_os_delete_cache
- *
- * PARAMETERS:  Cache           - Handle to cache object
- *
- * RETURN:      Status
- *
- * DESCRIPTION: Free all objects within the requested cache and delete the
- *              cache object.
- *
- ******************************************************************************/
-
-acpi_status acpi_os_delete_cache(acpi_cache_t * cache)
-{
-	kmem_cache_destroy(cache);
-	return (AE_OK);
-}
-
-/*******************************************************************************
- *
- * FUNCTION:    acpi_os_release_object
- *
- * PARAMETERS:  Cache       - Handle to cache object
- *              Object      - The object to be released
- *
- * RETURN:      None
- *
- * DESCRIPTION: Release an object to the specified cache.  If cache is full,
- *              the object is deleted.
- *
- ******************************************************************************/
-
-acpi_status acpi_os_release_object(acpi_cache_t * cache, void *object)
-{
-	kmem_cache_free(cache, object);
-	return (AE_OK);
-}
-#endif
-
 /**
  *	acpi_dmi_dump - dump DMI slots needed for blacklist entry
  *

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] acpi: do not use kmem caches

[Pekka Enberg]

Hi Nick,

On Mon, Dec 1, 2008 at 10:31 AM, Nick Piggin <npiggin@suse.de> wrote:
> What does everyone think about this patch?

Doesn't matter all that much for SLUB which already merges the ACPI
caches with the generic kmalloc caches. But for SLAB, it's an obvious
wil so:

Acked-by: Pekka Enberg <penberg@cs.helsinki.fi>

> ACPI subsystem creates a handful of kmem caches that are not particularly
> appropriate. Most of them seem to be empty or nearly empty most of the time,
> and the others don't have too many objects. In this situation, kmem caches
> can actually have more overhead than they save.
>
> Just use ACPI's generic code for its acpi_cache_t type.
> ---
>  drivers/acpi/osl.c              |   85 ----------------------------------------
>  include/acpi/acmacros.h         |    2
>  include/acpi/platform/aclinux.h |    9 ----
>  3 files changed, 3 insertions(+), 93 deletions(-)
>
> Index: linux-2.6/include/acpi/acmacros.h
> ===================================================================
> --- linux-2.6.orig/include/acpi/acmacros.h
> +++ linux-2.6/include/acpi/acmacros.h
> @@ -670,7 +670,7 @@ struct acpi_integer_overlay {
>  #define ACPI_ALLOCATE_ZEROED(a)     acpi_ut_allocate_zeroed((acpi_size)(a), ACPI_MEM_PARAMETERS)
>  #endif
>  #ifndef ACPI_FREE
> -#define ACPI_FREE(a)                acpio_os_free(a)
> +#define ACPI_FREE(a)                acpi_os_free(a)
>  #endif
>  #define ACPI_MEM_TRACKING(a)
>
> Index: linux-2.6/include/acpi/platform/aclinux.h
> ===================================================================
> --- linux-2.6.orig/include/acpi/platform/aclinux.h
> +++ linux-2.6/include/acpi/platform/aclinux.h
> @@ -65,7 +65,6 @@
>  /* Host-dependent types and defines */
>
>  #define ACPI_MACHINE_WIDTH          BITS_PER_LONG
> -#define acpi_cache_t                        struct kmem_cache
>  #define acpi_spinlock                   spinlock_t *
>  #define ACPI_EXPORT_SYMBOL(symbol)  EXPORT_SYMBOL(symbol);
>  #define strtoul                     simple_strtoul
> @@ -73,6 +72,8 @@
>  /* Full namespace pathname length limit - arbitrary */
>  #define ACPI_PATHNAME_MAX              256
>
> +#define ACPI_USE_LOCAL_CACHE
> +
>  #else                          /* !__KERNEL__ */
>
>  #include <stdarg.h>
> @@ -128,12 +129,6 @@ static inline void *acpi_os_allocate_zer
>        return kzalloc(size, irqs_disabled()? GFP_ATOMIC : GFP_KERNEL);
>  }
>
> -static inline void *acpi_os_acquire_object(acpi_cache_t * cache)
> -{
> -       return kmem_cache_zalloc(cache,
> -                                irqs_disabled()? GFP_ATOMIC : GFP_KERNEL);
> -}
> -
>  #define ACPI_ALLOCATE(a)       acpi_os_allocate(a)
>  #define ACPI_ALLOCATE_ZEROED(a)        acpi_os_allocate_zeroed(a)
>  #define ACPI_FREE(a)           kfree(a)
> Index: linux-2.6/drivers/acpi/osl.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/osl.c
> +++ linux-2.6/drivers/acpi/osl.c
> @@ -1212,91 +1212,6 @@ void acpi_os_release_lock(acpi_spinlock
>        spin_unlock_irqrestore(lockp, flags);
>  }
>
> -#ifndef ACPI_USE_LOCAL_CACHE
> -
> -/*******************************************************************************
> - *
> - * FUNCTION:    acpi_os_create_cache
> - *
> - * PARAMETERS:  name      - Ascii name for the cache
> - *              size      - Size of each cached object
> - *              depth     - Maximum depth of the cache (in objects) <ignored>
> - *              cache     - Where the new cache object is returned
> - *
> - * RETURN:      status
> - *
> - * DESCRIPTION: Create a cache object
> - *
> - ******************************************************************************/
> -
> -acpi_status
> -acpi_os_create_cache(char *name, u16 size, u16 depth, acpi_cache_t ** cache)
> -{
> -       *cache = kmem_cache_create(name, size, 0, 0, NULL);
> -       if (*cache == NULL)
> -               return AE_ERROR;
> -       else
> -               return AE_OK;
> -}
> -
> -/*******************************************************************************
> - *
> - * FUNCTION:    acpi_os_purge_cache
> - *
> - * PARAMETERS:  Cache           - Handle to cache object
> - *
> - * RETURN:      Status
> - *
> - * DESCRIPTION: Free all objects within the requested cache.
> - *
> - ******************************************************************************/
> -
> -acpi_status acpi_os_purge_cache(acpi_cache_t * cache)
> -{
> -       kmem_cache_shrink(cache);
> -       return (AE_OK);
> -}
> -
> -/*******************************************************************************
> - *
> - * FUNCTION:    acpi_os_delete_cache
> - *
> - * PARAMETERS:  Cache           - Handle to cache object
> - *
> - * RETURN:      Status
> - *
> - * DESCRIPTION: Free all objects within the requested cache and delete the
> - *              cache object.
> - *
> - ******************************************************************************/
> -
> -acpi_status acpi_os_delete_cache(acpi_cache_t * cache)
> -{
> -       kmem_cache_destroy(cache);
> -       return (AE_OK);
> -}
> -
> -/*******************************************************************************
> - *
> - * FUNCTION:    acpi_os_release_object
> - *
> - * PARAMETERS:  Cache       - Handle to cache object
> - *              Object      - The object to be released
> - *
> - * RETURN:      None
> - *
> - * DESCRIPTION: Release an object to the specified cache.  If cache is full,
> - *              the object is deleted.
> - *
> - ******************************************************************************/
> -
> -acpi_status acpi_os_release_object(acpi_cache_t * cache, void *object)
> -{
> -       kmem_cache_free(cache, object);
> -       return (AE_OK);
> -}
> -#endif
> -
>  /**
>  *     acpi_dmi_dump - dump DMI slots needed for blacklist entry
>  *
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] acpi: do not use kmem caches

[Nick Piggin]

On Mon, Dec 01, 2008 at 01:18:33PM +0200, Pekka Enberg wrote:
> Hi Nick,
> 
> On Mon, Dec 1, 2008 at 10:31 AM, Nick Piggin <npiggin@suse.de> wrote:
> > What does everyone think about this patch?
> 
> Doesn't matter all that much for SLUB which already merges the ACPI
> caches with the generic kmalloc caches. But for SLAB, it's an obvious
> wil so:
> 
> Acked-by: Pekka Enberg <penberg@cs.helsinki.fi>

Actually I think it is also somewhat of a bugfix (not to mention that it
seems like a good idea to share testing code with other operating systems).

Because acpi_os_purge_cache seems to want to free all active objects in the
cache, but kmem_cache_shrink actually does nothing of the sort. So there
ends up being a memory leak.

In practice, there are warnings in some of the allocators if this ever
happens and I don't think I have seen these trigger, so perhaps the ACPI
code which calls this never actually cares. But still seems like a good
idea to use the generic code (which seems to get this right)

Anyway, thanks for the ack. Yes it should help SLAB.

> 
> > ACPI subsystem creates a handful of kmem caches that are not particularly
> > appropriate. Most of them seem to be empty or nearly empty most of the time,
> > and the others don't have too many objects. In this situation, kmem caches
> > can actually have more overhead than they save.
> >
> > Just use ACPI's generic code for its acpi_cache_t type.
> > ---
> >  drivers/acpi/osl.c              |   85 ----------------------------------------
> >  include/acpi/acmacros.h         |    2
> >  include/acpi/platform/aclinux.h |    9 ----
> >  3 files changed, 3 insertions(+), 93 deletions(-)
> >
> > Index: linux-2.6/include/acpi/acmacros.h
> > ===================================================================
> > --- linux-2.6.orig/include/acpi/acmacros.h
> > +++ linux-2.6/include/acpi/acmacros.h
> > @@ -670,7 +670,7 @@ struct acpi_integer_overlay {
> >  #define ACPI_ALLOCATE_ZEROED(a)     acpi_ut_allocate_zeroed((acpi_size)(a), ACPI_MEM_PARAMETERS)
> >  #endif
> >  #ifndef ACPI_FREE
> > -#define ACPI_FREE(a)                acpio_os_free(a)
> > +#define ACPI_FREE(a)                acpi_os_free(a)
> >  #endif
> >  #define ACPI_MEM_TRACKING(a)
> >
> > Index: linux-2.6/include/acpi/platform/aclinux.h
> > ===================================================================
> > --- linux-2.6.orig/include/acpi/platform/aclinux.h
> > +++ linux-2.6/include/acpi/platform/aclinux.h
> > @@ -65,7 +65,6 @@
> >  /* Host-dependent types and defines */
> >
> >  #define ACPI_MACHINE_WIDTH          BITS_PER_LONG
> > -#define acpi_cache_t                        struct kmem_cache
> >  #define acpi_spinlock                   spinlock_t *
> >  #define ACPI_EXPORT_SYMBOL(symbol)  EXPORT_SYMBOL(symbol);
> >  #define strtoul                     simple_strtoul
> > @@ -73,6 +72,8 @@
> >  /* Full namespace pathname length limit - arbitrary */
> >  #define ACPI_PATHNAME_MAX              256
> >
> > +#define ACPI_USE_LOCAL_CACHE
> > +
> >  #else                          /* !__KERNEL__ */
> >
> >  #include <stdarg.h>
> > @@ -128,12 +129,6 @@ static inline void *acpi_os_allocate_zer
> >        return kzalloc(size, irqs_disabled()? GFP_ATOMIC : GFP_KERNEL);
> >  }
> >
> > -static inline void *acpi_os_acquire_object(acpi_cache_t * cache)
> > -{
> > -       return kmem_cache_zalloc(cache,
> > -                                irqs_disabled()? GFP_ATOMIC : GFP_KERNEL);
> > -}
> > -
> >  #define ACPI_ALLOCATE(a)       acpi_os_allocate(a)
> >  #define ACPI_ALLOCATE_ZEROED(a)        acpi_os_allocate_zeroed(a)
> >  #define ACPI_FREE(a)           kfree(a)
> > Index: linux-2.6/drivers/acpi/osl.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/acpi/osl.c
> > +++ linux-2.6/drivers/acpi/osl.c
> > @@ -1212,91 +1212,6 @@ void acpi_os_release_lock(acpi_spinlock
> >        spin_unlock_irqrestore(lockp, flags);
> >  }
> >
> > -#ifndef ACPI_USE_LOCAL_CACHE
> > -
> > -/*******************************************************************************
> > - *
> > - * FUNCTION:    acpi_os_create_cache
> > - *
> > - * PARAMETERS:  name      - Ascii name for the cache
> > - *              size      - Size of each cached object
> > - *              depth     - Maximum depth of the cache (in objects) <ignored>
> > - *              cache     - Where the new cache object is returned
> > - *
> > - * RETURN:      status
> > - *
> > - * DESCRIPTION: Create a cache object
> > - *
> > - ******************************************************************************/
> > -
> > -acpi_status
> > -acpi_os_create_cache(char *name, u16 size, u16 depth, acpi_cache_t ** cache)
> > -{
> > -       *cache = kmem_cache_create(name, size, 0, 0, NULL);
> > -       if (*cache == NULL)
> > -               return AE_ERROR;
> > -       else
> > -               return AE_OK;
> > -}
> > -
> > -/*******************************************************************************
> > - *
> > - * FUNCTION:    acpi_os_purge_cache
> > - *
> > - * PARAMETERS:  Cache           - Handle to cache object
> > - *
> > - * RETURN:      Status
> > - *
> > - * DESCRIPTION: Free all objects within the requested cache.
> > - *
> > - ******************************************************************************/
> > -
> > -acpi_status acpi_os_purge_cache(acpi_cache_t * cache)
> > -{
> > -       kmem_cache_shrink(cache);
> > -       return (AE_OK);
> > -}
> > -
> > -/*******************************************************************************
> > - *
> > - * FUNCTION:    acpi_os_delete_cache
> > - *
> > - * PARAMETERS:  Cache           - Handle to cache object
> > - *
> > - * RETURN:      Status
> > - *
> > - * DESCRIPTION: Free all objects within the requested cache and delete the
> > - *              cache object.
> > - *
> > - ******************************************************************************/
> > -
> > -acpi_status acpi_os_delete_cache(acpi_cache_t * cache)
> > -{
> > -       kmem_cache_destroy(cache);
> > -       return (AE_OK);
> > -}
> > -
> > -/*******************************************************************************
> > - *
> > - * FUNCTION:    acpi_os_release_object
> > - *
> > - * PARAMETERS:  Cache       - Handle to cache object
> > - *              Object      - The object to be released
> > - *
> > - * RETURN:      None
> > - *
> > - * DESCRIPTION: Release an object to the specified cache.  If cache is full,
> > - *              the object is deleted.
> > - *
> > - ******************************************************************************/
> > -
> > -acpi_status acpi_os_release_object(acpi_cache_t * cache, void *object)
> > -{
> > -       kmem_cache_free(cache, object);
> > -       return (AE_OK);
> > -}
> > -#endif
> > -
> >  /**
> >  *     acpi_dmi_dump - dump DMI slots needed for blacklist entry
> >  *
> >
> > --
> > To unsubscribe, send a message with 'unsubscribe linux-mm' in
> > the body to majordomo@kvack.org.  For more info on Linux MM,
> > see: http://www.linux-mm.org/ .
> > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> >

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] acpi: do not use kmem caches

[Alexey Starikovskiy]

Nick Piggin wrote:
> On Mon, Dec 01, 2008 at 01:18:33PM +0200, Pekka Enberg wrote:
>   
>> Hi Nick,
>>
>> On Mon, Dec 1, 2008 at 10:31 AM, Nick Piggin <npiggin@suse.de> wrote:
>>     
>>> What does everyone think about this patch?
>>>       
>> Doesn't matter all that much for SLUB which already merges the ACPI
>> caches with the generic kmalloc caches. But for SLAB, it's an obvious
>> wil so:
>>
>> Acked-by: Pekka Enberg <penberg@cs.helsinki.fi>
>>     
>
> Actually I think it is also somewhat of a bugfix (not to mention that it
> seems like a good idea to share testing code with other operating systems).
>
>   
It is not "kind of a bugfix". Caches were used to allocate all frequenly
created objects of fixed size. Removing native cache interface will
increase memory consumption and increase code size, and will make it harder
to spot actual memory leaks.
> Because acpi_os_purge_cache seems to want to free all active objects in the
> cache, but kmem_cache_shrink actually does nothing of the sort. So there
> ends up being a memory leak.
>   
No. acpi_os_purge_cache wants to free only unused objects, so it is a 
direct map to

> In practice, there are warnings in some of the allocators if this ever
> happens and I don't think I have seen these trigger, so perhaps the ACPI
> code which calls this never actually cares. But still seems like a good
> idea to use the generic code (which seems to get this right)
>
> Anyway, thanks for the ack. Yes it should help SLAB.
>
>   
NACK.

Regards,
Alex.
>>> ACPI subsystem creates a handful of kmem caches that are not particularly
>>> appropriate. Most of them seem to be empty or nearly empty most of the time,
>>> and the others don't have too many objects. In this situation, kmem caches
>>> can actually have more overhead than they save.
>>>
>>> Just use ACPI's generic code for its acpi_cache_t type.
>>> ---
>>>  drivers/acpi/osl.c              |   85 ----------------------------------------
>>>  include/acpi/acmacros.h         |    2
>>>  include/acpi/platform/aclinux.h |    9 ----
>>>  3 files changed, 3 insertions(+), 93 deletions(-)
>>>
>>> Index: linux-2.6/include/acpi/acmacros.h
>>> ===================================================================
>>> --- linux-2.6.orig/include/acpi/acmacros.h
>>> +++ linux-2.6/include/acpi/acmacros.h
>>> @@ -670,7 +670,7 @@ struct acpi_integer_overlay {
>>>  #define ACPI_ALLOCATE_ZEROED(a)     acpi_ut_allocate_zeroed((acpi_size)(a), ACPI_MEM_PARAMETERS)
>>>  #endif
>>>  #ifndef ACPI_FREE
>>> -#define ACPI_FREE(a)                acpio_os_free(a)
>>> +#define ACPI_FREE(a)                acpi_os_free(a)
>>>  #endif
>>>  #define ACPI_MEM_TRACKING(a)
>>>
>>> Index: linux-2.6/include/acpi/platform/aclinux.h
>>> ===================================================================
>>> --- linux-2.6.orig/include/acpi/platform/aclinux.h
>>> +++ linux-2.6/include/acpi/platform/aclinux.h
>>> @@ -65,7 +65,6 @@
>>>  /* Host-dependent types and defines */
>>>
>>>  #define ACPI_MACHINE_WIDTH          BITS_PER_LONG
>>> -#define acpi_cache_t                        struct kmem_cache
>>>  #define acpi_spinlock                   spinlock_t *
>>>  #define ACPI_EXPORT_SYMBOL(symbol)  EXPORT_SYMBOL(symbol);
>>>  #define strtoul                     simple_strtoul
>>> @@ -73,6 +72,8 @@
>>>  /* Full namespace pathname length limit - arbitrary */
>>>  #define ACPI_PATHNAME_MAX              256
>>>
>>> +#define ACPI_USE_LOCAL_CACHE
>>> +
>>>  #else                          /* !__KERNEL__ */
>>>
>>>  #include <stdarg.h>
>>> @@ -128,12 +129,6 @@ static inline void *acpi_os_allocate_zer
>>>        return kzalloc(size, irqs_disabled()? GFP_ATOMIC : GFP_KERNEL);
>>>  }
>>>
>>> -static inline void *acpi_os_acquire_object(acpi_cache_t * cache)
>>> -{
>>> -       return kmem_cache_zalloc(cache,
>>> -                                irqs_disabled()? GFP_ATOMIC : GFP_KERNEL);
>>> -}
>>> -
>>>  #define ACPI_ALLOCATE(a)       acpi_os_allocate(a)
>>>  #define ACPI_ALLOCATE_ZEROED(a)        acpi_os_allocate_zeroed(a)
>>>  #define ACPI_FREE(a)           kfree(a)
>>> Index: linux-2.6/drivers/acpi/osl.c
>>> ===================================================================
>>> --- linux-2.6.orig/drivers/acpi/osl.c
>>> +++ linux-2.6/drivers/acpi/osl.c
>>> @@ -1212,91 +1212,6 @@ void acpi_os_release_lock(acpi_spinlock
>>>        spin_unlock_irqrestore(lockp, flags);
>>>  }
>>>
>>> -#ifndef ACPI_USE_LOCAL_CACHE
>>> -
>>> -/*******************************************************************************
>>> - *
>>> - * FUNCTION:    acpi_os_create_cache
>>> - *
>>> - * PARAMETERS:  name      - Ascii name for the cache
>>> - *              size      - Size of each cached object
>>> - *              depth     - Maximum depth of the cache (in objects) <ignored>
>>> - *              cache     - Where the new cache object is returned
>>> - *
>>> - * RETURN:      status
>>> - *
>>> - * DESCRIPTION: Create a cache object
>>> - *
>>> - ******************************************************************************/
>>> -
>>> -acpi_status
>>> -acpi_os_create_cache(char *name, u16 size, u16 depth, acpi_cache_t ** cache)
>>> -{
>>> -       *cache = kmem_cache_create(name, size, 0, 0, NULL);
>>> -       if (*cache == NULL)
>>> -               return AE_ERROR;
>>> -       else
>>> -               return AE_OK;
>>> -}
>>> -
>>> -/*******************************************************************************
>>> - *
>>> - * FUNCTION:    acpi_os_purge_cache
>>> - *
>>> - * PARAMETERS:  Cache           - Handle to cache object
>>> - *
>>> - * RETURN:      Status
>>> - *
>>> - * DESCRIPTION: Free all objects within the requested cache.
>>> - *
>>> - ******************************************************************************/
>>> -
>>> -acpi_status acpi_os_purge_cache(acpi_cache_t * cache)
>>> -{
>>> -       kmem_cache_shrink(cache);
>>> -       return (AE_OK);
>>> -}
>>> -
>>> -/*******************************************************************************
>>> - *
>>> - * FUNCTION:    acpi_os_delete_cache
>>> - *
>>> - * PARAMETERS:  Cache           - Handle to cache object
>>> - *
>>> - * RETURN:      Status
>>> - *
>>> - * DESCRIPTION: Free all objects within the requested cache and delete the
>>> - *              cache object.
>>> - *
>>> - ******************************************************************************/
>>> -
>>> -acpi_status acpi_os_delete_cache(acpi_cache_t * cache)
>>> -{
>>> -       kmem_cache_destroy(cache);
>>> -       return (AE_OK);
>>> -}
>>> -
>>> -/*******************************************************************************
>>> - *
>>> - * FUNCTION:    acpi_os_release_object
>>> - *
>>> - * PARAMETERS:  Cache       - Handle to cache object
>>> - *              Object      - The object to be released
>>> - *
>>> - * RETURN:      None
>>> - *
>>> - * DESCRIPTION: Release an object to the specified cache.  If cache is full,
>>> - *              the object is deleted.
>>> - *
>>> - ******************************************************************************/
>>> -
>>> -acpi_status acpi_os_release_object(acpi_cache_t * cache, void *object)
>>> -{
>>> -       kmem_cache_free(cache, object);
>>> -       return (AE_OK);
>>> -}
>>> -#endif
>>> -
>>>  /**
>>>  *     acpi_dmi_dump - dump DMI slots needed for blacklist entry
>>>  *
>>>
>>> --
>>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>>> the body to majordomo@kvack.org.  For more info on Linux MM,
>>> see: http://www.linux-mm.org/ .
>>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>>>
>>>       
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] acpi: do not use kmem caches

[Nick Piggin]

On Mon, Dec 01, 2008 at 04:12:35PM +0300, Alexey Starikovskiy wrote:
> Nick Piggin wrote:
> >On Mon, Dec 01, 2008 at 01:18:33PM +0200, Pekka Enberg wrote:
> >  
> >>Hi Nick,
> >>
> >>On Mon, Dec 1, 2008 at 10:31 AM, Nick Piggin <npiggin@suse.de> wrote:
> >>    
> >>>What does everyone think about this patch?
> >>>      
> >>Doesn't matter all that much for SLUB which already merges the ACPI
> >>caches with the generic kmalloc caches. But for SLAB, it's an obvious
> >>wil so:
> >>
> >>Acked-by: Pekka Enberg <penberg@cs.helsinki.fi>
> >>    
> >
> >Actually I think it is also somewhat of a bugfix (not to mention that it
> >seems like a good idea to share testing code with other operating systems).
> >
> >  
> It is not "kind of a bugfix". Caches were used to allocate all frequenly
> created objects of fixed size. Removing native cache interface will
> increase memory consumption and increase code size, and will make it harder
> to spot actual memory leaks.

Slabs can take a non-trivial amount of memory. On bigger machines it can
be many megabytes. On smaller machines perhaps not, but what is the benefit??
The only ACPI slabs I have with anything in them total a couple of hundred
kB, and anyway they are 64 and 32 bytes so they will pack exactly into
kmalloc slabs.

Code size... Does it matter? Is it really performance critical? If you are
worried about code size, then I will implement them directly with kmalloc
and kfree for you.

kmem caches are not exactly an appropriate tool to detect memory leaks. If
that were the case then we'd have million kmem caches everywhere.


> >Because acpi_os_purge_cache seems to want to free all active objects in the
> >cache, but kmem_cache_shrink actually does nothing of the sort. So there
> >ends up being a memory leak.
> >  
> No. acpi_os_purge_cache wants to free only unused objects, so it is a 
> direct map to

Ah OK I misread, that's the cache's freelist... ACPI shouldn't be poking
this button inside the slab allocator anyway, honestly. What is it
for?

 
> >In practice, there are warnings in some of the allocators if this ever
> >happens and I don't think I have seen these trigger, so perhaps the ACPI
> >code which calls this never actually cares. But still seems like a good
> >idea to use the generic code (which seems to get this right)
> >
> >Anyway, thanks for the ack. Yes it should help SLAB.
> >
> >  
> NACK.

Is there a reasonable performance or memory win by using kmem cache? If
not, then they should not be used.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] acpi: do not use kmem caches

[Alexey Starikovskiy]

Nick Piggin wrote:
> On Mon, Dec 01, 2008 at 04:12:35PM +0300, Alexey Starikovskiy wrote:
>   
>> Nick Piggin wrote:
>>     
>>> On Mon, Dec 01, 2008 at 01:18:33PM +0200, Pekka Enberg wrote:
>>>  
>>>       
>>>> Hi Nick,
>>>>
>>>> On Mon, Dec 1, 2008 at 10:31 AM, Nick Piggin <npiggin@suse.de> wrote:
>>>>    
>>>>         
>>>>> What does everyone think about this patch?
>>>>>      
>>>>>           
>>>> Doesn't matter all that much for SLUB which already merges the ACPI
>>>> caches with the generic kmalloc caches. But for SLAB, it's an obvious
>>>> wil so:
>>>>
>>>> Acked-by: Pekka Enberg <penberg@cs.helsinki.fi>
>>>>    
>>>>         
>>> Actually I think it is also somewhat of a bugfix (not to mention that it
>>> seems like a good idea to share testing code with other operating systems).
>>>
>>>  
>>>       
>> It is not "kind of a bugfix". Caches were used to allocate all frequenly
>> created objects of fixed size. Removing native cache interface will
>> increase memory consumption and increase code size, and will make it harder
>> to spot actual memory leaks.
>>     
>
> Slabs can take a non-trivial amount of memory. On bigger machines it can
> be many megabytes. On smaller machines perhaps not, but what is the benefit??
> The only ACPI slabs I have with anything in them total a couple of hundred
> kB, and anyway they are 64 and 32 bytes so they will pack exactly into
> kmalloc slabs.
>   
Oh right, we don't care about memory footprint any longer...
> Code size... Does it matter? Is it really performance critical? If you are
> worried about code size, then I will implement them directly with kmalloc
> and kfree for you.
>   
Why then you try to delete ACPICA code, which might be just disabled by
undefining ACPI_USE_LOCAL_CACHE?
If you do want to go that path, you need to create patch against ACPICA, not
Linux code.
> kmem caches are not exactly an appropriate tool to detect memory leaks. If
> that were the case then we'd have million kmem caches everywhere.
>
>
>   
>>> Because acpi_os_purge_cache seems to want to free all active objects in the
>>> cache, but kmem_cache_shrink actually does nothing of the sort. So there
>>> ends up being a memory leak.
>>>  
>>>       
>> No. acpi_os_purge_cache wants to free only unused objects, so it is a 
>> direct map to
>>     
>
> Ah OK I misread, that's the cache's freelist... ACPI shouldn't be poking
> this button inside the slab allocator anyway, honestly. What is it
> for?
>   
And it is not actually used -- you cannot unload ACPI interpreter, and
this function is called only from there.
>>> In practice, there are warnings in some of the allocators if this ever
>>> happens and I don't think I have seen these trigger, so perhaps the ACPI
>>> code which calls this never actually cares. But still seems like a good
>>> idea to use the generic code (which seems to get this right)
>>>
>>> Anyway, thanks for the ack. Yes it should help SLAB.
>>>
>>>  
>>>       
>> NACK.
>>     
>
> Is there a reasonable performance or memory win by using kmem cache? If
> not, then they should not be used
ACPI is still working in machines with several megabytes of RAM and 
100mhz Pentium processors. Do you say we should just not consider them 
any longer?
If so, then just delete all ACPICA caches altogether.
And this still needs to be patch against ACPICA, not Linux -- at least 
with BSD license attached.

Regards,
Alex.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] acpi: do not use kmem caches

[Nick Piggin]

On Mon, Dec 01, 2008 at 05:14:36PM +0300, Alexey Starikovskiy wrote:
> Nick Piggin wrote:
> >On Mon, Dec 01, 2008 at 04:12:35PM +0300, Alexey Starikovskiy wrote:
> >  
> >>Nick Piggin wrote:
> >>    
> >>>On Mon, Dec 01, 2008 at 01:18:33PM +0200, Pekka Enberg wrote:
> >>> 
> >>>      
> >>>>Hi Nick,
> >>>>
> >>>>On Mon, Dec 1, 2008 at 10:31 AM, Nick Piggin <npiggin@suse.de> wrote:
> >>>>   
> >>>>        
> >>>>>What does everyone think about this patch?
> >>>>>     
> >>>>>          
> >>>>Doesn't matter all that much for SLUB which already merges the ACPI
> >>>>caches with the generic kmalloc caches. But for SLAB, it's an obvious
> >>>>wil so:
> >>>>
> >>>>Acked-by: Pekka Enberg <penberg@cs.helsinki.fi>
> >>>>   
> >>>>        
> >>>Actually I think it is also somewhat of a bugfix (not to mention that it
> >>>seems like a good idea to share testing code with other operating 
> >>>systems).
> >>>
> >>> 
> >>>      
> >>It is not "kind of a bugfix". Caches were used to allocate all frequenly
> >>created objects of fixed size. Removing native cache interface will
> >>increase memory consumption and increase code size, and will make it 
> >>harder
> >>to spot actual memory leaks.
> >>    
> >
> >Slabs can take a non-trivial amount of memory. On bigger machines it can
> >be many megabytes. On smaller machines perhaps not, but what is the 
> >benefit??
> >The only ACPI slabs I have with anything in them total a couple of hundred
> >kB, and anyway they are 64 and 32 bytes so they will pack exactly into
> >kmalloc slabs.
> >  
> Oh right, we don't care about memory footprint any longer...

On the contrary
http://marc.info/?l=linux-fsdevel&m=122812043206762&w=2

Some people still do. I'm hoping to save memory. With my configuration, the
patch definitely saves memory.


> >Code size... Does it matter? Is it really performance critical? If you are
> >worried about code size, then I will implement them directly with kmalloc
> >and kfree for you.
> >  
> Why then you try to delete ACPICA code, which might be just disabled by
> undefining ACPI_USE_LOCAL_CACHE?
> If you do want to go that path, you need to create patch against ACPICA, not
> Linux code.

I don't know what else uses ACPICA, so no.


> >kmem caches are not exactly an appropriate tool to detect memory leaks. If
> >that were the case then we'd have million kmem caches everywhere.
> >
> >
> >  
> >>>Because acpi_os_purge_cache seems to want to free all active objects in 
> >>>the
> >>>cache, but kmem_cache_shrink actually does nothing of the sort. So there
> >>>ends up being a memory leak.
> >>> 
> >>>      
> >>No. acpi_os_purge_cache wants to free only unused objects, so it is a 
> >>direct map to
> >>    
> >
> >Ah OK I misread, that's the cache's freelist... ACPI shouldn't be poking
> >this button inside the slab allocator anyway, honestly. What is it
> >for?
> >  
> And it is not actually used -- you cannot unload ACPI interpreter, and
> this function is called only from there.

Should be deleted anyway. This seems to be the only user of kmem_cache_shrink
in the kernel and it's bogus anyway.


> >Is there a reasonable performance or memory win by using kmem cache? If
> >not, then they should not be used
> ACPI is still working in machines with several megabytes of RAM and 
> 100mhz Pentium processors. Do you say we should just not consider them 
> any longer?

No. As I said, I am trying to save RAM. If it turns out not to save RAM
in some configurations then of course I will reconsider.


> If so, then just delete all ACPICA caches altogether.
> And this still needs to be patch against ACPICA, not Linux -- at least 
> with BSD license attached.

I don't have a problem with that.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] acpi: do not use kmem caches

[Christoph Hellwig]

On Mon, Dec 01, 2008 at 05:14:36PM +0300, Alexey Starikovskiy wrote:
> Why then you try to delete ACPICA code, which might be just disabled by
> undefining ACPI_USE_LOCAL_CACHE?
> If you do want to go that path, you need to create patch against ACPICA, not
> Linux code.

Sorry dude, but that's not how Linux development works.   Please talk to
some intel OTC folks to get an advice on how it does.

>> Ah OK I misread, that's the cache's freelist... ACPI shouldn't be poking
>> this button inside the slab allocator anyway, honestly. What is it
>> for?
>>   
> And it is not actually used -- you cannot unload ACPI interpreter, and
> this function is called only from there.

Care to remove all this dead code?

>> Is there a reasonable performance or memory win by using kmem cache? If
>> not, then they should not be used
> ACPI is still working in machines with several megabytes of RAM and  
> 100mhz Pentium processors. Do you say we should just not consider them  
> any longer?
> If so, then just delete all ACPICA caches altogether.

As Nick is trying to explain you for a while it's not actually going
to be a performance benefit for these, quite contrary because of how
slab caches waste a lot of memory when only used very lightly or not
at all.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] acpi: do not use kmem caches

[Alexey Starikovskiy]

Christoph Hellwig wrote:
> On Mon, Dec 01, 2008 at 05:14:36PM +0300, Alexey Starikovskiy wrote:
>   
>> Why then you try to delete ACPICA code, which might be just disabled by
>> undefining ACPI_USE_LOCAL_CACHE?
>> If you do want to go that path, you need to create patch against ACPICA, not
>> Linux code.
>>     
>
> Sorry dude, but that's not how Linux development works.   Please talk to
> some intel OTC folks to get an advice on how it does.
>
>   
We are not speaking about Linux code here -- Nick changed ACPICA files.
And he already admits, that his patch is at least half-way wrong.
Sorry dude, ACPICA code is not Linux only, so one needs some care while
dropping some functionality from it.
>>> Ah OK I misread, that's the cache's freelist... ACPI shouldn't be poking
>>> this button inside the slab allocator anyway, honestly. What is it
>>> for?
>>>   
>>>       
>> And it is not actually used -- you cannot unload ACPI interpreter, and
>> this function is called only from there.
>>     
>
> Care to remove all this dead code?
>
>   
It is used at least in Windows userspace programs. So, removing these 4 
lines only from Linux will
create another headache for Len during his merging of each new ACPICA 
release into Linux.
>>> Is there a reasonable performance or memory win by using kmem cache? If
>>> not, then they should not be used
>>>       
>> ACPI is still working in machines with several megabytes of RAM and  
>> 100mhz Pentium processors. Do you say we should just not consider them  
>> any longer?
>> If so, then just delete all ACPICA caches altogether.
>>     
>
> As Nick is trying to explain you for a while it's not actually going
> to be a performance benefit for these, quite contrary because of how
> slab caches waste a lot of memory when only used very lightly or not
> at all.
>
>   
If you care to do the math, and I helped you in another posting, he may 
save about 11k in 32bit mode on thinkpad, and loose 70k in 64bit mode on 
similar thinkpad.

Regards,
Alex.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] acpi: do not use kmem caches

[Pekka Enberg]

Hi,

On Mon, 2008-12-01 at 16:12 +0300, Alexey Starikovskiy wrote:
> > Actually I think it is also somewhat of a bugfix (not to mention that it
> > seems like a good idea to share testing code with other operating systems).
> 
> It is not "kind of a bugfix". Caches were used to allocate all frequenly
> created objects of fixed size. Removing native cache interface will
> increase memory consumption and increase code size, and will make it harder
> to spot actual memory leaks.

Excuse me?

Why do you think Nick's patch is going to _increase_ memory consumption?
SLUB _already_ merges the ACPI caches with kmalloc caches so you won't
see any difference there. For SLAB, it's a gain because there's not
enough activity going on which results in lots of unused space in the
slabs (which is, btw, the reason SLUB does slab merging in the first
place).

I'm also wondering why you think it's going to increase text size.
Unless the ACPI code is doing something weird, the kmalloc() and
kzalloc() shouldn't be a problem at all.

For memory leaks, CONFIG_SLAB_LEAK has been in mainline for a long time
plus there are the kmemleak patches floating around. So I fail to see
how it's going to be harder to spot the memory leaks. After all, the
rest of the kernel manages fine without a special wrapper, so how is
ACPI any different here?

			Pekka

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] acpi: do not use kmem caches

[Alexey Starikovskiy]

Pekka Enberg wrote:
> Hi,
>
> On Mon, 2008-12-01 at 16:12 +0300, Alexey Starikovskiy wrote:
>   
>>> Actually I think it is also somewhat of a bugfix (not to mention that it
>>> seems like a good idea to share testing code with other operating systems).
>>>       
>> It is not "kind of a bugfix". Caches were used to allocate all frequenly
>> created objects of fixed size. Removing native cache interface will
>> increase memory consumption and increase code size, and will make it harder
>> to spot actual memory leaks.
>>     
>
> Excuse me?
>
> Why do you think Nick's patch is going to _increase_ memory consumption?
> SLUB _already_ merges the ACPI caches with kmalloc caches so you won't
> see any difference there. For SLAB, it's a gain because there's not
> enough activity going on which results in lots of unused space in the
> slabs (which is, btw, the reason SLUB does slab merging in the first
> place).
>
>   
Because SLAB has standard memory wells of 2^x size. None of cached ACPI
objects has exactly this size, so bigger block will be used. Plus, 
internal ACPICA
caching will add some overhead.
> I'm also wondering why you think it's going to increase text size.
> Unless the ACPI code is doing something weird, the kmalloc() and
> kzalloc() shouldn't be a problem at all.
>
>   
if you don't use ACPI_USE_LOCAL_CACHE
ACPICA will enable it's own cache implementation, so it will increase
code size.
> For memory leaks, CONFIG_SLAB_LEAK has been in mainline for a long time
> plus there are the kmemleak patches floating around. So I fail to see
> how it's going to be harder to spot the memory leaks.
It will give you a memory leak, not the kind of it, right?
>  After all, the
> rest of the kernel manages fine without a special wrapper, so how is
> ACPI any different here?
>   
Do you have another interpreter in kernel space?


Regards,
Alex.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] acpi: do not use kmem caches

[Nick Piggin]

On Mon, Dec 01, 2008 at 05:02:50PM +0300, Alexey Starikovskiy wrote:
> Pekka Enberg wrote:
> >Hi,
> >
> >On Mon, 2008-12-01 at 16:12 +0300, Alexey Starikovskiy wrote:
> >  
> >>>Actually I think it is also somewhat of a bugfix (not to mention that it
> >>>seems like a good idea to share testing code with other operating 
> >>>systems).
> >>>      
> >>It is not "kind of a bugfix". Caches were used to allocate all frequenly
> >>created objects of fixed size. Removing native cache interface will
> >>increase memory consumption and increase code size, and will make it 
> >>harder
> >>to spot actual memory leaks.
> >>    
> >
> >Excuse me?
> >
> >Why do you think Nick's patch is going to _increase_ memory consumption?
> >SLUB _already_ merges the ACPI caches with kmalloc caches so you won't
> >see any difference there. For SLAB, it's a gain because there's not
> >enough activity going on which results in lots of unused space in the
> >slabs (which is, btw, the reason SLUB does slab merging in the first
> >place).
> >
> >  
> Because SLAB has standard memory wells of 2^x size. None of cached ACPI
> objects has exactly this size, so bigger block will be used. Plus, 
> internal ACPICA
> caching will add some overhead.

That's an insane looking caching thing now that I come to closely read
the code. There is so much stuff there that I thought it must have been
doing something useful which is why I didn't replace the Linux functions
with kmalloc/kfree directly.

There is really some operating system you support that has such a poor
allocator that you think ACPI can do better in 300 lines of code? Why
not just rip that whole thing out?


> >I'm also wondering why you think it's going to increase text size.
> >Unless the ACPI code is doing something weird, the kmalloc() and
> >kzalloc() shouldn't be a problem at all.
> >
> >  
> if you don't use ACPI_USE_LOCAL_CACHE
> ACPICA will enable it's own cache implementation, so it will increase
> code size.
> >For memory leaks, CONFIG_SLAB_LEAK has been in mainline for a long time
> >plus there are the kmemleak patches floating around. So I fail to see
> >how it's going to be harder to spot the memory leaks.
> It will give you a memory leak, not the kind of it, right?
> > After all, the
> >rest of the kernel manages fine without a special wrapper, so how is
> >ACPI any different here?
> >  
> Do you have another interpreter in kernel space?

So what makes it special?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] acpi: do not use kmem caches

[Alexey Starikovskiy]

Nick Piggin wrote:
> On Mon, Dec 01, 2008 at 05:02:50PM +0300, Alexey Starikovskiy wrote:
>   
>> Because SLAB has standard memory wells of 2^x size. None of cached ACPI
>> objects has exactly this size, so bigger block will be used. Plus, 
>> internal ACPICA caching will add some overhead.
>>     
>
> That's an insane looking caching thing now that I come to closely read
> the code. There is so much stuff there that I thought it must have been
> doing something useful which is why I didn't replace the Linux functions
> with kmalloc/kfree directly.
>
> There is really some operating system you support that has such a poor
> allocator that you think ACPI can do better in 300 lines of code? Why
> not just rip that whole thing out?
>   
You would laugh, this is due to Windows userspace debug library -- it 
checks for
memory leaks by default, and it takes ages to do this.
And ACPICA maintainer is sitting on Windows, so he _cares_.
>> Do you have another interpreter in kernel space?
>>     
>
> So what makes it special?
>
>   
You don't know what size of program you will end up with.
DSDT could be almost empty, or you could have several thousand of SSDT 
tables.



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] acpi: do not use kmem caches

[Nick Piggin]

On Mon, Dec 01, 2008 at 07:45:14PM +0300, Alexey Starikovskiy wrote:
> Nick Piggin wrote:
> >On Mon, Dec 01, 2008 at 05:02:50PM +0300, Alexey Starikovskiy wrote:
> >  
> >>Because SLAB has standard memory wells of 2^x size. None of cached ACPI
> >>objects has exactly this size, so bigger block will be used. Plus, 
> >>internal ACPICA caching will add some overhead.
> >>    
> >
> >That's an insane looking caching thing now that I come to closely read
> >the code. There is so much stuff there that I thought it must have been
> >doing something useful which is why I didn't replace the Linux functions
> >with kmalloc/kfree directly.
> >
> >There is really some operating system you support that has such a poor
> >allocator that you think ACPI can do better in 300 lines of code? Why
> >not just rip that whole thing out?
> >  
> You would laugh, this is due to Windows userspace debug library -- it 
> checks for
> memory leaks by default, and it takes ages to do this.

OK... circumvent the debug library? :) I won't argue. But I agree it is
wrong for Linux so my patch is not good. It needs to use kmalloc at least.


> And ACPICA maintainer is sitting on Windows, so he _cares_.
> >>Do you have another interpreter in kernel space?
> >>    
> >
> >So what makes it special?
> >
> >  
> You don't know what size of program you will end up with.
> DSDT could be almost empty, or you could have several thousand of SSDT 
> tables.

OK. And I guess the sizes of the objects seem to be variable as well.
I still think kmalloc would be fine, and would save memory in some
situations. On tiny systems it is less likely to because each slab
costs less memory.

On a big system, the caches can take some MB each in some configurations.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: [patch][rfc] acpi: do not use kmem caches

[Moore, Robert]

As I recall, the ACPICA local cache greatly improves performance of the iASL compiler and AcpiExec on Windows (for BIOS writers, iASL on Windows is most important).


>-----Original Message-----
>From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
>owner@vger.kernel.org] On Behalf Of Alexey Starikovskiy
>Sent: Monday, December 01, 2008 8:45 AM
>To: Nick Piggin
>Cc: Pekka Enberg; Linux Memory Management List; linux-acpi@vger.kernel.org;
>lenb@kernel.org
>Subject: Re: [patch][rfc] acpi: do not use kmem caches
>
>Nick Piggin wrote:
>> On Mon, Dec 01, 2008 at 05:02:50PM +0300, Alexey Starikovskiy wrote:
>>
>>> Because SLAB has standard memory wells of 2^x size. None of cached ACPI
>>> objects has exactly this size, so bigger block will be used. Plus,
>>> internal ACPICA caching will add some overhead.
>>>
>>
>> That's an insane looking caching thing now that I come to closely read
>> the code. There is so much stuff there that I thought it must have been
>> doing something useful which is why I didn't replace the Linux functions
>> with kmalloc/kfree directly.
>>
>> There is really some operating system you support that has such a poor
>> allocator that you think ACPI can do better in 300 lines of code? Why
>> not just rip that whole thing out?
>>
>You would laugh, this is due to Windows userspace debug library -- it
>checks for
>memory leaks by default, and it takes ages to do this.
>And ACPICA maintainer is sitting on Windows, so he _cares_.
>>> Do you have another interpreter in kernel space?
>>>
>>
>> So what makes it special?
>>
>>
>You don't know what size of program you will end up with.
>DSDT could be almost empty, or you could have several thousand of SSDT
>tables.
>
>
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] acpi: do not use kmem caches

[Andi Kleen]

"Moore, Robert" <robert.moore@intel.com> writes:

> As I recall, the ACPICA local cache greatly improves performance of the iASL compiler and AcpiExec on Windows (for BIOS writers, iASL on Windows is most important).
>

Perhaps it would be a possibility to isolate the cache in a special layer
that is only compiled in for Windows?

-Andi

-- 
ak@linux.intel.com

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: [patch][rfc] acpi: do not use kmem caches

[Moore, Robert]

It essentially already is, with a compile-time switch.


>-----Original Message-----
>From: Andi Kleen [mailto:andi@firstfloor.org]
>Sent: Monday, December 01, 2008 9:31 AM
>To: Moore, Robert
>Cc: Alexey Starikovskiy; Nick Piggin; Pekka Enberg; Linux Memory Management
>List; linux-acpi@vger.kernel.org; lenb@kernel.org
>Subject: Re: [patch][rfc] acpi: do not use kmem caches
>
>"Moore, Robert" <robert.moore@intel.com> writes:
>
>> As I recall, the ACPICA local cache greatly improves performance of the
>iASL compiler and AcpiExec on Windows (for BIOS writers, iASL on Windows is
>most important).
>>
>
>Perhaps it would be a possibility to isolate the cache in a special layer
>that is only compiled in for Windows?
>
>-Andi
>
>--
>ak@linux.intel.com

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] acpi: do not use kmem caches

[Christoph Hellwig]

On Mon, Dec 01, 2008 at 07:45:14PM +0300, Alexey Starikovskiy wrote:
> You would laugh, this is due to Windows userspace debug library -- it  
> checks for
> memory leaks by default, and it takes ages to do this.
> And ACPICA maintainer is sitting on Windows, so he _cares_.

So what about getting a non-moronic maintainer instead?  Really this
whole ACPI code is a piece of turd exactly because of shit like this.
Can't Intel get their act together and do a proper ACPI implementation
for Linux instead of this junk?

Or at least stop arguing and throwing bureaucratic stones in the way of
those wanting to sort out this mess.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] acpi: do not use kmem caches

[Alexey Starikovskiy]

Christoph Hellwig wrote:
> On Mon, Dec 01, 2008 at 07:45:14PM +0300, Alexey Starikovskiy wrote:
>   
>> You would laugh, this is due to Windows userspace debug library -- it  
>> checks for
>> memory leaks by default, and it takes ages to do this.
>> And ACPICA maintainer is sitting on Windows, so he _cares_.
>>     
>
> So what about getting a non-moronic maintainer instead?  Really this
> whole ACPI code is a piece of turd exactly because of shit like this.
> Can't Intel get their act together and do a proper ACPI implementation
> for Linux instead of this junk?
>
> Or at least stop arguing and throwing bureaucratic stones in the way of
> those wanting to sort out this mess.
>
>   
Christoph, please, I don't work for Intel :)
How long will it take for _you_ to write another ACPICA ?
I assume it will be shining diamond?

Regards,
Alex.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] acpi: do not use kmem caches

[Len Brown]

> Or at least stop arguing and throwing bureaucratic stones in the way of
> those wanting to sort out this mess.

I think we all would be better served if there were more facts
and fewer insults on this thread, can we do that please?

I do not think the extra work we need to do for ACPICA changes
are a significant hurdle here. We will do what is best for Linux --
which is what we though we were doing when we changed ACPICA
so Linux could use native caching in the first place.

The only question that should be on the table here is how
to make Linux be the best it can be.

thanks,
-Len


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] acpi: do not use kmem caches

[Nick Piggin]

On Mon, Dec 01, 2008 at 12:53:04PM -0500, Len Brown wrote:
> 
> > Or at least stop arguing and throwing bureaucratic stones in the way of
> > those wanting to sort out this mess.
> 
> I think we all would be better served if there were more facts
> and fewer insults on this thread, can we do that please?
> 
> I do not think the extra work we need to do for ACPICA changes
> are a significant hurdle here. We will do what is best for Linux --
> which is what we though we were doing when we changed ACPICA
> so Linux could use native caching in the first place.
> 
> The only question that should be on the table here is how
> to make Linux be the best it can be.

If there is good reason to keep them around, I'm fine with that.
I think Pekka's suggestion of not doing unions but have better
typing in the code and then allocate the smaller types from
kmalloc sounds like a good idea.

If the individual kmem caches are here to stay, then the
kmem_cache_shrink call should go away. Either way we can delete
some code from slab.

The OS agnostic code that implements its own allocator is kind
of a hack -- I don't understand why you would turn on allocator
debugging and then circumvent it because you find it too slow.
But I will never maintain that so if it is compiled out for
Linux, then OK.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] acpi: do not use kmem caches

[Len Brown]

On Mon, 1 Dec 2008, Nick Piggin wrote:

> If there is good reason to keep them around, I'm fine with that.
> I think Pekka's suggestion of not doing unions but have better
> typing in the code and then allocate the smaller types from
> kmalloc sounds like a good idea.

Yes, I'll take that up with Bob when he comes back from break.
Maybe the ACPICA code can be improved here.

> If the individual kmem caches are here to stay, then the
> kmem_cache_shrink call should go away. Either way we can delete
> some code from slab.

I think they are here to stay.  We are running
an interpreter in kernel-space with arbitrary input,
so I think the ability to easily isolate run-time memory leaks
on a non-debug system is important.

You may hardly ever see the interpreter run on systems
with few run-time ACPI features, but it runs quite routinely
on many systems.

That said, we have not discovered a memory leak
in a very long time...


BTW.
I question that SLUB combining caches is a good idea.
It seems to fly in the face of how zone allocators
avoid fragmentation -- assuming that "like size"
equates to "like use".

But more important to me is that it reduces visibility.

> The OS agnostic code that implements its own allocator is kind
> of a hack -- I don't understand why you would turn on allocator
> debugging and then circumvent it because you find it too slow.
> But I will never maintain that so if it is compiled out for
> Linux, then OK.

The ACPI interpreter also builds into a user-space simulator
and a debugger.  It is extremely valuable for us to be able
to run the same code in the kernel and also in a user-space
test environment.  So there are a number of features in
the interpreter that we shut off when we build into the
Linux kernel.  Sometimes shutting them off is elegant,
sometime it is clumzy.

"Slabs can take a non-trivial amount of memory.
 On bigger machines it can be many megabytes."

I don't think this thread addressed this concern.
Is it something we should follow-up on?

thanks,
Len Brown, Intel Open Source Technology Center

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] acpi: do not use kmem caches

[Nick Piggin]

On Wed, Dec 31, 2008 at 05:04:22PM -0500, Len Brown wrote:
> On Mon, 1 Dec 2008, Nick Piggin wrote:
> 
> > If there is good reason to keep them around, I'm fine with that.
> > I think Pekka's suggestion of not doing unions but have better
> > typing in the code and then allocate the smaller types from
> > kmalloc sounds like a good idea.
> 
> Yes, I'll take that up with Bob when he comes back from break.
> Maybe the ACPICA code can be improved here.
> 
> > If the individual kmem caches are here to stay, then the
> > kmem_cache_shrink call should go away. Either way we can delete
> > some code from slab.
> 
> I think they are here to stay.  We are running
> an interpreter in kernel-space with arbitrary input,
> so I think the ability to easily isolate run-time memory leaks
> on a non-debug system is important.

I don't really see the connection. Or why being an interpreter is so
special. Filesystems, network stack, etc run in kernel with arbitrary
input. If kmem caches are part of a security strategy, then it's
broken... You'd surely have to detect bad input before the interpreter
turns it into a memory leak (or recover afterward, in which case it
isn't a leak).


> You may hardly ever see the interpreter run on systems
> with few run-time ACPI features, but it runs quite routinely
> on many systems.
> 
> That said, we have not discovered a memory leak
> in a very long time...
> 
> 
> BTW.
> I question that SLUB combining caches is a good idea.
> It seems to fly in the face of how zone allocators
> avoid fragmentation -- assuming that "like size"
> equates to "like use".
> 
> But more important to me is that it reduces visibility.

Yeah, that's another issue.


> > The OS agnostic code that implements its own allocator is kind
> > of a hack -- I don't understand why you would turn on allocator
> > debugging and then circumvent it because you find it too slow.
> > But I will never maintain that so if it is compiled out for
> > Linux, then OK.
> 
> The ACPI interpreter also builds into a user-space simulator
> and a debugger.  It is extremely valuable for us to be able
> to run the same code in the kernel and also in a user-space
> test environment.  So there are a number of features in
> the interpreter that we shut off when we build into the
> Linux kernel.  Sometimes shutting them off is elegant,
> sometime it is clumzy.
> 
> "Slabs can take a non-trivial amount of memory.
>  On bigger machines it can be many megabytes."
> 
> I don't think this thread addressed this concern.
> Is it something we should follow-up on?

There are some fundamental issues like per-cpu/node queues and
external fragmentation in slab pages that means a kmem_cache is
never going to be free (unless it is combined with another one,
but at that point you lose this tracking info anyway). SLAB has
bigger problems with data structures growing N^2 with the size
of the machine, but it is still the best choice in some situations.

Rather than rely on an arbitrary implementation of slab allocator
and tracking details, I would like to see your wrapper layer used
to collect exactly the details that are required for your acpi
work. You would then have all this available whether you ran in
userspace or on other OSes. Then investigate whether there would be
any performance or memory consumption regression introduced if you
move to kmalloc.

It's not a huge issue I guess, although if you split up object
types finely, you don't want to end up with a huge number of
kmem caches that are not frequently used. Splitting up objects that
way, together with tracking infrastructure, should result in even
better visibility than you have today too.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: [patch][rfc] acpi: do not use kmem caches

[Skywing]

-----Original Message-----
From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Nick Piggin
Sent: Sunday, January 04, 2009 11:15 PM
To: Len Brown
Cc: Christoph Hellwig; Alexey Starikovskiy; Pekka Enberg; Linux Memory Management List; linux-acpi@vger.kernel.org
Subject: Re: [patch][rfc] acpi: do not use kmem caches

> > I think they are here to stay.  We are running
> > an interpreter in kernel-space with arbitrary input,
> > so I think the ability to easily isolate run-time memory leaks
> > on a non-debug system is important.
> I don't really see the connection. Or why being an interpreter is so
> special. Filesystems, network stack, etc run in kernel with arbitrary
> input. If kmem caches are part of a security strategy, then it's
> broken... You'd surely have to detect bad input before the interpreter
> turns it into a memory leak (or recover afterward, in which case it
> isn't a leak).

I think that the purposes of these was to act as a debugging aid, for example, if there were BIOS-supplied AML that was triggering a leak.  The point being here that a network card driver has a much more well-defined set of what can happen than a fully pluggable interpreter for third party code.

[Of course, this is just my interpretation from following the discussion; I'm not otherwise involved.]

- S

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] acpi: do not use kmem caches

[Nick Piggin]

On Sun, Jan 04, 2009 at 11:43:55PM -0600, Skywing wrote:
> -----Original Message-----
> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Nick Piggin
> Sent: Sunday, January 04, 2009 11:15 PM
> To: Len Brown
> Cc: Christoph Hellwig; Alexey Starikovskiy; Pekka Enberg; Linux Memory Management List; linux-acpi@vger.kernel.org
> Subject: Re: [patch][rfc] acpi: do not use kmem caches
> 
> > > I think they are here to stay.  We are running
> > > an interpreter in kernel-space with arbitrary input,
> > > so I think the ability to easily isolate run-time memory leaks
> > > on a non-debug system is important.
> > I don't really see the connection. Or why being an interpreter is so
> > special. Filesystems, network stack, etc run in kernel with arbitrary
> > input. If kmem caches are part of a security strategy, then it's
> > broken... You'd surely have to detect bad input before the interpreter
> > turns it into a memory leak (or recover afterward, in which case it
> > isn't a leak).
> 
> I think that the purposes of these was to act as a debugging aid, for example, if there were BIOS-supplied AML that was triggering a leak.  The point being here that a network card driver has a much more well-defined set of what can happen than a fully pluggable interpreter for third party code.

It just seems like different shades to me, rather than some completely
different thing. A single network driver, maybe, but consider that untrusted
input influences a very large part of the entire network stack... Or a
filesystem.

Basically, if the data is really untrusted or likely to result in a leak,
then it should be detected and sanitized properly, rather than being allowed
to leak.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] acpi: do not use kmem caches

[Christoph Lameter]

On Mon, 1 Dec 2008, Pekka Enberg wrote:

> Why do you think Nick's patch is going to _increase_ memory consumption?
> SLUB _already_ merges the ACPI caches with kmalloc caches so you won't
> see any difference there. For SLAB, it's a gain because there's not
> enough activity going on which results in lots of unused space in the
> slabs (which is, btw, the reason SLUB does slab merging in the first
> place).

The patch is going to increase memory consumption because the use of
the kmalloc array means that the allocated object sizes are rounded up to
the next power of two.

I would recommend to keep the caches. Subsystem specific caches help to
simplify debugging and track the memory allocated for various purposes in
addition to saving the rounding up to power of two overhead.
And with SLUB the creation of such caches usually does not require
additional memory.

Maybe it would be best to avoid kmalloc as much as possible.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] acpi: do not use kmem caches

[Alexey Starikovskiy]

Christoph Lameter wrote:
> On Mon, 1 Dec 2008, Pekka Enberg wrote:
>
>   
>> Why do you think Nick's patch is going to _increase_ memory consumption?
>> SLUB _already_ merges the ACPI caches with kmalloc caches so you won't
>> see any difference there. For SLAB, it's a gain because there's not
>> enough activity going on which results in lots of unused space in the
>> slabs (which is, btw, the reason SLUB does slab merging in the first
>> place).
>>     
>
> The patch is going to increase memory consumption because the use of
> the kmalloc array means that the allocated object sizes are rounded up to
> the next power of two.
>
> I would recommend to keep the caches. Subsystem specific caches help to
> simplify debugging and track the memory allocated for various purposes in
> addition to saving the rounding up to power of two overhead.
> And with SLUB the creation of such caches usually does not require
> additional memory.
>
> Maybe it would be best to avoid kmalloc as much as possible.
>
>   
Christoph,
Thanks for support, these were my thoughts, when I changed ACPICA to use 
kmem_cache instead of
it's own on top of kmalloc 4 years ago...
Here are two acpi caches on my thinkpad z61m, IMHO any laptop will show 
similar numbers:

aystarik@thinkpad:~$ cat /proc/slabinfo | grep Acpi
Acpi-ParseExt       2856   2856     72   56    1 : tunables    0    0    
0 : slabdata     51     51      0
Acpi-Parse           170    170     48   85    1 : tunables    0    0    
0 : slabdata      2      2      0

Size of first will become 96 and size of second will be 64 if kmalloc is 
used, and we don't count ACPICA internal overhead.
Number of used blocks is not smallest in the list of slabs, actually it 
is among the highest.

Regards,
Alex.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] acpi: do not use kmem caches

[Nick Piggin]

On Mon, Dec 01, 2008 at 05:48:05PM +0300, Alexey Starikovskiy wrote:
> Christoph Lameter wrote:
> >On Mon, 1 Dec 2008, Pekka Enberg wrote:
> >
> >  
> >>Why do you think Nick's patch is going to _increase_ memory consumption?
> >>SLUB _already_ merges the ACPI caches with kmalloc caches so you won't
> >>see any difference there. For SLAB, it's a gain because there's not
> >>enough activity going on which results in lots of unused space in the
> >>slabs (which is, btw, the reason SLUB does slab merging in the first
> >>place).
> >>    
> >
> >The patch is going to increase memory consumption because the use of
> >the kmalloc array means that the allocated object sizes are rounded up to
> >the next power of two.
> >
> >I would recommend to keep the caches. Subsystem specific caches help to
> >simplify debugging and track the memory allocated for various purposes in
> >addition to saving the rounding up to power of two overhead.
> >And with SLUB the creation of such caches usually does not require
> >additional memory.
> >
> >Maybe it would be best to avoid kmalloc as much as possible.
> >
> >  
> Christoph,
> Thanks for support, these were my thoughts, when I changed ACPICA to use 
> kmem_cache instead of
> it's own on top of kmalloc 4 years ago...
> Here are two acpi caches on my thinkpad z61m, IMHO any laptop will show 
> similar numbers:
> 
> aystarik@thinkpad:~$ cat /proc/slabinfo | grep Acpi
> Acpi-ParseExt       2856   2856     72   56    1 : tunables    0    0    
> 0 : slabdata     51     51      0
> Acpi-Parse           170    170     48   85    1 : tunables    0    0    
> 0 : slabdata      2      2      0
> 
> Size of first will become 96 and size of second will be 64 if kmalloc is 
> used, and we don't count ACPICA internal overhead.
> Number of used blocks is not smallest in the list of slabs, actually it 
> is among the highest.

Hmm.
Acpi-Operand        2641   2773     64   59    1 : tunables  120   60    8 : slabdata     47     47     0
Acpi-ParseExt          0      0     64   59    1 : tunables  120   60    8 : slabdata      0      0     0
Acpi-Parse             0      0     40   92    1 : tunables  120   60    8 : slabdata      0      0     0
Acpi-State             0      0     80   48    1 : tunables  120   60    8 : slabdata      0      0     0
Acpi-Namespace      1711   1792     32  112    1 : tunables  120   60    8 : slabdata     16     16     0

Looks different for my thinkpad.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] acpi: do not use kmem caches

[Alexey Starikovskiy]

Nick Piggin wrote:
> On Mon, Dec 01, 2008 at 05:48:05PM +0300, Alexey Starikovskiy wrote:
>   
>> Christoph Lameter wrote:
>>     
>>> On Mon, 1 Dec 2008, Pekka Enberg wrote:
>>>
>>>  
>>>       
>>>> Why do you think Nick's patch is going to _increase_ memory consumption?
>>>> SLUB _already_ merges the ACPI caches with kmalloc caches so you won't
>>>> see any difference there. For SLAB, it's a gain because there's not
>>>> enough activity going on which results in lots of unused space in the
>>>> slabs (which is, btw, the reason SLUB does slab merging in the first
>>>> place).
>>>>    
>>>>         
>>> The patch is going to increase memory consumption because the use of
>>> the kmalloc array means that the allocated object sizes are rounded up to
>>> the next power of two.
>>>
>>> I would recommend to keep the caches. Subsystem specific caches help to
>>> simplify debugging and track the memory allocated for various purposes in
>>> addition to saving the rounding up to power of two overhead.
>>> And with SLUB the creation of such caches usually does not require
>>> additional memory.
>>>
>>> Maybe it would be best to avoid kmalloc as much as possible.
>>>
>>>  
>>>       
>> Christoph,
>> Thanks for support, these were my thoughts, when I changed ACPICA to use 
>> kmem_cache instead of
>> it's own on top of kmalloc 4 years ago...
>> Here are two acpi caches on my thinkpad z61m, IMHO any laptop will show 
>> similar numbers:
>>
>> aystarik@thinkpad:~$ cat /proc/slabinfo | grep Acpi
>> Acpi-ParseExt       2856   2856     72   56    1 : tunables    0    0    
>> 0 : slabdata     51     51      0
>> Acpi-Parse           170    170     48   85    1 : tunables    0    0    
>> 0 : slabdata      2      2      0
>>
>> Size of first will become 96 and size of second will be 64 if kmalloc is 
>> used, and we don't count ACPICA internal overhead.
>> Number of used blocks is not smallest in the list of slabs, actually it 
>> is among the highest.
>>     
>
> Hmm.
> Acpi-Operand        2641   2773     64   59    1 : tunables  120   60    8 : slabdata     47     47     0
> Acpi-ParseExt          0      0     64   59    1 : tunables  120   60    8 : slabdata      0      0     0
> Acpi-Parse             0      0     40   92    1 : tunables  120   60    8 : slabdata      0      0     0
> Acpi-State             0      0     80   48    1 : tunables  120   60    8 : slabdata      0      0     0
> Acpi-Namespace      1711   1792     32  112    1 : tunables  120   60    8 : slabdata     16     16     0
>
>   
> Looks different for my thinkpad.
>   
Probably this is SLUB vs. SLAB thing Pecca was talking about...
And, probably you run at 32-bit? This is part of my .config:
--------------------------------------------
CONFIG_SLUB_DEBUG=y
# CONFIG_SLAB is not set
CONFIG_SLUB=y
# CONFIG_SLOB is not set
-------------------------------------------

With your patch you would be able to save 64*(2773 - 2641) + 32 * 
(1792-1711)= 8448 + 2592 = 11040 bytes of memory, less than 3 pages?

2856 * (96-72) = 68544 bytes and 170 * (64-48) = 2720 bytes, so you will be wasting 5 times more memory in 64 bit case.



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] acpi: do not use kmem caches

[Nick Piggin]

On Mon, Dec 01, 2008 at 08:04:21PM +0300, Alexey Starikovskiy wrote:
> Nick Piggin wrote:
> >Hmm.
> >Acpi-Operand        2641   2773     64   59    1 : tunables  120   60    8 
> >: slabdata     47     47     0
> >Acpi-ParseExt          0      0     64   59    1 : tunables  120   60    8 
> >: slabdata      0      0     0
> >Acpi-Parse             0      0     40   92    1 : tunables  120   60    8 
> >: slabdata      0      0     0
> >Acpi-State             0      0     80   48    1 : tunables  120   60    8 
> >: slabdata      0      0     0
> >Acpi-Namespace      1711   1792     32  112    1 : tunables  120   60    8 
> >: slabdata     16     16     0
> >
> >  
> >Looks different for my thinkpad.
> >  
> Probably this is SLUB vs. SLAB thing Pecca was talking about...

Sizes should not be bigger with SLUB. Although if you have SLUB debugging
turned on then maybe the size gets padded with redzones, but in that
configuration you don't expect memory saving anyway because padding bloats
things up.


> And, probably you run at 32-bit? This is part of my .config:

No, 64 bit.


> --------------------------------------------
> CONFIG_SLUB_DEBUG=y
> # CONFIG_SLAB is not set
> CONFIG_SLUB=y
> # CONFIG_SLOB is not set
> -------------------------------------------
> 
> With your patch you would be able to save 64*(2773 - 2641) + 32 * 
> (1792-1711)= 8448 + 2592 = 11040 bytes of memory, less than 3 pages?

You don't account the cost of the kmem cache. Or fragmentation that
can be caused with extra kmem caches.  I guess neither is any problem
with SLOB, which is used by tiny systems...

 
> 2856 * (96-72) = 68544 bytes and 170 * (64-48) = 2720 bytes, so you will be 
> wasting 5 times more memory in 64 bit case.

With debugging on, in which case you're wasting memory anyway.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] acpi: do not use kmem caches

[Pekka Enberg]

On Mon, Dec 01, 2008 at 08:04:21PM +0300, Alexey Starikovskiy wrote:
>> Nick Piggin wrote:
>> >Hmm.
>> >Acpi-Operand        2641   2773     64   59    1 : tunables  120   60    8
>> >: slabdata     47     47     0
>> >Acpi-ParseExt          0      0     64   59    1 : tunables  120   60    8
>> >: slabdata      0      0     0
>> >Acpi-Parse             0      0     40   92    1 : tunables  120   60    8
>> >: slabdata      0      0     0
>> >Acpi-State             0      0     80   48    1 : tunables  120   60    8
>> >: slabdata      0      0     0
>> >Acpi-Namespace      1711   1792     32  112    1 : tunables  120   60    8
>> >: slabdata     16     16     0
>> >
>> >
>> >Looks different for my thinkpad.
>> >
>> Probably this is SLUB vs. SLAB thing Pecca was talking about...

On Mon, Dec 1, 2008 at 7:12 PM, Nick Piggin <npiggin@suse.de> wrote:
> Sizes should not be bigger with SLUB. Although if you have SLUB debugging
> turned on then maybe the size gets padded with redzones, but in that
> configuration you don't expect memory saving anyway because padding bloats
> things up.

Please keep in mind that SLUB slab merging kicks in and at least on
32-bit merges some of the caches with dentry caches and so forth. So
with SLUB, separate caches are probably OK. Unfortunately I don't have
any machines running with SLAB currently so I don't have any numbers.
But again, for SLAB, if there's not enough activity going on, you end
up with partially filled slabs which wastes memory.

Though I suspect using kmem caches to combat the internal
fragmentation caused by kmalloc() rounding is not worth it in this
case.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] acpi: do not use kmem caches

[Pekka Enberg]

On Mon, Dec 1, 2008 at 7:25 PM, Pekka Enberg <penberg@cs.helsinki.fi> wrote:
> Though I suspect using kmem caches to combat the internal
> fragmentation caused by kmalloc() rounding is not worth it in this
> case.

Btw, just for the record, the ACPI objects are indeed a bad fit for
kmalloc() as reported by SLUB statistics:

[ The size of ACPI kmem caches with wasted bytes per object in parenthesis. ]

                 32-bit size  64-bit size
  Acpi-Namespace  24 (8)       32 (0)
  Acpi-Operand    40 (24)      72 (24)
  Acpi-Parse      32 (0)       48 (16)
  Acpi-ParseExt   44 (20)      72 (24)
  Acpi-State      44 (20)      80 (16)

Though I suspect this situation could be improved by avoiding those
fairly big unions ACPI does (like union acpi_operand_object).

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] acpi: do not use kmem caches

[Alexey Starikovskiy]

Pekka Enberg wrote:
> On Mon, Dec 1, 2008 at 7:25 PM, Pekka Enberg <penberg@cs.helsinki.fi> wrote:
>   
>> Though I suspect using kmem caches to combat the internal
>> fragmentation caused by kmalloc() rounding is not worth it in this
>> case.
>>     
>
> Btw, just for the record, the ACPI objects are indeed a bad fit for
> kmalloc() as reported by SLUB statistics:
>
> [ The size of ACPI kmem caches with wasted bytes per object in parenthesis. ]
>
>                  32-bit size  64-bit size
>   Acpi-Namespace  24 (8)       32 (0)
>   Acpi-Operand    40 (24)      72 (24)
>   Acpi-Parse      32 (0)       48 (16)
>   Acpi-ParseExt   44 (20)      72 (24)
>   Acpi-State      44 (20)      80 (16)
>
> Though I suspect this situation could be improved by avoiding those
> fairly big unions ACPI does (like union acpi_operand_object).
>   
No, last time I checked, operand may get down to 16 bytes in 32-bit case 
-- save byte by having 3 types of operands... and making 2 more caches :)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] acpi: do not use kmem caches

[Pekka Enberg]

On Mon, Dec 1, 2008 at 7:36 PM, Alexey Starikovskiy <aystarik@gmail.com> wrote:
>> [ The size of ACPI kmem caches with wasted bytes per object in
>> parenthesis. ]
>>
>>                 32-bit size  64-bit size
>>  Acpi-Namespace  24 (8)       32 (0)
>>  Acpi-Operand    40 (24)      72 (24)
>>  Acpi-Parse      32 (0)       48 (16)
>>  Acpi-ParseExt   44 (20)      72 (24)
>>  Acpi-State      44 (20)      80 (16)
>>
>> Though I suspect this situation could be improved by avoiding those
>> fairly big unions ACPI does (like union acpi_operand_object).
>
> No, last time I checked, operand may get down to 16 bytes in 32-bit case --
> save byte by having 3 types of operands... and making 2 more caches :)

I'm not sure what you mean. I wasn't suggesting adding new caches but
instead, avoid big unions and allocate plain structs with kmalloc()
instead. If you look at union acpi_operand_object, for example, it's
such a bad fit on 64-bit (72 bytes) only because of struct
acpi_object_mutex. Other structs in that union fit in a kmalloc-64
cache just fine.

So really, ACPI should probably be fixing the unions rather than paper
over the problem by adding new kmem caches.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] acpi: do not use kmem caches

[Christoph Lameter]

On Mon, 1 Dec 2008, Alexey Starikovskiy wrote:

> > Though I suspect this situation could be improved by avoiding those
> > fairly big unions ACPI does (like union acpi_operand_object).
> >
> No, last time I checked, operand may get down to 16 bytes in 32-bit case --
> save byte by having 3 types of operands... and making 2 more caches :)

SLAB has a minimum allocation size of 32 bytes so it would not make a
difference there.

SLUB can go down to 8 bytes which would enable you to save more. Adding
new caches most of the time simply lead to incrementing a counter in
a similar kmem_cache structure.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] acpi: do not use kmem caches

[Alexey Starikovskiy]

Nick Piggin wrote:
> On Mon, Dec 01, 2008 at 08:04:21PM +0300, Alexey Starikovskiy wrote:
>   
>> Nick Piggin wrote:
>>     
>>> Hmm.
>>> Acpi-Operand        2641   2773     64   59    1 : tunables  120   60    8 
>>> : slabdata     47     47     0
>>> Acpi-ParseExt          0      0     64   59    1 : tunables  120   60    8 
>>> : slabdata      0      0     0
>>> Acpi-Parse             0      0     40   92    1 : tunables  120   60    8 
>>> : slabdata      0      0     0
>>> Acpi-State             0      0     80   48    1 : tunables  120   60    8 
>>> : slabdata      0      0     0
>>> Acpi-Namespace      1711   1792     32  112    1 : tunables  120   60    8 
>>> : slabdata     16     16     0
>>>
>>>  
>>> Looks different for my thinkpad.
>>>  
>>>       
>> Probably this is SLUB vs. SLAB thing Pecca was talking about...
>>     
>
> Sizes should not be bigger with SLUB. Although if you have SLUB debugging
> turned on then maybe the size gets padded with redzones, but in that
> configuration you don't expect memory saving anyway because padding bloats
> things up.
>
>
>   
>> And, probably you run at 32-bit? This is part of my .config:
>>     
>
> No, 64 bit.
>
>
>   
>> --------------------------------------------
>> CONFIG_SLUB_DEBUG=y
>> # CONFIG_SLAB is not set
>> CONFIG_SLUB=y
>> # CONFIG_SLOB is not set
>> -------------------------------------------
>>
>> With your patch you would be able to save 64*(2773 - 2641) + 32 * 
>> (1792-1711)= 8448 + 2592 = 11040 bytes of memory, less than 3 pages?
>>     
>
> You don't account the cost of the kmem cache. Or fragmentation that
> can be caused with extra kmem caches.  I guess neither is any problem
> with SLOB, which is used by tiny systems...
>   
There is very small amount of fragmentation -- underfill, maybe 3 pages 
out of 63, less than 5%.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] acpi: do not use kmem caches

[Len Brown]

> What does everyone think about this patch?

Unexpected, Interesting.

We cut over to the native Linux allocator cache
from the ACPICA cache at a time when we had some
memory leaks, and it was important to be able
to walk up to a machine in the field that didn't
have any special build options and look in /proc
to find out what the different parts of our
sub-system were allocating.

I don't know the merits of SLAB vs. SLUB
or why Linux has two.  My local configs use SLAB
but I notice that recent Fedora kernels us SLUB.

>From an observability point of view, I guess I like SLAB
better because I can still see the 5 different ACPI caches
in /proc/slabinfo, while with SLUB I can see only one or two.

Note that these caches are used to interpret AML,
and how much AML you interpret depends a lot on the machine.
Some laptops will interpret AML all day long, while some
desktops and servers will run AML only at boot-time.

I guess my opinion is that I like the observatiblity we have now,
and that I'd need to see measurements showing that we're paying
too much for that observability.  I've also just now formed
an initial opinion on SLAB vs SLUB where I had none before.

thanks,
-Len

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>