Eelco Chaudron <echau...@redhat.com> 于2022年9月30日周五 23:15写道：
>
>
> On 30 Sep 2022, at 16:46, Peng He wrote:
>
> > Eelco Chaudron <echau...@redhat.com> 于2022年9月30日周五 22:37写道：
> >
> >>
> >>
> >> On 23 Sep 2022, at 18:29, Peng He 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.
> >>
> >> Can you give a call trace example, as try_ukey_replace() should handle
> >> this?
> >>
> >> If it is a valid state change, it should be handled transition_ukey()
> not
> >> by bypassing the call to it.
> >>
> >
> > It means if the ukey->state >= UKEY_EVICTED, it's not needed to do the
> > state transition.
> > It's not a state change. since we have actually two threads moving
> forward
> > the state machine.
>
> Well, I think it should be handled through the state machine, as we still
> take additional actions as if we would have entered this state.
>
> Can you explain what code path is causing this, as try_ukey_replace() will
> fail if it’s not in UKEY_EVICTED state?
>
Yes, I have missed the check in try_ukey_replace(), if so, then the patch
is not needed.
>
> >>
> >>> 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.
> >>>
> >>> Signed-off-by: Peng He <hepeng.0...@bytedance.com>
> >>> ---
> >>> 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 7ea2a30f5..e8bbcfeaf 100644
> >>> --- a/ofproto/ofproto-dpif-upcall.c
> >>> +++ b/ofproto/ofproto-dpif-upcall.c
> >>> @@ -2420,7 +2420,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
> >>> @@ -2441,7 +2443,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
> >>> d...@openvswitch.org
> >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >>
> >>
> >
> > --
> > hepeng
>
>
--
hepeng
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
