* Re: + schedule_on_each_cpu-use-preempt_disable.patch added to -mm tree [not found] <200612150823.kBF8NV2u011171@shell0.pdx.osdl.net> @ 2006-12-15 8:31 ` Ingo Molnar 2006-12-15 16:11 ` Andrew Morton 0 siblings, 1 reply; 6+ messages in thread From: Ingo Molnar @ 2006-12-15 8:31 UTC (permalink / raw) To: akpm; +Cc: mm-commits, linux-kernel * akpm@osdl.org <akpm@osdl.org> wrote: > - mutex_lock(&workqueue_mutex); > + preempt_disable(); /* CPU hotplug */ > for_each_online_cpu(cpu) { > INIT_WORK(per_cpu_ptr(works, cpu), func); > __queue_work(per_cpu_ptr(keventd_wq->cpu_wq, cpu), > per_cpu_ptr(works, cpu)); > } > - mutex_unlock(&workqueue_mutex); > + preempt_enable(); Why not cpu_hotplug_lock()? Ingo ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: + schedule_on_each_cpu-use-preempt_disable.patch added to -mm tree 2006-12-15 8:31 ` + schedule_on_each_cpu-use-preempt_disable.patch added to -mm tree Ingo Molnar @ 2006-12-15 16:11 ` Andrew Morton 2006-12-15 16:24 ` Ingo Molnar 2006-12-15 17:51 ` [PATCH] struct vfsmount : keep mnt_count & mnt_expiry_mark away from mnt_flags Eric Dumazet 0 siblings, 2 replies; 6+ messages in thread From: Andrew Morton @ 2006-12-15 16:11 UTC (permalink / raw) To: Ingo Molnar; +Cc: linux-kernel On Fri, 15 Dec 2006 09:31:12 +0100 Ingo Molnar <mingo@elte.hu> wrote: > > * akpm@osdl.org <akpm@osdl.org> wrote: > > > - mutex_lock(&workqueue_mutex); > > + preempt_disable(); /* CPU hotplug */ > > for_each_online_cpu(cpu) { > > INIT_WORK(per_cpu_ptr(works, cpu), func); > > __queue_work(per_cpu_ptr(keventd_wq->cpu_wq, cpu), > > per_cpu_ptr(works, cpu)); > > } > > - mutex_unlock(&workqueue_mutex); > > + preempt_enable(); > > Why not cpu_hotplug_lock()? > Because the workqueue code was explicitly switched over to per-subsystem cpu-hotplug locking. Because lock_cpu_hotplug() is a complete turkey, source of deadlocks and overall bad idea. This is actually a pretty simple problem. A subsystem has per-cpu reosurces, and it needs to lock them while using them. duh. We know how to do that sort of thing. But because the first implementation of lock_cpu_hotplug() was conceived with magical properties, we seem to think we need to retain magical properties. We don't... ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: + schedule_on_each_cpu-use-preempt_disable.patch added to -mm tree 2006-12-15 16:11 ` Andrew Morton @ 2006-12-15 16:24 ` Ingo Molnar 2006-12-15 19:27 ` Andrew Morton 2006-12-15 17:51 ` [PATCH] struct vfsmount : keep mnt_count & mnt_expiry_mark away from mnt_flags Eric Dumazet 1 sibling, 1 reply; 6+ messages in thread From: Ingo Molnar @ 2006-12-15 16:24 UTC (permalink / raw) To: Andrew Morton; +Cc: linux-kernel * Andrew Morton <akpm@osdl.org> wrote: > > > for_each_online_cpu(cpu) { > > > INIT_WORK(per_cpu_ptr(works, cpu), func); > > > __queue_work(per_cpu_ptr(keventd_wq->cpu_wq, cpu), > > > per_cpu_ptr(works, cpu)); > > > } > > > - mutex_unlock(&workqueue_mutex); > > > + preempt_enable(); > > > > Why not cpu_hotplug_lock()? > > > > Because the workqueue code was explicitly switched over to > per-subsystem cpu-hotplug locking. > > Because lock_cpu_hotplug() is a complete turkey, source of deadlocks > and overall bad idea. not in the locking model i outlined earlier, which would turn it into a read-lock in essence. > This is actually a pretty simple problem. A subsystem has per-cpu > reosurces, and it needs to lock them while using them. duh. We know > how to do that sort of thing. But because the first implementation of > lock_cpu_hotplug() was conceived with magical properties, we seem to > think we need to retain magical properties. We don't... actually, we use two things here: cpu_online_map and the per-cpu keventd workqueues. cpu_online_map is pretty much attached to the CPU hotplug subsystem so it would be quite natural to use cpu_hotplug_read_lock() for that. so i disagree that CPU hotplug locking should be per-subsystem. We should have one lightweight and scalable primitive that protects cpu_online_map use, and that same primitive can be used to protect other per-CPU resources too. Ingo ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: + schedule_on_each_cpu-use-preempt_disable.patch added to -mm tree 2006-12-15 16:24 ` Ingo Molnar @ 2006-12-15 19:27 ` Andrew Morton 0 siblings, 0 replies; 6+ messages in thread From: Andrew Morton @ 2006-12-15 19:27 UTC (permalink / raw) To: Ingo Molnar; +Cc: linux-kernel On Fri, 15 Dec 2006 17:24:16 +0100 Ingo Molnar <mingo@elte.hu> wrote: > > * Andrew Morton <akpm@osdl.org> wrote: > > > > > for_each_online_cpu(cpu) { > > > > INIT_WORK(per_cpu_ptr(works, cpu), func); > > > > __queue_work(per_cpu_ptr(keventd_wq->cpu_wq, cpu), > > > > per_cpu_ptr(works, cpu)); > > > > } > > > > - mutex_unlock(&workqueue_mutex); > > > > + preempt_enable(); > > > > > > Why not cpu_hotplug_lock()? > > > > > > > Because the workqueue code was explicitly switched over to > > per-subsystem cpu-hotplug locking. > > > > Because lock_cpu_hotplug() is a complete turkey, source of deadlocks > > and overall bad idea. > > not in the locking model i outlined earlier, which would turn it into a > read-lock in essence. > > > This is actually a pretty simple problem. A subsystem has per-cpu > > reosurces, and it needs to lock them while using them. duh. We know > > how to do that sort of thing. But because the first implementation of > > lock_cpu_hotplug() was conceived with magical properties, we seem to > > think we need to retain magical properties. We don't... > > actually, we use two things here: cpu_online_map and the per-cpu keventd > workqueues. cpu_online_map is pretty much attached to the CPU hotplug > subsystem so it would be quite natural to use cpu_hotplug_read_lock() > for that. The two are connected, because cpu add/remove creates and kills keventd threads. > so i disagree that CPU hotplug locking should be per-subsystem. We > should have one lightweight and scalable primitive that protects > cpu_online_map use, and that same primitive can be used to protect other > per-CPU resources too. This problem can be (is being) solved using existing locking primitives. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] struct vfsmount : keep mnt_count & mnt_expiry_mark away from mnt_flags 2006-12-15 16:11 ` Andrew Morton 2006-12-15 16:24 ` Ingo Molnar @ 2006-12-15 17:51 ` Eric Dumazet 2006-12-15 18:38 ` [PATCH] avoid one conditional branch in touch_atime() Eric Dumazet 1 sibling, 1 reply; 6+ messages in thread From: Eric Dumazet @ 2006-12-15 17:51 UTC (permalink / raw) To: Andrew Morton; +Cc: linux-kernel [-- Attachment #1: Type: text/plain, Size: 860 bytes --] I noticed cache misses in touch_atime() that can be avoided if we keep mnt_count & mnt_expiry_mark in a different cache line than mnt_flags (mostly read) mnt_count & mnt_expiry_mark are modified each time a file is opened/closed in a file system. touch_atime() is called each time a file is read, and generally needs to read mnt_flags. Other fields of struct vfsmount are mostly read so I chose to move mnt_count & mnt_expiry_mark at the end of struct vfsmount. And adding a comment so that nobody tries to re-arrange fields to fill the holes :) On 64bits platforms, the new offsetof(mnt_count) is 0xC0 On 32bits platforms, it is 0x60, so I didnot add a ____cacheline_aligned_in_smp because it would have a too big impact on the size of this object (in particular if CONFIG_X86_L1_CACHE_SHIFT=7) Signed-off-by: Eric Dumazet <dada1@cosmosbay.com> [-- Attachment #2: vfsmount.patch --] [-- Type: text/plain, Size: 1234 bytes --] --- linux-2.6.20-rc1-mm1/include/linux/mount.h 2006-12-14 02:14:23.000000000 +0100 +++ linux-2.6.20-rc1-mm1-ed/include/linux/mount.h 2006-12-15 19:30:54.000000000 +0100 @@ -43,9 +43,8 @@ struct vfsmount { struct super_block *mnt_sb; /* pointer to superblock */ struct list_head mnt_mounts; /* list of children, anchored here */ struct list_head mnt_child; /* and going through their mnt_child */ - atomic_t mnt_count; int mnt_flags; - int mnt_expiry_mark; /* true if marked for expiry */ + /* 4 bytes hole on 64bits arches */ char *mnt_devname; /* Name of device e.g. /dev/dsk/hda1 */ struct list_head mnt_list; struct list_head mnt_expire; /* link in fs-specific expiry list */ @@ -54,6 +53,13 @@ struct vfsmount { struct list_head mnt_slave; /* slave list entry */ struct vfsmount *mnt_master; /* slave is on master->mnt_slave_list */ struct mnt_namespace *mnt_ns; /* containing namespace */ + /* + * We put mnt_count & mnt_expiry_mark at the end of struct vfsmount + * to let these frequently modified fields in a separate cache line + * (so that reads of mnt_flags wont ping-pong on SMP machines) + */ + atomic_t mnt_count; + int mnt_expiry_mark; /* true if marked for expiry */ int mnt_pinned; }; ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] avoid one conditional branch in touch_atime() 2006-12-15 17:51 ` [PATCH] struct vfsmount : keep mnt_count & mnt_expiry_mark away from mnt_flags Eric Dumazet @ 2006-12-15 18:38 ` Eric Dumazet 0 siblings, 0 replies; 6+ messages in thread From: Eric Dumazet @ 2006-12-15 18:38 UTC (permalink / raw) To: Andrew Morton; +Cc: linux-kernel [-- Attachment #1: Type: text/plain, Size: 279 bytes --] I added IS_NOATIME(inode) macro definition in include/linux/fs.h, true if the inode superblock is marked readonly or noatime. This new macro is then used in touch_atime() instead of separatly testing MS_RDONLY and MS_NOATIME Signed-off-by: Eric Dumazet <dada1@cosmosbay.com> [-- Attachment #2: touch_atime.patch --] [-- Type: text/plain, Size: 1093 bytes --] --- linux-2.6.20-rc1-mm1/fs/inode.c 2006-12-14 02:14:23.000000000 +0100 +++ linux-2.6.20-rc1-mm1-ed/fs/inode.c 2006-12-15 20:14:31.000000000 +0100 @@ -1160,11 +1160,9 @@ void touch_atime(struct vfsmount *mnt, s struct inode *inode = dentry->d_inode; struct timespec now; - if (IS_RDONLY(inode)) - return; if (inode->i_flags & S_NOATIME) return; - if (inode->i_sb->s_flags & MS_NOATIME) + if (IS_NOATIME(inode)) return; if ((inode->i_sb->s_flags & MS_NODIRATIME) && S_ISDIR(inode->i_mode)) return; --- linux-2.6.20-rc1-mm1/include/linux/fs.h 2006-12-15 15:46:16.000000000 +0100 +++ linux-2.6.20-rc1-mm1-ed/include/linux/fs.h 2006-12-15 20:16:13.000000000 +0100 @@ -169,6 +169,7 @@ extern int dir_notify_enable; #define IS_DIRSYNC(inode) (__IS_FLG(inode, MS_SYNCHRONOUS|MS_DIRSYNC) || \ ((inode)->i_flags & (S_SYNC|S_DIRSYNC))) #define IS_MANDLOCK(inode) __IS_FLG(inode, MS_MANDLOCK) +#define IS_NOATIME(inode) __IS_FLG(inode, MS_RDONLY|MS_NOATIME) #define IS_NOQUOTA(inode) ((inode)->i_flags & S_NOQUOTA) #define IS_APPEND(inode) ((inode)->i_flags & S_APPEND) ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2006-12-15 19:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <200612150823.kBF8NV2u011171@shell0.pdx.osdl.net>
2006-12-15 8:31 ` + schedule_on_each_cpu-use-preempt_disable.patch added to -mm tree Ingo Molnar
2006-12-15 16:11 ` Andrew Morton
2006-12-15 16:24 ` Ingo Molnar
2006-12-15 19:27 ` Andrew Morton
2006-12-15 17:51 ` [PATCH] struct vfsmount : keep mnt_count & mnt_expiry_mark away from mnt_flags Eric Dumazet
2006-12-15 18:38 ` [PATCH] avoid one conditional branch in touch_atime() Eric Dumazet
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®