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

Submitted by Christoph Paasch on Dec. 15, 2013, 12:10 p.m.

Details

Message ID 1387109444-1104-4-git-send-email-christoph.paasch@uclouvain.be
State Changes Requested
Delegated to: David Miller
Headers show

Commit Message

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(-)

Comments

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 hide | download patch | download mbox

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