[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