cpumask: fix lg_lock/br_lock.
Subsystems (4)
PATCH 1/1 cpumask: fix lg_lock/br_lock.
Use a cpumask_var_t instead of cpumask_t. We're doing plenty of allocations here anyway, so it's not really an issue, and it sets a good example.
(cpumask_t is obsolescent, as are the cpus_* functions).
--- fs/file_table.c | 2 +- fs/namespace.c | 4 +++- include/linux/lglock.h | 23 +++++++++++++---------- 3 files changed, 17 insertions(+), 12 deletions(-)
fs/file_table.c +1 -1
@@ -526,6 +526,6 @@ void __init files_init(unsigned long mem n = (mempages * (PAGE_SIZE /1024 )) /10 ; files_stat.max_files = max_t(unsigned long, n, NR_FILE);files_defer_init (); -lg_lock_init (files_lglock); +lg_lock_init (files_lglock,GFP_KERNEL );percpu_counter_init (&nr_files,0 ); }
fs/namespace.c +3 -1
@@ -2576,7 +2576,9 @@ void __init mnt_init(void)for (u =0 ; u <HASH_SIZE ; u++)INIT_LIST_HEAD (&mount_hashtable[u]); -br_lock_init (vfsmount_lock); + err =br_lock_init (vfsmount_lock,GFP_KERNEL ); +if (err) +panic ("Failed to init vfsmount_lock: %d\n" , err); err =sysfs_init ();if (err)
include/linux/lglock.h +13 -10
@@ -25,7 +25,7 @@#include <linux/cpu.h> /* can make br locks by using local lock for read side, global lock for write */ -#define br_lock_init (name) name##_lock_init() +#define br_lock_init (name, gfp) name##_lock_init(gfp)#define br_read_lock (name) name##_local_lock()#define br_read_unlock (name) name##_local_unlock()#define br_write_lock (name) name##_global_lock_online()
@@ -35,7 +35,7 @@#define DEFINE_BRLOCK (name) DEFINE_LGLOCK(name) -#define lg_lock_init (name) name##_lock_init() +#define lg_lock_init (name, gfp) name##_lock_init(gfp)#define lg_local_lock (name) name##_local_lock()#define lg_local_unlock (name) name##_local_unlock()#define lg_local_lock_cpu (name, cpu) name##_local_lock_cpu(cpu)
@@ -61,7 +61,7 @@#define DECLARE_LGLOCK (name) \ - extern void name##_lock_init(void); \ + extern int name##_lock_init(gfp_t gfp); \ extern void name##_local_lock(void); \ extern void name##_local_unlock(void); \ extern void name##_local_lock_cpu(int cpu); \
@@ -74,7 +74,7 @@#define DEFINE_LGLOCK (name) \ \ DEFINE_SPINLOCK(name##_cpu_lock); \ - cpumask_t name##_cpus __read_mostly; \ + cpumask_var_t name##_cpus __read_mostly; \ DEFINE_PER_CPU(arch_spinlock_t, name##_lock); \ DEFINE_LGLOCK_LOCKDEP(name); \ \
@@ -85,12 +85,12 @@switch (action & ~CPU_TASKS_FROZEN ) { \case CPU_UP_PREPARE : \ spin_lock(&name##_cpu_lock); \ - cpu_set((unsigned long)hcpu, name##_cpus); \ + cpumask_set_cpu((unsigned long)hcpu, name##_cpus); \ spin_unlock(&name##_cpu_lock); \break ; \case CPU_UP_CANCELED :case CPU_DEAD : \ spin_lock(&name##_cpu_lock); \ - cpu_clear((unsigned long)hcpu, name##_cpus); \ + cpumask_clear_cpu((unsigned long)hcpu, name##_cpus); \ spin_unlock(&name##_cpu_lock); \ } \return NOTIFY_OK ; \
@@ -98,7 +98,7 @@ static struct notifier_block name##_lg_cpu_notifier = { \ .notifier_call = name##_lg_cpu_callback, \ }; \ - void name##_lock_init(void) { \ + int name##_lock_init(gfp_t gfp) { \int i; \ LOCKDEP_INIT_MAP(&name##_lock_dep_map, #name, &name##_lock_key, 0); \for_each_possible_cpu (i) { \
@@ -107,10 +107,13 @@ *lock = (arch_spinlock_t )__ARCH_SPIN_LOCK_UNLOCKED; \ } \ register_hotcpu_notifier(&name##_lg_cpu_notifier); \ + if (!alloc_cpumask_var(&name##_cpus, gfp)) \ +return -ENOMEM ; \get_online_cpus (); \for_each_online_cpu (i) \ - cpu_set(i, name##_cpus); \ + cpumask_set_cpu(i, name##_cpus); \put_online_cpus (); \ +return 0 ; \ } \ EXPORT_SYMBOL(name##_lock_init); \ \
@@ -154,7 +157,7 @@int i; \ spin_lock(&name##_cpu_lock); \ rwlock_acquire(&name##_lock_dep_map, 0, 0, _RET_IP_); \ - for_each_cpu(i, &name##_cpus) { \ + for_each_cpu(i, name##_cpus) { \arch_spinlock_t *lock; \ lock = &per_cpu(name##_lock, i); \arch_spin_lock (lock); \
@@ -165,7 +168,7 @@ void name##_global_unlock_online(void) { \int i; \ rwlock_release(&name##_lock_dep_map, 1, _RET_IP_); \ - for_each_cpu(i, &name##_cpus) { \ + for_each_cpu(i, name##_cpus) { \arch_spinlock_t *lock; \ lock = &per_cpu(name##_lock, i); \arch_spin_unlock (lock); \
Quoted 5 lines · On Tue, 28 Feb 2012 09:52:30 +1030 Rusty Russell <[email protected]> wrote:
Use a
cpumask_var_tinstead ofcpumask_t. We're doing plenty of allocations here anyway, so it's not really an issue, and it sets a good example.(
cpumask_tis obsolescent, as are the cpus_* functions).
Congratulations to yourself and Andi:
patching file fs/file_table.c
Hunk #1 FAILED at 526.
1 out of 1 hunk FAILED -- saving rejects to file fs/file_table.c.rej patching file fs/namespace.c
Hunk #1 FAILED at 2576.
1 out of 1 hunk FAILED -- saving rejects to file fs/namespace.c.rej patching file include/linux/lglock.h
Hunk #1 FAILED at 25.
Hunk #2 FAILED at 35.
Hunk #3 FAILED at 61.
Hunk #4 FAILED at 74.
Hunk #5 FAILED at 85.
Hunk #6 FAILED at 98.
Hunk #7 FAILED at 107.
Hunk #8 FAILED at 154.
Hunk #9 FAILED at 165.
9 out of 9 hunks FAILED -- saving rejects to file include/linux/lglock.h.rej Failed to apply cpumask-fix-lg_lock-br_lock
a clean sweep!
It's due to brlocks-lglocks-cleanups.patch, which I didn't think to cc you on. Thoughts?
From: Andi Kleen <[email protected]>
Subject: brlocks/lglocks: cleanups
lglocks and brlocks are currently generated with some complicated macros in lglock.h. But there's no reason to not just use common utility functions and put all the data into a common data structure.
Since there are at least two users it makes sense to share this code in a library. This is also easier maintainable than a macro forest.
This will also make it later possible to dynamically allocate lglocks and also use them in modules (this would both still need some additional, but now straightforward, code)
In general the users now look more like normal function calls with pointers, not magic macros.
The patch is rather large because I move over all users in one go to keep it bisectable. This impacts the VFS somewhat in terms of lines changed. But no actual behaviour change.
---
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(-)
fs/dcache.c renamed from fs/dcache.c~brlocks-lglocks-cleanups +2 -2
@@ -2442,7 +2442,7 @@ static int prepend_path(const struct patbool slash = false;int error =0 ; -br_read_lock (vfsmount_lock); +br_read_lock (&vfsmount_lock);while (dentry != root->dentry || vfsmnt != root->mnt) {struct dentry * parent;
@@ -2473,7 +2473,7 @@ static int prepend_path(const struct pat error =prepend (buffer, buflen,"/" ,1 ); out: -br_read_unlock (vfsmount_lock); +br_read_unlock (&vfsmount_lock);return error; global_root:
fs/file_table.c renamed from fs/file_table.c~brlocks-lglocks-cleanups +8 -9
@@ -34,7 +34,6 @@ struct files_stat_struct files_stat = { .max_files = NR_FILE }; -DECLARE_LGLOCK (files_lglock);DEFINE_LGLOCK (files_lglock);/* SLAB cache for file structures */
@@ -421,9 +420,9 @@ static inline void __file_sb_list_add(st*/ void file_sb_list_add (struct file *file,struct super_block *sb) { -lg_local_lock (files_lglock); +lg_local_lock (&files_lglock);__file_sb_list_add (file, sb); -lg_local_unlock (files_lglock); +lg_local_unlock (&files_lglock); }/**
@@ -436,9 +435,9 @@ void file_sb_list_add(struct file *file,void file_sb_list_del (struct file *file) {if (!list_empty (&file->f_u.fu_list)) { -lg_local_lock_cpu (files_lglock,file_list_cpu (file)); +lg_local_lock_cpu (&files_lglock,file_list_cpu (file));list_del_init (&file->f_u.fu_list); -lg_local_unlock_cpu (files_lglock,file_list_cpu (file)); +lg_local_unlock_cpu (&files_lglock,file_list_cpu (file)); } }
@@ -485,7 +484,7 @@ void mark_files_ro(struct super_block *sstruct file *f; retry: -lg_global_lock (files_lglock); +lg_global_lock (&files_lglock);do_file_list_for_each_entry (sb, f) {struct vfsmount *mnt;if (!S_ISREG (f->f_path.dentry->d_inode->i_mode))
@@ -502,12 +501,12 @@ retry:file_release_write (f); mnt =mntget (f->f_path.mnt);/* This can sleep, so we can't hold the spinlock. */ -lg_global_unlock (files_lglock); +lg_global_unlock (&files_lglock);mnt_drop_write (mnt);mntput (mnt); goto retry; } while_file_list_for_each_entry; -lg_global_unlock (files_lglock); +lg_global_unlock (&files_lglock); }void __init files_init (unsigned long mempages)
@@ -525,6 +524,6 @@ void __init files_init(unsigned long mem n = (mempages * (PAGE_SIZE /1024 )) /10 ; files_stat.max_files = max_t(unsigned long, n, NR_FILE);files_defer_init (); -lg_lock_init (files_lglock); +lg_lock_init (&files_lglock,"files_lglock" );percpu_counter_init (&nr_files,0 ); }
fs/internal.h renamed from fs/internal.h~brlocks-lglocks-cleanups +1 -2
@@ -56,8 +56,7 @@ extern int sb_prepare_remount_readonly(sextern void __init mnt_init (void ); -DECLARE_BRLOCK (vfsmount_lock); - +extern struct lglock vfsmount_lock;/* * fs_struct.c
fs/namei.c renamed from fs/namei.c~brlocks-lglocks-cleanups +12 -12
@@ -462,7 +462,7 @@ static int unlazy_walk(struct nameidatamntget (nd->path.mnt);rcu_read_unlock (); -br_read_unlock (vfsmount_lock); +br_read_unlock (&vfsmount_lock); nd->flags &= ~LOOKUP_RCU ;return 0 ;
@@ -520,14 +520,14 @@ static int complete_walk(struct nameidatif (unlikely (!__d_rcu_to_refcount (dentry, nd->seq))) {spin_unlock (&dentry->d_lock);rcu_read_unlock (); -br_read_unlock (vfsmount_lock); +br_read_unlock (&vfsmount_lock);return -ECHILD ; }BUG_ON (nd->inode != dentry->d_inode);spin_unlock (&dentry->d_lock);mntget (nd->path.mnt);rcu_read_unlock (); -br_read_unlock (vfsmount_lock); +br_read_unlock (&vfsmount_lock); }if (likely (!(nd->flags &LOOKUP_JUMPED )))
@@ -694,15 +694,15 @@ int follow_up(struct path *path)struct mount *parent;struct dentry *mountpoint; -br_read_lock (vfsmount_lock); +br_read_lock (&vfsmount_lock); parent = mnt->mnt_parent;if (&parent->mnt == path->mnt) { -br_read_unlock (vfsmount_lock); +br_read_unlock (&vfsmount_lock);return 0 ; }mntget (&parent->mnt); mountpoint =dget (mnt->mnt_mountpoint); -br_read_unlock (vfsmount_lock); +br_read_unlock (&vfsmount_lock);dput (path->dentry); path->dentry = mountpoint;mntput (path->mnt);
@@ -960,7 +960,7 @@ failed:if (!(nd->flags &LOOKUP_ROOT )) nd->root.mnt =NULL ;rcu_read_unlock (); -br_read_unlock (vfsmount_lock); +br_read_unlock (&vfsmount_lock);return -ECHILD ; }
@@ -1256,7 +1256,7 @@ static void terminate_walk(struct nameidif (!(nd->flags &LOOKUP_ROOT )) nd->root.mnt =NULL ;rcu_read_unlock (); -br_read_unlock (vfsmount_lock); +br_read_unlock (&vfsmount_lock); } }
@@ -1490,7 +1490,7 @@ static int path_init(int dfd, const char nd->path = nd->root; nd->inode = inode;if (flags &LOOKUP_RCU ) { -br_read_lock (vfsmount_lock); +br_read_lock (&vfsmount_lock);rcu_read_lock (); nd->seq =__read_seqcount_begin (&nd->path.dentry->d_seq); }else {
@@ -1503,7 +1503,7 @@ static int path_init(int dfd, const charif (*name=='/' ) {if (flags &LOOKUP_RCU ) { -br_read_lock (vfsmount_lock); +br_read_lock (&vfsmount_lock);rcu_read_lock ();set_root_rcu (nd); }else {
@@ -1516,7 +1516,7 @@ static int path_init(int dfd, const charstruct fs_struct *fs = current->fs;unsigned seq; -br_read_lock (vfsmount_lock); +br_read_lock (&vfsmount_lock);rcu_read_lock (); do {
@@ -1552,7 +1552,7 @@ static int path_init(int dfd, const charif (fput_needed) *fp = file; nd->seq =__read_seqcount_begin (&nd->path.dentry->d_seq); -br_read_lock (vfsmount_lock); +br_read_lock (&vfsmount_lock);rcu_read_lock (); }else {path_get (&file->f_path);
fs/namespace.c renamed from fs/namespace.c~brlocks-lglocks-cleanups +70 -70
424 lines not on this page · raw message
fs/pnode.c renamed from fs/pnode.c~brlocks-lglocks-cleanups +2 -2
@@ -257,12 +257,12 @@ int propagate_mnt(struct mount *dest_mnt prev_src_mnt = child; } out: -br_write_lock (vfsmount_lock); +br_write_lock (&vfsmount_lock);while (!list_empty (&tmp_list)) { child = list_first_entry(&tmp_list, struct mount, mnt_hash);umount_tree (child,0 , &umount_list); } -br_write_unlock (vfsmount_lock); +br_write_unlock (&vfsmount_lock);release_mounts (&umount_list);return ret; }
fs/proc_namespace.c renamed from fs/proc_namespace.c~brlocks-lglocks-cleanups +2 -2
@@ -23,12 +23,12 @@ static unsigned mounts_poll(struct filepoll_wait (file, &p->ns->poll, wait); -br_read_lock (vfsmount_lock); +br_read_lock (&vfsmount_lock);if (p->m.poll_event != ns->event) { p->m.poll_event = ns->event; res |=POLLERR |POLLPRI ; } -br_read_unlock (vfsmount_lock); +br_read_unlock (&vfsmount_lock);return res; }
include/linux/lglock.h renamed from include/linux/lglock.h~brlocks-lglocks-cleanups +35 -154
205 lines not on this page · raw message
kernel/Makefile renamed from kernel/Makefile~brlocks-lglocks-cleanups +1 -1
@@ -10,7 +10,7 @@ obj-y = fork.o exec_domain.o panic.o kthread.o wait.o kfifo.o sys_ni.o posix-cpu-timers.o mutex.o \ hrtimer.o rwsem.o nsproxy.o srcu.o semaphore.o \ notifier.o ksysfs.o cred.o \ - async.o range.o groups.o + async.o range.o groups.o lglock.o ifdef CONFIG_FUNCTION_TRACER # Do not trace debug files and internal ftrace files
kernel/lglock.c new file +136 -0
136 lines not on this page · raw message
_
Quoted 2 lines · Andrew Morton
From: Andi Kleen <[email protected]>
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?
Quoted 12 lines
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*.
include/linux/lglock.h line 60
Quoted 10 lines
include/linux/lglock.h renamed from include/linux/lglock.h~brlocks-lglocks-cleanups +10
-0+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.
Quoted 1 line
+/* Only valid for statics */
what does this comment mean?
include/linux/lglock.h line 79
Quoted 11 lines
include/linux/lglock.h renamed from include/linux/lglock.h~brlocks-lglocks-cleanups +11
-0+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.
Quoted 4 lines
+/* 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 undolg_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?
kernel/lglock.c line 29
Quoted 4 lines
kernel/lglock.c new file +4
-0+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.
kernel/lglock.c line 39
Quoted 4 lines
kernel/lglock.c new file +4
-0+void lg_local_unlock (struct lglock *lg) +{ +arch_spinlock_t *lock; +rwlock_release (&lg->lock_dep_map,1 , _RET_IP_);
ditto.
kernel/lglock.c line 49
Quoted 4 lines
kernel/lglock.c new file +4
-0+void lg_local_lock_cpu (struct lglock *lg,int cpu) +{ +arch_spinlock_t *lock; +preempt_disable ();
ditto.
Review
Quoted 2 lines
(quoted diff) +2
-0+ arch_spinlock_t *lock; + rwlock_release(&lg->lock_dep_map, 1, _RET_IP_);
ditto.
kernel/lglock.c line 69
Quoted 3 lines
kernel/lglock.c new file +3
-0+{ +int i; +spin_lock (&lg->cpu_lock);
ditto.
kernel/lglock.c line 39
Quoted 3 lines
kernel/lglock.c new file +3
-0+{ +int i; +rwlock_release (&lg->lock_dep_map,1 , _RET_IP_);
ditto.
kernel/lglock.c line 29
Quoted 3 lines
kernel/lglock.c new file +3
-0+{ +int i; +preempt_disable ();
ditto.
kernel/lglock.c line 39
Quoted 3 lines
kernel/lglock.c new file +3
-0+{ +int i; +rwlock_release (&lg->lock_dep_map,1 , _RET_IP_);
ditto.
kernel/lglock.c line 119
Quoted 2 lines
kernel/lglock.c new file +2
-0+int lg_cpu_callback (struct notifier_block *nb, +unsigned long action,void *hcpu)
ugly linebreak, we can do better.
kernel/lglock.c line 122
Quoted 3 lines
kernel/lglock.c new file +3
-0+{ + 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:
Thanks,
Ingo
As far as I can tell from skimming that lengthy rant it's only about some white space.
So the patch must be perfect in substance!
-Andi
Quoted 2 lines · Andi Kleen
As far as I can tell from skimming that lengthy rant it's only about some white space.
Read it again.
Thanks,
Ingo
Quoted 3 lines · On Tue, 28 Feb 2012 09:43:59 +0100 Ingo Molnar <[email protected]> wrote:
This patch should also probably go upstream through the locking/lockdep tree? Mind sending it us once you think it's ready?
Oh goody, that means you own
http://marc.info/?l=linux-kernel&m=131419353511653&w=2.
I do think the brlock code is sick, but Nick has turned into an ex-Nick and nobody works on it.
Hi Andrew,
Quoted 10 lines · Andrew Morton
Quoted 3 lines · On Tue, 28 Feb 2012 09:43:59 +0100 Ingo Molnar <[email protected]> wrote:
This patch should also probably go upstream through the locking/lockdep tree? Mind sending it us once you think it's ready?
Oh goody, that means you own
http://marc.info/?l=linux-kernel&m=131419353511653&w=2.
That bug got fixed sometime around Dec 2011. See commit e30e2fdf (VFS: Fix race between CPU hotplug and lglocks)
Quoted 3 lines
I do think the brlock code is sick, but Nick has turned into an ex-Nick and nobody works on it.
Regards,
Srivatsa S. Bhat
Quoted 11 lines · Srivatsa S. Bhat
Hi Andrew,
Quoted 7 lines · Andrew Morton
Quoted 3 lines · On Tue, 28 Feb 2012 09:43:59 +0100 Ingo Molnar <[email protected]> wrote:
This patch should also probably go upstream through the locking/lockdep tree? Mind sending it us once you think it's ready?
Quoted 7 lines
Quoted 3 lines
Oh goody, that means you own
http://marc.info/?l=linux-kernel&m=131419353511653&w=2.That bug got fixed sometime around Dec 2011. See commit
e30e2fdf(VFS: Fix race between CPU hotplug and lglocks)
The lglocks code is still CPU-hotplug racy AFAICS, despite the ->cpu_lock complication:
Consider a taken global lock on a CPU:
CPU#1 ... br_write_lock(vfsmount_lock);
this takes the lock of all online CPUs: say CPU#1 and CPU#2. Now CPU#3 comes online and takes the read lock:
CPU#3 br_read_lock(vfsmount_lock);
This will succeed while the br_write_lock() is still active, because CPU#1 has only taken the locks of CPU#1 and CPU#2.
Crash!
The proper fix would be for CPU-online to serialize with all known lglocks, via the notifier callback, i.e. to do something like this:
case CPU_UP_PREPARE:
for_each_online_cpu(cpu) {
spin_lock(&name##_cpu_lock);
spin_unlock(&name##_cpu_lock);
}
...I.e. in essence do:
case CPU_UP_PREPARE: name##_global_lock_online(); name##_global_unlock_online();
Another detail I noticed, this bit:
register_hotcpu_notifier(&name##_lg_cpu_notifier); \
get_online_cpus(); \
for_each_online_cpu(i) \
cpu_set(i, name##_cpus); \
put_online_cpus(); \could be something simpler and loop-less, like:
get_online_cpus(); cpumask_copy(name##_cpus, cpu_online_mask); register_hotcpu_notifier(&name##_lg_cpu_notifier); put_online_cpus();
Thanks,
Ingo
Quoted 25 lines · Ingo Molnar
Quoted 11 lines · Srivatsa S. Bhat
Hi Andrew,
Quoted 7 lines · Andrew Morton
Quoted 3 lines · On Tue, 28 Feb 2012 09:43:59 +0100 Ingo Molnar <[email protected]> wrote:
This patch should also probably go upstream through the locking/lockdep tree? Mind sending it us once you think it's ready?
Quoted 7 lines
Quoted 3 lines
Oh goody, that means you own
http://marc.info/?l=linux-kernel&m=131419353511653&w=2.That bug got fixed sometime around Dec 2011. See commit
e30e2fdf(VFS: Fix race between CPU hotplug and lglocks)The lglocks code is still CPU-hotplug racy AFAICS, despite the ->cpu_lock complication:
Quoted 8 lines
Consider a taken global lock on a CPU:
CPU#1 ... br_write_lock(vfsmount_lock);this takes the lock of all online CPUs: say CPU#1 and CPU#2. Now CPU#3 comes online and takes the read lock:
CPU#3 cannot come online! :-)
No new CPU can come online until that corresponding br_write_unlock() is completed. That is because br_write_lock acquires &name##_cpu_lock and only br_write_unlock will release it.
And, CPU_UP_PREPARE callback tries to acquire that very same spinlock, and hence will keep spinning until br_write_unlock() is run. And hence, the CPU#3 or any new CPU online for that matter will not complete until br_write_unlock() is done.
It is of course debatable as to how good this design really is, but IMHO, the lglocks code is not CPU-hotplug racy now..
Here is the link to the original discussion during the development of that patch: thread.gmane.org/gmane.linux.file-systems/59750/
Quoted 34 lines
CPU#3 br_read_lock(vfsmount_lock);This will succeed while the
br_write_lock()is still active, because CPU#1 has only taken the locks of CPU#1 and CPU#2.Crash!
The proper fix would be for CPU-online to serialize with all known lglocks, via the notifier callback, i.e. to do something like this:
case CPU_UP_PREPARE: for_each_online_cpu(cpu) { spin_lock(&name##_cpu_lock); spin_unlock(&name##_cpu_lock); } ...I.e. in essence do:
case CPU_UP_PREPARE: name##_global_lock_online(); name##_global_unlock_online();Another detail I noticed, this bit:
register_hotcpu_notifier(&name##_lg_cpu_notifier); \ get_online_cpus(); \ for_each_online_cpu(i) \ cpu_set(i, name##_cpus); \ put_online_cpus(); \
Quoted 7 lines
could be something simpler and loop-less, like:
get_online_cpus(); cpumask_copy(name##_cpus, cpu_online_mask); register_hotcpu_notifier(&name##_lg_cpu_notifier); put_online_cpus();
While the cpumask_copy is definitely better, we can't put the register_hotcpu_notifier() within get/put_online_cpus() because it will lead to ABBA deadlock with a newly initiated CPU Hotplug operation, the 2 locks involved being the cpu_add_remove_lock and the cpu_hotplug lock.
IOW, at the moment there is no "absolutely race-free way" way to do CPU Hotplug callback registration. Some time ago, while going through the asynchronous booting patch by Arjan [1] I had written up a patch to fix that race because that race got transformed from "purely theoretical" to "very real" with the async boot patch, as shown by the powerpc boot failures [2].
But then I stopped short of posting that patch to the lists because I started wondering how important that race would actually turn out to be, in case the async booting design takes a totally different approach altogether.. [And the reason why I didn't post it is also because it would require lots of changes in many parts where CPU Hotplug registration is done, and that wouldn't probably be justified (I don't know..) if the race remained only theoretical, as it is now.]
[1]. http://thread.gmane.org/gmane.linux.kernel/1246209
[2]. https://lkml.org/lkml/2012/2/13/383
Regards,
Srivatsa S. Bhat
Quoted 37 lines · Srivatsa S. Bhat
Quoted 25 lines · Ingo Molnar
Quoted 11 lines · Srivatsa S. Bhat
Hi Andrew,
Quoted 7 lines · Andrew Morton
Quoted 3 lines · On Tue, 28 Feb 2012 09:43:59 +0100 Ingo Molnar <[email protected]> wrote:
This patch should also probably go upstream through the locking/lockdep tree? Mind sending it us once you think it's ready?
Quoted 7 lines
Quoted 3 lines
Oh goody, that means you own
http://marc.info/?l=linux-kernel&m=131419353511653&w=2.That bug got fixed sometime around Dec 2011. See commit
e30e2fdf(VFS: Fix race between CPU hotplug and lglocks)The lglocks code is still CPU-hotplug racy AFAICS, despite the ->cpu_lock complication:
Quoted 8 lines
Consider a taken global lock on a CPU:
CPU#1 ... br_write_lock(vfsmount_lock);this takes the lock of all online CPUs: say CPU#1 and CPU#2. Now CPU#3 comes online and takes the read lock:
Quoted 5 lines
CPU#3 cannot come online! :-)
No new CPU can come online until that corresponding
br_write_unlock()is completed. That is becausebr_write_lockacquires &name##_cpu_lock and onlybr_write_unlockwill release it.
Indeed, you are right.
Note that ->cpu_lock is an entirely superfluous complication in br_write_lock(): the whole CPU hotplug race can be addressed by doing a br_write_lock()/unlock() barrier in the hotplug callback
...
Quoted 30 lines
Quoted 8 lines
Another detail I noticed, this bit:
register_hotcpu_notifier(&name##_lg_cpu_notifier); \ get_online_cpus(); \ for_each_online_cpu(i) \ cpu_set(i, name##_cpus); \ put_online_cpus(); \Quoted 7 lines
could be something simpler and loop-less, like:
get_online_cpus(); cpumask_copy(name##_cpus, cpu_online_mask); register_hotcpu_notifier(&name##_lg_cpu_notifier); put_online_cpus();While the
cpumask_copyis definitely better, we can't put theregister_hotcpu_notifier()within get/put_online_cpus() because it will lead to ABBA deadlock with a newly initiated CPU Hotplug operation, the 2 locks involved being thecpu_add_remove_lockand thecpu_hotpluglock.IOW, at the moment there is no "absolutely race-free way" way to do CPU Hotplug callback registration. Some time ago, while going through the asynchronous booting patch by Arjan [1] I had written up a patch to fix that race because that race got transformed from "purely theoretical" to "very real" with the async boot patch, as shown by the powerpc boot failures [2].
Quoted 8 lines
But then I stopped short of posting that patch to the lists because I started wondering how important that race would actually turn out to be, in case the async booting design takes a totally different approach altogether.. [And the reason why I didn't post it is also because it would require lots of changes in many parts where CPU Hotplug registration is done, and that wouldn't probably be justified (I don't know..) if the race remained only theoretical, as it is now.]
A fairly simple solution would be to eliminate the _cpus mask as well, and do a for_each_possible_cpu() loop in the super-slow loop - like dozens and dozens of other places do it in the kernel.
At a first quick glance that way the code gets a lot simpler and the only CPU hotplug related change needed are the CPU_* callbacks to do the lock barrier.
Thanks,
Ingo
Quoted 46 lines · Ingo Molnar
Quoted 37 lines · Srivatsa S. Bhat
Quoted 25 lines · Ingo Molnar
Quoted 11 lines · Srivatsa S. Bhat
Hi Andrew,
Quoted 7 lines · Andrew Morton
Quoted 3 lines · On Tue, 28 Feb 2012 09:43:59 +0100 Ingo Molnar <[email protected]> wrote:
This patch should also probably go upstream through the locking/lockdep tree? Mind sending it us once you think it's ready?
Quoted 7 lines
Quoted 3 lines
Oh goody, that means you own
http://marc.info/?l=linux-kernel&m=131419353511653&w=2.That bug got fixed sometime around Dec 2011. See commit
e30e2fdf(VFS: Fix race between CPU hotplug and lglocks)The lglocks code is still CPU-hotplug racy AFAICS, despite the ->cpu_lock complication:
Quoted 8 lines
Consider a taken global lock on a CPU:
CPU#1 ... br_write_lock(vfsmount_lock);this takes the lock of all online CPUs: say CPU#1 and CPU#2. Now CPU#3 comes online and takes the read lock:
Quoted 5 lines
CPU#3 cannot come online! :-)
No new CPU can come online until that corresponding
br_write_unlock()is completed. That is becausebr_write_lockacquires &name##_cpu_lock and onlybr_write_unlockwill release it.
Quoted 5 lines
Indeed, you are right.
Note that ->cpu_lock is an entirely superfluous complication in
br_write_lock(): the whole CPU hotplug race can be addressed by doing a br_write_lock()/unlock() barrier in the hotplug callback
I don't think I understood your point completely, but please see below...
Quoted 42 lines
...
Quoted 30 lines
Quoted 8 lines
Another detail I noticed, this bit:
register_hotcpu_notifier(&name##_lg_cpu_notifier); \ get_online_cpus(); \ for_each_online_cpu(i) \ cpu_set(i, name##_cpus); \ put_online_cpus(); \Quoted 7 lines
could be something simpler and loop-less, like:
get_online_cpus(); cpumask_copy(name##_cpus, cpu_online_mask); register_hotcpu_notifier(&name##_lg_cpu_notifier); put_online_cpus();While the
cpumask_copyis definitely better, we can't put theregister_hotcpu_notifier()within get/put_online_cpus() because it will lead to ABBA deadlock with a newly initiated CPU Hotplug operation, the 2 locks involved being thecpu_add_remove_lockand thecpu_hotpluglock.IOW, at the moment there is no "absolutely race-free way" way to do CPU Hotplug callback registration. Some time ago, while going through the asynchronous booting patch by Arjan [1] I had written up a patch to fix that race because that race got transformed from "purely theoretical" to "very real" with the async boot patch, as shown by the powerpc boot failures [2].
Quoted 8 lines
But then I stopped short of posting that patch to the lists because I started wondering how important that race would actually turn out to be, in case the async booting design takes a totally different approach altogether.. [And the reason why I didn't post it is also because it would require lots of changes in many parts where CPU Hotplug registration is done, and that wouldn't probably be justified (I don't know..) if the race remained only theoretical, as it is now.]
Quoted 5 lines
A fairly simple solution would be to eliminate the
_cpusmask as well, and do afor_each_possible_cpu()loop in the super-slow loop - like dozens and dozens of other places do it in the kernel.
(I am assuming you are referring to the lglocks problem here, and not to the ABBA deadlock/racy registration etc discussed immediately above.)
We wanted to avoid doing for_each_possible_cpu() to avoid the unnecessary performance hit. In fact, that was the very first solution proposed, by Cong Meng. See this:
http://thread.gmane.org/gmane.linux.file-systems/59750/
http://thread.gmane.org/gmane.linux.file-systems/59750/focus=59751
So we developed a solution that avoids for_each_possible_cpu(), and yet works. Also, another point to be noted is that (referring to your previous mail actually), doing for_each_online_cpu() at CPU_UP_PREPARE time won't really work since the cpus are marked online only much later. So, the solution we chose was to keep a consistent _cpus mask throughout the lock-unlock sequence and perform the per-cpu lock/unlock only on the cpus in that cpu mask; and ensuring that that mask won't change in between... and also by delaying any new CPU online event during that time period using the new ->cpu_lock spinlock as I mentioned in the other mail.
This (complexity) explains why the commit message of e30e2fdf looks more like a mathematical theorem ;-)
Quoted 4 lines
At a first quick glance that way the code gets a lot simpler and the only CPU hotplug related change needed are the CPU_* callbacks to do the lock barrier.
Regards,
Srivatsa S. Bhat
IBM Linux Technology Center
Quoted 2 lines · Srivatsa S. Bhat
We wanted to avoid doing
for_each_possible_cpu()to avoid the unnecessary performance hit. [...]
That was done at the cost of making the code rather complex.
The thing is, *ANY* cpu-mask loop is an utter slowpath, so the "performance hit" is an overstatement. There's already dozens of of for_each_possible_cpu() loops in the kernel, and it's a perfectly acceptable solution in such cases.
I suspect it does not matter much now as the code appears to be correct, but in general we want to opt for simpler designs for rare and fragile codepaths.
Thanks,
Ingo
Quoted 8 lines · Ingo Molnar
Quoted 2 lines · Srivatsa S. Bhat
We wanted to avoid doing
for_each_possible_cpu()to avoid the unnecessary performance hit. [...]That was done at the cost of making the code rather complex.
Sadly, yes..
Quoted 9 lines
The thing is, *ANY* cpu-mask loop is an utter slowpath, so the "performance hit" is an overstatement. There's already dozens of of
for_each_possible_cpu()loops in the kernel, and it's a perfectly acceptable solution in such cases.I suspect it does not matter much now as the code appears to be correct, but in general we want to opt for simpler designs for rare and fragile codepaths.
Ok, makes sense. And now looking back at the amount of complexity we built into this just to avoid the for_each_possible_cpu() loop, I wonder why we went to such lengths at all! (considering what you said above about any cpu-mask loop..)
Regards,
Srivatsa S. Bhat
Quoted 116 lines · Srivatsa S. Bhat
Quoted 25 lines · Ingo Molnar
Quoted 11 lines · Srivatsa S. Bhat
Hi Andrew,
Quoted 7 lines · Andrew Morton
Quoted 3 lines · On Tue, 28 Feb 2012 09:43:59 +0100 Ingo Molnar <[email protected]> wrote:
This patch should also probably go upstream through the locking/lockdep tree? Mind sending it us once you think it's ready?
Quoted 7 lines
Quoted 3 lines
Oh goody, that means you own
http://marc.info/?l=linux-kernel&m=131419353511653&w=2.That bug got fixed sometime around Dec 2011. See commit
e30e2fdf(VFS: Fix race between CPU hotplug and lglocks)The lglocks code is still CPU-hotplug racy AFAICS, despite the ->cpu_lock complication:
Quoted 8 lines
Consider a taken global lock on a CPU:
CPU#1 ... br_write_lock(vfsmount_lock);this takes the lock of all online CPUs: say CPU#1 and CPU#2. Now CPU#3 comes online and takes the read lock:
CPU#3 cannot come online! :-)
No new CPU can come online until that corresponding
br_write_unlock()is completed. That is becausebr_write_lockacquires &name##_cpu_lock and onlybr_write_unlockwill release it.
And,CPU_UP_PREPAREcallback tries to acquire that very same spinlock, and hence will keep spinning untilbr_write_unlock()is run. And hence, the CPU#3 or any new CPU online for that matter will not complete untilbr_write_unlock()is done.It is of course debatable as to how good this design really is, but IMHO, the lglocks code is not CPU-hotplug racy now..
Here is the link to the original discussion during the development of that patch: thread.gmane.org/gmane.linux.file-systems/59750/
Quoted 34 lines
CPU#3 br_read_lock(vfsmount_lock);This will succeed while the
br_write_lock()is still active, because CPU#1 has only taken the locks of CPU#1 and CPU#2.Crash!
The proper fix would be for CPU-online to serialize with all known lglocks, via the notifier callback, i.e. to do something like this:
case CPU_UP_PREPARE: for_each_online_cpu(cpu) { spin_lock(&name##_cpu_lock); spin_unlock(&name##_cpu_lock); } ...I.e. in essence do:
case CPU_UP_PREPARE: name##_global_lock_online(); name##_global_unlock_online();Another detail I noticed, this bit:
register_hotcpu_notifier(&name##_lg_cpu_notifier); \ get_online_cpus(); \ for_each_online_cpu(i) \ cpu_set(i, name##_cpus); \ put_online_cpus(); \Quoted 7 lines
could be something simpler and loop-less, like:
get_online_cpus(); cpumask_copy(name##_cpus, cpu_online_mask); register_hotcpu_notifier(&name##_lg_cpu_notifier); put_online_cpus();While the
cpumask_copyis definitely better, we can't put theregister_hotcpu_notifier()within get/put_online_cpus() because it will lead to ABBA deadlock with a newly initiated CPU Hotplug operation, the 2 locks involved being thecpu_add_remove_lockand thecpu_hotpluglock.IOW, at the moment there is no "absolutely race-free way" way to do CPU Hotplug callback registration. Some time ago, while going through the asynchronous booting patch by Arjan [1] I had written up a patch to fix that race because that race got transformed from "purely theoretical" to "very real" with the async boot patch, as shown by the powerpc boot failures [2].
But then I stopped short of posting that patch to the lists because I started wondering how important that race would actually turn out to be, in case the async booting design takes a totally different approach altogether.. [And the reason why I didn't post it is also because it would require lots of changes in many parts where CPU Hotplug registration is done, and that wouldn't probably be justified (I don't know..) if the race remained only theoretical, as it is now.]
Quoted 3 lines
[1]. http://thread.gmane.org/gmane.linux.kernel/1246209
[2]. https://lkml.org/lkml/2012/2/13/383
Ok, now that I mentioned about my patch, let me as well show it some daylight.. It is totally untested, incomplete and probably won't even compile.. (given that I had abandoned working on it some time ago, since I was not sure in what direction the async boot design was headed, which was the original motivation for me to try to fix this race)
I really hate to post it when it is in such a state, but atleast let me get the idea out, now that the discussion is around it, atleast just to get some thoughts about whether it is even worth pursuing!
(I'll post the patches as a reply to this mail.)
By the way, it should solve the powerpc boot failure, atleast in principle, considering what the root cause of the failure was..
Regards,
Srivatsa S. Bhat
PATCH 1/3 CPU hotplug: Fix issues with callback registration
Currently, there are several intertwined problems with CPU hotplug callback registration:
Code which needs to get notified of CPU hotplug events and additionally wants to do something for each already online CPU, would typically do something like:
register_cpu_notifier(&foobar_cpu_notifier);
<============ "A"
get_online_cpus();
for_each_online_cpu(cpu) {
/* Do something */ }
put_online_cpus();
At the point marked as "A", a CPU hotplug event could sneak in, leaving the code confused. Moving the registration to after put_online_cpus() won't help either, because we could be losing a CPU hotplug event between put_online_cpus() and the callback registration. Also, doing the registration inside the get/put_online_cpus() block is also not going to help, because it will lead to ABBA deadlock with CPU hotplug, the 2 locks being cpu_add_remove_lock and cpu_hotplug lock.
It is also to be noted that, at times, we might want to do different setups or initializations depending on whether a CPU is coming online for the first time (as part of booting) or whether it is being only soft-onlined at a later point in time. To achieve this, doing something like the code shown above, with the "Do something" being different than what the registered callback does wouldn't work out, because of the race conditions mentioned above.
The solution to all this is to include "history replay upon request" within the CPU hotplug callback registration code, while also providing an option for a different callback to be invoked while replaying history.
Though the above mentioned race condition was mostly theoretical before, it gets all real when things like asynchronous booting[1] come into the picture, as shown by the PowerPC boot failure in [2]. So this fix is also a step forward in getting cool things like asynchronous booting to work properly.
References:
[1]. https://lkml.org/lkml/2012/2/14/62
---
include/linux/cpu.h | 15 +++++++++++++++ kernel/cpu.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 61 insertions(+), 3 deletions(-)
include/linux/cpu.h +15 -0
@@ -124,16 +124,25 @@ enum {#endif /* #else #if defined(CONFIG_HOTPLUG_CPU) || !defined(MODULE) */ #ifdef CONFIG_HOTPLUG_CPU extern int register_cpu_notifier (struct notifier_block *nb); +extern int register_allcpu_notifier (struct notifier_block *nb, +bool replay_history,int (*history_setup)(void ));extern void unregister_cpu_notifier (struct notifier_block *nb);#else #ifndef MODULE extern int register_cpu_notifier (struct notifier_block *nb); +extern int register_allcpu_notifier (struct notifier_block *nb, +bool replay_history,int (*history_setup)(void ));#else static inline int register_cpu_notifier (struct notifier_block *nb) {return 0 ; } +static inline int register_allcpu_notifier (struct notifier_block *nb, +bool replay_history,int (*history_setup)(void )) +{ +return 0 ; +}#endif static inline void unregister_cpu_notifier (struct notifier_block *nb)
@@ -155,6 +164,12 @@ static inline int register_cpu_notifier(struct notifier_block *nb)return 0 ; } +static inline int register_allcpu_notifier (struct notifier_block *nb, +bool replay_history,int (*history_setup)(void )) +{ +return 0 ; +} +static inline void unregister_cpu_notifier (struct notifier_block *nb) { }
kernel/cpu.c +46 -3
65 lines not on this page · raw message
PATCH 2/3 CPU hotplug, arch/powerpc: Fix CPU hotplug callback registration
In reply to Srivatsa S. BhatRestructure CPU hotplug setup and callback registration in topology_init so as to be race-free.
---
arch/powerpc/kernel/sysfs.c | 44 +++++++++++++++++++++++++++++++++++-------- arch/powerpc/mm/numa.c | 11 ++++++++--- 2 files changed, 44 insertions(+), 11 deletions(-)
arch/powerpc/kernel/sysfs.c +36 -8
@@ -496,6 +496,38 @@ ssize_t arch_cpu_release(const char *buf, size_t count)#endif /* CONFIG_HOTPLUG_CPU */ +static void cpu_register_helper (struct cpu *c,int cpu) +{ +register_cpu (c, cpu); +device_create_file (&c->dev, &dev_attr_physical_id); +} + +static int __cpuinit sysfs_cpu_notify_first_time (struct notifier_block *self, +unsigned long action,void *hcpu) +{ +unsigned int cpu = (unsigned int )(long )hcpu; +struct cpu *c = &per_cpu (cpu_devices, cpu); + +if (action ==CPU_ONLINE ) +if (!c->hotpluggable)/* Avoid duplicate registrations */ +cpu_register_helper (c, cpu); +register_cpu_online (cpu); + } +return NOTIFY_OK ; +} +static int __cpuinit sysfs_cpu_notify_setup (void ) +{ +int cpu; + +/* +* We don't race with CPU hotplug because we are called from +* the CPU hotplug callback registration function. +*/ +for_each_online_cpu (cpu) +sysfs_cpu_notify_first_time (NULL ,CPU_ONLINE , cpu); + +return 0 ; +}static int __cpuinit sysfs_cpu_notify (struct notifier_block *self,unsigned long action,void *hcpu) {
@@ -637,7 +669,6 @@ static int __init topology_init(void)int cpu;register_nodes (); -register_cpu_notifier (&sysfs_cpu_nb);for_each_possible_cpu (cpu) {struct cpu *c = &per_cpu (cpu_devices, cpu);
@@ -652,15 +683,12 @@ static int __init topology_init(void)if (ppc_md.cpu_die) c->hotpluggable =1 ; -if (cpu_online (cpu) || c->hotpluggable) { -register_cpu (c, cpu); +if (c->hotpluggable) +cpu_register_helper (c, cpu); + } -device_create_file (&c->dev, &dev_attr_physical_id); - } +register_allcpu_notifier (&sysfs_cpu_nb, true, &sysfs_cpu_notify_setup); -if (cpu_online (cpu)) -register_cpu_online (cpu); - }#ifdef CONFIG_PPC64 sysfs_create_dscr_default ();#endif /* CONFIG_PPC64 */
arch/powerpc/mm/numa.c +8 -3
@@ -1014,6 +1014,13 @@ static void __init mark_reserved_regions_for_nid(int nid) } } +static int __cpuinit cpu_numa_callback_setup (void ) +{ +cpu_numa_callback (&ppc64_numa_nb,CPU_UP_PREPARE , + (void *)(unsigned long )boot_cpuid); +return 0 ; +} +void __init do_init_bootmem (void ) {
@@ -1088,9 +1095,7 @@ void __init do_init_bootmem(void)*/ setup_node_to_cpumask_map (); -register_cpu_notifier (&ppc64_numa_nb); -cpu_numa_callback (&ppc64_numa_nb,CPU_UP_PREPARE , - (void *)(unsigned long )boot_cpuid); +register_allcpu_notifier (&ppc64_numa_nb, true, &cpu_numa_callback_setup); }void __init paging_init (void )
PATCH 3/3 CPU hotplug, arch/sparc: Fix CPU hotplug callback registration
In reply to Srivatsa S. BhatRestructure CPU hotplug setup and callback registration in topology_init so as to be race-free.
---
arch/sparc/kernel/sysfs.c | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-)
arch/sparc/kernel/sysfs.c +2 -4
@@ -300,16 +300,14 @@ static int __init topology_init(void)check_mmu_stats (); -register_cpu_notifier (&sysfs_cpu_nb); -for_each_possible_cpu (cpu) {struct cpu *c = &per_cpu (cpu_devices, cpu);register_cpu (c, cpu); -if (cpu_online (cpu)) -register_cpu_online (cpu); } +register_allcpu_notifier (&sysfs_cpu_nb, true,NULL ); +return 0 ; }
Quoted 7 lines · Andrew Morton
Quoted 3 lines · On Tue, 28 Feb 2012 09:43:59 +0100 Ingo Molnar <[email protected]> wrote:
This patch should also probably go upstream through the locking/lockdep tree? Mind sending it us once you think it's ready?
Quoted 5 lines
Oh goody, that means you own
http://marc.info/?l=linux-kernel&m=131419353511653&w=2.I do think the brlock code is sick, but Nick has turned into an ex-Nick and nobody works on it.
The main problem, highlighted by the above soft lockup report, is that lglocks and brlocks go outside the regular spinlock facilities and thus avoid quite a bit of generic lock debugging for no good reason. Those patches should never have gone upstream in their incomplete form via the VFS tree.
As part of any cleanup they should first be converted from arch_spinlock_t to regular spinlock_t - I bet if that is done then that not only simplifies the wrappers massively, it also turns the above soft lockup report into a nice, actionable lockdep splat.
Andi's deficient patch does not address that main shortcoming of brlocks/lglocks, it just addresses a symptom - while introducing a handful of new symptoms of suckage.
I'll review and process resubmitted patches from Andi if he wants to submit a proper, complete series - but there's a quality threshold and in this case I'd rather keep 1970's preprocessor code that sucks very visibly and possibly attracts someone with a clue than have a sloppy patch hiding the deeper design problems done by a clueless person who is also openly hostile towards the concept of producing quality code.
Thanks,
Ingo
Quoted 5 lines · Ingo Molnar
As part of any cleanup they should first be converted from
arch_spinlock_tto regularspinlock_t- I bet if that is done then that not only simplifies the wrappers massively, it also turns the above soft lockup report into a nice, actionable lockdep splat.
It might help if you'd actually read the code.. that will simply not work.
Quoted 10 lines · Peter Zijlstra
Quoted 5 lines · Ingo Molnar
As part of any cleanup they should first be converted from
arch_spinlock_tto regularspinlock_t- I bet if that is done then that not only simplifies the wrappers massively, it also turns the above soft lockup report into a nice, actionable lockdep splat.It might help if you'd actually read the code.. that will simply not work.
It cannot find all bugs - such as the CPU hotplug race that is still present in the code.
Still there's no excuse to go outside regular spinlock debug primitives via arch_spinlock_t.
If lockdep blows up in br_write_lock() due to holding up to 4096 individual locks then we should add the exceptions to this particular write lock when the CPU count is too high - but:
- do not disable the checking on saner configs
- not disable all the *OTHER* lock debugging checks such as:
- spin-lockup detection [this works even without ->held_locks]
- allocate/free failure detection:
The percpu code could be extended to run the equivalent of debug_check_no_locks_freed() over the percpu area that is going away, to make sure no held locks are freed.
etc.
Thanks,
Ingo
Quoted 10 lines · Andrew Morton
Quoted 5 lines · On Tue, 28 Feb 2012 09:52:30 +1030 Rusty Russell <[email protected]> wrote:
Use a
cpumask_var_tinstead ofcpumask_t. We're doing plenty of allocations here anyway, so it's not really an issue, and it sets a good example.(
cpumask_tis obsolescent, as are the cpus_* functions).Congratulations to yourself and Andi:
Well the only way to avoid that problem is to merge it ASAP.
The patch -- like most code movement patches -- is totally unsuitable for keeping around for months in trees.
Usually I found the best way to do merges with code movement patches is to edit the patches itself if possible.
Maybe some day we'll get patch like tools that can deal with this stuff better.
Rusty: you should probably have cocci rules for this stuff, not manual patches.
-Andi
Quoted 2 lines · Andi Kleen
Rusty: you should probably have cocci rules for this stuff, not manual patches.
Yes, though hopefully this is the last.
I've updated your patch based on Ingo's peevish feedback and a patch to rip out the online stuff; posting now...
Thanks,
Rusty.
Signature · 1 line
Please can you help my fiance Alex? http://baldalex.org
PATCH 1/3 lglock: remove online variants of lock
In reply to Andi KleenOptimizing the slow paths adds a lot of complexity. If you need to grab every lock often, you have other problems.
--- include/linux/lglock.h | 58 +------------------------------------------------ 1 file changed, 2 insertions(+), 56 deletions(-)
include/linux/lglock.h +2 -56
@@ -28,8 +28,8 @@#define br_lock_init (name) name##_lock_init()#define br_read_lock (name) name##_local_lock()#define br_read_unlock (name) name##_local_unlock() -#define br_write_lock (name) name##_global_lock_online() -#define br_write_unlock (name) name##_global_unlock_online() +#define br_write_lock (name) name##_global_lock() +#define br_write_unlock (name) name##_global_unlock()#define DECLARE_BRLOCK (name) DECLARE_LGLOCK(name)#define DEFINE_BRLOCK (name) DEFINE_LGLOCK(name)
@@ -42,8 +42,6 @@#define lg_local_unlock_cpu (name, cpu) name##_local_unlock_cpu(cpu)#define lg_global_lock (name) name##_global_lock()#define lg_global_unlock (name) name##_global_unlock() -#define lg_global_lock_online (name) name##_global_lock_online() -#define lg_global_unlock_online (name) name##_global_unlock_online()#ifdef CONFIG_DEBUG_LOCK_ALLOC #define LOCKDEP_INIT_MAP lockdep_init_map
@@ -68,36 +66,13 @@ extern void name##_local_unlock_cpu(int cpu); \ extern void name##_global_lock(void); \ extern void name##_global_unlock(void); \ - extern void name##_global_lock_online(void); \ - extern void name##_global_unlock_online(void); \#define DEFINE_LGLOCK (name) \ \ DEFINE_SPINLOCK(name##_cpu_lock); \ - cpumask_t name##_cpus __read_mostly; \ DEFINE_PER_CPU(arch_spinlock_t, name##_lock); \ DEFINE_LGLOCK_LOCKDEP(name); \ \ - static int \ - name##_lg_cpu_callback(struct notifier_block *nb, \ - unsigned long action, void *hcpu) \ - { \ - switch (action & ~CPU_TASKS_FROZEN) { \ - case CPU_UP_PREPARE: \ - spin_lock(&name##_cpu_lock); \ - cpu_set((unsigned long)hcpu, name##_cpus); \ - spin_unlock(&name##_cpu_lock); \ - break; \ - case CPU_UP_CANCELED: case CPU_DEAD: \ - spin_lock(&name##_cpu_lock); \ - cpu_clear((unsigned long)hcpu, name##_cpus); \ - spin_unlock(&name##_cpu_lock); \ - } \ - return NOTIFY_OK; \ - } \ - static struct notifier_block name##_lg_cpu_notifier = { \ - .notifier_call = name##_lg_cpu_callback, \ - }; \ void name##_lock_init(void) { \ int i; \ LOCKDEP_INIT_MAP(&name##_lock_dep_map, #name, &name##_lock_key, 0); \
@@ -106,11 +81,6 @@ lock = &per_cpu(name##_lock, i); \ *lock = (arch_spinlock_t )__ARCH_SPIN_LOCK_UNLOCKED; \ } \ - register_hotcpu_notifier(&name##_lg_cpu_notifier); \ -get_online_cpus (); \ -for_each_online_cpu (i) \ - cpu_set(i, name##_cpus); \ -put_online_cpus (); \ } \ EXPORT_SYMBOL(name##_lock_init); \ \
@@ -150,30 +120,6 @@ } \ EXPORT_SYMBOL(name##_local_unlock_cpu); \ \ - void name##_global_lock_online(void) { \ -int i; \ - spin_lock(&name##_cpu_lock); \ - rwlock_acquire(&name##_lock_dep_map, 0, 0, _RET_IP_); \ - for_each_cpu(i, &name##_cpus) { \ -arch_spinlock_t *lock; \ - lock = &per_cpu(name##_lock, i); \ -arch_spin_lock (lock); \ - } \ - } \ - EXPORT_SYMBOL(name##_global_lock_online); \ - \ - void name##_global_unlock_online(void) { \ -int i; \ - rwlock_release(&name##_lock_dep_map, 1, _RET_IP_); \ - for_each_cpu(i, &name##_cpus) { \ -arch_spinlock_t *lock; \ - lock = &per_cpu(name##_lock, i); \ -arch_spin_unlock (lock); \ - } \ - spin_unlock(&name##_cpu_lock); \ - } \ - EXPORT_SYMBOL(name##_global_unlock_online); \ - \ void name##_global_lock(void) { \int i; \preempt_disable (); \
Re: [PATCH 1/3] lglock: remove online variants of lock
Quoted 5 lines · Rusty Russell
Optimizing the slow paths adds a lot of complexity. If you need to grab every lock often, you have other problems.
Signed-off-by Rusty Russell <[email protected]>
Someone might cry about online vs possible discrepancy, but I don't really mind.
PATCH 2/3 brlocks/lglocks: API cleanups
In reply to Andi KleenFrom: Andi Kleen <[email protected]>
lglocks and brlocks are currently generated with some complicated macros in lglock.h. But there's no reason to not just use common utility functions and put all the data into a common data structure.
In preparation, this patch changes the API to look more like normal function calls with pointers, not magic macros.
The patch is rather large because I move over all users in one go to keep it bisectable. This impacts the VFS somewhat in terms of lines changed. But no actual behaviour change.
--- fs/dcache.c | 4 - fs/file_table.c | 16 ++--- fs/namei.c | 24 ++++---- fs/namespace.c | 140 ++++++++++++++++++++++++++-------------------------- fs/pnode.c | 4 - fs/proc_namespace.c | 4 - 6 files changed, 96 insertions(+), 96 deletions(-)
fs/dcache.c +2 -2
@@ -2489,7 +2489,7 @@ static int prepend_path(const struct patbool slash = false;int error =0 ; -br_read_lock (vfsmount_lock); +br_read_lock (&vfsmount_lock);while (dentry != root->dentry || vfsmnt != root->mnt) {struct dentry * parent;
@@ -2520,7 +2520,7 @@ static int prepend_path(const struct pat error =prepend (buffer, buflen,"/" ,1 ); out: -br_read_unlock (vfsmount_lock); +br_read_unlock (&vfsmount_lock);return error; global_root:
fs/file_table.c +8 -8
@@ -422,9 +422,9 @@ static inline void __file_sb_list_add(st*/ void file_sb_list_add (struct file *file,struct super_block *sb) { -lg_local_lock (files_lglock); +lg_local_lock (&files_lglock);__file_sb_list_add (file, sb); -lg_local_unlock (files_lglock); +lg_local_unlock (&files_lglock); }/**
@@ -437,9 +437,9 @@ void file_sb_list_add(struct file *file,void file_sb_list_del (struct file *file) {if (!list_empty (&file->f_u.fu_list)) { -lg_local_lock_cpu (files_lglock,file_list_cpu (file)); +lg_local_lock_cpu (&files_lglock,file_list_cpu (file));list_del_init (&file->f_u.fu_list); -lg_local_unlock_cpu (files_lglock,file_list_cpu (file)); +lg_local_unlock_cpu (&files_lglock,file_list_cpu (file)); } }
@@ -486,7 +486,7 @@ void mark_files_ro(struct super_block *sstruct file *f; retry: -lg_global_lock (files_lglock); +lg_global_lock (&files_lglock);do_file_list_for_each_entry (sb, f) {struct vfsmount *mnt;if (!S_ISREG (f->f_path.dentry->d_inode->i_mode))
@@ -503,12 +503,12 @@ retry:file_release_write (f); mnt =mntget (f->f_path.mnt);/* This can sleep, so we can't hold the spinlock. */ -lg_global_unlock (files_lglock); +lg_global_unlock (&files_lglock);mnt_drop_write (mnt);mntput (mnt); goto retry; } while_file_list_for_each_entry; -lg_global_unlock (files_lglock); +lg_global_unlock (&files_lglock); }void __init files_init (unsigned long mempages)
@@ -526,6 +526,6 @@ void __init files_init(unsigned long mem n = (mempages * (PAGE_SIZE /1024 )) /10 ; files_stat.max_files = max_t(unsigned long, n, NR_FILE);files_defer_init (); -lg_lock_init (files_lglock); +lg_lock_init (&files_lglock,"files_lglock" );percpu_counter_init (&nr_files,0 ); }
fs/namei.c +12 -12
@@ -462,7 +462,7 @@ static int unlazy_walk(struct nameidatamntget (nd->path.mnt);rcu_read_unlock (); -br_read_unlock (vfsmount_lock); +br_read_unlock (&vfsmount_lock); nd->flags &= ~LOOKUP_RCU ;return 0 ;
@@ -520,14 +520,14 @@ static int complete_walk(struct nameidatif (unlikely (!__d_rcu_to_refcount (dentry, nd->seq))) {spin_unlock (&dentry->d_lock);rcu_read_unlock (); -br_read_unlock (vfsmount_lock); +br_read_unlock (&vfsmount_lock);return -ECHILD ; }BUG_ON (nd->inode != dentry->d_inode);spin_unlock (&dentry->d_lock);mntget (nd->path.mnt);rcu_read_unlock (); -br_read_unlock (vfsmount_lock); +br_read_unlock (&vfsmount_lock); }if (likely (!(nd->flags &LOOKUP_JUMPED )))
@@ -694,15 +694,15 @@ int follow_up(struct path *path)struct mount *parent;struct dentry *mountpoint; -br_read_lock (vfsmount_lock); +br_read_lock (&vfsmount_lock); parent = mnt->mnt_parent;if (&parent->mnt == path->mnt) { -br_read_unlock (vfsmount_lock); +br_read_unlock (&vfsmount_lock);return 0 ; }mntget (&parent->mnt); mountpoint =dget (mnt->mnt_mountpoint); -br_read_unlock (vfsmount_lock); +br_read_unlock (&vfsmount_lock);dput (path->dentry); path->dentry = mountpoint;mntput (path->mnt);
@@ -960,7 +960,7 @@ failed:if (!(nd->flags &LOOKUP_ROOT )) nd->root.mnt =NULL ;rcu_read_unlock (); -br_read_unlock (vfsmount_lock); +br_read_unlock (&vfsmount_lock);return -ECHILD ; }
@@ -1256,7 +1256,7 @@ static void terminate_walk(struct nameidif (!(nd->flags &LOOKUP_ROOT )) nd->root.mnt =NULL ;rcu_read_unlock (); -br_read_unlock (vfsmount_lock); +br_read_unlock (&vfsmount_lock); } }
@@ -1515,7 +1515,7 @@ static int path_init(int dfd, const char nd->path = nd->root; nd->inode = inode;if (flags &LOOKUP_RCU ) { -br_read_lock (vfsmount_lock); +br_read_lock (&vfsmount_lock);rcu_read_lock (); nd->seq =__read_seqcount_begin (&nd->path.dentry->d_seq); }else {
@@ -1528,7 +1528,7 @@ static int path_init(int dfd, const charif (*name=='/' ) {if (flags &LOOKUP_RCU ) { -br_read_lock (vfsmount_lock); +br_read_lock (&vfsmount_lock);rcu_read_lock ();set_root_rcu (nd); }else {
@@ -1541,7 +1541,7 @@ static int path_init(int dfd, const charstruct fs_struct *fs = current->fs;unsigned seq; -br_read_lock (vfsmount_lock); +br_read_lock (&vfsmount_lock);rcu_read_lock (); do {
@@ -1577,7 +1577,7 @@ static int path_init(int dfd, const charif (fput_needed) *fp = file; nd->seq =__read_seqcount_begin (&nd->path.dentry->d_seq); -br_read_lock (vfsmount_lock); +br_read_lock (&vfsmount_lock);rcu_read_lock (); }else {path_get (&file->f_path);
fs/namespace.c +70 -70
424 lines not on this page · raw message
fs/pnode.c +2 -2
@@ -257,12 +257,12 @@ int propagate_mnt(struct mount *dest_mnt prev_src_mnt = child; } out: -br_write_lock (vfsmount_lock); +br_write_lock (&vfsmount_lock);while (!list_empty (&tmp_list)) { child = list_first_entry(&tmp_list, struct mount, mnt_hash);umount_tree (child,0 , &umount_list); } -br_write_unlock (vfsmount_lock); +br_write_unlock (&vfsmount_lock);release_mounts (&umount_list);return ret; }
fs/proc_namespace.c +2 -2
@@ -23,12 +23,12 @@ static unsigned mounts_poll(struct filepoll_wait (file, &p->ns->poll, wait); -br_read_lock (vfsmount_lock); +br_read_lock (&vfsmount_lock);if (p->m.poll_event != ns->event) { p->m.poll_event = ns->event; res |=POLLERR |POLLPRI ; } -br_read_unlock (vfsmount_lock); +br_read_unlock (&vfsmount_lock);return res; }
Signature · 1 line
Please can you help my fiance Alex? http://baldalex.org
PATCH 3/3 brlocks/lglocks: turn into functions
In reply to Andi KleenFrom: Andi Kleen <[email protected]>
lglocks and brlocks are currently generated with some complicated macros in lglock.h. But there's no reason to not just use common utility functions and put all the data into a common data structure.
Since there are at least two users it makes sense to share this code in a library. This is also easier maintainable than a macro forest.
This will also make it later possible to dynamically allocate lglocks and also use them in modules (this would both still need some additional, but now straightforward, code)
--- fs/file_table.c | 1 fs/internal.h | 2 include/linux/lglock.h | 125 ++++++++++--------------------------------------- kernel/Makefile | 2 kernel/lglock.c | 89 ++++++++++++++++++++++++++++++++++ 5 files changed, 117 insertions(+), 102 deletions(-)
fs/file_table.c +0 -1
@@ -34,7 +34,6 @@ struct files_stat_struct files_stat = { .max_files = NR_FILE }; -DECLARE_LGLOCK (files_lglock);DEFINE_LGLOCK (files_lglock);/* SLAB cache for file structures */
fs/internal.h +1 -1
@@ -56,7 +56,7 @@ extern int sb_prepare_remount_readonly(sextern void __init mnt_init (void ); -DECLARE_BRLOCK (vfsmount_lock); +extern struct lglock vfsmount_lock;/*
include/linux/lglock.h +26 -99
142 lines not on this page · raw message
kernel/Makefile +1 -1
@@ -10,7 +10,7 @@ obj-y = fork.o exec_domain.o panic.o kthread.o wait.o kfifo.o sys_ni.o posix-cpu-timers.o mutex.o \ hrtimer.o rwsem.o nsproxy.o srcu.o semaphore.o \ notifier.o ksysfs.o cred.o \ - async.o range.o groups.o + async.o range.o groups.o lglock.o ifdef CONFIG_FUNCTION_TRACER # Do not trace debug files and internal ftrace files
kernel/lglock.c new file +89 -0
89 lines not on this page · raw message
Re: [PATCH 3/3] brlocks/lglocks: turn into functions
This still not merged?
Quoted 5 lines · Rusty Russell
From: Andi Kleen <[email protected]>
lglocks and brlocks are currently generated with some complicated macros in
lglock.h. But there's no reason to not just use common utility functions and put all the data into a common data structure.
There is a reason, which is performance. Extra function call, but also IIRC the percpu accessor was not so fast doing it this way. Maybe that's improved...
So what's the performance difference?
Quoted 7 lines
Since there are at least two users it makes sense to share this code in a library. This is also easier maintainable than a macro forest.
This will also make it later possible to dynamically allocate lglocks and also use them in modules (this would both still need some additional, but now straightforward, code)
Yes, but let's not do either of those things :)
I was slightly crazy when committing that patch to the kernel, I'll admit. So if performance isn't significantly affected, then definitely. If it is... well, it's much easier to gain 1% performance by maintaining 100 self contained lines of hilarious code like this than to actually use your brain to improve somewhere else!
Thanks,
Nick
Re: [PATCH 3/3] brlocks/lglocks: turn into functions
Quoted 1 line · Nick Piggin
This still not merged?
No, I've been away. I've put it in -next for tomorrow, though I'm not sure what the best way to get it to Linus next merge window.
Quoted 5 lines
There is a reason, which is performance. Extra function call, but also IIRC the percpu accessor was not so fast doing it this way. Maybe that's improved...
So what's the performance difference?
What benchmarks you usually run? Feel free to try it out and report back; I only have small hardware here.
Quoted 8 lines
Quoted 7 lines
Since there are at least two users it makes sense to share this code in a library. This is also easier maintainable than a macro forest.
This will also make it later possible to dynamically allocate lglocks and also use them in modules (this would both still need some additional, but now straightforward, code)
Quoted 7 lines
Yes, but let's not do either of those things :)
I was slightly crazy when committing that patch to the kernel, I'll admit. So if performance isn't significantly affected, then definitely. If it is... well, it's much easier to gain 1% performance by maintaining 100 self contained lines of hilarious code like this than to actually use your brain to improve somewhere else!
Cheers,
Rusty.
Al Viro
maintainerRe: [PATCH 3/3] brlocks/lglocks: turn into functions
Quoted 5 lines · Rusty Russell
Quoted 1 line · Nick Piggin
This still not merged?
No, I've been away. I've put it in -next for tomorrow, though I'm not sure what the best way to get it to Linus next merge window.
Resend and I'll pick it.
PATCH 1/3 lglock: remove online variants of lock
Optimizing the slow paths adds a lot of complexity. If you need to grab every lock often, you have other problems.
--- include/linux/lglock.h | 58 +------------------------------------------------ 1 file changed, 2 insertions(+), 56 deletions(-)
include/linux/lglock.h +2 -56
@@ -28,8 +28,8 @@#define br_lock_init (name) name##_lock_init()#define br_read_lock (name) name##_local_lock()#define br_read_unlock (name) name##_local_unlock() -#define br_write_lock (name) name##_global_lock_online() -#define br_write_unlock (name) name##_global_unlock_online() +#define br_write_lock (name) name##_global_lock() +#define br_write_unlock (name) name##_global_unlock()#define DECLARE_BRLOCK (name) DECLARE_LGLOCK(name)#define DEFINE_BRLOCK (name) DEFINE_LGLOCK(name)
@@ -42,8 +42,6 @@#define lg_local_unlock_cpu (name, cpu) name##_local_unlock_cpu(cpu)#define lg_global_lock (name) name##_global_lock()#define lg_global_unlock (name) name##_global_unlock() -#define lg_global_lock_online (name) name##_global_lock_online() -#define lg_global_unlock_online (name) name##_global_unlock_online()#ifdef CONFIG_DEBUG_LOCK_ALLOC #define LOCKDEP_INIT_MAP lockdep_init_map
@@ -68,36 +66,13 @@ extern void name##_local_unlock_cpu(int cpu); \ extern void name##_global_lock(void); \ extern void name##_global_unlock(void); \ - extern void name##_global_lock_online(void); \ - extern void name##_global_unlock_online(void); \#define DEFINE_LGLOCK (name) \ \ DEFINE_SPINLOCK(name##_cpu_lock); \ - cpumask_t name##_cpus __read_mostly; \ DEFINE_PER_CPU(arch_spinlock_t, name##_lock); \ DEFINE_LGLOCK_LOCKDEP(name); \ \ - static int \ - name##_lg_cpu_callback(struct notifier_block *nb, \ - unsigned long action, void *hcpu) \ - { \ - switch (action & ~CPU_TASKS_FROZEN) { \ - case CPU_UP_PREPARE: \ - spin_lock(&name##_cpu_lock); \ - cpu_set((unsigned long)hcpu, name##_cpus); \ - spin_unlock(&name##_cpu_lock); \ - break; \ - case CPU_UP_CANCELED: case CPU_DEAD: \ - spin_lock(&name##_cpu_lock); \ - cpu_clear((unsigned long)hcpu, name##_cpus); \ - spin_unlock(&name##_cpu_lock); \ - } \ - return NOTIFY_OK; \ - } \ - static struct notifier_block name##_lg_cpu_notifier = { \ - .notifier_call = name##_lg_cpu_callback, \ - }; \ void name##_lock_init(void) { \ int i; \ LOCKDEP_INIT_MAP(&name##_lock_dep_map, #name, &name##_lock_key, 0); \
@@ -106,11 +81,6 @@ lock = &per_cpu(name##_lock, i); \ *lock = (arch_spinlock_t )__ARCH_SPIN_LOCK_UNLOCKED; \ } \ - register_hotcpu_notifier(&name##_lg_cpu_notifier); \ -get_online_cpus (); \ -for_each_online_cpu (i) \ - cpu_set(i, name##_cpus); \ -put_online_cpus (); \ } \ EXPORT_SYMBOL(name##_lock_init); \ \
@@ -150,30 +120,6 @@ } \ EXPORT_SYMBOL(name##_local_unlock_cpu); \ \ - void name##_global_lock_online(void) { \ -int i; \ - spin_lock(&name##_cpu_lock); \ - rwlock_acquire(&name##_lock_dep_map, 0, 0, _RET_IP_); \ - for_each_cpu(i, &name##_cpus) { \ -arch_spinlock_t *lock; \ - lock = &per_cpu(name##_lock, i); \ -arch_spin_lock (lock); \ - } \ - } \ - EXPORT_SYMBOL(name##_global_lock_online); \ - \ - void name##_global_unlock_online(void) { \ -int i; \ - rwlock_release(&name##_lock_dep_map, 1, _RET_IP_); \ - for_each_cpu(i, &name##_cpus) { \ -arch_spinlock_t *lock; \ - lock = &per_cpu(name##_lock, i); \ -arch_spin_unlock (lock); \ - } \ - spin_unlock(&name##_cpu_lock); \ - } \ - EXPORT_SYMBOL(name##_global_unlock_online); \ - \ void name##_global_lock(void) { \int i; \preempt_disable (); \
Al Viro
maintainerRe: [PATCH 1/3] lglock: remove online variants of lock
Quoted 2 lines · Rusty Russell
Optimizing the slow paths adds a lot of complexity. If you need to grab every lock often, you have other problems.
Applied, but I'm not too happy about the situation with vfsmount_lock ;-/ On kernels built for a lot of possible CPUs the loss of ..._online() versions will get painful. OTOH, we can always put the map + single spinlock + single notifier into lglock.c and reproduce the old logics. I'll do a patch along those lines and put it on a separate branch; then we'll be able to test and compare.
Contention from cpu map spinlock becoming shared between different lglocks (all two of them) is not an issue - we never use the files_lock one anyway (there we can't use _online variants).
Re: [PATCH 1/3] lglock: remove online variants of lock
Quoted 7 lines · Al Viro
Quoted 2 lines · Rusty Russell
Optimizing the slow paths adds a lot of complexity. If you need to grab every lock often, you have other problems.
Applied, but I'm not too happy about the situation with
vfsmount_lock;-/ On kernels built for a lot of possible CPUs the loss of ..._online() versions will get painful.
My original motivation was to get rid of that cpumask_t (and replace it with a cpumask_var_t). A simple enough patch, but I couldn't bring myself to leave that complex logic in place without clear justification.
Quoted 4 lines
OTOH, we can always put the map + single
spinlock + single notifier intolglock.cand reproduce the old logics. I'll do a patch along those lines and put it on a separate branch; then we'll be able to test and compare.
I'll be interested in the results: virtual systems are classic for wanting large # of CPUs which aren't actually online, so we might actually care.
I'd also like to get rid of the cpu_possible_map altogether, and just have NR_CPUS/nr_cpu_ids, since last I checked no arch really wants sparse numbers.
Cheers,
Rusty.
PATCH 2/3 brlocks/lglocks: API cleanups
In reply to Al ViroFrom: Andi Kleen <[email protected]>
lglocks and brlocks are currently generated with some complicated macros in lglock.h. But there's no reason to not just use common utility functions and put all the data into a common data structure.
In preparation, this patch changes the API to look more like normal function calls with pointers, not magic macros.
The patch is rather large because I move over all users in one go to keep it bisectable. This impacts the VFS somewhat in terms of lines changed. But no actual behaviour change.
--- fs/dcache.c | 4 - fs/file_table.c | 16 ++--- fs/namei.c | 24 ++++---- fs/namespace.c | 140 ++++++++++++++++++++++++++-------------------------- fs/pnode.c | 4 - fs/proc_namespace.c | 4 - 6 files changed, 96 insertions(+), 96 deletions(-)
fs/dcache.c +2 -2
@@ -2489,7 +2489,7 @@ static int prepend_path(const struct patbool slash = false;int error =0 ; -br_read_lock (vfsmount_lock); +br_read_lock (&vfsmount_lock);while (dentry != root->dentry || vfsmnt != root->mnt) {struct dentry * parent;
@@ -2520,7 +2520,7 @@ static int prepend_path(const struct pat error =prepend (buffer, buflen,"/" ,1 ); out: -br_read_unlock (vfsmount_lock); +br_read_unlock (&vfsmount_lock);return error; global_root:
fs/file_table.c +8 -8
@@ -422,9 +422,9 @@ static inline void __file_sb_list_add(st*/ void file_sb_list_add (struct file *file,struct super_block *sb) { -lg_local_lock (files_lglock); +lg_local_lock (&files_lglock);__file_sb_list_add (file, sb); -lg_local_unlock (files_lglock); +lg_local_unlock (&files_lglock); }/**
@@ -437,9 +437,9 @@ void file_sb_list_add(struct file *file,void file_sb_list_del (struct file *file) {if (!list_empty (&file->f_u.fu_list)) { -lg_local_lock_cpu (files_lglock,file_list_cpu (file)); +lg_local_lock_cpu (&files_lglock,file_list_cpu (file));list_del_init (&file->f_u.fu_list); -lg_local_unlock_cpu (files_lglock,file_list_cpu (file)); +lg_local_unlock_cpu (&files_lglock,file_list_cpu (file)); } }
@@ -486,7 +486,7 @@ void mark_files_ro(struct super_block *sstruct file *f; retry: -lg_global_lock (files_lglock); +lg_global_lock (&files_lglock);do_file_list_for_each_entry (sb, f) {struct vfsmount *mnt;if (!S_ISREG (f->f_path.dentry->d_inode->i_mode))
@@ -503,12 +503,12 @@ retry:file_release_write (f); mnt =mntget (f->f_path.mnt);/* This can sleep, so we can't hold the spinlock. */ -lg_global_unlock (files_lglock); +lg_global_unlock (&files_lglock);mnt_drop_write (mnt);mntput (mnt); goto retry; } while_file_list_for_each_entry; -lg_global_unlock (files_lglock); +lg_global_unlock (&files_lglock); }void __init files_init (unsigned long mempages)
@@ -526,6 +526,6 @@ void __init files_init(unsigned long mem n = (mempages * (PAGE_SIZE /1024 )) /10 ; files_stat.max_files = max_t(unsigned long, n, NR_FILE);files_defer_init (); -lg_lock_init (files_lglock); +lg_lock_init (&files_lglock,"files_lglock" );percpu_counter_init (&nr_files,0 ); }
fs/namei.c +12 -12
@@ -462,7 +462,7 @@ static int unlazy_walk(struct nameidatamntget (nd->path.mnt);rcu_read_unlock (); -br_read_unlock (vfsmount_lock); +br_read_unlock (&vfsmount_lock); nd->flags &= ~LOOKUP_RCU ;return 0 ;
@@ -520,14 +520,14 @@ static int complete_walk(struct nameidatif (unlikely (!__d_rcu_to_refcount (dentry, nd->seq))) {spin_unlock (&dentry->d_lock);rcu_read_unlock (); -br_read_unlock (vfsmount_lock); +br_read_unlock (&vfsmount_lock);return -ECHILD ; }BUG_ON (nd->inode != dentry->d_inode);spin_unlock (&dentry->d_lock);mntget (nd->path.mnt);rcu_read_unlock (); -br_read_unlock (vfsmount_lock); +br_read_unlock (&vfsmount_lock); }if (likely (!(nd->flags &LOOKUP_JUMPED )))
@@ -694,15 +694,15 @@ int follow_up(struct path *path)struct mount *parent;struct dentry *mountpoint; -br_read_lock (vfsmount_lock); +br_read_lock (&vfsmount_lock); parent = mnt->mnt_parent;if (&parent->mnt == path->mnt) { -br_read_unlock (vfsmount_lock); +br_read_unlock (&vfsmount_lock);return 0 ; }mntget (&parent->mnt); mountpoint =dget (mnt->mnt_mountpoint); -br_read_unlock (vfsmount_lock); +br_read_unlock (&vfsmount_lock);dput (path->dentry); path->dentry = mountpoint;mntput (path->mnt);
@@ -960,7 +960,7 @@ failed:if (!(nd->flags &LOOKUP_ROOT )) nd->root.mnt =NULL ;rcu_read_unlock (); -br_read_unlock (vfsmount_lock); +br_read_unlock (&vfsmount_lock);return -ECHILD ; }
@@ -1256,7 +1256,7 @@ static void terminate_walk(struct nameidif (!(nd->flags &LOOKUP_ROOT )) nd->root.mnt =NULL ;rcu_read_unlock (); -br_read_unlock (vfsmount_lock); +br_read_unlock (&vfsmount_lock); } }
@@ -1515,7 +1515,7 @@ static int path_init(int dfd, const char nd->path = nd->root; nd->inode = inode;if (flags &LOOKUP_RCU ) { -br_read_lock (vfsmount_lock); +br_read_lock (&vfsmount_lock);rcu_read_lock (); nd->seq =__read_seqcount_begin (&nd->path.dentry->d_seq); }else {
@@ -1528,7 +1528,7 @@ static int path_init(int dfd, const charif (*name=='/' ) {if (flags &LOOKUP_RCU ) { -br_read_lock (vfsmount_lock); +br_read_lock (&vfsmount_lock);rcu_read_lock ();set_root_rcu (nd); }else {
@@ -1541,7 +1541,7 @@ static int path_init(int dfd, const charstruct fs_struct *fs = current->fs;unsigned seq; -br_read_lock (vfsmount_lock); +br_read_lock (&vfsmount_lock);rcu_read_lock (); do {
@@ -1577,7 +1577,7 @@ static int path_init(int dfd, const charif (fput_needed) *fp = file; nd->seq =__read_seqcount_begin (&nd->path.dentry->d_seq); -br_read_lock (vfsmount_lock); +br_read_lock (&vfsmount_lock);rcu_read_lock (); }else {path_get (&file->f_path);
fs/namespace.c +70 -70
424 lines not on this page · raw message
fs/pnode.c +2 -2
@@ -257,12 +257,12 @@ int propagate_mnt(struct mount *dest_mnt prev_src_mnt = child; } out: -br_write_lock (vfsmount_lock); +br_write_lock (&vfsmount_lock);while (!list_empty (&tmp_list)) { child = list_first_entry(&tmp_list, struct mount, mnt_hash);umount_tree (child,0 , &umount_list); } -br_write_unlock (vfsmount_lock); +br_write_unlock (&vfsmount_lock);release_mounts (&umount_list);return ret; }
fs/proc_namespace.c +2 -2
@@ -23,12 +23,12 @@ static unsigned mounts_poll(struct filepoll_wait (file, &p->ns->poll, wait); -br_read_lock (vfsmount_lock); +br_read_lock (&vfsmount_lock);if (p->m.poll_event != ns->event) { p->m.poll_event = ns->event; res |=POLLERR |POLLPRI ; } -br_read_unlock (vfsmount_lock); +br_read_unlock (&vfsmount_lock);return res; }
PATCH 3/3 brlocks/lglocks: turn into functions
In reply to Al ViroFrom: Andi Kleen <[email protected]>
lglocks and brlocks are currently generated with some complicated macros in lglock.h. But there's no reason to not just use common utility functions and put all the data into a common data structure.
Since there are at least two users it makes sense to share this code in a library. This is also easier maintainable than a macro forest.
This will also make it later possible to dynamically allocate lglocks and also use them in modules (this would both still need some additional, but now straightforward, code)
--- fs/file_table.c | 1 fs/internal.h | 2 include/linux/lglock.h | 125 ++++++++++--------------------------------------- kernel/Makefile | 2 kernel/lglock.c | 89 ++++++++++++++++++++++++++++++++++ 5 files changed, 117 insertions(+), 102 deletions(-)
fs/file_table.c +0 -1
@@ -34,7 +34,6 @@ struct files_stat_struct files_stat = { .max_files = NR_FILE }; -DECLARE_LGLOCK (files_lglock);DEFINE_LGLOCK (files_lglock);/* SLAB cache for file structures */
fs/internal.h +1 -1
@@ -56,7 +56,7 @@ extern int sb_prepare_remount_readonly(sextern void __init mnt_init (void ); -DECLARE_BRLOCK (vfsmount_lock); +extern struct lglock vfsmount_lock;/*
include/linux/lglock.h +26 -99
142 lines not on this page · raw message
kernel/Makefile +1 -1
@@ -10,7 +10,7 @@ obj-y = fork.o exec_domain.o panic.o kthread.o wait.o kfifo.o sys_ni.o posix-cpu-timers.o mutex.o \ hrtimer.o rwsem.o nsproxy.o srcu.o semaphore.o \ notifier.o ksysfs.o cred.o \ - async.o range.o groups.o + async.o range.o groups.o lglock.o ifdef CONFIG_FUNCTION_TRACER # Do not trace debug files and internal ftrace files
kernel/lglock.c new file +89 -0
89 lines not on this page · raw message
Nick Piggin
reviewerRe: [PATCH 3/3] brlocks/lglocks: turn into functions
In reply to Rusty RussellQuoted 6 lines · Rusty Russell
Quoted 1 line · Nick Piggin
This still not merged?
No, I've been away. I've put it in -next for tomorrow, though I'm not sure what the best way to get it to Linus next merge window.
Quoted 8 lines
Quoted 5 lines
There is a reason, which is performance. Extra function call, but also IIRC the percpu accessor was not so fast doing it this way. Maybe that's improved...
So what's the performance difference?
What benchmarks you usually run? Feel free to try it out and report back; I only have small hardware here.
Nothing big. The actual scalability should be unchanged, because you're not going to be dirtying any shared cachelines. It's the use of dynamic percpu allocator and out of line calls etc which will slow down straight line performance.
How many microseconds does it take to stat("./dir1/dir2/myfile"), for example?
git diff of a large tree, for something more realistic but still going to exercise that path.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html