slab quirks in DEBUG, ctor, and initialization

slab quirks in DEBUG, ctor, and initialization

[John Reiser]

In mm/slab.c, the DEBUG variant of cache_alloc_debugcheck_after
might call  cachep->ctor(objp, cachep, 0);  but the non-DEBUG
variant does absolutely nothing.  idr_pre_get is a routine
which notices the difference.

Even when cache_alloc_debugcheck_after does invoke the ctor,
then it is conditional upon  cachep->flags & SLAB_POISON .  This
assumes that the only two states are poisoned and all-zero
(from .bss static, or via a cleared new page frame.)
So if SLAB_POISON is not specified, then a ctor which
does anything other than memset(,0,) is out of luck.
Instead: if a ctor is specified then it should be called
for each successful allocation.

Invoking the ctor from within cache_alloc_debugcheck_after makes it
awkward for a dynamic checker of allocations and  initializations.
Valgrind would be happier if allocation and initialization were
invoked at the same subroutine nesting level, such as in __cache_alloc:

        cache_alloc_debugcheck_before(cachep, flags);
        local_irq_save(save_flags);
        objp = __do_cache_alloc(cachep, flags);
	/* checker marks the space as allocated */
        local_irq_restore(save_flags);
        objp = cache_alloc_debugcheck_after(cachep, flags, objp, caller);
	if (objp && cachep->ctor)
		cachep->ctor(objp, cachep, 0);
	/* checker notes initializations during ctor [above] */

-- 
John Reiser, jreiser@BitWagon.com

Re: slab quirks in DEBUG, ctor, and initialization

[Pekka Enberg]

Hi John,

On Dec 17, 2007 5:47 PM, John Reiser <jreiser@bitwagon.com> wrote:
> In mm/slab.c, the DEBUG variant of cache_alloc_debugcheck_after
> might call  cachep->ctor(objp, cachep, 0);  but the non-DEBUG
> variant does absolutely nothing.  idr_pre_get is a routine
> which notices the difference.

How does ipr_pre_get notice this?

On Dec 17, 2007 5:47 PM, John Reiser <jreiser@bitwagon.com> wrote:
> Even when cache_alloc_debugcheck_after does invoke the ctor,
> then it is conditional upon  cachep->flags & SLAB_POISON.  This
> assumes that the only two states are poisoned and all-zero
> (from .bss static, or via a cleared new page frame.)
> So if SLAB_POISON is not specified, then a ctor which
> does anything other than memset(,0,) is out of luck.
> Instead: if a ctor is specified then it should be called
> for each successful allocation.

Sorry, I don't understand at all what's the problem is here. For the
common (non-poison) case, we initialize all objects *once* whenever a
cache is grown (see cache_grow calling cache_init_objs) which is the
whole point of having constructors. When poisoning is enabled, we
obviously cannot do this which is why we call the constructor for
every allocation.

                                Pekka

Re: slab quirks in DEBUG, ctor, and initialization

[John Reiser]

Hi Pekka,

>>In mm/slab.c, the DEBUG variant of cache_alloc_debugcheck_after
>>might call  cachep->ctor(objp, cachep, 0);  but the non-DEBUG
>>variant does absolutely nothing.  idr_pre_get is a routine
>>which notices the difference.
> 
> 
> How does ipr_pre_get notice this?

idr_pre_get calls kmem_cache_alloc, which constructs 'struct idr_layer'
via the cachep->ctor() call from cache_alloc_debugcheck_after to
idr_cache_ctor, and not via cache_init_objs.  So if DEBUG is off,
then idr_cache_ctor does not get its chance to call memset,
which could leave the struct idr_layer potentially undefined.

>>Even when cache_alloc_debugcheck_after does invoke the ctor,
>>then it is conditional upon  cachep->flags & SLAB_POISON.  This
>>assumes that the only two states are poisoned and all-zero
>>(from .bss static, or via a cleared new page frame.)
>>So if SLAB_POISON is not specified, then a ctor which
>>does anything other than memset(,0,) is out of luck.
>>Instead: if a ctor is specified then it should be called
>>for each successful allocation.
> 
> 
> Sorry, I don't understand at all what's the problem is here. For the
> common (non-poison) case, we initialize all objects *once* whenever a
> cache is grown (see cache_grow calling cache_init_objs) which is the
> whole point of having constructors. When poisoning is enabled, we
> obviously cannot do this which is why we call the constructor for
> every allocation.

There is a comment near the beginning of mm/slab.c:
 * This means, that your constructor is used only for newly allocated
 * slabs and you must pass objects with the same initializations to
 * kmem_cache_free.
Note that the comment should be updated to account for the constructor
also being called if SLAB_POISON is specified.  A significant implication
is that calling the constructor always establishes good state, even
though the call might not be necessary, and although it might take
more time than not calling it.

In order to avoid the allocator having to call the constructor, then the
caller of kmem_cache_free must call the constructor just before freeing,
or establish the same values.  In contrast to calling the constructor
every time, the existing convention increases complexity and risk of bugs,
in exchange for faster performance when actions equivalent to the ctor
are performed in the dtor instead, or when state that is equivalent to
the ctor occurs as a natural byproduct of being done with the object.

The real gripe is that the current code makes it cumbersome for a
dynamic checking program such as valgrind(memcheck) to record correctly
the state of the object.  Is the object initialized (and which parts
of it, and were the initializations performed into allocated space),
or is it merely allocated and uninitialized?

-- 
John Reiser, jreiser@BitWagon.com

Re: slab quirks in DEBUG, ctor, and initialization

[Pekka J Enberg]

Hi John,

On Mon, 17 Dec 2007, John Reiser wrote:
> idr_pre_get calls kmem_cache_alloc, which constructs 'struct idr_layer'
> via the cachep->ctor() call from cache_alloc_debugcheck_after to
> idr_cache_ctor, and not via cache_init_objs.  So if DEBUG is off,
> then idr_cache_ctor does not get its chance to call memset,
> which could leave the struct idr_layer potentially undefined.

No, init_cache_objs() will call the constructor, if the cache has one when 
DEBUG is not set so the struct idr_layer can never be undefined. However, 
struct idr_layer can contain non-zero elements if someone does a 
kmem_cache_free() on an object that hasn't been zeroed. If that matters 
here, idr_pre_get should call kmem_cache_zalloc() and drop the 
constructor.

				Pekka