[PATCH net-next v3 0/2] socket statistics for ss

[PATCH net-next v3 0/2] socket statistics for ss

[Stephen Hemminger]

The eventual goal is to remove all accesses to slab statistics
by iproute2 ss command. This are preliminary steps to add two
statistics which are not working now, because of slab merging
and changes that were done years ago to allow for common inet_hash
tables.

Stephen Hemminger (2):
  inet: add bound ports statistic
  socket: keep track of the number of sockets allocated

v3
  - fix build :-(
  - add allocated sockets statistic

 include/net/inet_hashtables.h    |  3 +++
 include/net/inet_timewait_sock.h |  2 ++
 net/dccp/proto.c                 |  1 +
 net/ipv4/inet_hashtables.c       | 22 +++++++++++++++++++---
 net/ipv4/inet_timewait_sock.c    |  8 +++++---
 net/ipv4/proc.c                  |  5 +++--
 net/ipv4/tcp.c                   |  1 +
 net/socket.c                     | 21 +++++++++++++++++++--
 8 files changed, 53 insertions(+), 10 deletions(-)

-- 
2.17.0

[PATCH net-next v3 1/2] inet: add bound ports statistic

[Stephen Hemminger]

This adds a number of bound ports which fixes socket summary
command.  The ss -s has been broken since changes to slab info
and this is one way to recover the missing value by adding a
field onto /proc/net/sockstat.

Since this is an informational value only, there is no need
for locking.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 include/net/inet_hashtables.h    |  3 +++
 include/net/inet_timewait_sock.h |  2 ++
 net/dccp/proto.c                 |  1 +
 net/ipv4/inet_hashtables.c       | 22 +++++++++++++++++++---
 net/ipv4/inet_timewait_sock.c    |  8 +++++---
 net/ipv4/proc.c                  |  5 +++--
 net/ipv4/tcp.c                   |  1 +
 7 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/include/net/inet_hashtables.h b/include/net/inet_hashtables.h
index 9141e95529e7..b02524e2571e 100644
--- a/include/net/inet_hashtables.h
+++ b/include/net/inet_hashtables.h
@@ -103,6 +103,7 @@ static inline struct net *ib_net(struct inet_bind_bucket *ib)
 
 struct inet_bind_hashbucket {
 	spinlock_t		lock;
+	unsigned int		count;
 	struct hlist_head	chain;
 };
 
@@ -193,7 +194,9 @@ inet_bind_bucket_create(struct kmem_cache *cachep, struct net *net,
 			struct inet_bind_hashbucket *head,
 			const unsigned short snum);
 void inet_bind_bucket_destroy(struct kmem_cache *cachep,
+			      struct inet_bind_hashbucket *head,
 			      struct inet_bind_bucket *tb);
+unsigned int inet_bind_bucket_count(const struct proto *prot);
 
 static inline u32 inet_bhashfn(const struct net *net, const __u16 lport,
 			       const u32 bhash_size)
diff --git a/include/net/inet_timewait_sock.h b/include/net/inet_timewait_sock.h
index c7be1ca8e562..4cdb8034ad80 100644
--- a/include/net/inet_timewait_sock.h
+++ b/include/net/inet_timewait_sock.h
@@ -87,7 +87,9 @@ static inline struct inet_timewait_sock *inet_twsk(const struct sock *sk)
 void inet_twsk_free(struct inet_timewait_sock *tw);
 void inet_twsk_put(struct inet_timewait_sock *tw);
 
+struct inet_bind_hashbucket;
 void inet_twsk_bind_unhash(struct inet_timewait_sock *tw,
+			   struct inet_bind_hashbucket *head,
 			   struct inet_hashinfo *hashinfo);
 
 struct inet_timewait_sock *inet_twsk_alloc(const struct sock *sk,
diff --git a/net/dccp/proto.c b/net/dccp/proto.c
index 84cd4e3fd01b..25f03e62cfea 100644
--- a/net/dccp/proto.c
+++ b/net/dccp/proto.c
@@ -1208,6 +1208,7 @@ static int __init dccp_init(void)
 	for (i = 0; i < dccp_hashinfo.bhash_size; i++) {
 		spin_lock_init(&dccp_hashinfo.bhash[i].lock);
 		INIT_HLIST_HEAD(&dccp_hashinfo.bhash[i].chain);
+		dccp_hashinfo.bhash[i].count = 0;
 	}
 
 	rc = dccp_mib_init();
diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index 31ff46daae97..8ba6b17d95d5 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -58,6 +58,18 @@ static u32 sk_ehashfn(const struct sock *sk)
 			    sk->sk_daddr, sk->sk_dport);
 }
 
