[PATCH 0/4] Make tcp-metrics source-address aware

[PATCH 0/4] Make tcp-metrics source-address aware

[Christoph Paasch]

Currently tcp-metrics only stores per-destination addresses. This brings
problems, when a host has multiple interfaces (e.g., a smartphone having
WiFi/3G):

For example, a host contacting a server over WiFi will store the tcp-metrics
per destination IP. If then the host contacts the same server over 3G, the 
same tcp-metrics will be used, although the path-characteristics are completly
different (e.g., the ssthresh is probably not the same).

The same holds for the fast-open cookie. The server will generate a cookie
based on our source-address. So, if we contact the same server with another
source-IP we should request a new cookie.

This patchset makes tcp-metrics source-address aware (Patch 1 and 2).

Patches 3 and 4 handle the interface to "ip tcp_metrics". When deleting a
metric based on the destination-IP, all entries will be removed (Patch 3).
Further, a new attribute is added, so that the source-IP can be exposed to
"ip tcp_metrics" (Patch 4).

Note:
"ip tcp_metrics show ADDRESS" will only display the first element among
the entries for the specified destination-IP. It probably would be better to
show all entries for the specified IP. However, I do not see how to achieve 
this without fundamentally changing the netlink-API for the tcp-metrics.

Suggestions are very welcome.


Christoph Paasch (4):
  tcp: metrics: rename tcpm_addr to tcpm_daddr
  tcp: metrics: Add source-address to tcp-metrics
  tcp: metrics: Delete all entries matching a certain destination
  tcp: metrics: Dump info of the source-address in netlink-reply

 include/uapi/linux/tcp_metrics.h |   2 +
 net/ipv4/tcp_metrics.c           | 111 ++++++++++++++++++++++++---------------
 2 files changed, 71 insertions(+), 42 deletions(-)

-- 
1.8.3.2

[PATCH 1/4] tcp: metrics: rename tcpm_addr to tcpm_daddr

[Christoph Paasch]

As we will add also the source-address, we rename all accesses to the
tcp-metrics address to use "daddr".

Signed-off-by: Christoph Paasch <christoph.paasch@uclouvain.be>
---
 net/ipv4/tcp_metrics.c | 72 +++++++++++++++++++++++++-------------------------
 1 file changed, 36 insertions(+), 36 deletions(-)

diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
index 06493736fbc8..f9b5f519a4ea 100644
--- a/net/ipv4/tcp_metrics.c
+++ b/net/ipv4/tcp_metrics.c
@@ -31,7 +31,7 @@ struct tcp_fastopen_metrics {
 
 struct tcp_metrics_block {
 	struct tcp_metrics_block __rcu	*tcpm_next;
-	struct inetpeer_addr		tcpm_addr;
+	struct inetpeer_addr		tcpm_daddr;
 	unsigned long			tcpm_stamp;
 	u32				tcpm_ts;
 	u32				tcpm_ts_stamp;
@@ -131,7 +131,7 @@ static void tcpm_suck_dst(struct tcp_metrics_block *tm, struct dst_entry *dst,
 }
 
 static struct tcp_metrics_block *tcpm_new(struct dst_entry *dst,
-					  struct inetpeer_addr *addr,
+					  struct inetpeer_addr *daddr,
 					  unsigned int hash,
 					  bool reclaim)
 {
@@ -155,7 +155,7 @@ static struct tcp_metrics_block *tcpm_new(struct dst_entry *dst,
 		if (!tm)
 			goto out_unlock;
 	}
-	tm->tcpm_addr = *addr;
+	tm->tcpm_daddr = *daddr;
 
 	tcpm_suck_dst(tm, dst, true);
 
@@ -189,7 +189,7 @@ static struct tcp_metrics_block *tcp_get_encode(struct tcp_metrics_block *tm, in
 	return NULL;
 }
 
-static struct tcp_metrics_block *__tcp_get_metrics(const struct inetpeer_addr *addr,
+static struct tcp_metrics_block *__tcp_get_metrics(const struct inetpeer_addr *daddr,
 						   struct net *net, unsigned int hash)
 {
 	struct tcp_metrics_block *tm;
@@ -197,7 +197,7 @@ static struct tcp_metrics_block *__tcp_get_metrics(const struct inetpeer_addr *a
 
 	for (tm = rcu_dereference(net->ipv4.tcp_metrics_hash[hash].chain); tm;
 	     tm = rcu_dereference(tm->tcpm_next)) {
-		if (addr_same(&tm->tcpm_addr, addr))
+		if (addr_same(&tm->tcpm_daddr, daddr))
 			break;
 		depth++;
 	}
@@ -208,19 +208,19 @@ static struct tcp_metrics_block *__tcp_get_metrics_req(struct request_sock *req,
 						       struct dst_entry *dst)
 {
 	struct tcp_metrics_block *tm;
-	struct inetpeer_addr addr;
+	struct inetpeer_addr daddr;
 	unsigned int hash;
 	struct net *net;
 
-	addr.family = req->rsk_ops->family;
-	switch (addr.family) {
+	daddr.family = req->rsk_ops->family;
+	switch (daddr.family) {
 	case AF_INET:
-		addr.addr.a4 = inet_rsk(req)->ir_rmt_addr;
-		hash = (__force unsigned int) addr.addr.a4;
+		daddr.addr.a4 = inet_rsk(req)->ir_rmt_addr;
+		hash = (__force unsigned int) daddr.addr.a4;
 		break;
 #if IS_ENABLED(CONFIG_IPV6)
 	case AF_INET6:
-		*(struct in6_addr *)addr.addr.a6 = inet_rsk(req)->ir_v6_rmt_addr;
+		*(struct in6_addr *)daddr.addr.a6 = inet_rsk(req)->ir_v6_rmt_addr;
 		hash = ipv6_addr_hash(&inet_rsk(req)->ir_v6_rmt_addr);
 		break;
 #endif
@@ -233,7 +233,7 @@ static struct tcp_metrics_block *__tcp_get_metrics_req(struct request_sock *req,
 
 	for (tm = rcu_dereference(net->ipv4.tcp_metrics_hash[hash].chain); tm;
 	     tm = rcu_dereference(tm->tcpm_next)) {
-		if (addr_same(&tm->tcpm_addr, &addr))
+		if (addr_same(&tm->tcpm_daddr, &daddr))
 			break;
 	}
 	tcpm_check_stamp(tm, dst);
@@ -243,19 +243,19 @@ static struct tcp_metrics_block *__tcp_get_metrics_req(struct request_sock *req,
 static struct tcp_metrics_block *__tcp_get_metrics_tw(struct inet_timewait_sock *tw)
 {
 	struct tcp_metrics_block *tm;
-	struct inetpeer_addr addr;
+	struct inetpeer_addr daddr;
 	unsigned int hash;
 	struct net *net;
 
-	addr.family = tw->tw_family;
-	switch (addr.family) {
+	daddr.family = tw->tw_family;
+	switch (daddr.family) {
 	case AF_INET:
-		addr.addr.a4 = tw->tw_daddr;
-		hash = (__force unsigned int) addr.addr.a4;
+		daddr.addr.a4 = tw->tw_daddr;
+		hash = (__force unsigned int) daddr.addr.a4;
 		break;
 #if IS_ENABLED(CONFIG_IPV6)
 	case AF_INET6:
-		*(struct in6_addr *)addr.addr.a6 = tw->tw_v6_daddr;
+		*(struct in6_addr *)daddr.addr.a6 = tw->tw_v6_daddr;
 		hash = ipv6_addr_hash(&tw->tw_v6_daddr);
 		break;
 #endif
@@ -268,7 +268,7 @@ static struct tcp_metrics_block *__tcp_get_metrics_tw(struct inet_timewait_sock
 
 	for (tm = rcu_dereference(net->ipv4.tcp_metrics_hash[hash].chain); tm;
 	     tm = rcu_dereference(tm->tcpm_next)) {
-		if (addr_same(&tm->tcpm_addr, &addr))
+		if (addr_same(&tm->tcpm_daddr, &daddr))
 			break;
 	}
 	return tm;
@@ -279,20 +279,20 @@ static struct tcp_metrics_block *tcp_get_metrics(struct sock *sk,
 						 bool create)
 {
 	struct tcp_metrics_block *tm;
-	struct inetpeer_addr addr;
+	struct inetpeer_addr daddr;
 	unsigned int hash;
 	struct net *net;
 	bool reclaim;
 
-	addr.family = sk->sk_family;
-	switch (addr.family) {
+	daddr.family = sk->sk_family;
+	switch (daddr.family) {
 	case AF_INET:
-		addr.addr.a4 = inet_sk(sk)->inet_daddr;
-		hash = (__force unsigned int) addr.addr.a4;
+		daddr.addr.a4 = inet_sk(sk)->inet_daddr;
+		hash = (__force unsigned int) daddr.addr.a4;
 		break;
 #if IS_ENABLED(CONFIG_IPV6)
 	case AF_INET6:
-		*(struct in6_addr *)addr.addr.a6 = sk->sk_v6_daddr;
+		*(struct in6_addr *)daddr.addr.a6 = sk->sk_v6_daddr;
 		hash = ipv6_addr_hash(&sk->sk_v6_daddr);
 		break;
 #endif
@@ -303,14 +303,14 @@ static struct tcp_metrics_block *tcp_get_metrics(struct sock *sk,
 	net = dev_net(dst->dev);
 	hash = hash_32(hash, net->ipv4.tcp_metrics_hash_log);
 
-	tm = __tcp_get_metrics(&addr, net, hash);
+	tm = __tcp_get_metrics(&daddr, net, hash);
 	reclaim = false;
 	if (tm == TCP_METRICS_RECLAIM_PTR) {
 		reclaim = true;
 		tm = NULL;
 	}
 	if (!tm && create)
-		tm = tcpm_new(dst, &addr, hash, reclaim);
+		tm = tcpm_new(dst, &daddr, hash, reclaim);
 	else
 		tcpm_check_stamp(tm, dst);
 
@@ -724,15 +724,15 @@ static int tcp_metrics_fill_info(struct sk_buff *msg,
 	struct nlattr *nest;
 	int i;
 
-	switch (tm->tcpm_addr.family) {
+	switch (tm->tcpm_daddr.family) {
 	case AF_INET:
 		if (nla_put_be32(msg, TCP_METRICS_ATTR_ADDR_IPV4,
-				tm->tcpm_addr.addr.a4) < 0)
+				tm->tcpm_daddr.addr.a4) < 0)
 			goto nla_put_failure;
 		break;
 	case AF_INET6:
 		if (nla_put(msg, TCP_METRICS_ATTR_ADDR_IPV6, 16,
-			    tm->tcpm_addr.addr.a6) < 0)
+			    tm->tcpm_daddr.addr.a6) < 0)
 			goto nla_put_failure;
 		break;
 	default:
@@ -882,14 +882,14 @@ static int parse_nl_addr(struct genl_info *info, struct inetpeer_addr *addr,
 static int tcp_metrics_nl_cmd_get(struct sk_buff *skb, struct genl_info *info)
 {
 	struct tcp_metrics_block *tm;
-	struct inetpeer_addr addr;
+	struct inetpeer_addr daddr;
 	unsigned int hash;
 	struct sk_buff *msg;
 	struct net *net = genl_info_net(info);
 	void *reply;
 	int ret;
 
-	ret = parse_nl_addr(info, &addr, &hash, 0);
+	ret = parse_nl_addr(info, &daddr, &hash, 0);
 	if (ret < 0)
 		return ret;
 
@@ -907,7 +907,7 @@ static int tcp_metrics_nl_cmd_get(struct sk_buff *skb, struct genl_info *info)
 	rcu_read_lock();
 	for (tm = rcu_dereference(net->ipv4.tcp_metrics_hash[hash].chain); tm;
 	     tm = rcu_dereference(tm->tcpm_next)) {
-		if (addr_same(&tm->tcpm_addr, &addr)) {
+		if (addr_same(&tm->tcpm_daddr, &daddr)) {
 			ret = tcp_metrics_fill_info(msg, tm);
 			break;
 		}
@@ -962,12 +962,12 @@ static int tcp_metrics_nl_cmd_del(struct sk_buff *skb, struct genl_info *info)
 	struct tcpm_hash_bucket *hb;
 	struct tcp_metrics_block *tm;
 	struct tcp_metrics_block __rcu **pp;
-	struct inetpeer_addr addr;
+	struct inetpeer_addr daddr;
 	unsigned int hash;
 	struct net *net = genl_info_net(info);
 	int ret;
 
-	ret = parse_nl_addr(info, &addr, &hash, 1);
+	ret = parse_nl_addr(info, &daddr, &hash, 1);
 	if (ret < 0)
 		return ret;
 	if (ret > 0)
@@ -979,7 +979,7 @@ static int tcp_metrics_nl_cmd_del(struct sk_buff *skb, struct genl_info *info)
 	spin_lock_bh(&tcp_metrics_lock);
 	for (tm = deref_locked_genl(*pp); tm;
 	     pp = &tm->tcpm_next, tm = deref_locked_genl(*pp)) {
-		if (addr_same(&tm->tcpm_addr, &addr)) {
+		if (addr_same(&tm->tcpm_daddr, &daddr)) {
 			*pp = tm->tcpm_next;
 			break;
 		}
-- 
1.8.3.2

[PATCH 2/4] tcp: metrics: Add source-address to tcp-metrics

[Christoph Paasch]

We add the source-address to the tcp-metrics, so that different metrics
will be used per source/destination-pair. We use the destination-hash to
store the metric inside the hash-table. That way, deleting and dumping
via "ip tcp_metrics" is easy.

Signed-off-by: Christoph Paasch <christoph.paasch@uclouvain.be>
---
 net/ipv4/tcp_metrics.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
index f9b5f519a4ea..de32aa41a846 100644
--- a/net/ipv4/tcp_metrics.c
+++ b/net/ipv4/tcp_metrics.c
@@ -31,6 +31,7 @@ struct tcp_fastopen_metrics {
 
 struct tcp_metrics_block {
 	struct tcp_metrics_block __rcu	*tcpm_next;
+	struct inetpeer_addr		tcpm_saddr;
 	struct inetpeer_addr		tcpm_daddr;
 	unsigned long			tcpm_stamp;
 	u32				tcpm_ts;
@@ -131,6 +132,7 @@ static void tcpm_suck_dst(struct tcp_metrics_block *tm, struct dst_entry *dst,
 }
 
 static struct tcp_metrics_block *tcpm_new(struct dst_entry *dst,
+					  struct inetpeer_addr *saddr,
 					  struct inetpeer_addr *daddr,
 					  unsigned int hash,
 					  bool reclaim)
@@ -155,6 +157,7 @@ static struct tcp_metrics_block *tcpm_new(struct dst_entry *dst,
 		if (!tm)
 			goto out_unlock;
 	}
+	tm->tcpm_saddr = *saddr;
 	tm->tcpm_daddr = *daddr;
 
 	tcpm_suck_dst(tm, dst, true);
@@ -189,7 +192,8 @@ static struct tcp_metrics_block *tcp_get_encode(struct tcp_metrics_block *tm, in
 	return NULL;
 }
 
-static struct tcp_metrics_block *__tcp_get_metrics(const struct inetpeer_addr *daddr,
+static struct tcp_metrics_block *__tcp_get_metrics(const struct inetpeer_addr *saddr,
+						   const struct inetpeer_addr *daddr,
 						   struct net *net, unsigned int hash)
 {
 	struct tcp_metrics_block *tm;
@@ -197,7 +201,8 @@ static struct tcp_metrics_block *__tcp_get_metrics(const struct inetpeer_addr *d
 
 	for (tm = rcu_dereference(net->ipv4.tcp_metrics_hash[hash].chain); tm;
 	     tm = rcu_dereference(tm->tcpm_next)) {
-		if (addr_same(&tm->tcpm_daddr, daddr))
+		if (addr_same(&tm->tcpm_saddr, saddr) &&
+		    addr_same(&tm->tcpm_daddr, daddr))
 			break;
 		depth++;
 	}
@@ -208,18 +213,21 @@ static struct tcp_metrics_block *__tcp_get_metrics_req(struct request_sock *req,
 						       struct dst_entry *dst)
 {
 	struct tcp_metrics_block *tm;
-	struct inetpeer_addr daddr;
+	struct inetpeer_addr saddr, daddr;
 	unsigned int hash;
 	struct net *net;
 
+	saddr.family = req->rsk_ops->family;
 	daddr.family = req->rsk_ops->family;
 	switch (daddr.family) {
 	case AF_INET:
+		saddr.addr.a4 = inet_rsk(req)->ir_loc_addr;
 		daddr.addr.a4 = inet_rsk(req)->ir_rmt_addr;
 		hash = (__force unsigned int) daddr.addr.a4;
 		break;
 #if IS_ENABLED(CONFIG_IPV6)
 	case AF_INET6:
+		*(struct in6_addr *)saddr.addr.a6 = inet_rsk(req)->ir_v6_loc_addr;
 		*(struct in6_addr *)daddr.addr.a6 = inet_rsk(req)->ir_v6_rmt_addr;
 		hash = ipv6_addr_hash(&inet_rsk(req)->ir_v6_rmt_addr);
 		break;
@@ -233,7 +241,8 @@ static struct tcp_metrics_block *__tcp_get_metrics_req(struct request_sock *req,
 
 	for (tm = rcu_dereference(net->ipv4.tcp_metrics_hash[hash].chain); tm;
 	     tm = rcu_dereference(tm->tcpm_next)) {
-		if (addr_same(&tm->tcpm_daddr, &daddr))
+		if (addr_same(&tm->tcpm_saddr, &saddr) &&
+		    addr_same(&tm->tcpm_daddr, &daddr))
 			break;
 	}
 	tcpm_check_stamp(tm, dst);
@@ -243,18 +252,21 @@ static struct tcp_metrics_block *__tcp_get_metrics_req(struct request_sock *req,
 static struct tcp_metrics_block *__tcp_get_metrics_tw(struct inet_timewait_sock *tw)
 {
 	struct tcp_metrics_block *tm;
-	struct inetpeer_addr daddr;
+	struct inetpeer_addr saddr, daddr;
 	unsigned int hash;
 	struct net *net;
 
+	saddr.family = tw->tw_family;
 	daddr.family = tw->tw_family;
 	switch (daddr.family) {
 	case AF_INET:
+		saddr.addr.a4 = tw->tw_rcv_saddr;
 		daddr.addr.a4 = tw->tw_daddr;
 		hash = (__force unsigned int) daddr.addr.a4;
 		break;
 #if IS_ENABLED(CONFIG_IPV6)
 	case AF_INET6:
+		*(struct in6_addr *)saddr.addr.a6 = tw->tw_v6_rcv_saddr;
 		*(struct in6_addr *)daddr.addr.a6 = tw->tw_v6_daddr;
 		hash = ipv6_addr_hash(&tw->tw_v6_daddr);
 		break;
@@ -268,7 +280,8 @@ static struct tcp_metrics_block *__tcp_get_metrics_tw(struct inet_timewait_sock
 
 	for (tm = rcu_dereference(net->ipv4.tcp_metrics_hash[hash].chain); tm;
 	     tm = rcu_dereference(tm->tcpm_next)) {
-		if (addr_same(&tm->tcpm_daddr, &daddr))
+		if (addr_same(&tm->tcpm_saddr, &saddr) &&
+		    addr_same(&tm->tcpm_daddr, &daddr))
 			break;
 	}
 	return tm;
@@ -279,19 +292,22 @@ static struct tcp_metrics_block *tcp_get_metrics(struct sock *sk,
 						 bool create)
 {
 	struct tcp_metrics_block *tm;
-	struct inetpeer_addr daddr;
+	struct inetpeer_addr saddr, daddr;
 	unsigned int hash;
 	struct net *net;
 	bool reclaim;
 
+	saddr.family = sk->sk_family;
 	daddr.family = sk->sk_family;
 	switch (daddr.family) {
 	case AF_INET:
+		saddr.addr.a4 = inet_sk(sk)->inet_saddr;
 		daddr.addr.a4 = inet_sk(sk)->inet_daddr;
 		hash = (__force unsigned int) daddr.addr.a4;
 		break;
 #if IS_ENABLED(CONFIG_IPV6)
 	case AF_INET6:
+		*(struct in6_addr *)saddr.addr.a6 = sk->sk_v6_rcv_saddr;
 		*(struct in6_addr *)daddr.addr.a6 = sk->sk_v6_daddr;
 		hash = ipv6_addr_hash(&sk->sk_v6_daddr);
 		break;
@@ -303,14 +319,14 @@ static struct tcp_metrics_block *tcp_get_metrics(struct sock *sk,
 	net = dev_net(dst->dev);
 	hash = hash_32(hash, net->ipv4.tcp_metrics_hash_log);
 
-	tm = __tcp_get_metrics(&daddr, net, hash);
+	tm = __tcp_get_metrics(&saddr, &daddr, net, hash);
 	reclaim = false;
 	if (tm == TCP_METRICS_RECLAIM_PTR) {
 		reclaim = true;
 		tm = NULL;
 	}
 	if (!tm && create)
-		tm = tcpm_new(dst, &daddr, hash, reclaim);
+		tm = tcpm_new(dst, &saddr, &daddr, hash, reclaim);
 	else
 		tcpm_check_stamp(tm, dst);
 
-- 
1.8.3.2

[PATCH 3/4] tcp: metrics: Delete all entries matching a certain destination

[Christoph Paasch]

As we now can have multiple entries per destination-IP, the "ip
tcp_metrics delete address ADDRESS" command should delete all of them.

Signed-off-by: Christoph Paasch <christoph.paasch@uclouvain.be>
---
 net/ipv4/tcp_metrics.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
index de32aa41a846..8d544bb475dc 100644
--- a/net/ipv4/tcp_metrics.c
+++ b/net/ipv4/tcp_metrics.c
@@ -976,7 +976,7 @@ static int tcp_metrics_flush_all(struct net *net)
 static int tcp_metrics_nl_cmd_del(struct sk_buff *skb, struct genl_info *info)
 {
 	struct tcpm_hash_bucket *hb;
-	struct tcp_metrics_block *tm;
+	struct tcp_metrics_block *tm, *tmlist = NULL;
 	struct tcp_metrics_block __rcu **pp;
 	struct inetpeer_addr daddr;
 	unsigned int hash;
@@ -993,17 +993,22 @@ static int tcp_metrics_nl_cmd_del(struct sk_buff *skb, struct genl_info *info)
 	hb = net->ipv4.tcp_metrics_hash + hash;
 	pp = &hb->chain;
 	spin_lock_bh(&tcp_metrics_lock);
-	for (tm = deref_locked_genl(*pp); tm;
-	     pp = &tm->tcpm_next, tm = deref_locked_genl(*pp)) {
+	for (tm = deref_locked_genl(*pp); tm; tm = deref_locked_genl(*pp)) {
 		if (addr_same(&tm->tcpm_daddr, &daddr)) {
 			*pp = tm->tcpm_next;
-			break;
+			tm->tcpm_next = tmlist;
+			tmlist = tm;
+		} else {
+			pp = &tm->tcpm_next;
 		}
 	}
 	spin_unlock_bh(&tcp_metrics_lock);
-	if (!tm)
+	if (!tmlist)
 		return -ESRCH;
-	kfree_rcu(tm, rcu_head);
+	for (tm = tmlist; tm; tm = tmlist) {
+		tmlist = tm->tcpm_next;
+		kfree_rcu(tm, rcu_head);
+	}
 	return 0;
 }
 
-- 
1.8.3.2

Re: [PATCH 3/4] tcp: metrics: Delete all entries matching a certain destination

[David Miller]

From: Christoph Paasch <christoph.paasch@uclouvain.be>
Date: Sun, 15 Dec 2013 13:10:43 +0100

> As we now can have multiple entries per destination-IP, the "ip
> tcp_metrics delete address ADDRESS" command should delete all of them.
> 
> Signed-off-by: Christoph Paasch <christoph.paasch@uclouvain.be>

You have to adjust how you're doing patches #3 and #4.

Create the new attribute for the source address first, then
allow the source address to be specified in the deletion
command.

Make the source address optional in delete commands, to be
compatible with existing tools, and have it mean "ANY".

Re: [PATCH 3/4] tcp: metrics: Delete all entries matching a certain destination

[Christoph Paasch]

On 17/12/13 - 14:57:51, David Miller wrote:
> From: Christoph Paasch <christoph.paasch@uclouvain.be>
> Date: Sun, 15 Dec 2013 13:10:43 +0100
> 
> > As we now can have multiple entries per destination-IP, the "ip
> > tcp_metrics delete address ADDRESS" command should delete all of them.
> > 
> > Signed-off-by: Christoph Paasch <christoph.paasch@uclouvain.be>
> 
> You have to adjust how you're doing patches #3 and #4.
> 
> Create the new attribute for the source address first, then
> allow the source address to be specified in the deletion
> command.
> 
> Make the source address optional in delete commands, to be
> compatible with existing tools, and have it mean "ANY".

Thanks for the feedback, David.
I will resubmit when I have the time to do the changes.


Christoph

Re: [PATCH 3/4] tcp: metrics: Delete all entries matching a certain destination

[Christoph Paasch]

Hello,

On 17/12/13 - 14:57:51, David Miller wrote:
> From: Christoph Paasch <christoph.paasch@uclouvain.be>
> Date: Sun, 15 Dec 2013 13:10:43 +0100
> > As we now can have multiple entries per destination-IP, the "ip
> > tcp_metrics delete address ADDRESS" command should delete all of them.
> > 
> > Signed-off-by: Christoph Paasch <christoph.paasch@uclouvain.be>
> 
> You have to adjust how you're doing patches #3 and #4.
> 
> Create the new attribute for the source address first, then
> allow the source address to be specified in the deletion
> command.
> 
> Make the source address optional in delete commands, to be
> compatible with existing tools, and have it mean "ANY".

I have to come back on this one.
Can you explain what you mean by "ANY"?

Do you mean that if no source-IP is given in the netlink command that all
entries matching the dst should be deleted or rather only one of them (and that it
would be non-deterministic which one)?

Because, if I delete all of them, then "ip tcp_metrics flush PREFIX" of
today's iproute2 will complain, because iproute2 expects that for each entry of
"ip tcp_metrics show" a delete-call must be done.

But, the non-deterministic case also feels a bit odd to me.


Thanks,
Christoph

Re: [PATCH 3/4] tcp: metrics: Delete all entries matching a certain destination

[David Miller]

From: Christoph Paasch <christoph.paasch@uclouvain.be>
Date: Thu, 2 Jan 2014 10:18:23 +0100

> Do you mean that if no source-IP is given in the netlink command that all
> entries matching the dst should be deleted or rather only one of them (and that it
> would be non-deterministic which one)?

You would delete all of them, it's the only way to stay compatible with
the current code.

> Because, if I delete all of them, then "ip tcp_metrics flush PREFIX" of
> today's iproute2 will complain, because iproute2 expects that for each entry of
> "ip tcp_metrics show" a delete-call must be done.
> 
> But, the non-deterministic case also feels a bit odd to me.

iproute2 should not expect that if it doesn't specify a specific
source address, in fact it doesn't even know how to currently.  I
can't think of any other reasonable behavior.

[PATCH 4/4] tcp: metrics: Dump info of the source-address in netlink-reply

[Christoph Paasch]

Signed-off-by: Christoph Paasch <christoph.paasch@uclouvain.be>
---
 include/uapi/linux/tcp_metrics.h | 2 ++
 net/ipv4/tcp_metrics.c           | 6 ++++++
 2 files changed, 8 insertions(+)

diff --git a/include/uapi/linux/tcp_metrics.h b/include/uapi/linux/tcp_metrics.h
index cb5157b55f32..54a37b13f2c4 100644
--- a/include/uapi/linux/tcp_metrics.h
+++ b/include/uapi/linux/tcp_metrics.h
@@ -35,6 +35,8 @@ enum {
 	TCP_METRICS_ATTR_FOPEN_SYN_DROPS,	/* u16, count of drops */
 	TCP_METRICS_ATTR_FOPEN_SYN_DROP_TS,	/* msecs age */
 	TCP_METRICS_ATTR_FOPEN_COOKIE,		/* binary */
+	TCP_METRICS_ATTR_SADDR_IPV4,		/* u32 */
+	TCP_METRICS_ATTR_SADDR_IPV6,		/* binary */
 
 	__TCP_METRICS_ATTR_MAX,
 };
diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
index 8d544bb475dc..e150f264c8e2 100644
--- a/net/ipv4/tcp_metrics.c
+++ b/net/ipv4/tcp_metrics.c
@@ -745,11 +745,17 @@ static int tcp_metrics_fill_info(struct sk_buff *msg,
 		if (nla_put_be32(msg, TCP_METRICS_ATTR_ADDR_IPV4,
 				tm->tcpm_daddr.addr.a4) < 0)
 			goto nla_put_failure;
+		if (nla_put_be32(msg, TCP_METRICS_ATTR_SADDR_IPV4,
+				tm->tcpm_saddr.addr.a4) < 0)
+			goto nla_put_failure;
 		break;
 	case AF_INET6:
 		if (nla_put(msg, TCP_METRICS_ATTR_ADDR_IPV6, 16,
 			    tm->tcpm_daddr.addr.a6) < 0)
 			goto nla_put_failure;
+		if (nla_put(msg, TCP_METRICS_ATTR_SADDR_IPV6, 16,
+			    tm->tcpm_saddr.addr.a6) < 0)
+			goto nla_put_failure;
 		break;
 	default:
 		return -EAFNOSUPPORT;
-- 
1.8.3.2

Re: [PATCH 0/4] Make tcp-metrics source-address aware

[Eric Dumazet]

On Sun, 2013-12-15 at 13:10 +0100, Christoph Paasch wrote:
> Currently tcp-metrics only stores per-destination addresses. This brings
> problems, when a host has multiple interfaces (e.g., a smartphone having
> WiFi/3G):
> 
> For example, a host contacting a server over WiFi will store the tcp-metrics
> per destination IP. If then the host contacts the same server over 3G, the 
> same tcp-metrics will be used, although the path-characteristics are completly
> different (e.g., the ssthresh is probably not the same).

ssthresh caching is very problematic anyway.

hystart is way better to probe the actual capacity, as the real network 
conditions change every seconds or so.

> 
> The same holds for the fast-open cookie. The server will generate a cookie
> based on our source-address. So, if we contact the same server with another
> source-IP we should request a new cookie.

Yuchung, what do you think ? I think this should already be handled
gracefully, as client be behind a NAT device using a pool of external IP
addresses ?

Thanks Christoph !

Re: [PATCH 0/4] Make tcp-metrics source-address aware

[Yuchung Cheng]

On Sun, Dec 15, 2013 at 10:40 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Sun, 2013-12-15 at 13:10 +0100, Christoph Paasch wrote:
>> Currently tcp-metrics only stores per-destination addresses. This brings
>> problems, when a host has multiple interfaces (e.g., a smartphone having
>> WiFi/3G):
>>
>> For example, a host contacting a server over WiFi will store the tcp-metrics
>> per destination IP. If then the host contacts the same server over 3G, the
>> same tcp-metrics will be used, although the path-characteristics are completly
>> different (e.g., the ssthresh is probably not the same).
>
> ssthresh caching is very problematic anyway.
>
> hystart is way better to probe the actual capacity, as the real network
> conditions change every seconds or so.
>
>>
>> The same holds for the fast-open cookie. The server will generate a cookie
>> based on our source-address. So, if we contact the same server with another
>> source-IP we should request a new cookie.
>
> Yuchung, what do you think ? I think this should already be handled
> gracefully, as client be behind a NAT device using a pool of external IP
> addresses ?
Right. Today the Fast Open attempt will fall back to regular TCP
gracefully with the new cookie in SYN-ACK. So if the source ip changed
when the public IP/nat remain the same (common case?), the proposed
change will reduce Fast Open success rate. So it likely has negative
impact only.

This reminds me to add this specific issue in the fast open ietf draft.

>
> Thanks Christoph !
>
>

Re: [PATCH 0/4] Make tcp-metrics source-address aware

[Christoph Paasch]

On 16/12/13 - 10:45:13, Yuchung Cheng wrote:
> On Sun, Dec 15, 2013 at 10:40 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > On Sun, 2013-12-15 at 13:10 +0100, Christoph Paasch wrote:
> >> Currently tcp-metrics only stores per-destination addresses. This brings
> >> problems, when a host has multiple interfaces (e.g., a smartphone having
> >> WiFi/3G):
> >>
> >> For example, a host contacting a server over WiFi will store the tcp-metrics
> >> per destination IP. If then the host contacts the same server over 3G, the
> >> same tcp-metrics will be used, although the path-characteristics are completly
> >> different (e.g., the ssthresh is probably not the same).
> >
> > ssthresh caching is very problematic anyway.
> >
> > hystart is way better to probe the actual capacity, as the real network
> > conditions change every seconds or so.
> >
> >>
> >> The same holds for the fast-open cookie. The server will generate a cookie
> >> based on our source-address. So, if we contact the same server with another
> >> source-IP we should request a new cookie.
> >
> > Yuchung, what do you think ? I think this should already be handled
> > gracefully, as client be behind a NAT device using a pool of external IP
> > addresses ?
> Right. Today the Fast Open attempt will fall back to regular TCP
> gracefully with the new cookie in SYN-ACK. So if the source ip changed
> when the public IP/nat remain the same (common case?), the proposed
> change will reduce Fast Open success rate. So it likely has negative
> impact only.

Agreed, in this case it would be negative. Although I doubt that it happens
often that the source changes while the public IP remains the same.

But for the WiFi/3G-case it is really bad that parameters like
ssthresh/rtt/... are kept the same. We will exit slow-start too early when
going from WiFi to 3G because ssthresh is too low.

What if we copy the fast-open cookie across the different src/dst tcp_metrics-pairs
to handle Yuchung's case?
Or, maybe get rid of ssthresh/rtt/... in tcp-metrics? But that would probably be too
agressive...


Cheers,
Christoph

Re: [PATCH 0/4] Make tcp-metrics source-address aware

[Yuchung Cheng]

On Mon, Dec 16, 2013 at 11:30 AM, Christoph Paasch
<christoph.paasch@uclouvain.be> wrote:
> On 16/12/13 - 10:45:13, Yuchung Cheng wrote:
>> On Sun, Dec 15, 2013 at 10:40 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> > On Sun, 2013-12-15 at 13:10 +0100, Christoph Paasch wrote:
>> >> Currently tcp-metrics only stores per-destination addresses. This brings
>> >> problems, when a host has multiple interfaces (e.g., a smartphone having
>> >> WiFi/3G):
>> >>
>> >> For example, a host contacting a server over WiFi will store the tcp-metrics
>> >> per destination IP. If then the host contacts the same server over 3G, the
>> >> same tcp-metrics will be used, although the path-characteristics are completly
>> >> different (e.g., the ssthresh is probably not the same).
>> >
>> > ssthresh caching is very problematic anyway.
>> >
>> > hystart is way better to probe the actual capacity, as the real network
>> > conditions change every seconds or so.
>> >
>> >>
>> >> The same holds for the fast-open cookie. The server will generate a cookie
>> >> based on our source-address. So, if we contact the same server with another
>> >> source-IP we should request a new cookie.
>> >
>> > Yuchung, what do you think ? I think this should already be handled
>> > gracefully, as client be behind a NAT device using a pool of external IP
>> > addresses ?
>> Right. Today the Fast Open attempt will fall back to regular TCP
>> gracefully with the new cookie in SYN-ACK. So if the source ip changed
>> when the public IP/nat remain the same (common case?), the proposed
>> change will reduce Fast Open success rate. So it likely has negative
>> impact only.
>
> Agreed, in this case it would be negative. Although I doubt that it happens
> often that the source changes while the public IP remains the same.
>
> But for the WiFi/3G-case it is really bad that parameters like
> ssthresh/rtt/... are kept the same. We will exit slow-start too early when
> going from WiFi to 3G because ssthresh is too low.
>
> What if we copy the fast-open cookie across the different src/dst tcp_metrics-pairs
> to handle Yuchung's case?
> Or, maybe get rid of ssthresh/rtt/... in tcp-metrics? But that would probably be too
> agressive...
+1 to get rid of ssthresh caching. It often hurts than helps and
cubic-hystart already mitigates the SS overshoot. See sec 6.2.4 in a
recent research study
http://conferences.sigcomm.org/co-next/2013/program/p303.pdf

The only case caching RTT could help is the init RTO. But it's unclear
adding src-ip along will help like DM said.

Another metric that may have negative effect is reordering metrics.

>
>
> Cheers,
> Christoph
>

Re: [PATCH 0/4] Make tcp-metrics source-address aware

[Christoph Paasch]

On 16/12/13 - 11:53:31, Yuchung Cheng wrote:
> > Agreed, in this case it would be negative. Although I doubt that it happens
> > often that the source changes while the public IP remains the same.
> >
> > But for the WiFi/3G-case it is really bad that parameters like
> > ssthresh/rtt/... are kept the same. We will exit slow-start too early when
> > going from WiFi to 3G because ssthresh is too low.
> >
> > What if we copy the fast-open cookie across the different src/dst tcp_metrics-pairs
> > to handle Yuchung's case?
> > Or, maybe get rid of ssthresh/rtt/... in tcp-metrics? But that would probably be too
> > agressive...
> +1 to get rid of ssthresh caching. It often hurts than helps and
> cubic-hystart already mitigates the SS overshoot. See sec 6.2.4 in a
> recent research study
> http://conferences.sigcomm.org/co-next/2013/program/p303.pdf

I haven't yet read the paper, but in their presentation at CoNEXT they said
that the biggest impact comes from spurious retransmission timeouts and the
subsequent cwnd reduction. I would have thought that the cwnd-reduction
undo-procedure covers this case.

> The only case caching RTT could help is the init RTO. But it's unclear
> adding src-ip along will help like DM said.
> 
> Another metric that may have negative effect is reordering metrics.

So, let's get rid of all these metrics (except for TFO)? ;)


Christoph

Re: [PATCH 0/4] Make tcp-metrics source-address aware

[David Miller]

From: Yuchung Cheng <ycheng@google.com>
Date: Mon, 16 Dec 2013 11:53:31 -0800

> +1 to get rid of ssthresh caching.

Let's not discuss whether some metrics should be considered for
deletion here because such a discussion is entire separate from
what to key metrics upon.

If you want to talk about potentially removing ssthresh, please
start a new thread.

Thank you.

Re: [PATCH 0/4] Make tcp-metrics source-address aware

[David Miller]

From: Christoph Paasch <christoph.paasch@uclouvain.be>
Date: Sun, 15 Dec 2013 13:10:40 +0100

> Currently tcp-metrics only stores per-destination addresses. This brings
> problems, when a host has multiple interfaces (e.g., a smartphone having
> WiFi/3G):

So, at what point will we stop adding keys to tcp-metrics?

Several things can influence the network path, 'saddr' is just an
arbitrary and obvious one.

Even the TCP port numbers, via IPSEC rules, can make the packets go
over a completely different route with wildly different
characteristics, resulting in the same exact problem you say we need
to add 'saddr' for.

I'm not against adding 'saddr' to the metrics key, I just want to
know decide now (rather than later) far we need to go and implement
that fully rather than inching along one step at a time to the
same final result.

If the answer is "yes we must consider all potential keys" then we
will essentially have something approximating a single tcp-metrics
entry for every unique TCP connection ever openned because the source
and destination ports will be taken into account.

We won't be sharing metrics information at all at that point, so we
might as well remove it.

Arguably, when the metrics were in the routes, these details we hidden
away from us and we used fine grained metrics information when it
actually mattered.  If there were no IPSEC rules being used, we
wouldn't use keys detailed down to the port numbers.

However the routing cache always, at the very least, got us metrics
which were fine grained down to saddr/daddr/TOS/MARK etc.