Skip to contentSkip to conversation

Additions to .data.read_mostly section

PATCH In review Ravikiran G Thirumalai 6 participants 19 messages Aug 24, 2005 – Aug 26, 2005

Subsystems (10)
Networking Memory management VFS XFS filesystem +6 more subsystems
lore
Share & tools
lore

Ravikiran G Thirumalai

author
#0 lore raw

PATCH 1/1 Additions to .data.read_mostly section

Following patch moves a few static 'read mostly' variables to the .data.read_mostly section. Typically these are vector - irq tables, boot_cpu_data, node_maps etc., which are initialized once and read from often and rarely written to. Please include.

Thanks,
Kiran

Patch to mark variables which are usually accessed for reads with __readmostly.

Signed-off-by Alok N Kataria <[email protected]>
Signed-off-by Shai Fultheim <[email protected]>
Signed-off-by Ravikiran Thirumalai <[email protected]>
arch/i386/kernel/io_apic.c +5 -5
Index: linux-2.6.13-rc6/arch/i386/kernel/io_apic.c
===================================================================
@@ -77,7 +77,7 @@
 	int apic, pin, next;
 } irq_2_pin[PIN_MAP_SIZE];
 
-int vector_irq[NR_VECTORS] = { [0 ... NR_VECTORS - 1] = -1};
+int vector_irq[NR_VECTORS] __read_mostly = { [0 ... NR_VECTORS - 1] = -1};
 #ifdef CONFIG_PCI_MSI
 #define vector_to_irq(vector) 	\
 	(platform_legacy_irq(vector) ? vector : vector_irq[vector])

@@ -1127,7 +1127,7 @@
 }
 
 /* irq_vectors is indexed by the sum of all RTEs in all I/O APICs. */