+/* Count how many any entries are in the bind hash table */
+unsigned int inet_bind_bucket_count(const struct proto *prot)
+{
+	const struct inet_hashinfo *hinfo = prot->h.hashinfo;
+	unsigned int i, ports = 0;
+
+	for (i = 0; i < hinfo->bhash_size; i++)
+		ports += hinfo->bhash[i].count;
+
+	return ports;
+}
+
 /*
  * Allocate and initialize a new local port bind bucket.
  * The bindhash mutex for snum's hash chain must be held here.
@@ -76,6 +88,7 @@ struct inet_bind_bucket *inet_bind_bucket_create(struct kmem_cache *cachep,
 		tb->fastreuseport = 0;
 		INIT_HLIST_HEAD(&tb->owners);
 		hlist_add_head(&tb->node, &head->chain);
+		++head->count;
 	}
 	return tb;
 }
@@ -83,10 +96,13 @@ struct inet_bind_bucket *inet_bind_bucket_create(struct kmem_cache *cachep,
 /*
  * Caller must hold hashbucket lock for this tb with local BH disabled
  */
-void inet_bind_bucket_destroy(struct kmem_cache *cachep, struct inet_bind_bucket *tb)
+void inet_bind_bucket_destroy(struct kmem_cache *cachep,
+			      struct inet_bind_hashbucket *head,
+			      struct inet_bind_bucket *tb)
 {
 	if (hlist_empty(&tb->owners)) {
 		__hlist_del(&tb->node);
+		--head->count;
 		kmem_cache_free(cachep, tb);
 	}
 }
@@ -115,7 +131,7 @@ static void __inet_put_port(struct sock *sk)
 	__sk_del_bind_node(sk);
 	inet_csk(sk)->icsk_bind_hash = NULL;
 	inet_sk(sk)->inet_num = 0;
-	inet_bind_bucket_destroy(hashinfo->bind_bucket_cachep, tb);
+	inet_bind_bucket_destroy(hashinfo->bind_bucket_cachep, head, tb);
 	spin_unlock(&head->lock);
 }
 
@@ -756,7 +772,7 @@ int __inet_hash_connect(struct inet_timewait_death_row *death_row,
 		inet_ehash_nolisten(sk, (struct sock *)tw);
 	}
 	if (tw)
-		inet_twsk_bind_unhash(tw, hinfo);
+		inet_twsk_bind_unhash(tw, head, hinfo);
 	spin_unlock(&head->lock);
 	if (tw)
 		inet_twsk_deschedule_put(tw);
