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

login
register
mail settings
Submitter Amerigo Wang
Date April 30, 2013, 7:10 a.m.
Message ID <1367305819-14013-2-git-send-email-amwang@redhat.com>
Download mbox | patch
Permalink /patch/240573/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Amerigo Wang - April 30, 2013, 7:10 a.m.
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.

And "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."

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>
---
v3: fix the code in br_multicast_leave_group()
v2: add a flag to check if timer is fired or not

 net/bridge/br_multicast.c |   21 ++++++++++++---------
 net/bridge/br_private.h   |    1 +
 2 files changed, 13 insertions(+), 9 deletions(-)
Herbert Xu - May 2, 2013, 1:59 a.m.
On Tue, Apr 30, 2013 at 03:10:18PM +0800, Cong Wang wrote:
>
> @@ -1275,7 +1277,7 @@ static void br_multicast_leave_group(struct net_bridge *br,
>  		     br->multicast_last_member_interval;
>  
>  	if (!port) {
> -		if (mp->mglist &&
> +		if (mp->mglist && mp->timer_armed &&
>  		    (timer_pending(&mp->timer) ?
>  		     time_after(mp->timer.expires, time) :
>  		     try_to_del_timer_sync(&mp->timer) >= 0)) {

This whole section of code should now be removed since we're now
only arming the timer in response to a query to that group.  Of
course if we were the querier and sent out a query here then you'd
need to simulate the reception of a query.

Cheers,
Amerigo Wang - May 3, 2013, 3:50 a.m.
On Thu, 2013-05-02 at 09:59 +0800, Herbert Xu wrote:
> On Tue, Apr 30, 2013 at 03:10:18PM +0800, Cong Wang wrote:
> >
> > @@ -1275,7 +1277,7 @@ static void br_multicast_leave_group(struct net_bridge *br,
> >  		     br->multicast_last_member_interval;
> >  
> >  	if (!port) {
> > -		if (mp->mglist &&
> > +		if (mp->mglist && mp->timer_armed &&
> >  		    (timer_pending(&mp->timer) ?
> >  		     time_after(mp->timer.expires, time) :
> >  		     try_to_del_timer_sync(&mp->timer) >= 0)) {
> 
> This whole section of code should now be removed since we're now
> only arming the timer in response to a query to that group.  Of
> course if we were the querier and sent out a query here then you'd
> need to simulate the reception of a query.

Hmm, this piece of code is checking if bridge itself is in the mdb entry
and if it just receives a leave, which seems still necessary... What am
I missing?

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 - May 3, 2013, 4:01 a.m.
On Fri, May 03, 2013 at 11:50:22AM +0800, Cong Wang wrote:
> On Thu, 2013-05-02 at 09:59 +0800, Herbert Xu wrote:
> > On Tue, Apr 30, 2013 at 03:10:18PM +0800, Cong Wang wrote:
> > >
> > > @@ -1275,7 +1277,7 @@ static void br_multicast_leave_group(struct net_bridge *br,
> > >  		     br->multicast_last_member_interval;
> > >  
> > >  	if (!port) {
> > > -		if (mp->mglist &&
> > > +		if (mp->mglist && mp->timer_armed &&
> > >  		    (timer_pending(&mp->timer) ?
> > >  		     time_after(mp->timer.expires, time) :
> > >  		     try_to_del_timer_sync(&mp->timer) >= 0)) {
> > 
> > This whole section of code should now be removed since we're now
> > only arming the timer in response to a query to that group.  Of
> > course if we were the querier and sent out a query here then you'd
> > need to simulate the reception of a query.
> 
> Hmm, this piece of code is checking if bridge itself is in the mdb entry
> and if it just receives a leave, which seems still necessary... What am
> I missing?

I think I quoted the wrong hunk in the patch, I meant the code
that arms the timer should no longer be in the leave_group function
unless we just sent a query ourselves (and in that case the expiration
should also be adjusted accordingly).

Cheers,

Patch

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index c52d4f3..6a05522 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -617,8 +617,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++;
@@ -656,7 +654,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);
@@ -671,7 +668,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;
 	}
 
@@ -679,7 +675,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;
 	}
@@ -690,8 +686,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;
 
@@ -1131,6 +1125,10 @@  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);
+	mp->timer_armed = true;
+
 	max_delay *= br->multicast_last_member_count;
 
 	if (mp->mglist &&
@@ -1205,6 +1203,10 @@  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);
+	mp->timer_armed = true;
+
 	max_delay *= br->multicast_last_member_count;
 	if (mp->mglist &&
 	    (timer_pending(&mp->timer) ?
@@ -1263,7 +1265,7 @@  static void br_multicast_leave_group(struct net_bridge *br,
 			call_rcu_bh(&p->rcu, br_multicast_free_pg);
 			br_mdb_notify(br->dev, port, group, RTM_DELMDB);
 
-			if (!mp->ports && !mp->mglist &&
+			if (!mp->ports && !mp->mglist && mp->timer_armed &&
 			    netif_running(br->dev))
 				mod_timer(&mp->timer, jiffies);
 		}
@@ -1275,7 +1277,7 @@  static void br_multicast_leave_group(struct net_bridge *br,
 		     br->multicast_last_member_interval;
 
 	if (!port) {
-		if (mp->mglist &&
+		if (mp->mglist && mp->timer_armed &&
 		    (timer_pending(&mp->timer) ?
 		     time_after(mp->timer.expires, time) :
 		     try_to_del_timer_sync(&mp->timer) >= 0)) {
@@ -1674,6 +1676,7 @@  void br_multicast_stop(struct net_bridge *br)
 		hlist_for_each_entry_safe(mp, n, &mdb->mhash[i],
 					  hlist[ver]) {
 			del_timer(&mp->timer);
+			mp->timer_armed = false;
 			call_rcu_bh(&mp->rcu, br_multicast_free_group);
 		}
 	}
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 695033f..cd2552c 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -112,6 +112,7 @@  struct net_bridge_mdb_entry
 	struct timer_list		timer;
 	struct br_ip			addr;
 	bool				mglist;
+	bool				timer_armed;
 };
 
 struct net_bridge_mdb_htable