Gaëtan Rivet <[email protected]> writes: > On Mon, Nov 8, 2021, at 20:30, Paolo Valerio wrote: > [...] >>>> + >>>> + atomic_read_relaxed(&neigh->expires, &expired); >>> >>> I'm having doubts about unfenced read / writes on expires. >>> Technically on lookup there would be reads then writes without barriers. >>> I'm not convinced it makes a difference, WDYT? >>> >> >> Not sure if the store after the conditional here plays a role in >> ordering. >> >>> Otherwise looks good to me. >>> >>>> + >>>> + return expired <= time_msec(); >>>> +} >>>> + >>>> static struct tnl_neigh_entry * >>>> tnl_neigh_lookup__(const char br_name[IFNAMSIZ], const struct in6_addr >>>> *dst) >>>> { >>>> @@ -73,11 +84,12 @@ tnl_neigh_lookup__(const char br_name[IFNAMSIZ], >>>> const struct in6_addr *dst) >>>> hash = tnl_neigh_hash(dst); >>>> CMAP_FOR_EACH_WITH_HASH (neigh, cmap_node, hash, &table) { >>>> if (ipv6_addr_equals(&neigh->ip, dst) && >>>> !strcmp(neigh->br_name, br_name)) { >>>> - if (neigh->expires <= time_now()) { >>>> + if (tnl_neigh_expired(neigh)) { >>>> return NULL; >>>> } >>>> >>>> - neigh->expires = time_now() + >>>> NEIGH_ENTRY_DEFAULT_IDLE_TIME; >>>> + atomic_store_relaxed(&neigh->expires, time_msec() + >>>> + NEIGH_ENTRY_DEFAULT_IDLE_TIME); >>> >>> Here without lock there would be no fence. >>> (this write could be re-ordered before tnl_neigh_expired().) >> >> Can the control dependency avoid reorder in this case? >> Namely, can the write be speculated and committed before the branch >> outcome? > > I searched a little more info on this. > ARM will use the control dependency for ordering, so here it will ensure > the store is not happening before the load. > > Some weaker arch such as Alpha won't rely on the control dependency and > would require load_acquire and store_release. > > I don't think we're targeting this arch. 'tnl_neigh_expired()' would not > make sense without a conditional so I guess it should be fine. >
Thank you Gaetan for double checking. I may miss something here, but apparently, the standard does not guarantee ordering based on control dependency (I suppose for some architectures or potential aggressive compiler optimizations), so even if it could currently work in our case, it could make sense in terms of consistency to promote this explicitly to acquire/release as you suggested. It is also something relatively low rate and adding an acq/rel should not be a big deal. I'd respin for consistency with your first suggestion, if later turns out we really prefer to use relaxed, we can go back to that. >> >> In case it's not enough, I guess you're suggesting load/store (on >> expires) with explicit acquire/release to enforce that, right? > > -- > Gaetan Rivet _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