-u8 irq_vector[NR_IRQ_VECTORS] = { FIRST_DEVICE_VECTOR , 0 };
+u8 irq_vector[NR_IRQ_VECTORS] __read_mostly = { FIRST_DEVICE_VECTOR , 0 };
 
 int assign_irq_vector(int irq)
 {

@@ -1992,7 +1992,7 @@
  * edge-triggered handler, without risking IRQ storms and other ugly
  * races.
  */
-static struct hw_interrupt_type ioapic_edge_type = {
+static struct hw_interrupt_type ioapic_edge_type __read_mostly = {
 	.typename 	= "IO-APIC-edge",
 	.startup 	= startup_edge_ioapic,
 	.shutdown 	= shutdown_edge_ioapic,

@@ -2003,7 +2003,7 @@
 	.set_affinity 	= set_ioapic_affinity,
 };
 
-static struct hw_interrupt_type ioapic_level_type = {
+static struct hw_interrupt_type ioapic_level_type __read_mostly = {
 	.typename 	= "IO-APIC-level",
 	.startup 	= startup_level_ioapic,
 	.shutdown 	= shutdown_level_ioapic,

@@ -2074,7 +2074,7 @@
 
 static void end_lapic_irq (unsigned int i) { /* nothing */ }
 
-static struct hw_interrupt_type lapic_irq_type = {
+static struct hw_interrupt_type lapic_irq_type __read_mostly = {
 	.typename 	= "local-APIC-edge",
 	.startup 	= NULL, /* startup_irq() not used for IRQ0 */
 	.shutdown 	= NULL, /* shutdown_irq() not used for IRQ0 */
arch/i386/kernel/setup.c +1 -1
Index: linux-2.6.13-rc6/arch/i386/kernel/setup.c
===================================================================
@@ -82,7 +82,7 @@
 /* cpu data as detected by the assembly code in head.S */
 struct cpuinfo_x86 new_cpu_data __initdata = { 0, 0, 0, 0, -1, 1, 0, 0, -1 };
 /* common cpu data for all cpus */
-struct cpuinfo_x86 boot_cpu_data = { 0, 0, 0, 0, -1, 1, 0, 0, -1 };
+struct cpuinfo_x86 boot_cpu_data __read_mostly = { 0, 0, 0, 0, -1, 1, 0, 0, -1 };
 EXPORT_SYMBOL(boot_cpu_data);
 
 unsigned long mmu_cr4_features;
arch/i386/kernel/timers/timer_hpet.c +2 -2
Index: linux-2.6.13-rc6/arch/i386/kernel/timers/timer_hpet.c
===================================================================
@@ -18,8 +18,8 @@
 #include "mach_timer.h"
 #include <asm/hpet.h>
 
-static unsigned long __read_mostly hpet_usec_quotient;	/* convert hpet clks to usec */
-static unsigned long tsc_hpet_quotient;		/* convert tsc to hpet clks */
+static unsigned long hpet_usec_quotient __read_mostly;	/* convert hpet clks to usec */
+static unsigned long tsc_hpet_quotient __read_mostly;		/* convert tsc to hpet clks */
 static unsigned long hpet_last; 	/* hpet counter value at last tick*/
 static unsigned long last_tsc_low;	/* lsb 32 bits of Time Stamp Counter */
 static unsigned long last_tsc_high; 	/* msb 32 bits of Time Stamp Counter */
arch/i386/mm/discontig.c +4 -4
Index: linux-2.6.13-rc6/arch/i386/mm/discontig.c
===================================================================
@@ -37,7 +37,7 @@
 #include <asm/mmzone.h>
 #include <bios_ebda.h>
 
-struct pglist_data *node_data[MAX_NUMNODES];
+struct pglist_data *node_data[MAX_NUMNODES] __read_mostly;
 EXPORT_SYMBOL(node_data);
 bootmem_data_t node0_bdata;
 

@@ -49,8 +49,8 @@
  * 2) node_start_pfn   - the starting page frame number for a node
  * 3) node_end_pfn     - the ending page fram number for a node
  */
-unsigned long node_start_pfn[MAX_NUMNODES];
-unsigned long node_end_pfn[MAX_NUMNODES];
+unsigned long node_start_pfn[MAX_NUMNODES] __read_mostly;
+unsigned long node_end_pfn[MAX_NUMNODES] __read_mostly;
 
 
 #ifdef CONFIG_DISCONTIGMEM

@@ -66,7 +66,7 @@
  *     physnode_map[4-7] = 1;
  *     physnode_map[8- ] = -1;
  */
-s8 physnode_map[MAX_ELEMENTS] = { [0 ... (MAX_ELEMENTS - 1)] = -1};
+s8 physnode_map[MAX_ELEMENTS] __read_mostly = { [0 ... (MAX_ELEMENTS - 1)] = -1};
 EXPORT_SYMBOL(physnode_map);
 
 void memory_present(int nid, unsigned long start, unsigned long end)
arch/i386/mm/init.c +1 -1
Index: linux-2.6.13-rc6/arch/i386/mm/init.c
===================================================================
@@ -390,7 +390,7 @@
 }
 
 static int disable_nx __initdata = 0;
-u64 __supported_pte_mask = ~_PAGE_NX;
+u64 __supported_pte_mask __read_mostly = ~_PAGE_NX;
 
 /*
  * noexec = on|off
arch/x86_64/kernel/genapic.c +1 -1
Index: linux-2.6.13-rc6/arch/x86_64/kernel/genapic.c
===================================================================
@@ -25,7 +25,7 @@
 #endif
 
 /* which logical CPU number maps to which CPU (physical APIC ID) */
-u8 x86_cpu_to_apicid[NR_CPUS] = { [0 ... NR_CPUS-1] = BAD_APICID };
+u8 x86_cpu_to_apicid[NR_CPUS] __read_mostly = { [0 ... NR_CPUS-1] = BAD_APICID };
 EXPORT_SYMBOL(x86_cpu_to_apicid);
 u8 x86_cpu_to_log_apicid[NR_CPUS] = { [0 ... NR_CPUS-1] = BAD_APICID };
arch/x86_64/kernel/io_apic.c +5 -5
Index: linux-2.6.13-rc6/arch/x86_64/kernel/io_apic.c
===================================================================
@@ -70,7 +70,7 @@
 	short apic, pin, next;
 } irq_2_pin[PIN_MAP_SIZE];
 
-int vector_irq[NR_VECTORS] = { [0 ... NR_VECTORS - 1] = -1};
+int vector_irq[NR_VECTORS] __read_mostly = { [0 ... NR_VECTORS - 1] = -1};
 #ifdef CONFIG_PCI_MSI
 #define vector_to_irq(vector) 	\
 	(platform_legacy_irq(vector) ? vector : vector_irq[vector])

@@ -655,7 +655,7 @@
 }
 
 /* irq_vectors is indexed by the sum of all RTEs in all I/O APICs. */
-u8 irq_vector[NR_IRQ_VECTORS] = { FIRST_DEVICE_VECTOR , 0 };
+u8 irq_vector[NR_IRQ_VECTORS] __read_mostly = { FIRST_DEVICE_VECTOR , 0 };
 
 int assign_irq_vector(int irq)
 {

@@ -1424,7 +1424,7 @@
  * races.
  */
 
-static struct hw_interrupt_type ioapic_edge_type = {
+static struct hw_interrupt_type ioapic_edge_type __read_mostly = {
 	.typename = "IO-APIC-edge",
 	.startup 	= startup_edge_ioapic,
 	.shutdown 	= shutdown_edge_ioapic,

@@ -1435,7 +1435,7 @@
 	.set_affinity = set_ioapic_affinity,
 };
 
-static struct hw_interrupt_type ioapic_level_type = {
+static struct hw_interrupt_type ioapic_level_type __read_mostly = {
 	.typename = "IO-APIC-level",
 	.startup 	= startup_level_ioapic,
 	.shutdown 	= shutdown_level_ioapic,

@@ -1506,7 +1506,7 @@
 
 static void end_lapic_irq (unsigned int i) { /* nothing */ }
 
-static struct hw_interrupt_type lapic_irq_type = {
+static struct hw_interrupt_type lapic_irq_type __read_mostly = {
 	.typename = "local-APIC-edge",
 	.startup = NULL, /* startup_irq() not used for IRQ0 */
 	.shutdown = NULL, /* shutdown_irq() not used for IRQ0 */
arch/x86_64/kernel/setup.c +1 -1
Index: linux-2.6.13-rc6/arch/x86_64/kernel/setup.c
===================================================================
@@ -65,7 +65,7 @@
  * Machine setup..
  */
 
-struct cpuinfo_x86 boot_cpu_data;
+struct cpuinfo_x86 boot_cpu_data __read_mostly;
 
 unsigned long mmu_cr4_features;
arch/x86_64/kernel/setup64.c +1 -1
Index: linux-2.6.13-rc6/arch/x86_64/kernel/setup64.c
===================================================================
@@ -36,7 +36,7 @@
 
 char boot_cpu_stack[IRQSTACKSIZE] __attribute__((section(".bss.page_aligned")));
 
-unsigned long __supported_pte_mask = ~0UL;
+unsigned long __supported_pte_mask __read_mostly = ~0UL;
 static int do_not_nx __initdata = 0;
 
 /* noexec=on|off
arch/x86_64/kernel/smpboot.c +5 -5
Index: linux-2.6.13-rc6/arch/x86_64/kernel/smpboot.c
===================================================================
@@ -62,13 +62,13 @@
 /* Number of siblings per CPU package */
 int smp_num_siblings = 1;
 /* Package ID of each logical CPU */
-u8 phys_proc_id[NR_CPUS] = { [0 ... NR_CPUS-1] = BAD_APICID };
-u8 cpu_core_id[NR_CPUS] = { [0 ... NR_CPUS-1] = BAD_APICID };
+u8 phys_proc_id[NR_CPUS] __read_mostly = { [0 ... NR_CPUS-1] = BAD_APICID };
+u8 cpu_core_id[NR_CPUS] __read_mostly = { [0 ... NR_CPUS-1] = BAD_APICID };
 EXPORT_SYMBOL(phys_proc_id);
 EXPORT_SYMBOL(cpu_core_id);
 
 /* Bitmask of currently online CPUs */
-cpumask_t cpu_online_map;
+cpumask_t cpu_online_map __read_mostly;
 
 EXPORT_SYMBOL(cpu_online_map);
 

@@ -88,8 +88,8 @@
 /* Set when the idlers are all forked */
 int smp_threads_ready;
 
-cpumask_t cpu_sibling_map[NR_CPUS] __cacheline_aligned;
-cpumask_t cpu_core_map[NR_CPUS] __cacheline_aligned;
+cpumask_t cpu_sibling_map[NR_CPUS] __read_mostly;
+cpumask_t cpu_core_map[NR_CPUS] __read_mostly;
 EXPORT_SYMBOL(cpu_core_map);
 
 /*
arch/x86_64/mm/numa.c +3 -3
Index: linux-2.6.13-rc6/arch/x86_64/mm/numa.c
===================================================================
@@ -22,14 +22,14 @@
 #define Dprintk(x...)
 #endif
 
-struct pglist_data *node_data[MAX_NUMNODES];
+struct pglist_data *node_data[MAX_NUMNODES] __read_mostly;
 bootmem_data_t plat_node_bdata[MAX_NUMNODES];
 
 int memnode_shift;
 u8  memnodemap[NODEMAPSIZE];
 
-unsigned char cpu_to_node[NR_CPUS] = { [0 ... NR_CPUS-1] = NUMA_NO_NODE };
-cpumask_t     node_to_cpumask[MAX_NUMNODES];
+unsigned char cpu_to_node[NR_CPUS] __read_mostly = { [0 ... NR_CPUS-1] = NUMA_NO_NODE };
+cpumask_t     node_to_cpumask[MAX_NUMNODES] __read_mostly;
 
 int numa_off __initdata;
fs/namespace.c +1 -1
Index: linux-2.6.13-rc6/fs/namespace.c
===================================================================
@@ -40,7 +40,7 @@
  __cacheline_aligned_in_smp DEFINE_SPINLOCK(vfsmount_lock);
 
 static struct list_head *mount_hashtable;
-static int hash_mask, hash_bits;
+static int hash_mask __read_mostly, hash_bits __read_mostly;
 static kmem_cache_t *mnt_cache; 
 
 static inline unsigned long hash(struct vfsmount *mnt, struct dentry *dentry)
mm/mempolicy.c +1 -1
Index: linux-2.6.13-rc6/mm/mempolicy.c
===================================================================
@@ -88,7 +88,7 @@
    policied. */
 static int policy_zone;
 
-static struct mempolicy default_policy = {
+static struct mempolicy default_policy __read_mostly = {
 	.refcnt = ATOMIC_INIT(1), /* never free it */
 	.policy = MPOL_DEFAULT,
 };
mm/mmap.c +1 -1
Index: linux-2.6.13-rc6/mm/mmap.c
===================================================================
@@ -61,7 +61,7 @@
 
 int sysctl_overcommit_memory = OVERCOMMIT_GUESS;  /* heuristic overcommit */
 int sysctl_overcommit_ratio = 50;	/* default is 50% */
-int sysctl_max_map_count = DEFAULT_MAX_MAP_COUNT;
+int sysctl_max_map_count __read_mostly = DEFAULT_MAX_MAP_COUNT;
 atomic_t vm_committed_space = ATOMIC_INIT(0);
 
 /*
mm/page_alloc.c +6 -6
Index: linux-2.6.13-rc6/mm/page_alloc.c
===================================================================
@@ -42,13 +42,13 @@
  * MCD - HACK: Find somewhere to initialize this EARLY, or make this
  * initializer cleaner
  */
-nodemask_t node_online_map = { { [0] = 1UL } };
+nodemask_t node_online_map __read_mostly = { { [0] = 1UL } };
 EXPORT_SYMBOL(node_online_map);
-nodemask_t node_possible_map = NODE_MASK_ALL;
+nodemask_t node_possible_map __read_mostly = NODE_MASK_ALL;
 EXPORT_SYMBOL(node_possible_map);
-struct pglist_data *pgdat_list;
-unsigned long totalram_pages;
-unsigned long totalhigh_pages;
+struct pglist_data *pgdat_list __read_mostly;
+unsigned long totalram_pages __read_mostly;
+unsigned long totalhigh_pages __read_mostly;
 long nr_swap_pages;
 
 /*

@@ -68,7 +68,7 @@
  * Used by page_zone() to look up the address of the struct zone whose
  * id is encoded in the upper bits of page->flags
  */
-struct zone *zone_table[1 << ZONETABLE_SHIFT];
+struct zone *zone_table[1 << ZONETABLE_SHIFT] __read_mostly;
 EXPORT_SYMBOL(zone_table);
 
 static char *zone_names[MAX_NR_ZONES] = { "DMA", "Normal", "HighMem" };
mm/shmem.c +1 -1
Index: linux-2.6.13-rc6/mm/shmem.c
===================================================================
@@ -182,7 +182,7 @@
 static struct inode_operations shmem_special_inode_operations;
 static struct vm_operations_struct shmem_vm_ops;
 
-static struct backing_dev_info shmem_backing_dev_info = {
+static struct backing_dev_info shmem_backing_dev_info  __read_mostly = {
 	.ra_pages	= 0,	/* No readahead */
 	.capabilities	= BDI_CAP_NO_ACCT_DIRTY | BDI_CAP_NO_WRITEBACK,
 	.unplug_io_fn	= default_unplug_io_fn,
#1 lore raw
Quoted 3 lines · Ravikiran G Thirumalai

Following patch moves a few static 'read mostly' variables to the .data.read_mostly section. Typically these are vector - irq tables, boot_cpu_data, node_maps etc., which are initialized once and read from often and rarely written to. Please include.

Good candidates for read_mostly are all the 'kmem_cache_t *xxx_cache'

slab was carefuly designed to eliminate cache line ping pongs on SMP, but if the initial pointer to slab sits in a heavily modified cache line, we loose.

Eric

#2 lore raw
In reply to Ravikiran G Thirumalai
Quoted 4 lines · Ravikiran G Thirumalai

Following patch moves a few static 'read mostly' variables to the .data.read_mostly section. Typically these are vector - irq tables, boot_cpu_data, node_maps etc., which are initialized once and read from often and rarely written to. Please include.

it almost sounds like a "read_only_once_booted" is useful, esp for those who modify their kernel to make such sections truely read only ;)

#3 lore raw

PATCH 1/1 removes filp_count_lock and changes nr_files type to atomic_t

This patch removes filp_count_lock spinlock, used to protect files_stat.nr_files.

Just use atomic_t type and atomic_inc()/atomic_dec() operations.

This patch assumes that atomic_read() is a plain {return v->counter;} on all architectures. (keywords : sysctl, /proc/sys/fs/file-nr, proc_dointvec)

Signed-off-by Eric Dumazet <[email protected]>
fs/file_table.c +6 -14
diff -Nru linux-2.6.13-rc7/fs/file_table.c linux-2.6.13-rc7-ed/fs/file_table.c
@@ -28,29 +28,21 @@
 /* public. Not pretty! */
  __cacheline_aligned_in_smp DEFINE_SPINLOCK(files_lock);
 
-static DEFINE_SPINLOCK(filp_count_lock);
 
 /* slab constructors and destructors are called from arbitrary
- * context and must be fully threaded - use a local spinlock
- * to protect files_stat.nr_files
+ * context and must be fully threaded
  */
 void filp_ctor(void * objp, struct kmem_cache_s *cachep, unsigned long cflags)
 {
 	if ((cflags & (SLAB_CTOR_VERIFY|SLAB_CTOR_CONSTRUCTOR)) ==
 	    SLAB_CTOR_CONSTRUCTOR) {
-		unsigned long flags;
-		spin_lock_irqsave(&filp_count_lock, flags);
-		files_stat.nr_files++;
-		spin_unlock_irqrestore(&filp_count_lock, flags);
+		atomic_inc(&files_stat.nr_files);
 	}
 }
 
 void filp_dtor(void * objp, struct kmem_cache_s *cachep, unsigned long dflags)
 {
-	unsigned long flags;
-	spin_lock_irqsave(&filp_count_lock, flags);
-	files_stat.nr_files--;
-	spin_unlock_irqrestore(&filp_count_lock, flags);
+	atomic_dec(&files_stat.nr_files);
 }
 
 static inline void file_free(struct file *f)

@@ -70,7 +62,7 @@
 	/*
 	 * Privileged users can go above max_files
 	 */
-	if (files_stat.nr_files >= files_stat.max_files &&
+	if (atomic_read(&files_stat.nr_files) >= files_stat.max_files &&
 				!capable(CAP_SYS_ADMIN))
 		goto over;
 

@@ -94,10 +86,10 @@
 
 over:
 	/* Ran out of filps - report that */
-	if (files_stat.nr_files > old_max) {
+	if (atomic_read(&files_stat.nr_files) > old_max) {
 		printk(KERN_INFO "VFS: file-max limit %d reached\n",
 					files_stat.max_files);
-		old_max = files_stat.nr_files;
+		old_max = atomic_read(&files_stat.nr_files);
 	}
 	goto fail;
fs/xfs/linux-2.6/xfs_linux.h +1 -1
diff -Nru linux-2.6.13-rc7/fs/xfs/linux-2.6/xfs_linux.h linux-2.6.13-rc7-ed/fs/xfs/linux-2.6/xfs_linux.h
@@ -242,7 +242,7 @@
 
 /* IRIX uses the current size of the name cache to guess a good value */
 /* - this isn't the same but is a good enough starting point for now. */
-#define DQUOT_HASH_HEURISTIC	files_stat.nr_files
+#define DQUOT_HASH_HEURISTIC	atomic_read(&files_stat.nr_files)
 
 /* IRIX inodes maintain the project ID also, zero this field on Linux */
 #define DEFAULT_PROJID	0
include/linux/fs.h +1 -1
diff -Nru linux-2.6.13-rc7/include/linux/fs.h linux-2.6.13-rc7-ed/include/linux/fs.h
@@ -30,7 +30,7 @@
 
 /* And dynamically-tunable limits and defaults: */
 struct files_stat_struct {
-	int nr_files;		/* read only */
+	atomic_t nr_files;		/* read only */
 	int nr_free_files;	/* read only */
 	int max_files;		/* tunable */
 };
#4 lore raw

Re: [PATCH] removes filp_count_lock and changes nr_files type to atomic_t

Quoted 7 lines · Eric Dumazet

This patch removes filp_count_lock spinlock, used to protect files_stat.nr_files.

Just use atomic_t type and atomic_inc()/atomic_dec() operations.

This patch assumes that atomic_read() is a plain {return v->counter;} on all architectures. (keywords : sysctl, /proc/sys/fs/file-nr, proc_dointvec)

this patch adds atomic ops where there were none before

fs/file_table.c line 68

Quoted 10 lines

static inline void file_free(struct file *f)

fs/file_table.c +1 -1
@@ -70,7 +62,7 @@
 	/*
 	 * Privileged users can go above max_files
 	 */
-	if (files_stat.nr_files >= files_stat.max_files &&
+	if (atomic_read(&files_stat.nr_files) >= files_stat.max_files &&
 				!capable(CAP_SYS_ADMIN))
 		goto over;

here

fs/file_table.c line 94

Quoted 12 lines
fs/file_table.c +2 -2
@@ -94,10 +86,10 @@
 
 over:
 	/* Ran out of filps - report that */
-	if (files_stat.nr_files > old_max) {
+	if (atomic_read(&files_stat.nr_files) > old_max) {
 		printk(KERN_INFO "VFS: file-max limit %d reached\n",
 					files_stat.max_files);
-		old_max = files_stat.nr_files;
+		old_max = atomic_read(&files_stat.nr_files);
 	}
 	goto fail;

and here

for those architectures that need atomics for read (parisc? arm?)

however.. wouldn't it be better to make this a per cpu variable for write, and for read iterate or do something smart otherwise?

#5 lore raw

Re: [PATCH] removes filp_count_lock and changes nr_files type to atomic_t

Quoted 11 lines · Arjan van de Ven
Quoted 6 lines · Eric Dumazet

This patch removes filp_count_lock spinlock, used to protect files_stat.nr_files.

Just use atomic_t type and atomic_inc()/atomic_dec() operations.

This patch assumes that atomic_read() is a plain {return v->counter;} on all architectures. (keywords : sysctl, /proc/sys/fs/file-nr, proc_dointvec)

this patch adds atomic ops where there were none before

nope... a spinlock/spinunlock contains atomic ops.

Quoted 1 line

for those architectures that need atomics for read (parisc? arm?)

not today. No atomic needed for read.

Quoted 3 lines

however.. wouldn't it be better to make this a per cpu variable for write, and for read iterate or do something smart otherwise?

So on a machine with 256 CPUS, you want to iterate 256 counters ? nr_files is not heavily touched, no need to expand it.

#6 lore raw

Re: [PATCH] removes filp_count_lock and changes nr_files type to atomic_t

Quoted 4 lines
Quoted 2 lines

this patch adds atomic ops where there were none before

nope... a spinlock/spinunlock contains atomic ops.

unlock usually doesn't but ok

Quoted 10 lines
Quoted 2 lines

for those architectures that need atomics for read (parisc? arm?)

not today. No atomic needed for read.

Quoted 3 lines

however.. wouldn't it be better to make this a per cpu variable for write, and for read iterate or do something smart otherwise?

So on a machine with 256 CPUS, you want to iterate 256 counters ?

you can be smart about this, similar to what ext3 and others do for similar counters

#7 lore raw

Re: [PATCH] removes filp_count_lock and changes nr_files type to atomic_t

In reply to Eric Dumazet
Quoted 2 lines · Eric Dumazet

This patch assumes that atomic_read() is a plain {return v->counter;} on all architectures. (keywords : sysctl, /proc/sys/fs/file-nr, proc_dointvec)

But that's not true. You need to write you own sysctl handler for it, probably worth adding a generic atomic_t sysctl handler while you're at it.

#8 lore raw

Re: [PATCH] removes filp_count_lock and changes nr_files type to atomic_t

Quoted 9 lines · Christoph Hellwig
Quoted 1 line · Eric Dumazet

This patch assumes that atomic_read() is a plain {return v->counter;} on all architectures. (keywords : sysctl, /proc/sys/fs/file-nr, proc_dointvec)

But that's not true. You need to write you own sysctl handler for it, probably worth adding a generic atomic_t sysctl handler while you're at it.

I checked linux-2.6.13-rc7 tree, and atomic_read() is just a wrapper to read v->counter.

In the ancient times, yes, atomic_read() was different on some archs, but not today.

Eric

#9 lore raw

Re: [PATCH] removes filp_count_lock and changes nr_files type to atomic_t

Quoted 7 lines · Eric Dumazet
Quoted 4 lines

But that's not true. You need to write you own sysctl handler for it, probably worth adding a generic atomic_t sysctl handler while you're at it.

I checked linux-2.6.13-rc7 tree, and atomic_read() is just a wrapper to read v->counter.

That doesn't matter. atomic_t is an opaqueue type and you must use the atomic_* interfaces to access it.

#10 lore raw

Re: [PATCH] removes filp_count_lock and changes nr_files type to atomic_t

Quoted 7 lines · Christoph Hellwig
Quoted 5 lines · Eric Dumazet
Quoted 4 lines

But that's not true. You need to write you own sysctl handler for it, probably worth adding a generic atomic_t sysctl handler while you're at it.

Quoted 5 lines
Quoted 1 line

I checked linux-2.6.13-rc7 tree, and atomic_read() is just a wrapper to read v->counter.

That doesn't matter. atomic_t is an opaqueue type and you must use the atomic_* interfaces to access it.

OK, here is a new clean patch that address this problem (nothing assumed about atomics)

This patch removes filp_count_lock spinlock, used to protect files_stat.nr_files.

Introduce an atomic_t atomic_nr_files to keep the exact count, and mirror its value into nr_files.

Forcing atomic_nr_files to be in the same cache line than nr_files makes sure we dont dirty two cache lines.

There is still a locked memory operation on SMP, but it saves an sti/cli pair.

Thank you

Signed-off-by Eric Dumazet <[email protected]>
fs/file_table.c +5 -14
diff -Nru linux-2.6.13-rc7/fs/file_table.c linux-2.6.13-rc7-ed/fs/file_table.c
@@ -19,38 +19,28 @@
 #include <linux/fsnotify.h>
 
 /* sysctl tunables... */
-struct files_stat_struct files_stat = {
-	.max_files = NR_FILE
-};
+struct files_stat_struct files_stat;
 
 EXPORT_SYMBOL(files_stat); /* Needed by unix.o */
 
 /* public. Not pretty! */
  __cacheline_aligned_in_smp DEFINE_SPINLOCK(files_lock);
 
-static DEFINE_SPINLOCK(filp_count_lock);
 
 /* slab constructors and destructors are called from arbitrary
- * context and must be fully threaded - use a local spinlock
- * to protect files_stat.nr_files
+ * context and must be fully threaded
  */
 void filp_ctor(void * objp, struct kmem_cache_s *cachep, unsigned long cflags)
 {
 	if ((cflags & (SLAB_CTOR_VERIFY|SLAB_CTOR_CONSTRUCTOR)) ==
 	    SLAB_CTOR_CONSTRUCTOR) {
-		unsigned long flags;
-		spin_lock_irqsave(&filp_count_lock, flags);
-		files_stat.nr_files++;
-		spin_unlock_irqrestore(&filp_count_lock, flags);
+		files_stat.nr_files = atomic_add_return(1, &files_stat.atomic_nr_files);
 	}
 }
 
 void filp_dtor(void * objp, struct kmem_cache_s *cachep, unsigned long dflags)
 {
-	unsigned long flags;
-	spin_lock_irqsave(&filp_count_lock, flags);
-	files_stat.nr_files--;
-	spin_unlock_irqrestore(&filp_count_lock, flags);
+	files_stat.nr_files = atomic_sub_return(1, &files_stat.atomic_nr_files);
 }
 
 static inline void file_free(struct file *f)

@@ -258,4 +248,5 @@
 	files_stat.max_files = n; 
 	if (files_stat.max_files < NR_FILE)
 		files_stat.max_files = NR_FILE;
+	atomic_set(&files_stat.atomic_nr_files, 0);
 }
include/linux/fs.h +6 -2
diff -Nru linux-2.6.13-rc7/include/linux/fs.h linux-2.6.13-rc7-ed/include/linux/fs.h
@@ -9,6 +9,8 @@
 #include <linux/config.h>
 #include <linux/limits.h>
 #include <linux/ioctl.h>
+#include <linux/cache.h>
+#include <asm/atomic.h>
 
 /*
  * It's silly to have NR_OPEN bigger than NR_FILE, but you can change

@@ -30,10 +32,12 @@
 
 /* And dynamically-tunable limits and defaults: */
 struct files_stat_struct {
-	int nr_files;		/* read only */
+	int nr_files;		/* mirrors atomic_nr_files value */
 	int nr_free_files;	/* read only */
 	int max_files;		/* tunable */
-};
+
+	atomic_t atomic_nr_files;
+} ____cacheline_aligned;
 extern struct files_stat_struct files_stat;
 
 struct inodes_stat_t {
#11 lore raw

Re: [PATCH] removes filp_count_lock and changes nr_files type to atomic_t

Quoted 3 lines · Eric Dumazet

OK, here is a new clean patch that address this problem (nothing assumed about atomics)

Would you just be able to add the atomic sysctl handler that Christoph suggested?

This introduces lost update problems. 2 CPUs may store to nr_files in the opposite order that they incremented atomic_nr_files.

It is not terribly bad, because the drift is not cumulative and the field can't go negative... but its just ugly to add this hack because there is no atomic sysctl handler.

Eliminating the cli/sti is a good idea though, I think.

Signature · 5 lines
SUSE Labs, Novell Inc.



Send instant messages to your online friends http://au.messenger.yahoo.com
#12 lore raw

Re: [PATCH] removes filp_count_lock and changes nr_files type to atomic_t

Quoted 10 lines · Nick Piggin
Quoted 2 lines · Eric Dumazet

OK, here is a new clean patch that address this problem (nothing assumed about atomics)

Would you just be able to add the atomic sysctl handler that Christoph suggested?

Quite a lot of work indeed, and it would force to convert 3 int (nr_files, nr_free_files, max_files) to 3 atomic_t. I feel bad introducing a lot of sysctl rework for a tiny change (removing filp_count_lock)

Quoted 3 lines

This introduces lost update problems. 2 CPUs may store to nr_files in the opposite order that they incremented atomic_nr_files.

That's true, and the difference can be relatively important in case of preemption.

Each time the true and correct value (atomic_nr_files) is updated, a copy is done on nr_files : as nr_files is only used to be a guard value against too many file allocations, a somewhat 'lazy' value has no impact at all.

Quoted 6 lines

It is not terribly bad, because the drift is not cumulative and the field can't go negative... but its just ugly to add this hack because there is no atomic sysctl handler.

Eliminating the cli/sti is a good idea though, I think.

#13 lore raw

Re: [PATCH] removes filp_count_lock and changes nr_files type to atomic_t

Quoted 9 lines · Eric Dumazet
Quoted 3 lines · Nick Piggin

Would you just be able to add the atomic sysctl handler that Christoph suggested?

Quite a lot of work indeed, and it would force to convert 3 int (nr_files, nr_free_files, max_files) to 3 atomic_t. I feel bad introducing a lot of sysctl rework for a tiny change (removing filp_count_lock)

True, I didn't notice that.

Quoted 8 lines
Quoted 3 lines

This introduces lost update problems. 2 CPUs may store to nr_files in the opposite order that they incremented atomic_nr_files.

That's true, and the difference can be relatively important in case of preemption.

Each time the true and correct value (atomic_nr_files) is updated, a copy is done on nr_files : as nr_files is only used to be a guard value against too many file allocations, a somewhat 'lazy' value has no impact at all.

OK, well I would prefer you do the proper atomic operations throughout where it "really matters" in file_table.c, and do your lazy synchronize with just the sysctl exported value.

Unless the fs people had a problem with that.

And you may as well get rid of the atomic_inc_return which can be more expensive on some platforms and doesn't buy you much.
atomic_inc;
atomic_read;
Should be enough if you don't care about lost updates here, yeah?

Nick

Signature · 3 lines
SUSE Labs, Novell Inc.

Send instant messages to your online friends http://au.messenger.yahoo.com
#14 lore raw

Re: [PATCH] removes filp_count_lock and changes nr_files type to atomic_t

Quoted 8 lines · Nick Piggin
Quoted 7 lines · Eric Dumazet

Nick Piggin a ?crit :

Quoted 3 lines

Would you just be able to add the atomic sysctl handler that Christoph suggested?

Quoted 7 lines
Quoted 5 lines

Quite a lot of work indeed, and it would force to convert 3 int (nr_files, nr_free_files, max_files) to 3 atomic_t. I feel bad introducing a lot of sysctl rework for a tiny change (removing filp_count_lock)

True, I didn't notice that.

Well, it would with a generic atomic_t handler. With a special handler for this situation it's not needed.

#15 lore raw

Re: [PATCH] removes filp_count_lock and changes nr_files type to atomic_t

In reply to Nick Piggin
Quoted 4 lines · Nick Piggin

OK, well I would prefer you do the proper atomic operations throughout where it "really matters" in file_table.c, and do your lazy synchronize with just the sysctl exported value.

But... I got complains about atomic_read(&counter) being 'an atomic op' (untrue), so my second patch just doesnt touch the path where nr_files was read.

Furthermore, a lazy sync would mean to change sysctl proc_handler for "file-nr" to perform a synchronize before calling proc_dointvec, this would be really obscure.

Quoted 8 lines

Unless the fs people had a problem with that.

And you may as well get rid of the atomic_inc_return which can be more expensive on some platforms and doesn't buy you much.
atomic_inc;
atomic_read;
Should be enough if you don't care about lost updates here, yeah?

You mean :

atomic_inc(&counter);
lazeyvalue = atomic_read(&counter);

instead of

lazeyvalue = atomic_inc_return(&counter);

In fact I couldnt find one architecture where the later would be more expensive.

Quoted 2 lines

Nick

Eric

#16 lore raw

Re: [PATCH] removes filp_count_lock and changes nr_files type to atomic_t

Quoted 11 lines · Eric Dumazet
Quoted 4 lines · Nick Piggin

OK, well I would prefer you do the proper atomic operations throughout where it "really matters" in file_table.c, and do your lazy synchronize with just the sysctl exported value.

But... I got complains about atomic_read(&counter) being 'an atomic op' (untrue), so my second patch just doesnt touch the path where nr_files was read.

Here is a patch that I had done some time ago that uses atomic_t, yet retains the sysctl handler. Eric, you earlier patch is incorrect exactly for that reason.

One other thing - the claim that it removes filp_count_lock from fast path is bogus. The slab constructor/destructors are called only when we return the free file structs to the page allocator. That we don't do very often and therefore we don't acquire the lock - atleast not for every filp open and close.

This is not to say we don't want a better reference counter like a per-cpu counter, but there is some difficult stuff there and the returns need to justify that. I would appreciate if you or anyone can demonstrate this to be a problem.

The patch below was meant for debugging some suspected problems with -mm.

Thanks
Dipankar

This patch changes the file counting by removing the filp_count_lock. Instead we use a separate atomic_t, nr_files, for now and all accesses to it are through get_nr_files() api. In the sysctl handler for nr_files, we populate files_stat.nr_files before returning to user.

Counting files as an when they are created and destroyed (as opposed to inside slab) allows us to correctly count open files with RCU.

Signed-off-by Dipankar Sarma <[email protected]>

---

 fs/dcache.c                  |    2 -
 fs/file_table.c              |   71 ++++++++++++++++++++++++++-----------------
 fs/xfs/linux-2.6/xfs_linux.h |    3 +
 include/linux/file.h         |    2 -
 include/linux/fs.h           |    2 +
 kernel/sysctl.c              |    5 ++-
 net/unix/af_unix.c           |    2 -
 7 files changed, 54 insertions(+), 33 deletions(-)
fs/file_table.c renamed from fs/file_table.c~files-scale-file-counting +44 -27
diff -puN fs/file_table.c~files-scale-file-counting fs/file_table.c
@@ -5,6 +5,7 @@
  *  Copyright (C) 1997 David S. Miller ([email protected])
  */
 
+#include <linux/config.h>
 #include <linux/string.h>
 #include <linux/slab.h>
 #include <linux/file.h>

@@ -18,52 +19,67 @@
 #include <linux/mount.h>
 #include <linux/cdev.h>
 #include <linux/fsnotify.h>
+#include <linux/sysctl.h>
+#include <asm/atomic.h>
 
 /* sysctl tunables... */
 struct files_stat_struct files_stat = {
 	.max_files = NR_FILE
 };
 
-EXPORT_SYMBOL(files_stat); /* Needed by unix.o */
-
 /* public. Not pretty! */
  __cacheline_aligned_in_smp DEFINE_SPINLOCK(files_lock);
 
-static DEFINE_SPINLOCK(filp_count_lock);
+static atomic_t nr_files __cacheline_aligned_in_smp;
 
-/* slab constructors and destructors are called from arbitrary
- * context and must be fully threaded - use a local spinlock
- * to protect files_stat.nr_files
- */
-void filp_ctor(void * objp, struct kmem_cache_s *cachep, unsigned long cflags)
+static inline void file_free_rcu(struct rcu_head *head)
 {
-	if ((cflags & (SLAB_CTOR_VERIFY|SLAB_CTOR_CONSTRUCTOR)) ==
-	    SLAB_CTOR_CONSTRUCTOR) {
-		unsigned long flags;
-		spin_lock_irqsave(&filp_count_lock, flags);
-		files_stat.nr_files++;
-		spin_unlock_irqrestore(&filp_count_lock, flags);
-	}
+	struct file *f =  container_of(head, struct file, f_rcuhead);
+	kmem_cache_free(filp_cachep, f);
 }
 
-void filp_dtor(void * objp, struct kmem_cache_s *cachep, unsigned long dflags)
+static inline void file_free(struct file *f)
 {
-	unsigned long flags;
-	spin_lock_irqsave(&filp_count_lock, flags);
-	files_stat.nr_files--;
-	spin_unlock_irqrestore(&filp_count_lock, flags);
+	atomic_dec(&nr_files);
+	call_rcu(&f->f_rcuhead, file_free_rcu);
 }
 
-static inline void file_free_rcu(struct rcu_head *head)
+/*
+ * Return the total number of open files in the system
+ */
+int get_nr_files(void)
 {
-	struct file *f =  container_of(head, struct file, f_rcuhead);
-	kmem_cache_free(filp_cachep, f);
+	return atomic_read(&nr_files);
 }
 
-static inline void file_free(struct file *f)
+/*
+ * Return the maximum number of open files in the system
+ */
+int get_max_files(void)
 {
-	call_rcu(&f->f_rcuhead, file_free_rcu);
+	return files_stat.max_files;
+}
+
+EXPORT_SYMBOL(get_nr_files);
+EXPORT_SYMBOL(get_max_files);
+
+/*
+ * Handle nr_files sysctl
+ */
+#if defined(CONFIG_SYSCTL) && defined(CONFIG_PROC_FS)
+int proc_nr_files(ctl_table *table, int write, struct file *filp,
+                     void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+	files_stat.nr_files = get_nr_files();
+	proc_dointvec(table, write, filp, buffer, lenp, ppos);
+}
+#else
+int proc_nr_files(ctl_table *table, int write, struct file *filp,
+                     void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+	return -ENOSYS;
 }
+#endif
 
 /* Find an unused file structure and return a pointer to it.
  * Returns NULL, if there are no more free file structures or

@@ -77,7 +93,7 @@ struct file *get_empty_filp(void)
 	/*
 	 * Privileged users can go above max_files
 	 */
-	if (files_stat.nr_files >= files_stat.max_files &&
+	if (get_nr_files() >= files_stat.max_files &&
 				!capable(CAP_SYS_ADMIN))
 		goto over;
 

@@ -97,11 +113,12 @@ struct file *get_empty_filp(void)
 	/* f->f_version: 0 */
 	INIT_LIST_HEAD(&f->f_list);
 	f->f_maxcount = INT_MAX;
+	atomic_inc(&nr_files);
 	return f;
 
 over:
 	/* Ran out of filps - report that */
-	if (files_stat.nr_files > old_max) {
+	if (get_nr_files() > old_max) {
 		printk(KERN_INFO "VFS: file-max limit %d reached\n",
 					files_stat.max_files);
 		old_max = files_stat.nr_files;
include/linux/fs.h renamed from include/linux/fs.h~files-scale-file-counting +2 -0
diff -puN include/linux/fs.h~files-scale-file-counting include/linux/fs.h
@@ -36,6 +36,8 @@ struct files_stat_struct {
 	int max_files;		/* tunable */
 };
 extern struct files_stat_struct files_stat;
+extern int get_nr_files(void);
+extern int get_max_files(void);
 
 struct inodes_stat_t {
 	int nr_inodes;
fs/xfs/linux-2.6/xfs_linux.h renamed from fs/xfs/linux-2.6/xfs_linux.h~files-scale-file-counting +2 -1
diff -puN fs/xfs/linux-2.6/xfs_linux.h~files-scale-file-counting fs/xfs/linux-2.6/xfs_linux.h
@@ -88,6 +88,7 @@
 #include <linux/list.h>
 #include <linux/proc_fs.h>
 #include <linux/sort.h>
+#include <linux/fs.h>
 
 #include <asm/page.h>
 #include <asm/div64.h>

@@ -241,7 +242,7 @@ static inline void set_buffer_unwritten_
 
 /* IRIX uses the current size of the name cache to guess a good value */
 /* - this isn't the same but is a good enough starting point for now. */
-#define DQUOT_HASH_HEURISTIC	files_stat.nr_files
+#define DQUOT_HASH_HEURISTIC	get_nr_files()
 
 /* IRIX inodes maintain the project ID also, zero this field on Linux */
 #define DEFAULT_PROJID	0
net/unix/af_unix.c renamed from net/unix/af_unix.c~files-scale-file-counting +1 -1
diff -puN net/unix/af_unix.c~files-scale-file-counting net/unix/af_unix.c
@@ -547,7 +547,7 @@ static struct sock * unix_create1(struct
 	struct sock *sk = NULL;
 	struct unix_sock *u;
 
-	if (atomic_read(&unix_nr_socks) >= 2*files_stat.max_files)
+	if (atomic_read(&unix_nr_socks) >= 2*get_max_files())
 		goto out;
 
 	sk = sk_alloc(PF_UNIX, GFP_KERNEL, &unix_proto, 1);
fs/dcache.c renamed from fs/dcache.c~files-scale-file-counting +1 -1
diff -puN fs/dcache.c~files-scale-file-counting fs/dcache.c
@@ -1895,7 +1895,7 @@ void __init vfs_caches_init(unsigned lon
 			SLAB_HWCACHE_ALIGN|SLAB_PANIC, NULL, NULL);
 
 	filp_cachep = kmem_cache_create("filp", sizeof(struct file), 0,
-			SLAB_HWCACHE_ALIGN|SLAB_PANIC, filp_ctor, filp_dtor);
+			SLAB_HWCACHE_ALIGN|SLAB_PANIC, NULL, NULL);
 
 	dcache_init(mempages);
 	inode_init(mempages);
include/linux/file.h renamed from include/linux/file.h~files-scale-file-counting +0 -2
diff -puN include/linux/file.h~files-scale-file-counting include/linux/file.h
@@ -60,8 +60,6 @@ extern void put_filp(struct file *);
 extern int get_unused_fd(void);
 extern void FASTCALL(put_unused_fd(unsigned int fd));
 struct kmem_cache_s;
-extern void filp_ctor(void * objp, struct kmem_cache_s *cachep, unsigned long cflags);
-extern void filp_dtor(void * objp, struct kmem_cache_s *cachep, unsigned long dflags);
 
 extern struct file ** alloc_fd_array(int);
 extern void free_fd_array(struct file **, int);
kernel/sysctl.c renamed from kernel/sysctl.c~files-scale-file-counting +4 -1
diff -puN kernel/sysctl.c~files-scale-file-counting kernel/sysctl.c
@@ -49,6 +49,9 @@
 #include <linux/nfs_fs.h>
 #endif
 
+extern int proc_nr_files(ctl_table *table, int write, struct file *filp,
+                     void __user *buffer, size_t *lenp, loff_t *ppos);
+
 #if defined(CONFIG_SYSCTL)
 
 /* External variables not in a header file. */

@@ -881,7 +884,7 @@ static ctl_table fs_table[] = {
 		.data		= &files_stat,
 		.maxlen		= 3*sizeof(int),
 		.mode		= 0444,
-		.proc_handler	= &proc_dointvec,
+		.proc_handler	= &proc_nr_files,
 	},
 	{
 		.ctl_name	= FS_MAXFILE,

_

#17 lore raw

Re: [PATCH] removes filp_count_lock and changes nr_files type to atomic_t

Quoted 24 lines · Dipankar Sarma
Quoted 11 lines · Eric Dumazet

Nick Piggin a ?crit :

Quoted 4 lines

OK, well I would prefer you do the proper atomic operations throughout where it "really matters" in file_table.c, and do your lazy synchronize with just the sysctl exported value.

But... I got complains about atomic_read(&counter) being 'an atomic op' (untrue), so my second patch just doesnt touch the path where nr_files was read.

Here is a patch that I had done some time ago that uses atomic_t, yet retains the sysctl handler. Eric, you earlier patch is incorrect exactly for that reason.

One other thing - the claim that it removes filp_count_lock from fast path is bogus. The slab constructor/destructors are called only when we return the free file structs to the page allocator. That we don't do very often and therefore we don't acquire the lock - atleast not for every filp open and close.

Quoted 7 lines

This is not to say we don't want a better reference counter like a per-cpu counter, but there is some difficult stuff there and the returns need to justify that. I would appreciate if you or anyone can demonstrate this to be a problem.

The patch below was meant for debugging some suspected problems with -mm.

As mentioned when we last discussed it the nr_files usage in XFS is boguss and should go away first so we don't need the accessor and export. Hopefully we can get rid of the max_files usage in af_unix aswell, can you ping davem on it?

#18 lore raw

Re: [PATCH] removes filp_count_lock and changes nr_files type to atomic_t

In reply to Eric Dumazet
Quoted 2 lines · Eric Dumazet

Furthermore, a lazy sync would mean to change sysctl proc_handler for "file-nr" to perform a synchronize before calling proc_dointvec, this would be really obscure.

I was only using your terminology (ie. the 'lazy' synch after the atomic is updated).

Actually, a better idea would be to make a specific sysctl handler like Christoph said.

Unless you can show some improvement, it would better not to introduce the racy hack (even if it is mostly harmless).

Quoted 9 lines
Quoted 8 lines

Unless the fs people had a problem with that.

And you may as well get rid of the atomic_inc_return which can be more expensive on some platforms and doesn't buy you much.
atomic_inc;
atomic_read;
Should be enough if you don't care about lost updates here, yeah?

Quoted 9 lines

You mean :

atomic_inc(&counter);
lazeyvalue = atomic_read(&counter);

instead of

lazeyvalue = atomic_inc_return(&counter);

Yes.

Quoted 2 lines

In fact I couldnt find one architecture where the later would be more expensive.

atomic_inc_return guarantees a memory barrier, while the former statements do not. I'm fairly sure it will be more expensive on a POWER5.

Signature · 3 lines
SUSE Labs, Novell Inc.

Send instant messages to your online friends http://au.messenger.yahoo.com