diff mbox

[net-next,2/3] bridge: only expire the mdb entry when query is received

Message ID 1367220392-17496-2-git-send-email-amwang@redhat.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Amerigo Wang April 29, 2013, 7:26 a.m. UTC 
From: Cong Wang <amwang@redhat.com>

Currently we arm the expire timer when the mdb entry is added,
however, this causes problem when there is no querier sent
out after that.

So we should only arm the timer when a corresponding query is
received, as suggested by Herbert.

Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Adam Baker <linux@baker-net.org.uk>
Signed-off-by: Cong Wang <amwang@redhat.com>
---
 net/bridge/br_multicast.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

Comments

Amerigo Wang April 30, 2013, 3:17 a.m. UTC | #1 
On Mon, 2013-04-29 at 15:26 +0800, Cong Wang wrote:
> From: Cong Wang <amwang@redhat.com>
> 
> Currently we arm the expire timer when the mdb entry is added,
> however, this causes problem when there is no querier sent
> out after that.
> 
> So we should only arm the timer when a corresponding query is
> received, as suggested by Herbert.

One problem with this solution is that the temp mdb entry will no longer
be expired automatically any more, since no query no timer.

I am wondering if we should find other way to fix it, for example,
touching the timer as long as there is multicast traffic (non-IGMP).

Thanks!

--
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
Herbert Xu April 30, 2013, 3:22 a.m. UTC | #2 
On Tue, Apr 30, 2013 at 11:17:37AM +0800, Cong Wang wrote:
> 
> One problem with this solution is that the temp mdb entry will no longer
> be expired automatically any more, since no query no timer.
> 
> I am wondering if we should find other way to fix it, for example,
> touching the timer as long as there is multicast traffic (non-IGMP).

mdb entries represent group subscriptions.  So what you're saying
is that group subscriptions aren't expiring.

Which isn't a problem at all since if there is no querier then group
subscriptions shouldn't expire.  There has to be at least one querier
in the network for this thing to work.  Otherwise it just degenerates
into a non-snooping switch, which is OK.

Cheers,
Amerigo Wang April 30, 2013, 3:32 a.m. UTC | #3 
On Tue, 2013-04-30 at 11:22 +0800, Herbert Xu wrote:
> On Tue, Apr 30, 2013 at 11:17:37AM +0800, Cong Wang wrote:
> > 
> > One problem with this solution is that the temp mdb entry will no longer
> > be expired automatically any more, since no query no timer.
> > 
> > I am wondering if we should find other way to fix it, for example,
> > touching the timer as long as there is multicast traffic (non-IGMP).
> 
> mdb entries represent group subscriptions.  So what you're saying
> is that group subscriptions aren't expiring.
> 
> Which isn't a problem at all since if there is no querier then group
> subscriptions shouldn't expire.  There has to be at least one querier
> in the network for this thing to work.  Otherwise it just degenerates
> into a non-snooping switch, which is OK.

Makes sense. I will put this in the changelog.

Thanks.

--
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
diff mbox

Patch

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 6cb937c..a821a4e 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -616,8 +616,6 @@  rehash:
 
 	mp->br = br;
 	mp->addr = *group;
-	setup_timer(&mp->timer, br_multicast_group_expired,
-		    (unsigned long)mp);
 
 	hlist_add_head_rcu(&mp->hlist[mdb->ver], &mdb->mhash[hash]);
 	mdb->size++;
@@ -655,7 +653,6 @@  static int br_multicast_add_group(struct net_bridge *br,
 	struct net_bridge_mdb_entry *mp;
 	struct net_bridge_port_group *p;
 	struct net_bridge_port_group __rcu **pp;
-	unsigned long now = jiffies;
 	int err;
 
 	spin_lock(&br->multicast_lock);
@@ -670,7 +667,6 @@  static int br_multicast_add_group(struct net_bridge *br,
 
 	if (!port) {
 		mp->mglist = true;
-		mod_timer(&mp->timer, now + br->multicast_membership_interval);
 		goto out;
 	}
 
@@ -678,7 +674,7 @@  static int br_multicast_add_group(struct net_bridge *br,
 	     (p = mlock_dereference(*pp, br)) != NULL;
 	     pp = &p->next) {
 		if (p->port == port)
-			goto found;
+			goto out;
 		if ((unsigned long)p->port < (unsigned long)port)
 			break;
 	}
@@ -689,8 +685,6 @@  static int br_multicast_add_group(struct net_bridge *br,
 	rcu_assign_pointer(*pp, p);
 	br_mdb_notify(br->dev, port, group, RTM_NEWMDB);
 
-found:
-	mod_timer(&p->timer, now + br->multicast_membership_interval);
 out:
 	err = 0;
 
@@ -1130,6 +1124,9 @@  static int br_ip4_multicast_query(struct net_bridge *br,
 	if (!mp)
 		goto out;
 
+	setup_timer(&mp->timer, br_multicast_group_expired, (unsigned long)mp);
+	mod_timer(&mp->timer, now + br->multicast_membership_interval);
+
 	max_delay *= br->multicast_last_member_count;
 
 	if (mp->mglist &&
@@ -1204,6 +1201,8 @@  static int br_ip6_multicast_query(struct net_bridge *br,
 	if (!mp)
 		goto out;
 
+	setup_timer(&mp->timer, br_multicast_group_expired, (unsigned long)mp);
+	mod_timer(&mp->timer, now + br->multicast_membership_interval);
 	max_delay *= br->multicast_last_member_count;
 	if (mp->mglist &&
 	    (timer_pending(&mp->timer) ?