Re: [PATCH 04/11] Allow pooled nodes to be recycled back onto a free list

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 9, 2007, 22:27 UTC
Message-ID: <7v1wazrr50.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20071109110619.GD19368@spearce.org>

"Shawn O. Pearce" <spearce@spearce.org> writes:

Show 11 quoted lines

> In some cases and for some node types a caller may have used one of
> our allocated nodes for some temporary usage and then wants to return
> it back to the available free list.  We now define a function that
> will thread the object onto the front of a free list, which will be
> used only after the current block has been exhausted.
>
> We hold off on looking at the free list until we are sure the current
> block is empty.  This saves about 1000 tests of the (usually empty)
> free list per block.
>
> Signed-off-by: Shawn O. Pearce <spearce@spearce.org>

Show 8 quoted lines

> +void free_##name##_node(void *n)				\
> +{								\
> +	*((void**)n) = name##_pool.free_list;			\
> +	name##_pool.free_list = n;				\
> +}								\
>  static void report_##name(void)					\
>  {								\
>  	report(#name, &name##_pool, sizeof(t));			\

I wonder how well this will interact with object scrubbing. A freed node is still in the array in the block bucket and the scrubber walks the array without skipping any freed node, but the threading pointer already scribbled on the first sizeof(void**) bytes on the object memory. Worse, the caller may say "Hey, I am freeing this node, so I'll bzero() it before returning it to the pool", without realizing that the scrubber may still act on that freed node buffer.

The rules for a "class designer" on freeing a temporary node with this implementation as I understand it is:

 - do assume free_all will call scrubber on freed nodes;

 - do not have anything your scrubber needs to access near the
   beginning of the nodes managed by alloc.c API;

 - do not clobber anything scrubber needs to access when freeing
   a node.

For struct object and its descendant classes, I think this is a non issue -- the first bytes in the structure are the flags and sha1[] and the current implementation of the scrubbers would not look at these fields.

But it does look like a risk in the future.