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

Gaëtan Rivet grive at u256.net
Tue Nov 9 10:51:56 UTC 2021


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.

>
> 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