On 10/11/2015 02:42 PM, Nikolay Aleksandrov wrote: > On 10/11/2015 02:21 PM, Ido Schimmel wrote: >> Wed, Sep 30, 2015 at 09:16:54PM IDT, razor@xxxxxxxxxxxxx wrote: >>> From: Nikolay Aleksandrov <nikolay@xxxxxxxxxxxxxxxxxxx> >>> >>> When a new port is being added we need to make vlgrp available after >>> rhashtable has been initialized and when removing a port we need to >>> flush the vlans and free the resources after we're sure noone can use >>> the port, i.e. after it's removed from the port list and synchronize_rcu >>> is executed. >> >> Hi Nikolay, >> >> Changing the order of port deinit breaks symmetry with the init >> sequence. It also introduces a problem for switchdev drivers. Flushing >> the VLANs clears HW VLAN filters and then, when port is unlinked from >> bridge and CHANGEUPPER is received, port is configured to direct traffic >> to CPU (as it's not offloaded anymore). Doing the reverse (like in this >> patch) renders the port unusable. >> >> Regarding the reason for this change, are you afraid that vlgrp will be >> accessed in bridge's rx handler or xmit function after it's freed? If >> so, maybe we can access it using RCU primitives? That way, both the rx >> handler and xmit function (executed under RCU lock) will either have a >> valid copy or not. Looking at previous iterations of this code, I see >> that was the case with the 'net_port_vlans' struct. >> >> I can start working on a fix if you agree with the proposed solution. >> >> Thanks. >> > > Hi, > Ah, I didn't know about this, I feared that something might rely on the > particular order of the operations but didn't have a way to test this one in > particular. Anyway, your proposed solution sounds good to me. > > Thank you, > Nik One thing to be careful about is the creation/destruction of the rhashtable itself and the order of operations in regard to vlgrp visibility, so it's not only if vlgrp is visible or not - it should be visible after rhashtable has been initialized and should be removed before it's freed, the rest is pretty straight-forward.
