Gaëtan Rivet <gr...@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 <pvale...@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? _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev