* Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> From: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> 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@xxxxxxx>
Thanks,
Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
