[Bridge] [PATCH v9 net-next 01/12] bridge: Add vlan filtering infrastructure

Michał Mirosław mirqus at gmail.com
Sat Feb 2 01:04:09 UTC 2013


2013/2/1 Vlad Yasevich <vyasevic at redhat.com>:
> Adds an optional infrustructure component to bridge that would allow
> native vlan filtering in the bridge.  Each bridge port (as well
> as the bridge device) now get a VLAN bitmap.  Each bit in the bitmap
> is associated with a vlan id.  This way if the bit corresponding to
> the vid is set in the bitmap that the packet with vid is allowed to
> enter and exit the port.
[...]
+struct net_port_vlans {
+       u16                             port_idx;
+       void                            *parent;
+       struct rcu_head                 rcu;
+       unsigned long                   vlan_bitmap[BR_VLAN_BITMAP_LEN];
+};
[later, in br_vlan.c]
+#define vlans_to_parent(type, pv) ((type *)(pv)->parent)

A really don't like this pointer casting. It's easy to get it wrong
(and you did in __vlan_del).

1. union { struct net_bridge *br; struct net_bridge_port *port; } parent;

2. inline net_port_vlans __rcu **br_vlan_info_parent_ptr(struct
net_port_vlans *v)
{
	return v->port_idx ? &v->parent.port->vlan_info : &v->parent.br->vlan_info;
}

(I'm not insisting on #2. Just think about it. ;)

> @@ -156,6 +166,7 @@ struct net_bridge_port
>  #ifdef CONFIG_NET_POLL_CONTROLLER
>         struct netpoll                  *np;
>  #endif
> +       struct net_port_vlans __rcu     *vlan_info;
>  };

Missing #ifdef CONFIG_BRIDGE_VLAN_FILTERING?

> +static int __vlan_del(struct net_port_vlans *v, u16 vid)
> +{
> +       unsigned long first_bit;
> +       unsigned long last_bit;
> +
> +       if (!test_bit(vid, v->vlan_bitmap))
> +               return -EINVAL;
> +
> +       /* Check to see if any other vlans are in this table.  If this
> +        * is the last vlan, delete the whole structure.  If this is not the
> +        * last vlan, just clear the bit.
> +        */
> +       first_bit = find_first_bit(v->vlan_bitmap, BR_VLAN_BITMAP_LEN);
> +       last_bit = find_last_bit(v->vlan_bitmap, BR_VLAN_BITMAP_LEN);
> +
> +       if (v->port_idx && vid) {
> +               struct net_device *dev =
> +                       vlans_to_parent(struct net_bridge, v)->dev;

struct net_bridge_port (or union and v->parent.port)

> +
> +               if (dev->features & NETIF_F_HW_VLAN_FILTER)
> +                       dev->netdev_ops->ndo_vlan_rx_kill_vid(dev, vid);
> +       }
> +
> +       clear_bit(vid, v->vlan_bitmap);
> +       if (first_bit == last_bit) {

bitmap_empty() is faster than two times find_xxx_bit().

> +               if (v->port_idx) {
> +                       struct net_bridge_port *p =
> +                               vlans_to_parent(struct net_bridge_port, v);
> +                       rcu_assign_pointer(p->vlan_info, NULL);
> +               } else {
> +                       struct net_bridge *br =
> +                               vlans_to_parent(struct net_bridge, v);
> +                       rcu_assign_pointer(br->vlan_info, NULL);
> +               }

With inline function above:

rcu_assign_pointer(*br_vlan_info_parent_ptr(v), NULL);

> +               kfree_rcu(v, rcu);
> +       }
> +       return 0;
> +}
> +
> +static void __vlan_flush(struct net_port_vlans *v)
> +{
> +       bitmap_zero(v->vlan_bitmap, BR_VLAN_BITMAP_LEN);
> +       if (v->port_idx) {
> +               struct net_bridge_port *p =
> +                               vlans_to_parent(struct net_bridge_port, v);
> +               rcu_assign_pointer(p->vlan_info, NULL);
> +       } else {
> +               struct net_bridge *br =
> +                               vlans_to_parent(struct net_bridge, v);
> +               rcu_assign_pointer(br->vlan_info, NULL);
> +       }

Same here.

> +       kfree_rcu(v, rcu);
> +}

Best Regards,
Michał Mirosław


More information about the Bridge mailing list