On 13 May 2024, at 17:02, Ilya Maximets wrote:
> On 5/8/24 11:19, Eelco Chaudron wrote:
>> The flow_reval_monitor.py script incorrectly reported the reasons for
>> FDR_PURGE and FDR_TOO_EXPENSIVE, as their descriptions were swapped.
>> This patch rectifies the order using a dictionary to avoid similar
>> problems in the future.
>>
>> In addition this patch also syncs the delete reason output of the
>> script, with the comments in the code.
>>
>> Fixes: 86b9e653ef22 ("revalidator: Add a USDT probe during flow deletion
>> with purge reason.")
>> Signed-off-by: Eelco Chaudron <[email protected]>
>>
>> ---
>> v2: - Converted the list of strings to dictionary.
>> - Added comment to code to keep code and script in sync.
>> - Unified flow_delete reason comments and script output.
>> ---
>> ofproto/ofproto-dpif-upcall.c | 25 ++++++++-------
>> utilities/usdt-scripts/flow_reval_monitor.py | 32 ++++++++++----------
>> 2 files changed, 30 insertions(+), 27 deletions(-)
>>
>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>> index 73901b651..e4d348985 100644
>> --- a/ofproto/ofproto-dpif-upcall.c
>> +++ b/ofproto/ofproto-dpif-upcall.c
>> @@ -1,3 +1,4 @@
>> +
>> /* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016 Nicira, Inc.
>> *
>> * Licensed under the Apache License, Version 2.0 (the "License");
>> @@ -270,18 +271,20 @@ enum ukey_state {
>> };
>> #define N_UKEY_STATES (UKEY_DELETED + 1)
>>
>> +/* Ukey delete reasons used by USDT probes. Please keep in sync with the
>> + * definition in utilities/usdt-scripts/flow_reval_monitor.py. */
>> enum flow_del_reason {
>> - FDR_NONE = 0, /* No deletion reason for the flow. */
>> - FDR_AVOID_CACHING, /* Flow deleted to avoid caching. */
>> - FDR_BAD_ODP_FIT, /* The flow had a bad ODP flow fit. */
>> - FDR_FLOW_IDLE, /* The flow went unused and was deleted. */
>> - FDR_FLOW_LIMIT, /* All flows being killed. */
>> - FDR_FLOW_WILDCARDED, /* The flow needed a narrower wildcard mask. */
>> - FDR_NO_OFPROTO, /* The flow didn't have an associated ofproto.
>> */
>> - FDR_PURGE, /* User action caused flows to be killed. */
>> - FDR_TOO_EXPENSIVE, /* The flow was too expensive to revalidate. */
>> - FDR_UPDATE_FAIL, /* Flow state transition was unexpected. */
>> - FDR_XLATION_ERROR, /* There was an error translating the flow. */
>> + FDR_NONE = 0, /* No delete reason specified. */
>> + FDR_AVOID_CACHING, /* Cache avoidance flag set. */
>> + FDR_BAD_ODP_FIT, /* Bad ODP flow fit. */
>> + FDR_FLOW_IDLE, /* Flow idle timeout. */
>> + FDR_FLOW_LIMIT, /* Kill all flows condition reached. */
>> + FDR_FLOW_WILDCARDED, /* Flow needs a narrower wildcard mask. */
>> + FDR_NO_OFPROTO, /* Flow lacks ofproto dpif association. */
>> + FDR_PURGE, /* User requested flow deletion. */
>> + FDR_TOO_EXPENSIVE, /* Too expensive to revalidate. */
>> + FDR_UPDATE_FAIL, /* Datapath update failed. */
>> + FDR_XLATION_ERROR, /* Flow translation error. */
>> };
>>
>> /* 'udpif_key's are responsible for tracking the little bit of state udpif
>> diff --git a/utilities/usdt-scripts/flow_reval_monitor.py
>> b/utilities/usdt-scripts/flow_reval_monitor.py
>> index 534ba8fa2..bc3a28443 100755
>> --- a/utilities/usdt-scripts/flow_reval_monitor.py
>> +++ b/utilities/usdt-scripts/flow_reval_monitor.py
>> @@ -254,19 +254,19 @@ FdrReasons = IntEnum(
>> start=0,
>> )
>>
>> -FdrReasonStrings = [
>> - "No deletion reason",
>> - "Cache avoidance flag set",
>> - "Bad ODP flow fit",
>> - "Idle flow timed out",
>> - "Kill all flows condition detected",
>> - "Mask too wide - need narrower match",
>> - "No matching ofproto rules",
>> - "Too expensive to revalidate",
>> - "Purged with user action",
>> - "Flow state inconsistent after updates",
>> - "Flow translation error",
>> -]
>
> Should we also have a comment here describing from where these are
> coming from?
>
>> +FdrReasonStrings = {
>> + FdrReasons.FDR_NONE: "No delete reason specified",
>> + FdrReasons.FDR_AVOID_CACHING: "Cache avoidance flag set",
>> + FdrReasons.FDR_BAD_ODP_FIT: "Bad ODP flow fit",
>> + FdrReasons.FDR_FLOW_IDLE: "Flow idle timeout",
>> + FdrReasons.FDR_FLOW_LIMIT: "Kill all flows condition reached",
>> + FdrReasons.FDR_FLOW_WILDCARDED: "Flow needs a narrower wildcard mask",
>> + FdrReasons.FDR_NO_OFPROTO: "Flow lacks ofproto dpif association",
>
> Can we rename this one into "Bridge not found" maybe?
>
> 'ofproto' and 'dpif' are not user-friendly words. 'ofproto' is an entirely
> internal concept.
ACK on both. Just sent out a v3.
//Eelco
>> + FdrReasons.FDR_PURGE: "User requested flow deletion",
>> + FdrReasons.FDR_TOO_EXPENSIVE: "Too expensive to revalidate",
>> + FdrReasons.FDR_UPDATE_FAIL: "Datapath update failed",
>> + FdrReasons.FDR_XLATION_ERROR: "Flow translation error"
>> +}
>>
>>
>> def err(msg, code=-1):
>> @@ -572,10 +572,10 @@ def print_expiration(event):
>> """Prints a UFID eviction with a reason."""
>> ufid_str = format_ufid(event.ufid)
>>
>> - if event.reason > len(FdrReasons):
>> - reason = f"Unknown reason '{event.reason}'"
>> - else:
>> + try:
>> reason = FdrReasonStrings[event.reason]
>> + except KeyError:
>> + reason = f"Unknown reason '{event.reason}'"
>>
>> print(
>> "{:<10} {:<18.9f} {:<36} {:<17}".format(
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
