Re: [Bridge] [PATCH net-next v3 05/15] net: bridge: mcast: factor out port group del

[Nikolay Aleksandrov]

On 9/6/20 11:56 PM, Jakub Kicinski wrote:

On Sat,  5 Sep 2020 11:24:00 +0300 Nikolay Aleksandrov wrote:

@@ -843,24 +843,11 @@ static int __br_mdb_del(struct net_bridge *br, struct 
br_mdb_entry *entry)
                if (!p->port || p->port->dev->ifindex != entry->ifindex)
                        continue;

- if (!hlist_empty(&p->src_list)) {

-                       err = -EINVAL;
-                       goto unlock;
-               }
-
                if (p->port->state == BR_STATE_DISABLED)
                        goto unlock;

- __mdb_entry_fill_flags(entry, p->flags);

Just from staring at the code it's unclear why the list_empty() check
and this __mdb_entry_fill_flags() are removed as well.

The hlist_empty check is added by patch 01 temporarily for correctness.
Otherwise I'd have to edit all open-coded pg del places and add src delete
code and then remove it here.

__mdb_entry_fill_flags() are called by __mdb_fill_info() which is the only
function used to fill an mdb both for dumping and notifications after patch 08.

-               rcu_assign_pointer(*pp, p->next);
-               hlist_del_init(&p->mglist);
-               del_timer(&p->timer);
-               kfree_rcu(p, rcu);
+               br_multicast_del_pg(mp, p, pp);
                err = 0;
-
-               if (!mp->ports && !mp->host_joined &&
-                   netif_running(br->dev))
-                       mod_timer(&mp->timer, jiffies);
                break;

+void br_multicast_del_pg(struct net_bridge_mdb_entry *mp,
+                        struct net_bridge_port_group *pg,
+                        struct net_bridge_port_group __rcu **pp)
+{
+       struct net_bridge *br = pg->port->br;
+       struct net_bridge_group_src *ent;
+       struct hlist_node *tmp;
+
+       rcu_assign_pointer(*pp, pg->next);
+       hlist_del_init(&pg->mglist);
+       del_timer(&pg->timer);
+       hlist_for_each_entry_safe(ent, tmp, &pg->src_list, node)
+               br_multicast_del_group_src(ent);
+       br_mdb_notify(br->dev, pg->port, &pg->addr, RTM_DELMDB, pg->flags);
+       kfree_rcu(pg, rcu);
+
+       if (!mp->ports && !mp->host_joined && netif_running(br->dev))
+               mod_timer(&mp->timer, jiffies);
+}

@@ -1641,16 +1647,7 @@ br_multicast_leave_group(struct net_bridge *br,
                        if (p->flags & MDB_PG_FLAGS_PERMANENT)
                                break;

- rcu_assign_pointer(*pp, p->next);

-                       hlist_del_init(&p->mglist);
-                       del_timer(&p->timer);
-                       kfree_rcu(p, rcu);
-                       br_mdb_notify(br->dev, port, group, RTM_DELMDB,
-                                     p->flags | MDB_PG_FLAGS_FAST_LEAVE);

And here we'll loose MDB_PG_FLAGS_FAST_LEAVE potentially?

Good catch, we will lose the fast leave indeed.
This is a 1 line change to set the flag before notifying. Would you prefer
me to send a v4 or a follow up for it?

Thanks,
 Nik

-                       if (!mp->ports && !mp->host_joined &&
-                           netif_running(br->dev))
-                               mod_timer(&mp->timer, jiffies);
+                       br_multicast_del_pg(mp, p, pp);