On Tue, Mar 2, 2021 at 4:15 AM Ilya Maximets <[email protected]> wrote:
>
> On 2/27/21 9:53 AM, 贺鹏 wrote:
> > Hi,
> >
> > Thanks William and Ilya for a detailed revisiting of the origin of the
> > problem. I learned a lot.
> >
> > I now understand that the mix using of RCU and refcounts is not
> > intended in the first place.
> > But my point is that mix using RCU and refcounts now gives you more
> > choices, and actually eases the code changes.
> >
> > For example, the code for * ofproto_dpif_lookup_by_name* or other
> > ofproto lookup function,
> > when only using refcounts, you need to change it to:
> >
> > struct ofproto_dpif *
> > ofproto_dpif_lookup_by_uuid(const struct uuid *uuid)
> > {
> > struct ofproto_dpif *ofproto;
> >
> > HMAP_FOR_EACH_WITH_HASH (ofproto, all_ofproto_dpifs_by_uuid_node,
> > uuid_hash(uuid), &all_ofproto_dpifs_by_uuid) {
> > if (uuid_equals(&ofproto->uuid, uuid)) {
> >
> > ---> if ovs_refcount_try_ref(ofproto)
> >
> > return ofproto;
> > }
> > }
> > return NULL;
> > }
>
> That is an interesting example here...
> I can't help but notice that this function is typically called
> from different handler or pmd threads and modified by the main thread
> while upcalls enabled. And hmap is not a thread-safe structure.
> I guess, we have another possible problem here. We need to protect
> at least this hmap and the other one with rw lock or something...
> Am I right or am I missing something? What else we need to protect?
I guess the function needs to be changed into using cmap ....
But fortunately, ofproto_dpif_lookup_by_uuid is called by
upcall_receive, and for most upcalls
(with the type MISS_UPCALL), it will not be called.
...
} else if (upcall->type == MISS_UPCALL) {
error = xlate_lookup(backer, flow, &upcall->ofproto, &upcall->ipfix,
&upcall->sflow, NULL, &upcall->ofp_in_port);
if (error) {
return error;
}
} else {
struct ofproto_dpif *ofproto
= ofproto_dpif_lookup_by_uuid(&upcall->cookie.ofproto_uuid);
if (!ofproto) {
VLOG_INFO_RL(&rl, "upcall could not find ofproto");
return ENODEV;
}
upcall->ofproto = ofproto;
upcall->ipfix = ofproto->ipfix;
upcall->sflow = ofproto->sflow;
upcall->ofp_in_port = upcall->cookie.ofp_in_port;
}
...
So for the people who use sflow, I guess there could be a risk, but
this is also rare,
as the bridges rarely change.
another case ofproto_dpif_lookup_by_name looks fine, it will only be called
in the main thread.
>
> >
> > and after finish its usage, you have to do ofproto_unref the ofproto.
> >
> > This is why I said, most accessing to ofproto is ephemeral. If you
> > change to use the pure refcounts solution, you have
> > to add refcount and release it every time you access the ofproto. We
> > should be more careful and remember to unref
> > the ofproto.
> >
> > However, if using RCU and refcounts, in the above case, you do not
> > need to change the code, since the RCU ensures that
> > these ephemeral accesses are safe.
> >
> > you only need to add refcount, when you find that the pointer to
> > ofproto lives longer than one grace period.
> >
> >
> > This is why in my patch, I do not add ref to ofproto after its
> > creation. I agree the patch is not complete and has issues,
> > and understand it could confuse people if changes into mix RCU and
> > refcounts version.
> >
> >
> > William Tu <[email protected]> 于2021年2月26日周五 上午2:32写道：
> >>
> >> On Thu, Feb 25, 2021 at 4:54 AM Ilya Maximets <[email protected]> wrote:
> >>>
> >>> On 2/25/21 4:32 AM, Guohongzhi (Russell Lab) wrote:
> >>>> Refcount and RCU are not mutually exclusive.
> >>>> IMO, the main reason for this problem is that the rule uses both
> >>>> refcount and rcu, while the ofproto uses only rcu, and the rule retains
> >>>> the pointer of the ofproto. More importantly, ofproto cannot guarantee a
> >>>> longer grace period than the rule.
> >>>>
> >>>
> >>> I understand that refcount and RCU are not mutually exclusive.
> >>> However, in this particular case 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.
> >>>
> >>> Now we're introducing refcounts to wait for all rules to be destroyed
> >>> before destroying the ofproto. But what is the point of having
> >>> RCU if we already know that if refcount is zero than all rules are
> >>> already destroyed and we don't need to wait anything and could just
> >>> destroy the ofproto?
> >>>
> >>> RCU doesn't protect ofproto itself here, it actually protects rules,
> >>> i.e. keeps ofproto alive while rules alive, but we can fully replace
> >>> this with refcounts and I see no value in having RCU additionally.
> >>>
> >>> To have a full picture: right now we also have groups protected by
> >>> RCU, so we need to refcount ofproto for them too. But that is almost
> >>> exactly same situation as we have with rules.
> >>>
> >> Thanks Guohongzhi, Hepeng, and Ilya, I learned a lot from your discussions.
> >>
> >> I think refcount and RCU are mutually exclusive. They are two different
> >> ways of
> >> doing synchronization and somehow we mix them together by using RCU to
> >> optimize refcount.
> >>
> >> Before the commit f416c8d61601 ("ofproto: RCU postpone rule destruction."),
> >> we are using refcount to protect rules, and as a result every time OVS
> >> references
> >> a rule, we have to take refcount. It works ok but this probably has
> >> high performance
> >> overhead because it's doing atomic operations.
> >>
> >> So the commit f416c8d61601 optimizes it by doing
> >> 1) not taking refcount of rule "within a grace period"
> >> 2) introduce RCU for rule
> >> The assumption is that a grace period is like refcount == 0, so it's
> >> valid to do so.
> >> A side effect is that ofproto_destroy__() needs to be postponed.
> >> Note that if a rule is alive across grace period, it has to take refcount.
> >>
> >> However, later commit 39c9459355b6 ("Use classifier versioning.")
> >> makes rule alive across grace period. It breaks the first commit's
> >> assumption
> >> and it has to introduce double postponing for ofproto, which we found
> >> out is the problem now.
> >>
> >> Since my point is RCU and refcount are mutually exclusive (A grace period
> >> is like refcount ==0) , I agree with Ilya that we can just use refcount to
> >> fix
> >> ofproto issue.
> >>
> >> Regards,
> >> William
> >
> >
> >
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
