On Fri, Dec 3, 2021, at 12:25, Yunjian Wang via dev wrote: > When handler threads lookup a "ofproto" and use it, main thread maybe > remove and free the "ofproto" at the same time. The "ofproto" has not > been protected well, which can lead to an OVS crash. > > This patch fixes this by making the "ofproto" lookup RCU-safe by using > cmap instead of hmap and moving remove "ofproto" call before > xlate_txn_commit(). >
I don't understand the point of moving the cmap_remove() call before xlate_txn_commit(). It is 'nice-to-have', maybe. In essence, it is about making sure the ofproto reference cannot be held by another thread while proceeding with the destruction. It simplifies the execution pattern for such destruction. But it seems other systems relying on ofproto access were written with the possibility that it is currently in the process of being destroyed. Its freeing is deferred, rules and groups are meant to proceed within this grace period. Granted, there is currently a bug in the rule management, but this is a bug and it is being fixed. So while it is correct, and while it simplifies the mental model when looking at the lifetime of ofproto, it does not seem necessary? Am I mistaken? If it is necessary, it would be better to be explicit about it. If multiple levels of the object rely on RCU sync, a single overarching call with a proper comment would be safer to maintain. As it concerns destruct() safety, I think it is worth having it explicitly at that level. It makes the fix more complex and more dangerous with changes in xlate implementation however. -- Gaetan Rivet _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
