Re: [patch 01/28] cpu alloc: The allocator

[Peter Zijlstra <peterz@infradead.org>]

On Tue, 2007-11-06 at 11:51 -0800, Christoph Lameter wrote:

> +/*
> + * Lock to protect the bitmap and the meta data for the cpu allocator.
> + */
> +static DEFINE_SPINLOCK(cpu_alloc_map_lock);

I thought you got nightmares from global locks :-)

> +/*
> + * Allocate an object of a certain size
> + *
> + * Returns a special pointer that can be used with CPU_PTR to find the
> + * address of the object for a certain cpu.
> + */
> +void *cpu_alloc(unsigned long size, gfp_t gfpflags, unsigned long align)
> +{
> +	unsigned long start;
> +	int units = size_to_units(size);
> +	void *ptr;
> +	int first;
> +	unsigned long map_size;
> +
> +	BUG_ON(gfpflags & ~(GFP_RECLAIM_MASK | __GFP_ZERO));
> +
> +	spin_lock(&cpu_alloc_map_lock);
> +
> +restart:
> +	map_size = PAGE_SIZE << cpu_alloc_map_order;
> +	first = 1;
> +	start = first_free;
> +
> +	for ( ; ; ) {
> +
> +		start = find_next_zero_bit(cpu_alloc_map, map_size, start);
> +		if (first)
> +			first_free = start;
> +
> +		if (start >= units_total) {
> +			if (expand_cpu_area(gfpflags))
> +				goto out_of_memory;
> +			goto restart;
> +		}
> +
> +		/*
> +		 * Check alignment and that there is enough space after
> +		 * the starting unit.
> +		 */
> +		if (start % (align / UNIT_SIZE) == 0 &&
> +			find_next_bit(cpu_alloc_map, map_size, start + 1)
> +							>= start + units)
> +				break;
> +		start++;
> +		first = 0;
> +	}
> +
> +	if (first)
> +		first_free = start + units;
> +
> +	while (start + units > units_total) {
> +		if (expand_cpu_area(gfpflags))
> +			goto out_of_memory;
> +	}
> +
> +	set_map(start, units);
> +	units_free -= units;
> +	__count_vm_events(CPU_BYTES, units * UNIT_SIZE);
> +
> +	spin_unlock(&cpu_alloc_map_lock);
> +
> +	ptr = cpu_area + start * UNIT_SIZE;
> +
> +	if (gfpflags & __GFP_ZERO) {
> +		int cpu;
> +
> +		for_each_possible_cpu(cpu)
> +			memset(CPU_PTR(ptr, cpu), 0, size);
> +	}
> +
> +	return ptr;
> +
> +out_of_memory:
> +	spin_unlock(&cpu_alloc_map_lock);
> +	return NULL;
> +}
> +EXPORT_SYMBOL(cpu_alloc);
> +
> +/*
> + * Free an object. The pointer must be a cpu pointer allocated
> + * via cpu_alloc.
> + */
> +void cpu_free(void *start, unsigned long size)
> +{
> +	int units = size_to_units(size);
> +	int index;
> +	u8 *p = start;
> +
> +	BUG_ON(p < cpu_area);
> +	index = (p - cpu_area) / UNIT_SIZE;
> +	BUG_ON(!test_bit(index, cpu_alloc_map) ||
> +			index >= units_total);
> +
> +	spin_lock(&cpu_alloc_map_lock);
> +
> +	clear_map(index, units);
> +	units_free += units;
> +	__count_vm_events(CPU_BYTES, -units * UNIT_SIZE);
> +	if (index < first_free)
> +		first_free = index;
> +
> +	spin_unlock(&cpu_alloc_map_lock);
> +}
> +EXPORT_SYMBOL(cpu_free);

Why a bitmap allocator and not a heap allocator?

Also, looking at the lock usage, this thing is not IRQ safe, so it
should not be called from hardirq context. Please document this.

> +#ifndef _LINUX_CPU_ALLOC_H_
> +#define _LINUX_CPU_ALLOC_H_
> +
> +#define CPU_OFFSET(__cpu) \
> +	((unsigned long)(__cpu) << (CONFIG_CPU_AREA_ORDER + PAGE_SHIFT))
> +
> +#define CPU_PTR(__p, __cpu) ((__typeof__(__p))((void *)(__p) + \
> +							CPU_OFFSET(__cpu)))
> +
> +#define CPU_ALLOC(type, flags)	cpu_alloc(sizeof(type), flags, \
> +					__alignof__(type))
> +#define CPU_FREE(pointer)	cpu_free(pointer, sizeof(*(pointer)))
> +
> +#define THIS_CPU(__p)	CPU_PTR(__p, smp_processor_id())
> +#define __THIS_CPU(__p)	CPU_PTR(__p, raw_smp_processor_id())
> +
> +/*
> + * Raw calls
> + */
> +void *cpu_alloc(unsigned long size, gfp_t gfp, unsigned long align);
> +void cpu_free(void *cpu_pointer, unsigned long size);
> +
> +#endif /* _LINUX_CPU_ALLOC_H_ */

Like said in the previous mail (which due to creative mailing from your
end never made it out to the lists), I dislike those shouting macros.
Please lowercase them.