diff --git a/net/ipv4/inet_timewait_sock.c b/net/ipv4/inet_timewait_sock.c
index 88c5069b5d20..dd888c52f958 100644
--- a/net/ipv4/inet_timewait_sock.c
+++ b/net/ipv4/inet_timewait_sock.c
@@ -26,7 +26,8 @@
  *	Returns 1 if caller should call inet_twsk_put() after lock release.
  */
 void inet_twsk_bind_unhash(struct inet_timewait_sock *tw,
-			  struct inet_hashinfo *hashinfo)
+			   struct inet_bind_hashbucket *head,
+			   struct inet_hashinfo *hashinfo)
 {
 	struct inet_bind_bucket *tb = tw->tw_tb;
 
@@ -35,7 +36,8 @@ void inet_twsk_bind_unhash(struct inet_timewait_sock *tw,
 
 	__hlist_del(&tw->tw_bind_node);
 	tw->tw_tb = NULL;
-	inet_bind_bucket_destroy(hashinfo->bind_bucket_cachep, tb);
+	inet_bind_bucket_destroy(hashinfo->bind_bucket_cachep,
+				 head, tb);
 	__sock_put((struct sock *)tw);
 }
 
@@ -55,7 +57,7 @@ static void inet_twsk_kill(struct inet_timewait_sock *tw)
 			hashinfo->bhash_size)];
 
 	spin_lock(&bhead->lock);
-	inet_twsk_bind_unhash(tw, hashinfo);
+	inet_twsk_bind_unhash(tw, bhead, hashinfo);
 	spin_unlock(&bhead->lock);
 
 	atomic_dec(&tw->tw_dr->tw_count);
diff --git a/net/ipv4/proc.c b/net/ipv4/proc.c
index 261b71d0ccc5..83bc9a0f2785 100644
--- a/net/ipv4/proc.c
+++ b/net/ipv4/proc.c
@@ -60,10 +60,11 @@ static int sockstat_seq_show(struct seq_file *seq, void *v)
 	sockets = proto_sockets_allocated_sum_positive(&tcp_prot);
 
 	socket_seq_show(seq);
-	seq_printf(seq, "TCP: inuse %d orphan %d tw %d alloc %d mem %ld\n",
+	seq_printf(seq, "TCP: inuse %d orphan %d tw %d alloc %d mem %ld ports %u\n",
 		   sock_prot_inuse_get(net, &tcp_prot), orphans,
 		   atomic_read(&net->ipv4.tcp_death_row.tw_count), sockets,
-		   proto_memory_allocated(&tcp_prot));
+		   proto_memory_allocated(&tcp_prot),
+		   inet_bind_bucket_count(&tcp_prot));
 	seq_printf(seq, "UDP: inuse %d mem %ld\n",
 		   sock_prot_inuse_get(net, &udp_prot),
 		   proto_memory_allocated(&udp_prot));
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 868ed74a76a8..f62e2fb02fdf 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -3836,6 +3836,7 @@ void __init tcp_init(void)
 	for (i = 0; i < tcp_hashinfo.bhash_size; i++) {
 		spin_lock_init(&tcp_hashinfo.bhash[i].lock);
 		INIT_HLIST_HEAD(&tcp_hashinfo.bhash[i].chain);
+		tcp_hashinfo.bhash[i].count = 0;
 	}
 
 
-- 
2.17.0

[PATCH net-next v3 2/2] socket: keep track of the number of sockets allocated

[Stephen Hemminger]

Add a per-cpu counter to keep track of the number of inodes allocated
to sockets to fix incorrect statistics from ss command.

The ss command tries to keep track of the number of sockets
allocated but it was doing by the slabinfo statistics which are
wrong (due to merging) and not available when using slub.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 net/socket.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index f10f1d947c78..89ec7f41559d 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -234,6 +234,18 @@ static int move_addr_to_user(struct sockaddr_storage *kaddr, int klen,
 }
 
 static struct kmem_cache *sock_inode_cachep __ro_after_init;
+static unsigned int __percpu *sock_pcpu_allocated;
+
+static unsigned int sock_allocated(void)
+{
+	unsigned int res = 0;
+	int cpu;
+
+	for_each_possible_cpu(cpu)
+		res += *per_cpu_ptr(sock_pcpu_allocated, cpu);
+
+	return res;
+}
 
 static struct inode *sock_alloc_inode(struct super_block *sb)
 {
@@ -248,6 +260,7 @@ static struct inode *sock_alloc_inode(struct super_block *sb)
 		kmem_cache_free(sock_inode_cachep, ei);
 		return NULL;
 	}
