From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jiri Pirko Subject: Re: [patch net-next v3 09/17] bridge: add API to notify bridge driver of learned FBD on offloaded device Date: Wed, 26 Nov 2014 11:26:48 +0100 Message-ID: <20141126102648.GG1875@nanopsycho.orion> References: <1416911328-10979-1-git-send-email-jiri@resnulli.us> <1416911328-10979-10-git-send-email-jiri@resnulli.us> <20141125163853.GJ27416@gospo.rtplab.test> <20141125223643.GH3912@casper.infradead.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Thomas Graf , Andy Gospodarek , Netdev , "David S. Miller" , "nhorman@tuxdriver.com" , Andy Gospodarek , "dborkman@redhat.com" , "ogerlitz@mellanox.com" , "jesse@nicira.com" , "pshelar@nicira.com" , "azhou@nicira.com" , "ben@decadent.org.uk" , "stephen@networkplumber.org" , "Kirsher, Jeffrey T" , "vyasevic@redhat.com" , Cong Wang , "Fastabend, John R" , Eric Dumazet , Jamal Hadi Salim , Florian Fainelli , Roopa Prabhu Return-path: Received: from mail-wg0-f48.google.com ([74.125.82.48]:64511 "EHLO mail-wg0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750811AbaKZK0u (ORCPT ); Wed, 26 Nov 2014 05:26:50 -0500 Received: by mail-wg0-f48.google.com with SMTP id y19so3245654wgg.21 for ; Wed, 26 Nov 2014 02:26:49 -0800 (PST) Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: Wed, Nov 26, 2014 at 02:48:04AM CET, sfeldma@gmail.com wrote: >On Tue, Nov 25, 2014 at 12:36 PM, Thomas Graf wrote: >> On 11/25/14 at 11:38am, Andy Gospodarek wrote: >>> On Tue, Nov 25, 2014 at 11:28:40AM +0100, Jiri Pirko wrote: >>> > From: Scott Feldman >>> > >>> > When the swdev device learns a new mac/vlan on a port, it sends some async >>> > notification to the driver and the driver installs an FDB in the device. >>> > To give a holistic system view, the learned mac/vlan should be reflected >>> > in the bridge's FBD table, so the user, using normal iproute2 cmds, can view >>> > what is currently learned by the device. This API on the bridge driver gives >>> > a way for the swdev driver to install an FBD entry in the bridge FBD table. >>> > (And remove one). >>> > >>> > This is equivalent to the device running these cmds: >>> > >>> > bridge fdb [add|del] dev vid master >>> > >>> > This patch needs some extra eyeballs for review, in paricular around the >>> > locking and contexts. >>> > >>> > Signed-off-by: Scott Feldman >>> > Signed-off-by: Jiri Pirko >> >> I like the simplicity of this. That said, given we'll have multiple >> users of swdev including OVS, shouldn't this be a notifier or a >> callback that depends on who is controlling the device? > >I like the idea. When the switch port joins Linux bridge or OVS >datapath, a callback is registered with the driver. That way the >driver doesn't really care if the port is a bridge member or an OVS >vport in a datapath. It's just passing up the FDB entry >(port/mac/vlan) details to the container device. Can we hold this >idea until this patchset sticks? I think once OVS support comes back >into the swdev model would be the time to address this. Yep, I agree this is a good idea and I also vote for implemeting this as a follow-up. Thanks. > >> >>> > + spin_lock(&br->hash_lock); >>> (Since you asked to check locking...) >>> >>> Most of the other fdb_add/delete/insert/update calls take this with >>> spin_lock_bh. Did you try this with lockdep enabled just to see if that >>> is needed here? I suspect that anytime br->hash_lock is taken it will >>> need to be with softirqs disabled from this point forward. >> >> At least br_fdb_update() seems to be called from BH context so I would >> agree and argue the lock in br_fdb_cleanup() and br_fdb_update() need a >> fix too. I'll send a patch.