Re: [PATCH] cpumask: fix lg_lock/br_lock.

[Ingo Molnar <mingo@elte.hu>]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> From: Andi Kleen <ak@linux.intel.com>
> Subject: brlocks/lglocks: cleanups

This patch, while I agree with the goal of de-CPP-ing the 
lglocks code, violates my sense of taste and logic in numerous 
ways. If we touch this code it should be done right.

This patch should also probably go upstream through the 
locking/lockdep tree? Mind sending it us once you think it's 
ready?

> 
>  fs/dcache.c            |    4 
>  fs/file_table.c        |   17 +--
>  fs/internal.h          |    3 
>  fs/namei.c             |   24 ++--
>  fs/namespace.c         |  140 ++++++++++++++--------------
>  fs/pnode.c             |    4 
>  fs/proc_namespace.c    |    4 
>  include/linux/lglock.h |  189 +++++++--------------------------------
>  kernel/Makefile        |    2 
>  kernel/lglock.c        |  136 ++++++++++++++++++++++++++++
>  10 files changed, 269 insertions(+), 254 deletions(-)

It's absolutely crazy to do such a large patch that both 
uninlines the code and changes the API.

There is absolutely no good reason to do it like that - and the 
resulting merge pain on Andrew is a direct result of that: had 
it been kept separate it would be much easier and safer to 
update just the API change patch ...

Do those things in at least two patches, making the bigger API 
change patch plain and *OBVIOUS*.

> +struct lglock {
> +	arch_spinlock_t __percpu *lock;
> +	cpumask_t cpus;	/* XXX need to put on separate cacheline? */
> +	spinlock_t cpu_lock;
> +	struct notifier_block cpu_notifier;
> +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> +	struct lock_class_key lock_key;
> +	struct lockdep_map    lock_dep_map;
> +#endif
> +};

This is not a readable structure definition. Plenty of readable 
ones exist in the tree, they should be inspected for clues.

> +/* Only valid for statics */

what does this comment mean?

> +void lg_lock_init(struct lglock *lg, char *name);
> +void lg_local_lock(struct lglock *lg);
> +void lg_local_unlock(struct lglock *lg);
> +void lg_local_lock_cpu(struct lglock *lg, int cpu);
> +void lg_local_unlock_cpu(struct lglock *lg, int cpu);
> +void lg_global_lock_online(struct lglock *lg);
> +void lg_global_unlock_online(struct lglock *lg);
> +void lg_global_lock(struct lglock *lg);
> +void lg_global_unlock(struct lglock *lg);
> +
> +int lg_cpu_callback(struct notifier_block *nb, unsigned long action, void *hcpu);

These APIs lack documentation.

> +/* Note there is no uninit, so lglocks cannot be defined in
> + * modules (but it's fine to use them from there)
> + * Could be added though, just undo lg_lock_init
> + */

I'm sure Andi knows what the standard shape of multi-line 
comments is in the kernel? Why has he still not learned?

> +void lg_local_lock(struct lglock *lg)
> +{
> +	arch_spinlock_t *lock;
> +	preempt_disable();

I'm sure Andi knows what's wrong here, he has been reviewed 
dozens of times for similar mistakes in the x86 trees.

> +void lg_local_unlock(struct lglock *lg)
> +{
> +	arch_spinlock_t *lock;
> +	rwlock_release(&lg->lock_dep_map, 1, _RET_IP_);

ditto.

> +void lg_local_lock_cpu(struct lglock *lg, int cpu)
> +{
> +	arch_spinlock_t *lock;
> +	preempt_disable();

ditto.

> +	arch_spinlock_t *lock;
> +	rwlock_release(&lg->lock_dep_map, 1, _RET_IP_);

ditto.

> +{
> +	int i;
> +	spin_lock(&lg->cpu_lock);

ditto.

> +{
> +	int i;
> +	rwlock_release(&lg->lock_dep_map, 1, _RET_IP_);

ditto.

> +{
> +	int i;
> +	preempt_disable();

ditto.

> +{
> +	int i;
> +	rwlock_release(&lg->lock_dep_map, 1, _RET_IP_);

ditto.

> +int lg_cpu_callback(struct notifier_block *nb,
> +                              unsigned long action, void *hcpu)

ugly linebreak, we can do better.


> +{
> +	struct lglock *lglock = container_of(nb, struct lglock, cpu_notifier);
> +	switch (action & ~CPU_TASKS_FROZEN) {

sigh.

Until these defects (and probably more, haven't done a full 
check) are fixed:

Nacked-by: Ingo Molnar <mingo@elte.hu>

Thanks,

	Ingo