Linux Ethernet Bridging - Sep 2020 - [Bridge] [PATCH net-next v3 05/15] net: bridge: mcast: factor out port group del

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.
> -		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?
> -			if (!mp->ports && !mp->host_joined &&
> -			    netif_running(br->dev))
> -				mod_timer(&mp->timer, jiffies);
> +			br_multicast_del_pg(mp, p, pp);

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