On Thu, Aug 29, 2024 at 11:37:07AM +0200, Eelco Chaudron wrote:
>
>
> On 29 Aug 2024, at 10:57, Simon Horman wrote:
>
> > On Tue, Aug 27, 2024 at 11:00:10AM +0200, Eelco Chaudron wrote:
> >> It is observed in some environments that there are much more ukeys than
> >> actual DP flows. For example:
> >>
> >> $ ovs-appctl upcall/show
> >> system@ovs-system:
> >> flows : (current 7) (avg 6) (max 117) (limit 2125)
> >> offloaded flows : 525
> >> dump duration : 1063ms
> >> ufid enabled : true
> >>
> >> 23: (keys 3612)
> >> 24: (keys 3625)
> >> 25: (keys 3485)
> >>
> >> The revalidator threads are busy revalidating the stale ukeys leading to
> >> high CPU and long dump duration.
> >>
> >> This patch tracks the number of consecutive missed dumps. If four dumps
> >> are missed in a row, it is assumed that the datapath flow no longer
> >> exists, and the ukey can be deleted.
> >>
> >> Reported-by: Roi Dayan <[email protected]>
> >> Co-authored-by: Han Zhou <[email protected]>
> >> Co-authored-by: Roi Dayan <[email protected]>
> >> Signed-off-by: Han Zhou <[email protected]>
> >> Signed-off-by: Roi Dayan <[email protected]>
> >> Signed-off-by: Eelco Chaudron <[email protected]>
...
> >> @@ -318,6 +320,7 @@ struct udpif_key {
> >> uint64_t dump_seq OVS_GUARDED; /* Tracks udpif->dump_seq.
> >> */
> >> uint64_t reval_seq OVS_GUARDED; /* Tracks udpif->reval_seq.
> >> */
> >> enum ukey_state state OVS_GUARDED; /* Tracks ukey lifetime. */
> >> + uint32_t missed_dumps OVS_GUARDED; /* Missed consecutive
> >> dumps. */
> >>
> >> /* 'state' debug information. */
> >> unsigned int state_thread OVS_GUARDED; /* Thread that transitions.
> >> */
> >> @@ -3040,6 +3043,21 @@ revalidator_sweep__(struct revalidator
> >> *revalidator, bool purge)
> >> result = revalidate_ukey(udpif, ukey, &stats,
> >> &odp_actions,
> >> reval_seq, &recircs,
> >> &del_reason);
> >> }
> >
> > Hi Eelco,
> >
> > In practice this probably doesn't make much difference, but it caught my
> > eye.
> >
> > Is it intentional that the logic below may override
> > del_reason and result even in cases where result is
> > set to a value other than UKEY_KEEP above (not visible in diff)?
> >
> > IOW, is it intentional that this reason overrides the others,
> > even after work has been done determining the others?
>
> Thanks for reviewing this Simon.
>
> Yes, this is intentional, as there was no straightforward way to incorporate
> this check into the if-else-if chain.
>
> I did consider moving this on top of the chain, but than all existing parts
> would be indented in the below else chain, which looked messy.
Thanks, in that case I have nothing further to add.
Acked-by: Simon Horman <[email protected]>
...
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
