| Message ID | 72e49090a874b438d3a12e66cc80883cb9206bf5.1715692534.git.echaudro@redhat.com |
|---|---|
| State | Accepted |
| Commit | 0c8e626401252d0085b65742b9e4c2f682bad7c6 |
| Headers | show |
| Series | [ovs-dev,v3] utilities: Correct deletion reason in flow_reval_monitor.py. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 5/14/24 15:15, 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 <echaudro@redhat.com> > > --- > v3: - Renamed ofproto dpif to bridge in delete reasons. > - Added comment pointing back to delete reasons in .c. > 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. > --- I didn't test, but the code looks fine to me. Thanks! Acked-by: Ilya Maximets <i.maximets@ovn.org> BTW, is there a reason why we can't just report a static string from the USDT probe instead of an integer? If we did that we would not need to have this mapping in the script at all. Best regards, Ilya Maximets.
Eelco Chaudron <echaudro@redhat.com> writes: > 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 <echaudro@redhat.com> > > --- > v3: - Renamed ofproto dpif to bridge in delete reasons. > - Added comment pointing back to delete reasons in .c. > 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. > --- Acked-by: Aaron Conole <aconole@redhat.com>
On 16 May 2024, at 14:46, Ilya Maximets wrote: > On 5/14/24 15:15, 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 <echaudro@redhat.com> >> >> --- >> v3: - Renamed ofproto dpif to bridge in delete reasons. >> - Added comment pointing back to delete reasons in .c. >> 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. >> --- > > I didn't test, but the code looks fine to me. Thanks! > > Acked-by: Ilya Maximets <i.maximets@ovn.org> Thanks Aaron and Ilya, applied upstream. > BTW, is there a reason why we can't just report a static string > from the USDT probe instead of an integer? If we did that we > would not need to have this mapping in the script at all. Did not want to copy them into the ring buffer to save space and CPU. I guess we could copy in the address, but not sure how complex the Python code would become. //Eelco
Eelco Chaudron <echaudro@redhat.com> writes: > On 16 May 2024, at 14:46, Ilya Maximets wrote: > >> On 5/14/24 15:15, 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 <echaudro@redhat.com> >>> >>> --- >>> v3: - Renamed ofproto dpif to bridge in delete reasons. >>> - Added comment pointing back to delete reasons in .c. >>> 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. >>> --- >> >> I didn't test, but the code looks fine to me. Thanks! >> >> Acked-by: Ilya Maximets <i.maximets@ovn.org> > > Thanks Aaron and Ilya, applied upstream. > > >> BTW, is there a reason why we can't just report a static string >> from the USDT probe instead of an integer? If we did that we >> would not need to have this mapping in the script at all. > > Did not want to copy them into the ring buffer to save space and > CPU. I guess we could copy in the address, but not sure how complex > the Python code would become. Let's not try to ship "ephemeral" (since ASLR will move things) addresses around. That feels like a recipe for disaster. I guess one thing we could do is instead publish the strings and attributes in an appctl command and then the python program could read that to populate the fields. $ ovs-appctl usdt-details/flow-del-reasons That might be a way to shift the burden from maintaining this details twice, to just in a single place. > //Eelco
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 73901b651..83609ec62 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -270,18 +270,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, /* Bridge not found. */ + 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..28479a565 100755 --- a/utilities/usdt-scripts/flow_reval_monitor.py +++ b/utilities/usdt-scripts/flow_reval_monitor.py @@ -236,6 +236,11 @@ RevalResult = IntEnum( ], start=0, ) + +# +# The below FdrReasons and FdrReasonStrings definitions can be found in the +# ofproto/ofproto-dpif-upcall.c file. Please keep them in sync. +# FdrReasons = IntEnum( "flow_del_reason", [ @@ -254,19 +259,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", -] +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: "Bridge not found", + 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 +577,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(
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 <echaudro@redhat.com> --- v3: - Renamed ofproto dpif to bridge in delete reasons. - Added comment pointing back to delete reasons in .c. 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 | 24 +++++++------ utilities/usdt-scripts/flow_reval_monitor.py | 37 +++++++++++--------- 2 files changed, 34 insertions(+), 27 deletions(-)