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

Paolo Valerio pvalerio at redhat.com
Mon Nov 8 19:30:07 UTC 2021


Gaëtan Rivet <grive at u256.net> writes:

> On Tue, Nov 2, 2021, at 18:12, Paolo Valerio wrote:
>
> Hi Paolo,
>
> I think this commit needs more details.
> I'm guessing the threads involved are the PMDs and main, are there others?
>
> Coherency is implicit on x86 cores, but those parallel read/writes are
> undefined behavior & possibly incorrect on weaker memory model.
>
> So this patch seems good to me, but it would be nice to have more context
> in the log.
>
> IIRC in the cover letter there is a short description of what this patch does,
> it might be sufficient.
>

Sure

>> Signed-off-by: Paolo Valerio <pvalerio at redhat.com>
>> ---
>>  lib/tnl-neigh-cache.c |   32 +++++++++++++++++++++++---------
>>  1 file changed, 23 insertions(+), 9 deletions(-)
>>
>> diff --git a/lib/tnl-neigh-cache.c b/lib/tnl-neigh-cache.c
>> index 5bda4af7e..2769e5c3d 100644
>> --- a/lib/tnl-neigh-cache.c
>> +++ b/lib/tnl-neigh-cache.c
>> @@ -32,6 +32,7 @@
>>  #include "errno.h"
>>  #include "flow.h"
>>  #include "netdev.h"
>> +#include "ovs-atomic.h"
>>  #include "ovs-thread.h"
>>  #include "packets.h"
>>  #include "openvswitch/poll-loop.h"
>> @@ -44,14 +45,14 @@
>>  #include "openvswitch/vlog.h"
>> 
>> 
>> -/* In seconds */
>> -#define NEIGH_ENTRY_DEFAULT_IDLE_TIME  (15 * 60)
>> +/* In milliseconds */
>> +#define NEIGH_ENTRY_DEFAULT_IDLE_TIME  (15 * 60 * 1000)
>
> It is easier to check correctness of macro usage if the unit is part of
> the name. I think here using 'xxx_MS' would be helpful.
>

ACK

>> 
>>  struct tnl_neigh_entry {
>>      struct cmap_node cmap_node;
>>      struct in6_addr ip;
>>      struct eth_addr mac;
>> -    time_t expires;             /* Expiration time. */
>> +    atomic_llong expires;       /* Expiration time in ms. */
>>      char br_name[IFNAMSIZ];
>>  };
>> 
>> @@ -64,6 +65,16 @@ tnl_neigh_hash(const struct in6_addr *ip)
>>      return hash_bytes(ip->s6_addr, 16, 0);
>>  }
>> 
>> +static bool
>> +tnl_neigh_expired(struct tnl_neigh_entry *neigh)
>> +{
>> +    long long expired;
>
> This is a nit but I would use 'expires', to align with the field name.
>

ACK, will do.

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

In case it's not enough, I guess you're suggesting load/store (on
expires) with explicit acquire/release to enforce that, right?



More information about the dev mailing list