On 3 Jun 2024, at 10:07, Roi Dayan wrote:
> On 03/06/2024 10:18, Roi Dayan wrote:
>>
>>
>> On 30/05/2024 18:48, Eelco Chaudron wrote:
>>>
>>>
>>> On 23 May 2024, at 12:46, Roi Dayan via dev 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.
>>>>
>>>> There are some possible situations that may result in stale ukeys that
>>>> have no corresponding DP flows.
>>>>
>>>> In revalidator, push_dp_ops() doesn't check error if the op type is not
>>>> DEL. It is possible that a PUT(MODIFY) fails, especially for tc offload
>>>> case, where the old flow is deleted first and then the new one is
>>>> created. If the creation fails, the ukey will be stale (no corresponding
>>>> DP flow). This patch adds a warning in such case.
>>>
>>> Not sure I understand, this behavior should be captured by the
>>> UKEY_INCONSISTENT state.
>>
>> Hi Eelco,
>>
>> thanks for reviewing.
>> We started the patch on older branch as we didn't rebase for some time
>> and got a little later on sending it.
>> I see the addition now for UKEY_INCOSISTENT in the following patch:
>>
>> cf11766cbcf1 ofproto-dpif-upcall: Fix push_dp_ops to handle all errors.
>>
>> Looks like it handles the same situation we tried to handle in this patch.
>>
>>>
>>>> Another possible scenario is in handle_upcalls() if a PUT operation did
>>>> not succeed and op->error attribute was not set correctly it can lead to
>>>> stale ukey in operational state.
>>>
>>>
>>> Guess we need to figure out these cases, as these are the root cause of
>>> your problem.
>>
>> right. maybe. but this could help keep the system alive/clean while logging
>> the
>> bad situation instead of having increase in those stale/inconsistent ukeys.
>> I understand if it's not nice to handle it like this.
>>
>
> Hi Eelco,
>
> I remember now one of the reproduction scenarios we did is do some traffic
> to get rules added using TC and then delete those from tc command line
> and it got to stale ukeys.
> The revalidator dump thread not seeing the rules so not updating anything
> and sweep over the ukeys skipping them as well.
> This is why we checked against the timing stats of the ukey.
>
> I remember I tested on the upstream code and not our development branch
> and it reproduces. I didn't notice if the commit adding UKEY_INCONSISTENT
> existed but it handles errors from adding flows so I dont think it matters.
>
> I'll try to check and verify again but I think it's still here.
> So while cases getting dop.error handled with UKEY_INCONSISTENT,
> this case I think is not.
I think you are right this case is not covered by the UKEY_INCONSISTENT test
below. See my suggestion on fixing this in revalidate_ukey().
Maybe you could also add a test case for this in the offload test suite.
//Eelco
>>>> This patch adds checks in the sweep phase for such ukeys and move them
>>>> to DELETE so that they can be cleared eventually.
>>>>
>>>> Co-authored-by: Han Zhou <hz...@ovn.org>
>>>> Signed-off-by: Han Zhou <hz...@ovn.org>
>>>> Signed-off-by: Roi Dayan <r...@nvidia.com>
>>>> ---
>>>> ofproto/ofproto-dpif-upcall.c | 15 +++++++++++++++
>>>> 1 file changed, 15 insertions(+)
>>>>
>>>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>>>> index 83609ec62b63..e9520ebdf910 100644
>>>> --- a/ofproto/ofproto-dpif-upcall.c
>>>> +++ b/ofproto/ofproto-dpif-upcall.c
>>>> @@ -57,6 +57,7 @@ COVERAGE_DEFINE(dumped_inconsistent_flow);
>>>> COVERAGE_DEFINE(dumped_new_flow);
>>>> COVERAGE_DEFINE(handler_duplicate_upcall);
>>>> COVERAGE_DEFINE(revalidate_missed_dp_flow);
>>>> +COVERAGE_DEFINE(revalidate_missed_dp_flow_del);
>>>> COVERAGE_DEFINE(ukey_dp_change);
>>>> COVERAGE_DEFINE(ukey_invalid_stat_reset);
>>>> COVERAGE_DEFINE(ukey_replace_contention);
>>>> @@ -278,6 +279,7 @@ enum flow_del_reason {
>>>> FDR_BAD_ODP_FIT, /* Bad ODP flow fit. */
>>>> FDR_FLOW_IDLE, /* Flow idle timeout. */
>>>> FDR_FLOW_LIMIT, /* Kill all flows condition reached. */
>>>> + FDR_FLOW_STALE, /* Flow stale detected. */
>>>
>>> Please also update the associated script, see above comment.
>>>
>>>> FDR_FLOW_WILDCARDED, /* Flow needs a narrower wildcard mask. */
>>>> FDR_NO_OFPROTO, /* Bridge not found. */
>>>> FDR_PURGE, /* User requested flow deletion. */
>>>> @@ -2557,6 +2559,10 @@ push_dp_ops(struct udpif *udpif, struct ukey_op
>>>> *ops, size_t n_ops)
>>>
>>>
>>> The (op->dop.error) condition below will never be reached, as it’s
>>> explicitly checked above. So the error message will never happen. The
>>> condition you explain is already covered by UKEY_INCONSISTENT. Not sure if
>>> we need a log message for this either at warn level, maybe debug,
>>> especially as you say it can happen often.
>>>
>>
>> right. as replied above. I didn't notice this when I did the cherry-pick as
>> git didn't
>> show me a conflict in this area at all.
>> This commit was done before the change so we did hit this spot. we got
>> little off
>> checking the commit and sending it on time for review. sorry about that.
>> cf11766cbcf1 ofproto-dpif-upcall: Fix push_dp_ops to handle all errors.
>>
>> I think we should redo our checks with the commit adding the
>> UKEY_INCONSISTENT
>> state and see our result again.
>>
>> thanks for the review.
>>
>>
>>>> if (op->dop.type != DPIF_OP_FLOW_DEL) {
>>>> /* Only deleted flows need their stats pushed. */
>>>> + if (op->dop.error) {
>>>> + VLOG_WARN_RL(&rl, "push_dp_ops: error %d in op type %d,
>>>> ukey "
>>>> + "%p", op->dop.error, op->dop.type, op->ukey);
>>>> + }
>>>> continue;
>>>> }
>>>>
>>>> @@ -3027,6 +3033,15 @@ revalidator_sweep__(struct revalidator
>>>> *revalidator, bool purge)
>>>> del_reason = purge ? FDR_PURGE : FDR_UPDATE_FAIL;
>>>> } else if (!seq_mismatch) {
>>>> result = UKEY_KEEP;
>>>> + } else if (!ukey->stats.used &&
>>>> + udpif_flow_time_delta(udpif, ukey) * 1000 >
>>>> + ofproto_max_idle) {
>>>
>>> In theory, this is a missed dp flow, i.e. the same as the else {} case
>>> below.
>>> Maybe we should change revalidate_ukey() to have a test for this?
>>>
>>>
>>> Maybe something like this (not tested):
>>>
>>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>>> index e9520ebdf..c34978fc9 100644
>>> --- a/ofproto/ofproto-dpif-upcall.c
>>> +++ b/ofproto/ofproto-dpif-upcall.c
>>> @@ -279,7 +279,7 @@ enum flow_del_reason {
>>> FDR_BAD_ODP_FIT, /* Bad ODP flow fit. */
>>> FDR_FLOW_IDLE, /* Flow idle timeout. */
>>> FDR_FLOW_LIMIT, /* Kill all flows condition reached. */
>>> - FDR_FLOW_STALE, /* Flow stale detected. */
>>> + FDR_FLOW_NO_STATS_IDLE, /* Flow idled out without receiving
>>> statistics. */
>>> FDR_FLOW_WILDCARDED, /* Flow needs a narrower wildcard mask. */
>>> FDR_NO_OFPROTO, /* Bridge not found. */
>>> FDR_PURGE, /* User requested flow deletion. */
>>> @@ -2452,7 +2452,14 @@ revalidate_ukey(struct udpif *udpif, struct
>>> udpif_key *ukey,
>>> log_unexpected_stats_jump(ukey, stats);
>>> }
>>>
>>> - if (need_revalidate) {
>>> + if (!ukey->stats.used
>>> + && ukey->created < udpif->dpif->current_ms - ofproto_max_idle) {
>>> + /* If the flow has a 'used' value of 0, meaning no stats were
>>> received
>>> + * for this flow, and the configured idle time has elapsed, it
>>> might
>>> + * indicates a stale flow (i.e., a flow without an installed
>>> datapath
>>> + * rule). In this case, it is safe to mark this ukey for deletion.
>>> */
>>> + *del_reason = FDR_FLOW_NO_STATS_IDLE;
>>> + } else if (need_revalidate) {
>>> if (should_revalidate(udpif, ukey, push.n_packets)) {
>>> if (!ukey->xcache) {
>>> ukey->xcache = xlate_cache_new();
>>>
>>>
>>> Others, thoughts?
>>>
>>>> + COVERAGE_INC(revalidate_missed_dp_flow_del);
>>>> + VLOG_WARN_RL(&rl, "revalidator_sweep__: Remove stale
>>>> ukey "
>>>> + "%p delta %llus", ukey,
>>>> + udpif_flow_time_delta(udpif, ukey));
>>>> + result = UKEY_DELETE;
>>>> + del_reason = FDR_FLOW_STALE;
>>>> } else {
>>>> struct dpif_flow_stats stats;
>>>> COVERAGE_INC(revalidate_missed_dp_flow);
>>>> --
>>>> 2.21.0
>>>>
>>>> _______________________________________________
>>>> dev mailing list
>>>> d...@openvswitch.org
>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>
>>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
