Skip to contentSkip to conversation

Convert sockets_in_use to use per_cpu areas

PATCH In review Ravikiran G Thirumalai 3 participants 7 messages Dec 23, 2002 – Jan 7, 2003

Subsystems (3)
Networking Network drivers Sockets
lore
Share & tools
lore

Ravikiran G Thirumalai

author
#0 lore raw

PATCH 1/1 Convert sockets_in_use to use per_cpu areas

Hi,
Here's a simple patch to change sockets_in_use to make use of per-cpu areas.

I have couple of questions though...
1. Was this var made per-cpu just to avoid atomic_t or locking or are there real life workloads which cause too many sock_alloc and sock_releases to cause cacheline bouncing?
2. Is this var required? since we can just sum up all proto->stats.inuse and remove this var altogether? (This var is read only for /proc reporting)

Thanks,
Kiran

net/socket.c +6 -7
diff -ruN -X dontdiff linux-2.5.52/net/socket.c sockets_in_use-2.5.52/net/socket.c
@@ -189,10 +189,7 @@
  *	Statistics counters of the socket lists
  */
 
-static union {
-	int	counter;
-	char	__pad[SMP_CACHE_BYTES];
-} sockets_in_use[NR_CPUS] __cacheline_aligned = {{0}};
+static DEFINE_PER_CPU(int, sockets_in_use);
 
 /*
  *	Support routines. Move socket addresses back and forth across the kernel/user

@@ -475,7 +472,8 @@
 	inode->i_uid = current->fsuid;
 	inode->i_gid = current->fsgid;
 
-	sockets_in_use[smp_processor_id()].counter++;
+	get_cpu_var(sockets_in_use)++;
+	put_cpu_var(sockets_in_use);
 	return sock;
 }
 

@@ -511,7 +509,8 @@
 	if (sock->fasync_list)
 		printk(KERN_ERR "sock_release: fasync list not empty!\n");
 
-	sockets_in_use[smp_processor_id()].counter--;
+	get_cpu_var(sockets_in_use)--;
+	put_cpu_var(sockets_in_use);
 	if (!sock->file) {
 		iput(SOCK_INODE(sock));
 		return;

@@ -1851,7 +1850,7 @@
 	int counter = 0;
 
 	for (cpu = 0; cpu < NR_CPUS; cpu++)
-		counter += sockets_in_use[cpu].counter;
+		counter += per_cpu(sockets_in_use, cpu);
 
 	/* It can be negative, by the way. 8) */
 	if (counter < 0)
#1 lore raw

From: Ravikiran G Thirumalai <[email protected]>
Date: Mon, 23 Dec 2002 19:08:48 +0530

   -static union {
   -	int	counter;
   -	char	__pad[SMP_CACHE_BYTES];
   -} sockets_in_use[NR_CPUS] __cacheline_aligned = {{0}};
   +static DEFINE_PER_CPU(int, sockets_in_use);

You have to provide an explicit initializer for DEFINE_PER_CPU declarations or you break some platforms with older GCC's which otherwise won't put it into the proper section.

#2 lore raw
Quoted 3 lines · David S. Miller

You have to provide an explicit initializer for DEFINE_PER_CPU declarations or you break some platforms with older GCC's which otherwise won't put it into the proper section.

I wonder if "some platforms with older GCC's" will ever have these issues resolved...

--cw

#3 lore raw

From: Chris Wedgwood <[email protected]>
Date: Mon, 23 Dec 2002 16:00:48 -0800

Quoted 3 lines · David S. Miller

You have to provide an explicit initializer for DEFINE_PER_CPU declarations or you break some platforms with older GCC's which otherwise won't put it into the proper section.

I wonder if "some platforms with older GCC's" will ever have these issues resolved...

I still don't have gcc-3.2.1 working properly on sparc64.

I hope to have it working soon, but this does mean that 2.6.x cannot deprecate it, whereas 2.7.x certainly can.

author
#4 lore raw
In reply to David S. Miller
Quoted 11 lines · David S. Miller

...

   -static union {
   -	int	counter;
   -	char	__pad[SMP_CACHE_BYTES];
   -} sockets_in_use[NR_CPUS] __cacheline_aligned = {{0}};
   +static DEFINE_PER_CPU(int, sockets_in_use);

You have to provide an explicit initializer for DEFINE_PER_CPU declarations or you break some platforms with older GCC's which otherwise won't put it into the proper section.

Ok, here's the modified patch...

Thanks,
Kiran

net/socket.c +6 -7
diff -ruN -X dontdiff linux-2.5.52/net/socket.c sockets_in_use-2.5.52/net/socket.c
@@ -189,10 +189,7 @@
  *	Statistics counters of the socket lists
  */
 
-static union {
-	int	counter;
-	char	__pad[SMP_CACHE_BYTES];
-} sockets_in_use[NR_CPUS] __cacheline_aligned = {{0}};
+static DEFINE_PER_CPU(int, sockets_in_use) = 0;
 
 /*
  *	Support routines. Move socket addresses back and forth across the kernel/user

@@ -475,7 +472,8 @@
 	inode->i_uid = current->fsuid;
 	inode->i_gid = current->fsgid;
 
-	sockets_in_use[smp_processor_id()].counter++;
+	get_cpu_var(sockets_in_use)++;
+	put_cpu_var(sockets_in_use);
 	return sock;
 }
 

@@ -511,7 +509,8 @@
 	if (sock->fasync_list)
 		printk(KERN_ERR "sock_release: fasync list not empty!\n");
 
-	sockets_in_use[smp_processor_id()].counter--;
+	get_cpu_var(sockets_in_use)--;
+	put_cpu_var(sockets_in_use);
 	if (!sock->file) {
 		iput(SOCK_INODE(sock));
 		return;

@@ -1851,7 +1850,7 @@
 	int counter = 0;
 
 	for (cpu = 0; cpu < NR_CPUS; cpu++)
-		counter += sockets_in_use[cpu].counter;
+		counter += per_cpu(sockets_in_use, cpu);
 
 	/* It can be negative, by the way. 8) */
 	if (counter < 0)
#5 lore raw

From: Ravikiran G Thirumalai <[email protected]>
Date: Tue, 24 Dec 2002 12:18:52 +0530

Ok, here's the modified patch...

Ok I'll apply this when I get back from vacation in the new year.