+	this_cpu_inc(*sock_pcpu_allocated);
 	init_waitqueue_head(&wq->wait);
 	wq->fasync_list = NULL;
 	wq->flags = 0;
@@ -270,6 +283,7 @@ static void sock_destroy_inode(struct inode *inode)
 	ei = container_of(inode, struct socket_alloc, vfs_inode);
 	wq = rcu_dereference_protected(ei->socket.wq, 1);
 	kfree_rcu(wq, rcu);
+	this_cpu_dec(*sock_pcpu_allocated);
 	kmem_cache_free(sock_inode_cachep, ei);
 }
 
@@ -290,6 +304,8 @@ static void init_inodecache(void)
 					       SLAB_MEM_SPREAD | SLAB_ACCOUNT),
 					      init_once);
 	BUG_ON(sock_inode_cachep == NULL);
+	sock_pcpu_allocated = alloc_percpu(unsigned int);
+	BUG_ON(sock_pcpu_allocated == NULL);
 }
 
 static const struct super_operations sockfs_ops = {
@@ -2738,8 +2754,9 @@ core_initcall(sock_init);	/* early initcall */
 #ifdef CONFIG_PROC_FS
 void socket_seq_show(struct seq_file *seq)
 {
-	seq_printf(seq, "sockets: used %d\n",
-		   sock_inuse_get(seq->private));
+	seq_printf(seq, "sockets: used %d allocated %u\n",
+		   sock_inuse_get(seq->private),
+		   sock_allocated());
 }
 #endif				/* CONFIG_PROC_FS */
 
-- 
2.17.0

Re: [PATCH net-next v3 0/2] socket statistics for ss

[Stephen Hemminger]

On Mon,  7 May 2018 11:43:31 -0700
Stephen Hemminger <stephen@networkplumber.org> wrote:

> The eventual goal is to remove all accesses to slab statistics
> by iproute2 ss command. This are preliminary steps to add two
> statistics which are not working now, because of slab merging
> and changes that were done years ago to allow for common inet_hash
> tables.
> 
> Stephen Hemminger (2):
>   inet: add bound ports statistic
>   socket: keep track of the number of sockets allocated
> 
> v3
>   - fix build :-(
>   - add allocated sockets statistic
> 
>  include/net/inet_hashtables.h    |  3 +++
>  include/net/inet_timewait_sock.h |  2 ++
>  net/dccp/proto.c                 |  1 +
>  net/ipv4/inet_hashtables.c       | 22 +++++++++++++++++++---
>  net/ipv4/inet_timewait_sock.c    |  8 +++++---
>  net/ipv4/proc.c                  |  5 +++--
>  net/ipv4/tcp.c                   |  1 +
>  net/socket.c                     | 21 +++++++++++++++++++--
>  8 files changed, 53 insertions(+), 10 deletions(-)
> 

I am not sure if these patches are worth applying.
The 'ss -s' command has had missing values since 2.4 kernel.
And the first complaints came in only this year.

Another alternative would be just to remove these fields from ss -s
output and move on.

Re: [PATCH net-next v3 0/2] socket statistics for ss

[Eric Dumazet]

On 05/09/2018 08:22 AM, Stephen Hemminger wrote:

> I am not sure if these patches are worth applying.
> The 'ss -s' command has had missing values since 2.4 kernel.
> And the first complaints came in only this year.
> 
> Another alternative would be just to remove these fields from ss -s
> output and move on.
> 

Anyway your patches are not netns ready, so lets remove these fields from ss.

Or you have to spend _much_ more time on writing and testing the kernel part.

Thanks.

Re: [PATCH net-next v3 0/2] socket statistics for ss

[Stephen Hemminger]

On Wed, 9 May 2018 10:18:23 -0700
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> On 05/09/2018 08:22 AM, Stephen Hemminger wrote:
> 
> > I am not sure if these patches are worth applying.
> > The 'ss -s' command has had missing values since 2.4 kernel.
> > And the first complaints came in only this year.
> > 
> > Another alternative would be just to remove these fields from ss -s
> > output and move on.
> >   
> 
> Anyway your patches are not netns ready, so lets remove these fields from ss.
> 
> Or you have to spend _much_ more time on writing and testing the kernel part.
> 
> Thanks.

The patches only expose the existing TCP socket accounting infrastructure.
Several other pieces that sockstat has are not netns aware.
That is a completely different problem.

Re: [PATCH net-next v3 0/2] socket statistics for ss

[Eric Dumazet]

On 05/09/2018 10:31 AM, Stephen Hemminger wrote:
> On Wed, 9 May 2018 10:18:23 -0700
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 
>> On 05/09/2018 08:22 AM, Stephen Hemminger wrote:
>>
>>> I am not sure if these patches are worth applying.
>>> The 'ss -s' command has had missing values since 2.4 kernel.
>>> And the first complaints came in only this year.
>>>
>>> Another alternative would be just to remove these fields from ss -s
>>> output and move on.
>>>   
>>
>> Anyway your patches are not netns ready, so lets remove these fields from ss.
>>
>> Or you have to spend _much_ more time on writing and testing the kernel part.
>>
>> Thanks.
> 
> The patches only expose the existing TCP socket accounting infrastructure.
> Several other pieces that sockstat has are not netns aware.
> That is a completely different problem.


Adding a new field counting 'bounds ports' without being netns ready is a total mistake,
as it is useless by current standards.

The first thing that users will do is add proper netns support, with extra complexity in the kernel.

So, instead of pushing some incomplete feature, trying to fool ourselves with a sentiment of 'small cost'
that will later need another 100 lines of code in the kernel, please give us the complete picture.

I am just saying, you can of course ignore my feedback.

Re: [PATCH net-next v3 0/2] socket statistics for ss

[Stephen Hemminger]

On Wed, 9 May 2018 10:53:58 -0700
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> On 05/09/2018 10:31 AM, Stephen Hemminger wrote:
> > On Wed, 9 May 2018 10:18:23 -0700
> > Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >   
> >> On 05/09/2018 08:22 AM, Stephen Hemminger wrote:
> >>  
> >>> I am not sure if these patches are worth applying.
> >>> The 'ss -s' command has had missing values since 2.4 kernel.
> >>> And the first complaints came in only this year.
> >>>
> >>> Another alternative would be just to remove these fields from ss -s
> >>> output and move on.
> >>>     
> >>
> >> Anyway your patches are not netns ready, so lets remove these fields from ss.
> >>
> >> Or you have to spend _much_ more time on writing and testing the kernel part.
> >>
> >> Thanks.  
> > 
> > The patches only expose the existing TCP socket accounting infrastructure.
> > Several other pieces that sockstat has are not netns aware.
> > That is a completely different problem.  
> 
> 
> Adding a new field counting 'bounds ports' without being netns ready is a total mistake,
> as it is useless by current standards.
> 
> The first thing that users will do is add proper netns support, with extra complexity in the kernel.
> 
> So, instead of pushing some incomplete feature, trying to fool ourselves with a sentiment of 'small cost'
> that will later need another 100 lines of code in the kernel, please give us the complete picture.
> 
> I am just saying, you can of course ignore my feedback.

The current TCP hashinfo should be moved into netns. The current method of scanning and matching
by net namespace is a scalability issue now.

Re: [PATCH net-next v3 0/2] socket statistics for ss

[Eric Dumazet]

On 05/09/2018 11:44 AM, Stephen Hemminger wrote:
> On Wed, 9 May 2018 10:53:58 -0700
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 
>> On 05/09/2018 10:31 AM, Stephen Hemminger wrote:
>>> On Wed, 9 May 2018 10:18:23 -0700
>>> Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>>   
>>>> On 05/09/2018 08:22 AM, Stephen Hemminger wrote:
>>>>  
>>>>> I am not sure if these patches are worth applying.
>>>>> The 'ss -s' command has had missing values since 2.4 kernel.
>>>>> And the first complaints came in only this year.
>>>>>
>>>>> Another alternative would be just to remove these fields from ss -s
>>>>> output and move on.
>>>>>     
>>>>
>>>> Anyway your patches are not netns ready, so lets remove these fields from ss.
>>>>
>>>> Or you have to spend _much_ more time on writing and testing the kernel part.
>>>>
>>>> Thanks.  
>>>
>>> The patches only expose the existing TCP socket accounting infrastructure.
>>> Several other pieces that sockstat has are not netns aware.
>>> That is a completely different problem.  
>>
>>
>> Adding a new field counting 'bounds ports' without being netns ready is a total mistake,
>> as it is useless by current standards.
>>
>> The first thing that users will do is add proper netns support, with extra complexity in the kernel.
>>
>> So, instead of pushing some incomplete feature, trying to fool ourselves with a sentiment of 'small cost'
>> that will later need another 100 lines of code in the kernel, please give us the complete picture.
>>
>> I am just saying, you can of course ignore my feedback.
> 
> The current TCP hashinfo should be moved into netns. The current method of scanning and matching
> by net namespace is a scalability issue now.

It is not the plan yet, and we have no scalability issue.

Before switching to netns hash table, this would need rhashtable conversions
but so far this has not been done.

- Time to create/delete netns is critical.
- Adding few Mbytes of overhead per netns is a nogo,

Please do not change subject, this is adding noise to this particular thread.

Re: [PATCH net-next v3 0/2] socket statistics for ss

[Stephen Hemminger]

On Wed, 9 May 2018 11:53:50 -0700
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> On 05/09/2018 11:44 AM, Stephen Hemminger wrote:
> > On Wed, 9 May 2018 10:53:58 -0700
> > Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >   
> >> On 05/09/2018 10:31 AM, Stephen Hemminger wrote:  
> >>> On Wed, 9 May 2018 10:18:23 -0700
> >>> Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >>>     
> >>>> On 05/09/2018 08:22 AM, Stephen Hemminger wrote:
> >>>>    
> >>>>> I am not sure if these patches are worth applying.
> >>>>> The 'ss -s' command has had missing values since 2.4 kernel.
> >>>>> And the first complaints came in only this year.
> >>>>>
> >>>>> Another alternative would be just to remove these fields from ss -s
> >>>>> output and move on.
> >>>>>       
> >>>>
> >>>> Anyway your patches are not netns ready, so lets remove these fields from ss.
> >>>>
> >>>> Or you have to spend _much_ more time on writing and testing the kernel part.
> >>>>
> >>>> Thanks.    
> >>>
> >>> The patches only expose the existing TCP socket accounting infrastructure.
> >>> Several other pieces that sockstat has are not netns aware.
> >>> That is a completely different problem.    
> >>
> >>
> >> Adding a new field counting 'bounds ports' without being netns ready is a total mistake,
> >> as it is useless by current standards.
> >>
> >> The first thing that users will do is add proper netns support, with extra complexity in the kernel.
> >>
> >> So, instead of pushing some incomplete feature, trying to fool ourselves with a sentiment of 'small cost'
> >> that will later need another 100 lines of code in the kernel, please give us the complete picture.
> >>
> >> I am just saying, you can of course ignore my feedback.  
> > 
> > The current TCP hashinfo should be moved into netns. The current method of scanning and matching
> > by net namespace is a scalability issue now.  
> 
> It is not the plan yet, and we have no scalability issue.
> 
> Before switching to netns hash table, this would need rhashtable conversions
> but so far this has not been done.
> 
> - Time to create/delete netns is critical.
> - Adding few Mbytes of overhead per netns is a nogo,
> 
> Please do not change subject, this is adding noise to this particular thread.

Back to original subject. My current intention is to just pull all these statistics
from ss command. They are always zero now, and very few people noticed and  no one
really needs them.