Re: [net-next PATCH 2/2] bridge: netlink dump interface at par with brctl

On 06/10/2014 07:41 AM, Jamal Hadi Salim wrote:

On 06/09/14 12:41, Vlad Yasevich wrote:

quoted

On 06/07/2014 10:27 AM, Jamal Hadi Salim wrote:

quoted

From: Jamal Hadi Salim <jhs@mojatatu.com>

Actually better than brctl showmacs because we can filter by bridge
port in the kernel.
The current bridge netlink interface doesnt scale when you have many
bridges each with large fdbs or even bridges with many bridge ports

For example usage look at accompanying iproute2 patch.

The code was a bit tough to follow.  I think the main reason is
that you now always pass a filtering devices even when there was
no filtering information requested.

I am wondering if it could be made simpler...

The patch may be hard to follow i think. I cant think of a simple
way to do filtering by br and brport. If you have suggestions, shoot.

I gave it some thought and I think something like the following
pseudo-code would work.

dump_dev_fdbs(dev, filter)
{
        if (dev->dumper)
                dev->ndo_dumper(dev, filter);
        else
                default_dumper(dev, filter);
}

for_each_netdev() {
        if (bridge_filter) {
                if (dev->index != bridge_filter)
                        skip;

                dump_dev_fdbs(dev, port_filter);
        } else {
                if (port_filter) {
                        if (bridge_port &&
                            dev->index != port_filter)
                                skip;

                }

                if (bridge_port) {
                        br_dev = get_bridge();
                        dump_dev_fdbs(br_dev, port_filter);
                }

                dump_dev_fdbs(dev, port_filter);
        }
}


What do you think?

-vlad

quoted

quoted

      rcu_read_lock();
+    if (br_idx) {
+        br_dev = __dev_get_by_index(net, br_idx);
+        if (!br_dev) {
+            rcu_read_unlock();
+            return -ENODEV;
+        }
+        ops = br_dev->netdev_ops;
+        bdev = br_dev;
+    }
+

I think this can be outside of the rcu since you hold an rtnl at this
time.

Will fix on next iteration.

cheers,
jamal