On Sun, Feb 13, 2022, at 03:14, Peng He wrote: > From hepeng: > https://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/#2487473 > > also from guohongzhi <[email protected]>: > http://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/ > > also from a discussion about the mixing use of RCU and refcount in the mail > list with Ilya Maximets, William Tu, Ben Pfaf, and Gaëtan Rivet. > > A summary, as quoted from Ilya: > > " > RCU for ofproto was introduced for one > and only one reason - to avoid freeing ofproto while rules are still > alive. This was done in commit f416c8d61601 ("ofproto: RCU postpone > rule destruction."). The goal was to allow using rules without > refcounting them within a single grace period. And that forced us > to postpone destruction of the ofproto for a single grace period. > Later commit 39c9459355b6 ("Use classifier versioning.") made it > possible for rules to be alive for more than one grace period, so > the commit made ofproto wait for 2 grace periods by double postponing. > As we can see now, that wasn't enough and we have to wait for more > than 2 grace periods in certain cases. > " > > In a short, the ofproto should have a longer life time than rule, if > the rule lasts for more than 2 grace periods, the ofproto should live > longer to ensure rule->ofproto is valid. It's hard to predict how long > a ofproto should live, thus we need to use refcount on ofproto to make > things easy. The controversial part is that we have already used RCU postpone > to delay ofproto destrution, if we have to add refcount, is it simpler to > use just refcount without RCU postpone? > > IMO, I think going back to the pure refcount solution is more > complicated than mixing using both. > > Gaëtan Rive asks some questions on guohongzhi's v2 patch: > > during ofproto_rule_create, should we use ofproto_ref > or ofproto_try_ref? how can we make sure the ofproto is alive? > > By using RCU, ofproto has three states: > > state 1: alive, with refcount >= 1 > state 2: dying, with refcount == 0, however pointer is valid > state 3: died, memory freed, pointer might be dangling. > > Without using RCU, there is no state 2, thus, we have to be very careful > every time we see a ofproto pointer. In contrast, with RCU, we can be sure > that it's alive at least in this grace peroid, so we can just check if > it is dying by ofproto_try_ref. > > This shows that by mixing use of RCU and refcount we can save a lot of work > worrying if ofproto is dangling. > > In short, the RCU part makes sure the ofproto is alive when we use it, > and the refcount part makes sure it lives longer enough. > > In this patch, I have merged guohongzhi's patch and mine, and fixes > accoring to the previous comments. > > v4->v5: > * fix the commits, remove the ref to wangyunjian's patch and > remove the comments describing the previous ofproto destruction code. > * fix group alloc leak issues. > > v5->v6: > * fix ofproto unref leak > * fix comments > > Signed-off-by: Peng He <[email protected]> > Signed-off-by: guohongzhi <[email protected]> > Acked-by: Mike Pattrick <[email protected]>
Hi Peng, Thanks for fixing the comments I had. I still think this commit log might be too long, but the important infos are there to understand the issue. That being said, everything is understandable and not misleading I think, and the fix seems correct to me. I don't see any more issues. Acked-by: Gaetan Rivet <[email protected]> -- Gaetan Rivet _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
