[ovs-dev] [PATCH] RFC: datapath: Generic Netlink compatible locking.
jesse at nicira.com
Sat Dec 18 20:45:59 PST 2010
On Thu, Dec 16, 2010 at 3:35 PM, Ben Pfaff <blp at nicira.com> wrote:
> Open vSwitch is transitioning from a character device ioctl interface to
> one based on Generic Netlink. When Generic Netlink comes into the picture,
> the datapath module has to change its locking strategy.
> When Generic Netlink message receive calls into Open vSwitch, it always
> holds the genl_lock mutex. This means that there is no point in having
> per-datapath mutexes any longer, because no more than one would ever be
> held at a time, so this commit removes them.
> Given that genl_lock is always held, it would be nice to drop dp_mutex
> also, but we cannot because of the dp_notify path into OVS. On this path,
> rtnl_lock is held but genl_lock is not. We cannot simply take genl_lock
> on this path because it would cause a deadlock against the message receive
> path, which has to take rtnl_lock inside genl_lock. So this patch retains
> dp_mutex as an innermost lock that both message receive and dp_notify take.
I'm not sure that dp_mutex is, strictly speaking, necessary. Anything
that changes device state needs to hold RTNL lock (as it does now).
Things that read device state can use RCU for protection (There may be
some areas where this would need to be added, since they currently
assume protection by the per-dp lock. Also, the vport hash table
would need to be eliminated or RCU-ized.). Anything that is triggered
by dp_notify is always going to be related to device state, so RTNL
should be sufficient.
In other words, the general state of things would be:
* Writes to device state (add/remote datapath, port, set operations on
vports, etc.) are protected by RTNL.
* Writes to other state (flow table modifications, set miscellaneous
datapath parameters such as drop frags, etc.) are protected by genl.
* Reads are protected by RCU.
* There are a few special cases (mostly stats) that have their own
synchronization but they nest under all of above and don't interact
with each other.
So basically, operations from dp_notify can't change "other" state but
they don't currently and seem unlikely to need to do so.
I don't know, maybe it's simpler to have one lock that protects all
write state. On the other hand, eliminating a lock that is needed in
many places avoids a lot of headaches too.
More information about the dev