[ovs-dev] [PATCH 1/4] Native tunnel: Read/write expires atomically.

Paolo Valerio pvalerio at redhat.com
Tue Nov 9 22:05:53 UTC 2021


Gaëtan Rivet <grive at u256.net> 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



More information about the dev mailing list