Hi Mike Intuitionally, I do not think it is a good idea.
if src->ukey_state = DELETED, but dst is to bring it back to ALIVE (some state), then this is a bug, because it breaks the state machine behind. I think the checking we can relax here is only when dst and ukey stat is in EVICTED and DELETED, and this is only because specific state machine we are using there. transition_ukey should be as simple as possible, not related to any specific state machine used. Mike Pattrick <[email protected]>于2022年8月16日 周二22:05写道： > On Tue, Aug 16, 2022 at 4:04 AM Peng He <[email protected]> wrote: > > > > Hi, Mike > > > > by changing it into the function, you mean relax the checking if the > state is > > either EVICTED or DELETED? > > > > In my testing code, I changed it to this: > > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index 57f94df54..224bdfbb8 100644 > --- a/ofproto/ofproto-dpif-upcall.c > +++ b/ofproto/ofproto-dpif-upcall.c > @@ -1937,6 +1937,12 @@ transition_ukey_at(struct udpif_key *ukey, enum > ukey_state dst, > OVS_REQUIRES(ukey->mutex) > { > if (dst < ukey->state) { > + if (ukey->state >= UKEY_EVICTED) { > + VLOG_DBG("Possible race condition on ukey transition %d->%d " > + "(last transitioned from thread %u at > %s)"ukey->state, > + dst, ukey->state_thread, ukey->state_where); > + return; > + } > VLOG_ABORT("Invalid ukey transition %d->%d (last transitioned > from " > "thread %u at %s)", ukey->state, dst, > ukey->state_thread, > ukey->state_where); > > > > > > Mike Pattrick <[email protected]> 于2022年8月15日周一 20:16写道： > >> > >> On Sat, Jun 4, 2022 at 11:19 AM Peng He <[email protected]> wrote: > >> > > >> > There is a race condition between the revalidator threads and > >> > the handler/pmd threads. > >> > > >> > revalidator PMD threads > >> > push_dp_ops deletes a key and tries > >> > to del the dp magaflow. > >> > does the upcall, generates a new > ukey, > >> > and replaces the old ukey, now > the old > >> > ukey state is UKEY_DELETED > >> > > >> > dp_ops succeeds, tries to change > >> > the old ukey's state into > >> > UKEY_EVICTED, however, the old > >> > ukey's state is already UKEY_DELETED, > >> > so OVS aborts. > >> > > >> > I did not observe this in the real environment, as it takes time for > >> > PMDs to finish the upcall and replace the old ukeys. Normally, the > >> > revalidator will change ukey state into UKEY_EVICTED first. > >> > But it's better to cover this case. > >> > >> Hello Peng, > >> > >> I've run into a similar issue by running OVS under Massif, but with a > >> different caller. > >> > >> Do you think it makes sense to move this change into transition_ukey() ? > >> > >> > >> Cheers, > >> M > >> > >> > > >> > Signed-off-by: Peng He <[email protected]> > >> > --- > >> > ofproto/ofproto-dpif-upcall.c | 8 ++++++-- > >> > 1 file changed, 6 insertions(+), 2 deletions(-) > >> > > >> > diff --git a/ofproto/ofproto-dpif-upcall.c > b/ofproto/ofproto-dpif-upcall.c > >> > index 4f68f14f2..9c55e43c1 100644 > >> > --- a/ofproto/ofproto-dpif-upcall.c > >> > +++ b/ofproto/ofproto-dpif-upcall.c > >> > @@ -2411,7 +2411,9 @@ push_dp_ops(struct udpif *udpif, struct ukey_op > *ops, size_t n_ops) > >> > if (op->dop.error) { > >> > if (op->ukey) { > >> > ovs_mutex_lock(&op->ukey->mutex); > >> > - transition_ukey(op->ukey, UKEY_EVICTED); > >> > + if (op->ukey->state < UKEY_EVICTED) { > >> > + transition_ukey(op->ukey, UKEY_EVICTED); > >> > + } > >> > ovs_mutex_unlock(&op->ukey->mutex); > >> > } > >> > /* if it's a flow_del error, 'stats' is unusable, it's ok > >> > @@ -2432,7 +2434,9 @@ push_dp_ops(struct udpif *udpif, struct ukey_op > *ops, size_t n_ops) > >> > > >> > if (op->ukey) { > >> > ovs_mutex_lock(&op->ukey->mutex); > >> > - transition_ukey(op->ukey, UKEY_EVICTED); > >> > + if (op->ukey->state < UKEY_EVICTED) { > >> > + transition_ukey(op->ukey, UKEY_EVICTED); > >> > + } > >> > push->used = MAX(stats->used, op->ukey->stats.used); > >> > push->tcp_flags = stats->tcp_flags | > op->ukey->stats.tcp_flags; > >> > push->n_packets = stats->n_packets - > op->ukey->stats.n_packets; > >> > -- > >> > 2.25.1 > >> > > >> > _______________________________________________ > >> > dev mailing list > >> > [email protected] > >> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >> > > >> > > > > > > -- > > hepeng > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
