Re: [PATCH 7/9] mm: split page table lock

[Andrew Morton <akpm@osdl.org>]

Hugh Dickins <hugh@veritas.com> wrote:
>
> In this implementation, the spinlock is tucked inside the struct page of
>  the page table page: with a BUILD_BUG_ON in case it overflows - which it
>  would in the case of 32-bit PA-RISC with spinlock debugging enabled.

eh?   It's going to overflow an unsigned long on x86 too:

typedef struct {
	raw_spinlock_t raw_lock;
#if defined(CONFIG_PREEMPT) && defined(CONFIG_SMP)
	unsigned int break_lock;
#endif
#ifdef CONFIG_DEBUG_SPINLOCK
	unsigned int magic, owner_cpu;
	void *owner;
#endif
} spinlock_t;

I think we need a union here.

> +#define __pte_lockptr(page)	((spinlock_t *)&((page)->private))
> +#define pte_lock_init(_page)	do {					\
> +	BUILD_BUG_ON((size_t)(__pte_lockptr((struct page *)0) + 1) >	\
> +						sizeof(struct page));	\

The above assumes that page.private is the final field in struct page. 
That's fragile.

>  Splitting the lock is not quite for free: another cacheline access.
>  Ideally, I suppose we would use split ptlock only for multi-threaded
>  processes on multi-cpu machines; but deciding that dynamically would
>  have its own costs.  So for now enable it by config, at some number
>  of cpus - since the Kconfig language doesn't support inequalities, let
>  preprocessor compare that with NR_CPUS.  But I don't think it's worth
>  being user-configurable: for good testing of both split and unsplit
>  configs, split now at 4 cpus, and perhaps change that to 8 later.

I'll make it >= 2 for -mm.

> +#define __pte_lockptr(page)	((spinlock_t *)&((page)->private))
>  +#define pte_lock_init(_page)	do {					\
> +	BUILD_BUG_ON((size_t)(__pte_lockptr((struct page *)0) + 1) >	\
> +						sizeof(struct page));	\
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/