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

login
register
mail settings
Submitter Christoph Paasch
Date Dec. 15, 2013, 12:10 p.m.
Message ID <1387109444-1104-4-git-send-email-christoph.paasch@uclouvain.be>
Download mbox | patch
Permalink /patch/301343/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Christoph Paasch - Dec. 15, 2013, 12:10 p.m.
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(-)
David Miller - Dec. 17, 2013, 7:57 p.m.
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".
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Paasch - Dec. 18, 2013, 9:58 a.m.
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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Paasch - Jan. 2, 2014, 9:18 a.m.
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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Jan. 6, 2014, 9:10 p.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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;
 }