Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
On 21 Feb 2024, at 17:45, Adrian Moreno wrote: > On 2/21/24 16:42, Eelco Chaudron wrote: >> >> >> On 21 Feb 2024, at 16:03, Adrian Moreno wrote: >> >>> On 2/21/24 15:49, Aaron Conole wrote: Adrian Moreno writes: > On 2/20/24 19:06, Aaron Conole wrote: >> Eelco Chaudron writes: >> >>> On 19 Feb 2024, at 19:57, Aaron Conole wrote: >>> Eelco Chaudron writes: > On 12 Feb 2024, at 15:15, Aaron Conole wrote: > >> Aaron Conole writes: >> >>> Eelco Chaudron writes: >>> On 2 Feb 2024, at 11:31, Adrian Moreno wrote: > On 2/1/24 10:02, Eelco Chaudron wrote: >> >> >> On 31 Jan 2024, at 18:03, Aaron Conole wrote: >> >>> Eelco Chaudron writes: >>> On 25 Jan 2024, at 21:55, Aaron Conole wrote: > From: Kevin Sprague > > During normal operations, it is useful to understand when a > particular flow > gets removed from the system. This can be useful when > debugging performance > issues tied to ofproto flow changes, trying to determine > deployed traffic > patterns, or while debugging dynamic systems where ports come > and go. > > Prior to this change, there was a lack of visibility around > flow expiration. > The existing debugging infrastructure could tell us when a > flow was added to > the datapath, but not when it was removed or why. > > This change introduces a USDT probe at the point where the > revalidator > determines that the flow should be removed. Additionally, we > track the > reason for the flow eviction and provide that information as > well. With > this change, we can track the complete flow lifecycle for the > netlink > datapath by hooking the upcall tracepoint in kernel, the flow > put USDT, and > the revaldiator USDT, letting us watch as flows are added and > removed from > the kernel datapath. > > This change only enables this information via USDT probe, so > it won't be > possible to access this information any other way (see: > Documentation/topics/usdt-probes.rst). > > Also included is a script > (utilities/usdt-scripts/flow_reval_monitor.py) > which serves as a demonstration of how the new USDT probe > might be used > going forward. > > Signed-off-by: Kevin Sprague > Co-authored-by: Aaron Conole > Signed-off-by: Aaron Conole Thanks for following this up Aaron! See comments on this patch below. I have no additional comments on patch 2. Cheers, Eelco > --- > Documentation/topics/usdt-probes.rst | 1 + > ofproto/ofproto-dpif-upcall.c| 42 +- > utilities/automake.mk| 3 + > utilities/usdt-scripts/flow_reval_monitor.py | 653 > +++ > 4 files changed, 693 insertions(+), 6 deletions(-) > create mode 100755 > utilities/usdt-scripts/flow_reval_monitor.py > > diff --git a/Documentation/topics/usdt-probes.rst > b/Documentation/topics/usdt-probes.rst > index e527f43bab..a8da9bb1f7 100644 > --- a/Documentation/topics/usdt-probes.rst > +++ b/Documentation/topics/usdt-probes.rst > @@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``: > - dpif_recv:recv_upcall > - main:poll_block > - main:run_start > +- revalidate:flow_result > - revalidate_ukey\_\_:entry > - revalidate_ukey\_\_:exit > - udpif_revalidator:start_dump You are missing the specific flow_result result > section. This is from the previous patch: >>> >>> D'oh! Thanks for catching it. I'll re-add it. >>> @@ -358,6 +360,27 @@ See also the ``main:run_start`` probe
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
On 2/21/24 16:42, Eelco Chaudron wrote:
On 21 Feb 2024, at 16:03, Adrian Moreno wrote:
On 2/21/24 15:49, Aaron Conole wrote:
Adrian Moreno writes:
On 2/20/24 19:06, Aaron Conole wrote:
Eelco Chaudron writes:
On 19 Feb 2024, at 19:57, Aaron Conole wrote:
Eelco Chaudron writes:
On 12 Feb 2024, at 15:15, Aaron Conole wrote:
Aaron Conole writes:
Eelco Chaudron writes:
On 2 Feb 2024, at 11:31, Adrian Moreno wrote:
On 2/1/24 10:02, Eelco Chaudron wrote:
On 31 Jan 2024, at 18:03, Aaron Conole wrote:
Eelco Chaudron writes:
On 25 Jan 2024, at 21:55, Aaron Conole wrote:
From: Kevin Sprague
During normal operations, it is useful to understand when a particular flow
gets removed from the system. This can be useful when debugging performance
issues tied to ofproto flow changes, trying to determine deployed traffic
patterns, or while debugging dynamic systems where ports come and go.
Prior to this change, there was a lack of visibility around flow expiration.
The existing debugging infrastructure could tell us when a flow was added to
the datapath, but not when it was removed or why.
This change introduces a USDT probe at the point where the revalidator
determines that the flow should be removed. Additionally, we track the
reason for the flow eviction and provide that information as well. With
this change, we can track the complete flow lifecycle for the netlink
datapath by hooking the upcall tracepoint in kernel, the flow put USDT, and
the revaldiator USDT, letting us watch as flows are added and removed from
the kernel datapath.
This change only enables this information via USDT probe, so it won't be
possible to access this information any other way (see:
Documentation/topics/usdt-probes.rst).
Also included is a script (utilities/usdt-scripts/flow_reval_monitor.py)
which serves as a demonstration of how the new USDT probe might be used
going forward.
Signed-off-by: Kevin Sprague
Co-authored-by: Aaron Conole
Signed-off-by: Aaron Conole
Thanks for following this up Aaron! See comments on this patch
below. I have no additional comments on patch 2.
Cheers,
Eelco
---
Documentation/topics/usdt-probes.rst | 1 +
ofproto/ofproto-dpif-upcall.c| 42 +-
utilities/automake.mk| 3 +
utilities/usdt-scripts/flow_reval_monitor.py | 653 +++
4 files changed, 693 insertions(+), 6 deletions(-)
create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py
diff --git a/Documentation/topics/usdt-probes.rst
b/Documentation/topics/usdt-probes.rst
index e527f43bab..a8da9bb1f7 100644
--- a/Documentation/topics/usdt-probes.rst
+++ b/Documentation/topics/usdt-probes.rst
@@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``:
- dpif_recv:recv_upcall
- main:poll_block
- main:run_start
+- revalidate:flow_result
- revalidate_ukey\_\_:entry
- revalidate_ukey\_\_:exit
- udpif_revalidator:start_dump
You are missing the specific flow_result result
section. This is from the previous patch:
D'oh! Thanks for catching it. I'll re-add it.
@@ -358,6 +360,27 @@ See also the ``main:run_start`` probe above.
- ``utilities/usdt-scripts/bridge_loop.bt``
+probe revalidate:flow_result
+~
+
+**Description**:
+This probe is triggered when the revalidator decides whether or not to
+revalidate a flow. ``reason`` is an enum that denotes that either the flow
+is being kept, or the reason why the flow is being deleted. The
+``flow_reval_monitor.py`` script uses this probe to notify users when flows
+matching user-provided criteria are deleted.
+
+**Arguments**:
+
+- *arg0*: ``(enum flow_del_reason) reason``
+- *arg1*: ``(struct udpif *) udpif``
+- *arg2*: ``(struct udpif_key *) ukey``
+
+**Script references**:
+
+- ``utilities/usdt-scripts/flow_reval_monitor.py``
+
+
Adding your own probes
--
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index b5cbeed878..97d75833f7 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -269,6 +269,18 @@ enum ukey_state {
};
#define N_UKEY_STATES (UKEY_DELETED + 1)
+enum flow_del_reason {
+FDR_REVALIDATE = 0, /* The flow was revalidated. */
It was called FDR_FLOW_LIVE before, which might make more
sense. As the flow is just NOT deleted. It might or might not have
been revalidated. Thoughts?
I think it had to have been revalidated if we emit the reason, because
we only emit the reason code after revalidation. IE: there are many
places where we skip revalidation but the flow stays live - and we don't
emit reasons in those cases.
So at least for this patch, it MUST have been revalidated. But maybe in
the future, we would want to catch cases where the flow hasn't been. In
that case, it makes sense to add the FDR_FLOW_LIVE at that time - I
think.
Maybe you disagree?
Well, it depends on how you
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
On 21 Feb 2024, at 16:03, Adrian Moreno wrote: > On 2/21/24 15:49, Aaron Conole wrote: >> Adrian Moreno writes: >> >>> On 2/20/24 19:06, Aaron Conole wrote: Eelco Chaudron writes: > On 19 Feb 2024, at 19:57, Aaron Conole wrote: > >> Eelco Chaudron writes: >> >>> On 12 Feb 2024, at 15:15, Aaron Conole wrote: >>> Aaron Conole writes: > Eelco Chaudron writes: > >> On 2 Feb 2024, at 11:31, Adrian Moreno wrote: >> >>> On 2/1/24 10:02, Eelco Chaudron wrote: On 31 Jan 2024, at 18:03, Aaron Conole wrote: > Eelco Chaudron writes: > >> On 25 Jan 2024, at 21:55, Aaron Conole wrote: >> >>> From: Kevin Sprague >>> >>> During normal operations, it is useful to understand when a >>> particular flow >>> gets removed from the system. This can be useful when debugging >>> performance >>> issues tied to ofproto flow changes, trying to determine >>> deployed traffic >>> patterns, or while debugging dynamic systems where ports come >>> and go. >>> >>> Prior to this change, there was a lack of visibility around >>> flow expiration. >>> The existing debugging infrastructure could tell us when a flow >>> was added to >>> the datapath, but not when it was removed or why. >>> >>> This change introduces a USDT probe at the point where the >>> revalidator >>> determines that the flow should be removed. Additionally, we >>> track the >>> reason for the flow eviction and provide that information as >>> well. With >>> this change, we can track the complete flow lifecycle for the >>> netlink >>> datapath by hooking the upcall tracepoint in kernel, the flow >>> put USDT, and >>> the revaldiator USDT, letting us watch as flows are added and >>> removed from >>> the kernel datapath. >>> >>> This change only enables this information via USDT probe, so it >>> won't be >>> possible to access this information any other way (see: >>> Documentation/topics/usdt-probes.rst). >>> >>> Also included is a script >>> (utilities/usdt-scripts/flow_reval_monitor.py) >>> which serves as a demonstration of how the new USDT probe might >>> be used >>> going forward. >>> >>> Signed-off-by: Kevin Sprague >>> Co-authored-by: Aaron Conole >>> Signed-off-by: Aaron Conole >> >> Thanks for following this up Aaron! See comments on this patch >> below. I have no additional comments on patch 2. >> >> Cheers, >> >> Eelco >> >> >>> --- >>> Documentation/topics/usdt-probes.rst | 1 + >>> ofproto/ofproto-dpif-upcall.c| 42 +- >>> utilities/automake.mk| 3 + >>> utilities/usdt-scripts/flow_reval_monitor.py | 653 >>> +++ >>> 4 files changed, 693 insertions(+), 6 deletions(-) >>> create mode 100755 >>> utilities/usdt-scripts/flow_reval_monitor.py >>> >>> diff --git a/Documentation/topics/usdt-probes.rst >>> b/Documentation/topics/usdt-probes.rst >>> index e527f43bab..a8da9bb1f7 100644 >>> --- a/Documentation/topics/usdt-probes.rst >>> +++ b/Documentation/topics/usdt-probes.rst >>> @@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``: >>> - dpif_recv:recv_upcall >>> - main:poll_block >>> - main:run_start >>> +- revalidate:flow_result >>> - revalidate_ukey\_\_:entry >>> - revalidate_ukey\_\_:exit >>> - udpif_revalidator:start_dump >> >> You are missing the specific flow_result result >>> section. This is from the previous patch: > > D'oh! Thanks for catching it. I'll re-add it. > >> @@ -358,6 +360,27 @@ See also the ``main:run_start`` probe >> above. >> - ``utilities/usdt-scripts/bridge_loop.bt`` >> >> >> +probe revalidate:flow_result >> +~ >> + >> +**Description**: >> +This probe is triggered when the revalidator decides whether or >>
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
On 2/21/24 15:49, Aaron Conole wrote:
Adrian Moreno writes:
On 2/20/24 19:06, Aaron Conole wrote:
Eelco Chaudron writes:
On 19 Feb 2024, at 19:57, Aaron Conole wrote:
Eelco Chaudron writes:
On 12 Feb 2024, at 15:15, Aaron Conole wrote:
Aaron Conole writes:
Eelco Chaudron writes:
On 2 Feb 2024, at 11:31, Adrian Moreno wrote:
On 2/1/24 10:02, Eelco Chaudron wrote:
On 31 Jan 2024, at 18:03, Aaron Conole wrote:
Eelco Chaudron writes:
On 25 Jan 2024, at 21:55, Aaron Conole wrote:
From: Kevin Sprague
During normal operations, it is useful to understand when a particular flow
gets removed from the system. This can be useful when debugging performance
issues tied to ofproto flow changes, trying to determine deployed traffic
patterns, or while debugging dynamic systems where ports come and go.
Prior to this change, there was a lack of visibility around flow expiration.
The existing debugging infrastructure could tell us when a flow was added to
the datapath, but not when it was removed or why.
This change introduces a USDT probe at the point where the revalidator
determines that the flow should be removed. Additionally, we track the
reason for the flow eviction and provide that information as well. With
this change, we can track the complete flow lifecycle for the netlink
datapath by hooking the upcall tracepoint in kernel, the flow put USDT, and
the revaldiator USDT, letting us watch as flows are added and removed from
the kernel datapath.
This change only enables this information via USDT probe, so it won't be
possible to access this information any other way (see:
Documentation/topics/usdt-probes.rst).
Also included is a script (utilities/usdt-scripts/flow_reval_monitor.py)
which serves as a demonstration of how the new USDT probe might be used
going forward.
Signed-off-by: Kevin Sprague
Co-authored-by: Aaron Conole
Signed-off-by: Aaron Conole
Thanks for following this up Aaron! See comments on this patch
below. I have no additional comments on patch 2.
Cheers,
Eelco
---
Documentation/topics/usdt-probes.rst | 1 +
ofproto/ofproto-dpif-upcall.c| 42 +-
utilities/automake.mk| 3 +
utilities/usdt-scripts/flow_reval_monitor.py | 653 +++
4 files changed, 693 insertions(+), 6 deletions(-)
create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py
diff --git a/Documentation/topics/usdt-probes.rst
b/Documentation/topics/usdt-probes.rst
index e527f43bab..a8da9bb1f7 100644
--- a/Documentation/topics/usdt-probes.rst
+++ b/Documentation/topics/usdt-probes.rst
@@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``:
- dpif_recv:recv_upcall
- main:poll_block
- main:run_start
+- revalidate:flow_result
- revalidate_ukey\_\_:entry
- revalidate_ukey\_\_:exit
- udpif_revalidator:start_dump
You are missing the specific flow_result result
section. This is from the previous patch:
D'oh! Thanks for catching it. I'll re-add it.
@@ -358,6 +360,27 @@ See also the ``main:run_start`` probe above.
- ``utilities/usdt-scripts/bridge_loop.bt``
+probe revalidate:flow_result
+~
+
+**Description**:
+This probe is triggered when the revalidator decides whether or not to
+revalidate a flow. ``reason`` is an enum that denotes that either the flow
+is being kept, or the reason why the flow is being deleted. The
+``flow_reval_monitor.py`` script uses this probe to notify users when flows
+matching user-provided criteria are deleted.
+
+**Arguments**:
+
+- *arg0*: ``(enum flow_del_reason) reason``
+- *arg1*: ``(struct udpif *) udpif``
+- *arg2*: ``(struct udpif_key *) ukey``
+
+**Script references**:
+
+- ``utilities/usdt-scripts/flow_reval_monitor.py``
+
+
Adding your own probes
--
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index b5cbeed878..97d75833f7 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -269,6 +269,18 @@ enum ukey_state {
};
#define N_UKEY_STATES (UKEY_DELETED + 1)
+enum flow_del_reason {
+FDR_REVALIDATE = 0, /* The flow was revalidated. */
It was called FDR_FLOW_LIVE before, which might make more
sense. As the flow is just NOT deleted. It might or might not have
been revalidated. Thoughts?
I think it had to have been revalidated if we emit the reason, because
we only emit the reason code after revalidation. IE: there are many
places where we skip revalidation but the flow stays live - and we don't
emit reasons in those cases.
So at least for this patch, it MUST have been revalidated. But maybe in
the future, we would want to catch cases where the flow hasn't been. In
that case, it makes sense to add the FDR_FLOW_LIVE at that time - I
think.
Maybe you disagree?
Well, it depends on how you define revalidation, it might only have
updated the counters. i.e. it all depends on ‘bool need_revalidate =
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
Adrian Moreno writes: > On 2/20/24 19:06, Aaron Conole wrote: >> Eelco Chaudron writes: >> >>> On 19 Feb 2024, at 19:57, Aaron Conole wrote: >>> Eelco Chaudron writes: > On 12 Feb 2024, at 15:15, Aaron Conole wrote: > >> Aaron Conole writes: >> >>> Eelco Chaudron writes: >>> On 2 Feb 2024, at 11:31, Adrian Moreno wrote: > On 2/1/24 10:02, Eelco Chaudron wrote: >> >> >> On 31 Jan 2024, at 18:03, Aaron Conole wrote: >> >>> Eelco Chaudron writes: >>> On 25 Jan 2024, at 21:55, Aaron Conole wrote: > From: Kevin Sprague > > During normal operations, it is useful to understand when a > particular flow > gets removed from the system. This can be useful when debugging > performance > issues tied to ofproto flow changes, trying to determine deployed > traffic > patterns, or while debugging dynamic systems where ports come and > go. > > Prior to this change, there was a lack of visibility around flow > expiration. > The existing debugging infrastructure could tell us when a flow > was added to > the datapath, but not when it was removed or why. > > This change introduces a USDT probe at the point where the > revalidator > determines that the flow should be removed. Additionally, we > track the > reason for the flow eviction and provide that information as > well. With > this change, we can track the complete flow lifecycle for the > netlink > datapath by hooking the upcall tracepoint in kernel, the flow put > USDT, and > the revaldiator USDT, letting us watch as flows are added and > removed from > the kernel datapath. > > This change only enables this information via USDT probe, so it > won't be > possible to access this information any other way (see: > Documentation/topics/usdt-probes.rst). > > Also included is a script > (utilities/usdt-scripts/flow_reval_monitor.py) > which serves as a demonstration of how the new USDT probe might > be used > going forward. > > Signed-off-by: Kevin Sprague > Co-authored-by: Aaron Conole > Signed-off-by: Aaron Conole Thanks for following this up Aaron! See comments on this patch below. I have no additional comments on patch 2. Cheers, Eelco > --- >Documentation/topics/usdt-probes.rst | 1 + >ofproto/ofproto-dpif-upcall.c| 42 +- >utilities/automake.mk| 3 + >utilities/usdt-scripts/flow_reval_monitor.py | 653 > +++ >4 files changed, 693 insertions(+), 6 deletions(-) >create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py > > diff --git a/Documentation/topics/usdt-probes.rst > b/Documentation/topics/usdt-probes.rst > index e527f43bab..a8da9bb1f7 100644 > --- a/Documentation/topics/usdt-probes.rst > +++ b/Documentation/topics/usdt-probes.rst > @@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``: >- dpif_recv:recv_upcall >- main:poll_block >- main:run_start > +- revalidate:flow_result >- revalidate_ukey\_\_:entry >- revalidate_ukey\_\_:exit >- udpif_revalidator:start_dump You are missing the specific flow_result result > section. This is from the previous patch: >>> >>> D'oh! Thanks for catching it. I'll re-add it. >>> @@ -358,6 +360,27 @@ See also the ``main:run_start`` probe above. - ``utilities/usdt-scripts/bridge_loop.bt`` +probe revalidate:flow_result +~ + +**Description**: +This probe is triggered when the revalidator decides whether or not to +revalidate a flow. ``reason`` is an enum that denotes that either the flow +is being kept, or the reason why the flow is being deleted. The +``flow_reval_monitor.py`` script uses this probe to notify users when flows +matching user-provided criteria are deleted.
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
On 2/20/24 19:06, Aaron Conole wrote:
Eelco Chaudron writes:
On 19 Feb 2024, at 19:57, Aaron Conole wrote:
Eelco Chaudron writes:
On 12 Feb 2024, at 15:15, Aaron Conole wrote:
Aaron Conole writes:
Eelco Chaudron writes:
On 2 Feb 2024, at 11:31, Adrian Moreno wrote:
On 2/1/24 10:02, Eelco Chaudron wrote:
On 31 Jan 2024, at 18:03, Aaron Conole wrote:
Eelco Chaudron writes:
On 25 Jan 2024, at 21:55, Aaron Conole wrote:
From: Kevin Sprague
During normal operations, it is useful to understand when a particular flow
gets removed from the system. This can be useful when debugging performance
issues tied to ofproto flow changes, trying to determine deployed traffic
patterns, or while debugging dynamic systems where ports come and go.
Prior to this change, there was a lack of visibility around flow expiration.
The existing debugging infrastructure could tell us when a flow was added to
the datapath, but not when it was removed or why.
This change introduces a USDT probe at the point where the revalidator
determines that the flow should be removed. Additionally, we track the
reason for the flow eviction and provide that information as well. With
this change, we can track the complete flow lifecycle for the netlink
datapath by hooking the upcall tracepoint in kernel, the flow put USDT, and
the revaldiator USDT, letting us watch as flows are added and removed from
the kernel datapath.
This change only enables this information via USDT probe, so it won't be
possible to access this information any other way (see:
Documentation/topics/usdt-probes.rst).
Also included is a script (utilities/usdt-scripts/flow_reval_monitor.py)
which serves as a demonstration of how the new USDT probe might be used
going forward.
Signed-off-by: Kevin Sprague
Co-authored-by: Aaron Conole
Signed-off-by: Aaron Conole
Thanks for following this up Aaron! See comments on this patch
below. I have no additional comments on patch 2.
Cheers,
Eelco
---
Documentation/topics/usdt-probes.rst | 1 +
ofproto/ofproto-dpif-upcall.c| 42 +-
utilities/automake.mk| 3 +
utilities/usdt-scripts/flow_reval_monitor.py | 653 +++
4 files changed, 693 insertions(+), 6 deletions(-)
create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py
diff --git a/Documentation/topics/usdt-probes.rst
b/Documentation/topics/usdt-probes.rst
index e527f43bab..a8da9bb1f7 100644
--- a/Documentation/topics/usdt-probes.rst
+++ b/Documentation/topics/usdt-probes.rst
@@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``:
- dpif_recv:recv_upcall
- main:poll_block
- main:run_start
+- revalidate:flow_result
- revalidate_ukey\_\_:entry
- revalidate_ukey\_\_:exit
- udpif_revalidator:start_dump
You are missing the specific flow_result result section. This is from the
previous patch:
D'oh! Thanks for catching it. I'll re-add it.
@@ -358,6 +360,27 @@ See also the ``main:run_start`` probe above.
- ``utilities/usdt-scripts/bridge_loop.bt``
+probe revalidate:flow_result
+~
+
+**Description**:
+This probe is triggered when the revalidator decides whether or not to
+revalidate a flow. ``reason`` is an enum that denotes that either the flow
+is being kept, or the reason why the flow is being deleted. The
+``flow_reval_monitor.py`` script uses this probe to notify users when flows
+matching user-provided criteria are deleted.
+
+**Arguments**:
+
+- *arg0*: ``(enum flow_del_reason) reason``
+- *arg1*: ``(struct udpif *) udpif``
+- *arg2*: ``(struct udpif_key *) ukey``
+
+**Script references**:
+
+- ``utilities/usdt-scripts/flow_reval_monitor.py``
+
+
Adding your own probes
--
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index b5cbeed878..97d75833f7 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -269,6 +269,18 @@ enum ukey_state {
};
#define N_UKEY_STATES (UKEY_DELETED + 1)
+enum flow_del_reason {
+FDR_REVALIDATE = 0, /* The flow was revalidated. */
It was called FDR_FLOW_LIVE before, which might make more
sense. As the flow is just NOT deleted. It might or might not have
been revalidated. Thoughts?
I think it had to have been revalidated if we emit the reason, because
we only emit the reason code after revalidation. IE: there are many
places where we skip revalidation but the flow stays live - and we don't
emit reasons in those cases.
So at least for this patch, it MUST have been revalidated. But maybe in
the future, we would want to catch cases where the flow hasn't been. In
that case, it makes sense to add the FDR_FLOW_LIVE at that time - I
think.
Maybe you disagree?
Well, it depends on how you define revalidation, it might only have
updated the counters. i.e. it all depends on ‘bool need_revalidate =
ukey->reval_seq != reval_seq;’ in revalidate_ukey(). That was why I
opted for a m
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
On 20 Feb 2024, at 19:10, Adrian Moreno wrote: > On 2/20/24 13:43, Eelco Chaudron wrote: >> >> >> On 19 Feb 2024, at 19:57, Aaron Conole wrote: >> >>> Eelco Chaudron writes: >>> On 12 Feb 2024, at 15:15, Aaron Conole wrote: > Aaron Conole writes: > >> Eelco Chaudron writes: >> >>> On 2 Feb 2024, at 11:31, Adrian Moreno wrote: >>> On 2/1/24 10:02, Eelco Chaudron wrote: > > > On 31 Jan 2024, at 18:03, Aaron Conole wrote: > >> Eelco Chaudron writes: >> >>> On 25 Jan 2024, at 21:55, Aaron Conole wrote: >>> From: Kevin Sprague During normal operations, it is useful to understand when a particular flow gets removed from the system. This can be useful when debugging performance issues tied to ofproto flow changes, trying to determine deployed traffic patterns, or while debugging dynamic systems where ports come and go. Prior to this change, there was a lack of visibility around flow expiration. The existing debugging infrastructure could tell us when a flow was added to the datapath, but not when it was removed or why. This change introduces a USDT probe at the point where the revalidator determines that the flow should be removed. Additionally, we track the reason for the flow eviction and provide that information as well. With this change, we can track the complete flow lifecycle for the netlink datapath by hooking the upcall tracepoint in kernel, the flow put USDT, and the revaldiator USDT, letting us watch as flows are added and removed from the kernel datapath. This change only enables this information via USDT probe, so it won't be possible to access this information any other way (see: Documentation/topics/usdt-probes.rst). Also included is a script (utilities/usdt-scripts/flow_reval_monitor.py) which serves as a demonstration of how the new USDT probe might be used going forward. Signed-off-by: Kevin Sprague Co-authored-by: Aaron Conole Signed-off-by: Aaron Conole >>> >>> Thanks for following this up Aaron! See comments on this patch >>> below. I have no additional comments on patch 2. >>> >>> Cheers, >>> >>> Eelco >>> >>> --- Documentation/topics/usdt-probes.rst | 1 + ofproto/ofproto-dpif-upcall.c| 42 +- utilities/automake.mk| 3 + utilities/usdt-scripts/flow_reval_monitor.py | 653 +++ 4 files changed, 693 insertions(+), 6 deletions(-) create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py diff --git a/Documentation/topics/usdt-probes.rst b/Documentation/topics/usdt-probes.rst index e527f43bab..a8da9bb1f7 100644 --- a/Documentation/topics/usdt-probes.rst +++ b/Documentation/topics/usdt-probes.rst @@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``: - dpif_recv:recv_upcall - main:poll_block - main:run_start +- revalidate:flow_result - revalidate_ukey\_\_:entry - revalidate_ukey\_\_:exit - udpif_revalidator:start_dump >>> >>> You are missing the specific flow_result result section. This is >>> from the previous patch: >> >> D'oh! Thanks for catching it. I'll re-add it. >> >>> @@ -358,6 +360,27 @@ See also the ``main:run_start`` probe above. >>>- ``utilities/usdt-scripts/bridge_loop.bt`` >>> >>> >>> +probe revalidate:flow_result >>> +~ >>> + >>> +**Description**: >>> +This probe is triggered when the revalidator decides whether or >>> not to >>> +revalidate a flow. ``reason`` is an enum that denotes that either >>> the flow >>> +is being kept, or the reason why the flow is being deleted. The >>> +``flow_reval_monitor.py`` script uses this probe to notify users >>> when flows >>> +matching user-provided criteria are deleted. >>> + >>> +**Arguments**: >>> + >>> +- *arg0*: ``(enum flow_del_reason)
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
On 2/20/24 13:43, Eelco Chaudron wrote:
On 19 Feb 2024, at 19:57, Aaron Conole wrote:
Eelco Chaudron writes:
On 12 Feb 2024, at 15:15, Aaron Conole wrote:
Aaron Conole writes:
Eelco Chaudron writes:
On 2 Feb 2024, at 11:31, Adrian Moreno wrote:
On 2/1/24 10:02, Eelco Chaudron wrote:
On 31 Jan 2024, at 18:03, Aaron Conole wrote:
Eelco Chaudron writes:
On 25 Jan 2024, at 21:55, Aaron Conole wrote:
From: Kevin Sprague
During normal operations, it is useful to understand when a particular flow
gets removed from the system. This can be useful when debugging performance
issues tied to ofproto flow changes, trying to determine deployed traffic
patterns, or while debugging dynamic systems where ports come and go.
Prior to this change, there was a lack of visibility around flow expiration.
The existing debugging infrastructure could tell us when a flow was added to
the datapath, but not when it was removed or why.
This change introduces a USDT probe at the point where the revalidator
determines that the flow should be removed. Additionally, we track the
reason for the flow eviction and provide that information as well. With
this change, we can track the complete flow lifecycle for the netlink
datapath by hooking the upcall tracepoint in kernel, the flow put USDT, and
the revaldiator USDT, letting us watch as flows are added and removed from
the kernel datapath.
This change only enables this information via USDT probe, so it won't be
possible to access this information any other way (see:
Documentation/topics/usdt-probes.rst).
Also included is a script (utilities/usdt-scripts/flow_reval_monitor.py)
which serves as a demonstration of how the new USDT probe might be used
going forward.
Signed-off-by: Kevin Sprague
Co-authored-by: Aaron Conole
Signed-off-by: Aaron Conole
Thanks for following this up Aaron! See comments on this patch
below. I have no additional comments on patch 2.
Cheers,
Eelco
---
Documentation/topics/usdt-probes.rst | 1 +
ofproto/ofproto-dpif-upcall.c| 42 +-
utilities/automake.mk| 3 +
utilities/usdt-scripts/flow_reval_monitor.py | 653 +++
4 files changed, 693 insertions(+), 6 deletions(-)
create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py
diff --git a/Documentation/topics/usdt-probes.rst
b/Documentation/topics/usdt-probes.rst
index e527f43bab..a8da9bb1f7 100644
--- a/Documentation/topics/usdt-probes.rst
+++ b/Documentation/topics/usdt-probes.rst
@@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``:
- dpif_recv:recv_upcall
- main:poll_block
- main:run_start
+- revalidate:flow_result
- revalidate_ukey\_\_:entry
- revalidate_ukey\_\_:exit
- udpif_revalidator:start_dump
You are missing the specific flow_result result section. This is from the
previous patch:
D'oh! Thanks for catching it. I'll re-add it.
@@ -358,6 +360,27 @@ See also the ``main:run_start`` probe above.
- ``utilities/usdt-scripts/bridge_loop.bt``
+probe revalidate:flow_result
+~
+
+**Description**:
+This probe is triggered when the revalidator decides whether or not to
+revalidate a flow. ``reason`` is an enum that denotes that either the flow
+is being kept, or the reason why the flow is being deleted. The
+``flow_reval_monitor.py`` script uses this probe to notify users when flows
+matching user-provided criteria are deleted.
+
+**Arguments**:
+
+- *arg0*: ``(enum flow_del_reason) reason``
+- *arg1*: ``(struct udpif *) udpif``
+- *arg2*: ``(struct udpif_key *) ukey``
+
+**Script references**:
+
+- ``utilities/usdt-scripts/flow_reval_monitor.py``
+
+
Adding your own probes
--
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index b5cbeed878..97d75833f7 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -269,6 +269,18 @@ enum ukey_state {
};
#define N_UKEY_STATES (UKEY_DELETED + 1)
+enum flow_del_reason {
+FDR_REVALIDATE = 0, /* The flow was revalidated. */
It was called FDR_FLOW_LIVE before, which might make more
sense. As the flow is just NOT deleted. It might or might not have
been revalidated. Thoughts?
I think it had to have been revalidated if we emit the reason, because
we only emit the reason code after revalidation. IE: there are many
places where we skip revalidation but the flow stays live - and we don't
emit reasons in those cases.
So at least for this patch, it MUST have been revalidated. But maybe in
the future, we would want to catch cases where the flow hasn't been. In
that case, it makes sense to add the FDR_FLOW_LIVE at that time - I
think.
Maybe you disagree?
Well, it depends on how you define revalidation, it might only have
updated the counters. i.e. it all depends on ‘bool need_revalidate =
ukey->reval_seq != reval_seq;’ in revalidate_ukey(). That was why I
opted for a more general name.
+
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
Eelco Chaudron writes: > On 19 Feb 2024, at 19:57, Aaron Conole wrote: > >> Eelco Chaudron writes: >> >>> On 12 Feb 2024, at 15:15, Aaron Conole wrote: >>> Aaron Conole writes: > Eelco Chaudron writes: > >> On 2 Feb 2024, at 11:31, Adrian Moreno wrote: >> >>> On 2/1/24 10:02, Eelco Chaudron wrote: On 31 Jan 2024, at 18:03, Aaron Conole wrote: > Eelco Chaudron writes: > >> On 25 Jan 2024, at 21:55, Aaron Conole wrote: >> >>> From: Kevin Sprague >>> >>> During normal operations, it is useful to understand when a >>> particular flow >>> gets removed from the system. This can be useful when debugging >>> performance >>> issues tied to ofproto flow changes, trying to determine deployed >>> traffic >>> patterns, or while debugging dynamic systems where ports come and >>> go. >>> >>> Prior to this change, there was a lack of visibility around flow >>> expiration. >>> The existing debugging infrastructure could tell us when a flow was >>> added to >>> the datapath, but not when it was removed or why. >>> >>> This change introduces a USDT probe at the point where the >>> revalidator >>> determines that the flow should be removed. Additionally, we track >>> the >>> reason for the flow eviction and provide that information as well. >>> With >>> this change, we can track the complete flow lifecycle for the >>> netlink >>> datapath by hooking the upcall tracepoint in kernel, the flow put >>> USDT, and >>> the revaldiator USDT, letting us watch as flows are added and >>> removed from >>> the kernel datapath. >>> >>> This change only enables this information via USDT probe, so it >>> won't be >>> possible to access this information any other way (see: >>> Documentation/topics/usdt-probes.rst). >>> >>> Also included is a script >>> (utilities/usdt-scripts/flow_reval_monitor.py) >>> which serves as a demonstration of how the new USDT probe might be >>> used >>> going forward. >>> >>> Signed-off-by: Kevin Sprague >>> Co-authored-by: Aaron Conole >>> Signed-off-by: Aaron Conole >> >> Thanks for following this up Aaron! See comments on this patch >> below. I have no additional comments on patch 2. >> >> Cheers, >> >> Eelco >> >> >>> --- >>> Documentation/topics/usdt-probes.rst | 1 + >>> ofproto/ofproto-dpif-upcall.c| 42 +- >>> utilities/automake.mk| 3 + >>> utilities/usdt-scripts/flow_reval_monitor.py | 653 >>> +++ >>> 4 files changed, 693 insertions(+), 6 deletions(-) >>> create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py >>> >>> diff --git a/Documentation/topics/usdt-probes.rst >>> b/Documentation/topics/usdt-probes.rst >>> index e527f43bab..a8da9bb1f7 100644 >>> --- a/Documentation/topics/usdt-probes.rst >>> +++ b/Documentation/topics/usdt-probes.rst >>> @@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``: >>> - dpif_recv:recv_upcall >>> - main:poll_block >>> - main:run_start >>> +- revalidate:flow_result >>> - revalidate_ukey\_\_:entry >>> - revalidate_ukey\_\_:exit >>> - udpif_revalidator:start_dump >> >> You are missing the specific flow_result result section. This is >> from the previous patch: > > D'oh! Thanks for catching it. I'll re-add it. > >> @@ -358,6 +360,27 @@ See also the ``main:run_start`` probe above. >> - ``utilities/usdt-scripts/bridge_loop.bt`` >> >> >> +probe revalidate:flow_result >> +~ >> + >> +**Description**: >> +This probe is triggered when the revalidator decides whether or not >> to >> +revalidate a flow. ``reason`` is an enum that denotes that either >> the flow >> +is being kept, or the reason why the flow is being deleted. The >> +``flow_reval_monitor.py`` script uses this probe to notify users >> when flows >> +matching user-provided criteria are deleted. >> + >> +**Arguments**: >> + >> +- *arg0*: ``(enum flow_del_reason) reason`` >> +- *arg1*: ``(struct udpif *) udpif`` >> +- *arg2*: ``(struct udpif_key *) ukey`` >> + >> +**Script references**: >> + >> +- ``utilities/usdt-script
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
On 19 Feb 2024, at 19:57, Aaron Conole wrote: > Eelco Chaudron writes: > >> On 12 Feb 2024, at 15:15, Aaron Conole wrote: >> >>> Aaron Conole writes: >>> Eelco Chaudron writes: > On 2 Feb 2024, at 11:31, Adrian Moreno wrote: > >> On 2/1/24 10:02, Eelco Chaudron wrote: >>> >>> >>> On 31 Jan 2024, at 18:03, Aaron Conole wrote: >>> Eelco Chaudron writes: > On 25 Jan 2024, at 21:55, Aaron Conole wrote: > >> From: Kevin Sprague >> >> During normal operations, it is useful to understand when a >> particular flow >> gets removed from the system. This can be useful when debugging >> performance >> issues tied to ofproto flow changes, trying to determine deployed >> traffic >> patterns, or while debugging dynamic systems where ports come and go. >> >> Prior to this change, there was a lack of visibility around flow >> expiration. >> The existing debugging infrastructure could tell us when a flow was >> added to >> the datapath, but not when it was removed or why. >> >> This change introduces a USDT probe at the point where the >> revalidator >> determines that the flow should be removed. Additionally, we track >> the >> reason for the flow eviction and provide that information as well. >> With >> this change, we can track the complete flow lifecycle for the netlink >> datapath by hooking the upcall tracepoint in kernel, the flow put >> USDT, and >> the revaldiator USDT, letting us watch as flows are added and >> removed from >> the kernel datapath. >> >> This change only enables this information via USDT probe, so it >> won't be >> possible to access this information any other way (see: >> Documentation/topics/usdt-probes.rst). >> >> Also included is a script >> (utilities/usdt-scripts/flow_reval_monitor.py) >> which serves as a demonstration of how the new USDT probe might be >> used >> going forward. >> >> Signed-off-by: Kevin Sprague >> Co-authored-by: Aaron Conole >> Signed-off-by: Aaron Conole > > Thanks for following this up Aaron! See comments on this patch > below. I have no additional comments on patch 2. > > Cheers, > > Eelco > > >> --- >> Documentation/topics/usdt-probes.rst | 1 + >> ofproto/ofproto-dpif-upcall.c| 42 +- >> utilities/automake.mk| 3 + >> utilities/usdt-scripts/flow_reval_monitor.py | 653 >> +++ >> 4 files changed, 693 insertions(+), 6 deletions(-) >> create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py >> >> diff --git a/Documentation/topics/usdt-probes.rst >> b/Documentation/topics/usdt-probes.rst >> index e527f43bab..a8da9bb1f7 100644 >> --- a/Documentation/topics/usdt-probes.rst >> +++ b/Documentation/topics/usdt-probes.rst >> @@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``: >> - dpif_recv:recv_upcall >> - main:poll_block >> - main:run_start >> +- revalidate:flow_result >> - revalidate_ukey\_\_:entry >> - revalidate_ukey\_\_:exit >> - udpif_revalidator:start_dump > > You are missing the specific flow_result result section. This is from > the previous patch: D'oh! Thanks for catching it. I'll re-add it. > @@ -358,6 +360,27 @@ See also the ``main:run_start`` probe above. > - ``utilities/usdt-scripts/bridge_loop.bt`` > > > +probe revalidate:flow_result > +~ > + > +**Description**: > +This probe is triggered when the revalidator decides whether or not > to > +revalidate a flow. ``reason`` is an enum that denotes that either > the flow > +is being kept, or the reason why the flow is being deleted. The > +``flow_reval_monitor.py`` script uses this probe to notify users > when flows > +matching user-provided criteria are deleted. > + > +**Arguments**: > + > +- *arg0*: ``(enum flow_del_reason) reason`` > +- *arg1*: ``(struct udpif *) udpif`` > +- *arg2*: ``(struct udpif_key *) ukey`` > + > +**Script references**: > + > +- ``utilities/usdt-scripts/flow_reval_monitor.py`` > + > + > Adding your own probes > -- > >> diff --git a/ofproto/ofproto-dpif-up
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
Eelco Chaudron writes: > On 12 Feb 2024, at 15:15, Aaron Conole wrote: > >> Aaron Conole writes: >> >>> Eelco Chaudron writes: >>> On 2 Feb 2024, at 11:31, Adrian Moreno wrote: > On 2/1/24 10:02, Eelco Chaudron wrote: >> >> >> On 31 Jan 2024, at 18:03, Aaron Conole wrote: >> >>> Eelco Chaudron writes: >>> On 25 Jan 2024, at 21:55, Aaron Conole wrote: > From: Kevin Sprague > > During normal operations, it is useful to understand when a > particular flow > gets removed from the system. This can be useful when debugging > performance > issues tied to ofproto flow changes, trying to determine deployed > traffic > patterns, or while debugging dynamic systems where ports come and go. > > Prior to this change, there was a lack of visibility around flow > expiration. > The existing debugging infrastructure could tell us when a flow was > added to > the datapath, but not when it was removed or why. > > This change introduces a USDT probe at the point where the revalidator > determines that the flow should be removed. Additionally, we track > the > reason for the flow eviction and provide that information as well. > With > this change, we can track the complete flow lifecycle for the netlink > datapath by hooking the upcall tracepoint in kernel, the flow put > USDT, and > the revaldiator USDT, letting us watch as flows are added and removed > from > the kernel datapath. > > This change only enables this information via USDT probe, so it won't > be > possible to access this information any other way (see: > Documentation/topics/usdt-probes.rst). > > Also included is a script > (utilities/usdt-scripts/flow_reval_monitor.py) > which serves as a demonstration of how the new USDT probe might be > used > going forward. > > Signed-off-by: Kevin Sprague > Co-authored-by: Aaron Conole > Signed-off-by: Aaron Conole Thanks for following this up Aaron! See comments on this patch below. I have no additional comments on patch 2. Cheers, Eelco > --- > Documentation/topics/usdt-probes.rst | 1 + > ofproto/ofproto-dpif-upcall.c| 42 +- > utilities/automake.mk| 3 + > utilities/usdt-scripts/flow_reval_monitor.py | 653 > +++ > 4 files changed, 693 insertions(+), 6 deletions(-) > create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py > > diff --git a/Documentation/topics/usdt-probes.rst > b/Documentation/topics/usdt-probes.rst > index e527f43bab..a8da9bb1f7 100644 > --- a/Documentation/topics/usdt-probes.rst > +++ b/Documentation/topics/usdt-probes.rst > @@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``: > - dpif_recv:recv_upcall > - main:poll_block > - main:run_start > +- revalidate:flow_result > - revalidate_ukey\_\_:entry > - revalidate_ukey\_\_:exit > - udpif_revalidator:start_dump You are missing the specific flow_result result section. This is from the previous patch: >>> >>> D'oh! Thanks for catching it. I'll re-add it. >>> @@ -358,6 +360,27 @@ See also the ``main:run_start`` probe above. - ``utilities/usdt-scripts/bridge_loop.bt`` +probe revalidate:flow_result +~ + +**Description**: +This probe is triggered when the revalidator decides whether or not to +revalidate a flow. ``reason`` is an enum that denotes that either the flow +is being kept, or the reason why the flow is being deleted. The +``flow_reval_monitor.py`` script uses this probe to notify users when flows +matching user-provided criteria are deleted. + +**Arguments**: + +- *arg0*: ``(enum flow_del_reason) reason`` +- *arg1*: ``(struct udpif *) udpif`` +- *arg2*: ``(struct udpif_key *) ukey`` + +**Script references**: + +- ``utilities/usdt-scripts/flow_reval_monitor.py`` + + Adding your own probes -- > diff --git a/ofproto/ofproto-dpif-upcall.c > b/ofproto/ofproto-dpif-upcall.c > index b5cbeed878..97d75833f7 100644 > --- a/ofproto/ofproto-dpif-upcall.c > +++ b/ofproto/ofproto-dpif-upcall.c > @@ -
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
On 12 Feb 2024, at 15:15, Aaron Conole wrote:
> Aaron Conole writes:
>
>> Eelco Chaudron writes:
>>
>>> On 2 Feb 2024, at 11:31, Adrian Moreno wrote:
>>>
On 2/1/24 10:02, Eelco Chaudron wrote:
>
>
> On 31 Jan 2024, at 18:03, Aaron Conole wrote:
>
>> Eelco Chaudron writes:
>>
>>> On 25 Jan 2024, at 21:55, Aaron Conole wrote:
>>>
From: Kevin Sprague
During normal operations, it is useful to understand when a particular
flow
gets removed from the system. This can be useful when debugging
performance
issues tied to ofproto flow changes, trying to determine deployed
traffic
patterns, or while debugging dynamic systems where ports come and go.
Prior to this change, there was a lack of visibility around flow
expiration.
The existing debugging infrastructure could tell us when a flow was
added to
the datapath, but not when it was removed or why.
This change introduces a USDT probe at the point where the revalidator
determines that the flow should be removed. Additionally, we track the
reason for the flow eviction and provide that information as well.
With
this change, we can track the complete flow lifecycle for the netlink
datapath by hooking the upcall tracepoint in kernel, the flow put
USDT, and
the revaldiator USDT, letting us watch as flows are added and removed
from
the kernel datapath.
This change only enables this information via USDT probe, so it won't
be
possible to access this information any other way (see:
Documentation/topics/usdt-probes.rst).
Also included is a script
(utilities/usdt-scripts/flow_reval_monitor.py)
which serves as a demonstration of how the new USDT probe might be used
going forward.
Signed-off-by: Kevin Sprague
Co-authored-by: Aaron Conole
Signed-off-by: Aaron Conole
>>>
>>> Thanks for following this up Aaron! See comments on this patch
>>> below. I have no additional comments on patch 2.
>>>
>>> Cheers,
>>>
>>> Eelco
>>>
>>>
---
Documentation/topics/usdt-probes.rst | 1 +
ofproto/ofproto-dpif-upcall.c| 42 +-
utilities/automake.mk| 3 +
utilities/usdt-scripts/flow_reval_monitor.py | 653
+++
4 files changed, 693 insertions(+), 6 deletions(-)
create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py
diff --git a/Documentation/topics/usdt-probes.rst
b/Documentation/topics/usdt-probes.rst
index e527f43bab..a8da9bb1f7 100644
--- a/Documentation/topics/usdt-probes.rst
+++ b/Documentation/topics/usdt-probes.rst
@@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``:
- dpif_recv:recv_upcall
- main:poll_block
- main:run_start
+- revalidate:flow_result
- revalidate_ukey\_\_:entry
- revalidate_ukey\_\_:exit
- udpif_revalidator:start_dump
>>>
>>> You are missing the specific flow_result result section. This is from
>>> the previous patch:
>>
>> D'oh! Thanks for catching it. I'll re-add it.
>>
>>> @@ -358,6 +360,27 @@ See also the ``main:run_start`` probe above.
>>> - ``utilities/usdt-scripts/bridge_loop.bt``
>>>
>>>
>>> +probe revalidate:flow_result
>>> +~
>>> +
>>> +**Description**:
>>> +This probe is triggered when the revalidator decides whether or not to
>>> +revalidate a flow. ``reason`` is an enum that denotes that either the
>>> flow
>>> +is being kept, or the reason why the flow is being deleted. The
>>> +``flow_reval_monitor.py`` script uses this probe to notify users when
>>> flows
>>> +matching user-provided criteria are deleted.
>>> +
>>> +**Arguments**:
>>> +
>>> +- *arg0*: ``(enum flow_del_reason) reason``
>>> +- *arg1*: ``(struct udpif *) udpif``
>>> +- *arg2*: ``(struct udpif_key *) ukey``
>>> +
>>> +**Script references**:
>>> +
>>> +- ``utilities/usdt-scripts/flow_reval_monitor.py``
>>> +
>>> +
>>> Adding your own probes
>>> --
>>>
diff --git a/ofproto/ofproto-dpif-upcall.c
b/ofproto/ofproto-dpif-upcall.c
index b5cbeed878..97d75833f7 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -269,6 +269,18 @@ enum ukey_state {
};
#define N_UKEY_STATES (UKEY_DELETED + 1)
+enum flow_del_reason {
+FDR_REVALIDATE = 0,
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
Aaron Conole writes:
> Eelco Chaudron writes:
>
>> On 2 Feb 2024, at 11:31, Adrian Moreno wrote:
>>
>>> On 2/1/24 10:02, Eelco Chaudron wrote:
On 31 Jan 2024, at 18:03, Aaron Conole wrote:
> Eelco Chaudron writes:
>
>> On 25 Jan 2024, at 21:55, Aaron Conole wrote:
>>
>>> From: Kevin Sprague
>>>
>>> During normal operations, it is useful to understand when a particular
>>> flow
>>> gets removed from the system. This can be useful when debugging
>>> performance
>>> issues tied to ofproto flow changes, trying to determine deployed
>>> traffic
>>> patterns, or while debugging dynamic systems where ports come and go.
>>>
>>> Prior to this change, there was a lack of visibility around flow
>>> expiration.
>>> The existing debugging infrastructure could tell us when a flow was
>>> added to
>>> the datapath, but not when it was removed or why.
>>>
>>> This change introduces a USDT probe at the point where the revalidator
>>> determines that the flow should be removed. Additionally, we track the
>>> reason for the flow eviction and provide that information as well. With
>>> this change, we can track the complete flow lifecycle for the netlink
>>> datapath by hooking the upcall tracepoint in kernel, the flow put USDT,
>>> and
>>> the revaldiator USDT, letting us watch as flows are added and removed
>>> from
>>> the kernel datapath.
>>>
>>> This change only enables this information via USDT probe, so it won't be
>>> possible to access this information any other way (see:
>>> Documentation/topics/usdt-probes.rst).
>>>
>>> Also included is a script (utilities/usdt-scripts/flow_reval_monitor.py)
>>> which serves as a demonstration of how the new USDT probe might be used
>>> going forward.
>>>
>>> Signed-off-by: Kevin Sprague
>>> Co-authored-by: Aaron Conole
>>> Signed-off-by: Aaron Conole
>>
>> Thanks for following this up Aaron! See comments on this patch
>> below. I have no additional comments on patch 2.
>>
>> Cheers,
>>
>> Eelco
>>
>>
>>> ---
>>> Documentation/topics/usdt-probes.rst | 1 +
>>> ofproto/ofproto-dpif-upcall.c| 42 +-
>>> utilities/automake.mk| 3 +
>>> utilities/usdt-scripts/flow_reval_monitor.py | 653 +++
>>> 4 files changed, 693 insertions(+), 6 deletions(-)
>>> create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py
>>>
>>> diff --git a/Documentation/topics/usdt-probes.rst
>>> b/Documentation/topics/usdt-probes.rst
>>> index e527f43bab..a8da9bb1f7 100644
>>> --- a/Documentation/topics/usdt-probes.rst
>>> +++ b/Documentation/topics/usdt-probes.rst
>>> @@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``:
>>> - dpif_recv:recv_upcall
>>> - main:poll_block
>>> - main:run_start
>>> +- revalidate:flow_result
>>> - revalidate_ukey\_\_:entry
>>> - revalidate_ukey\_\_:exit
>>> - udpif_revalidator:start_dump
>>
>> You are missing the specific flow_result result section. This is from
>> the previous patch:
>
> D'oh! Thanks for catching it. I'll re-add it.
>
>> @@ -358,6 +360,27 @@ See also the ``main:run_start`` probe above.
>> - ``utilities/usdt-scripts/bridge_loop.bt``
>>
>>
>> +probe revalidate:flow_result
>> +~
>> +
>> +**Description**:
>> +This probe is triggered when the revalidator decides whether or not to
>> +revalidate a flow. ``reason`` is an enum that denotes that either the
>> flow
>> +is being kept, or the reason why the flow is being deleted. The
>> +``flow_reval_monitor.py`` script uses this probe to notify users when
>> flows
>> +matching user-provided criteria are deleted.
>> +
>> +**Arguments**:
>> +
>> +- *arg0*: ``(enum flow_del_reason) reason``
>> +- *arg1*: ``(struct udpif *) udpif``
>> +- *arg2*: ``(struct udpif_key *) ukey``
>> +
>> +**Script references**:
>> +
>> +- ``utilities/usdt-scripts/flow_reval_monitor.py``
>> +
>> +
>> Adding your own probes
>> --
>>
>>> diff --git a/ofproto/ofproto-dpif-upcall.c
>>> b/ofproto/ofproto-dpif-upcall.c
>>> index b5cbeed878..97d75833f7 100644
>>> --- a/ofproto/ofproto-dpif-upcall.c
>>> +++ b/ofproto/ofproto-dpif-upcall.c
>>> @@ -269,6 +269,18 @@ enum ukey_state {
>>> };
>>> #define N_UKEY_STATES (UKEY_DELETED + 1)
>>>
>>> +enum flow_del_reason {
>>> +FDR_REVALIDATE = 0, /* The flow was revalidated. */
>>
>> It was called FDR_FLOW_LIVE before, which might make more
>> sense. As the flow is just NOT deleted. It might or might not have
>> been revalidated. Thoughts?
>
>>>
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
On 2/2/24 15:37, Eelco Chaudron wrote: On 2 Feb 2024, at 15:16, Aaron Conole wrote: +OVS_USDT_PROBE(revalidate, flow_result, reason, udpif, ukey); I have been experimenting with several upcall tracking techniques that would make it easier to correlate upcalls with their subsequent related events. To achieve that, we need (among other things) some easy-to-compare unique value in the events. For revalidation events, I think a good candidate would be "ukey->ufid" and so does the script in this patch. However, requiring all external tools to know the layout of "struct udpif_key" in order to get that value makes things quite complicated for CORE tools (e.g: retis). With all this, would you consider adding the ufid to probe payload directly? Dont see why we can not, but if we need anything else it would quickly explode in too much arguments. I guess CORE needs a good solution for userspace. I think having the ufid and udpif makes sense for now. Actually, maybe for the long term we can do something as an appctl command which will dump the relevant datastructures. That way a tool like this, which needs the pid anyway, can generate an appctl command against the daemon and get the exact right header layout. WDYT? I think we discussed this in the past, but I can’t remember what the arguments against this approach were. Adrian, can you remember? We explored a generic approach using pahole to extract the layout of the running daemon, generate a BTF file and use with libbpf to do _actual_ CORE, but at the time pahole was missing some support for dwz-compressed files (such as the ones in debug packages). The process would then be: - 1. (build time) Generate a header (also using pahole?) that we can use to compile against - 2. (build time) Build an ebpf program - 3. (run-time) Get the OVS debuginfo (using debuginfod?) - 4. (run-time) Build a BTF file from the debuginfo - 5. (run-time) Pass that BTF file to libbpf and let it do CORE relocation as normal I'll check again if pahole limitations still persist. -- Adrián Moreno ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
On 2 Feb 2024, at 15:16, Aaron Conole wrote: >>> +OVS_USDT_PROBE(revalidate, flow_result, reason, udpif, >>> ukey); >>> >>> I have been experimenting with several upcall tracking techniques >> that would make it easier to correlate upcalls with their subsequent >> related events. >>> To achieve that, we need (among other things) some easy-to-compare >> unique value in the events. For revalidation events, I think a good >> candidate would be "ukey->ufid" and so does the script in this patch. >>> >>> However, requiring all external tools to know the layout of "struct >> udpif_key" in order to get that value makes things quite complicated >> for CORE tools (e.g: retis). >>> >>> With all this, would you consider adding the ufid to probe payload directly? >> >> Dont see why we can not, but if we need anything else it would quickly >> explode in too much arguments. I guess CORE needs a good solution for >> userspace. > > I think having the ufid and udpif makes sense for now. Actually, maybe > for the long term we can do something as an appctl command which will > dump the relevant datastructures. That way a tool like this, which > needs the pid anyway, can generate an appctl command against the daemon > and get the exact right header layout. > > WDYT? I think we discussed this in the past, but I can’t remember what the arguments against this approach were. Adrian, can you remember? ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
Eelco Chaudron writes:
> On 2 Feb 2024, at 11:31, Adrian Moreno wrote:
>
>> On 2/1/24 10:02, Eelco Chaudron wrote:
>>>
>>>
>>> On 31 Jan 2024, at 18:03, Aaron Conole wrote:
>>>
Eelco Chaudron writes:
> On 25 Jan 2024, at 21:55, Aaron Conole wrote:
>
>> From: Kevin Sprague
>>
>> During normal operations, it is useful to understand when a particular
>> flow
>> gets removed from the system. This can be useful when debugging
>> performance
>> issues tied to ofproto flow changes, trying to determine deployed traffic
>> patterns, or while debugging dynamic systems where ports come and go.
>>
>> Prior to this change, there was a lack of visibility around flow
>> expiration.
>> The existing debugging infrastructure could tell us when a flow was
>> added to
>> the datapath, but not when it was removed or why.
>>
>> This change introduces a USDT probe at the point where the revalidator
>> determines that the flow should be removed. Additionally, we track the
>> reason for the flow eviction and provide that information as well. With
>> this change, we can track the complete flow lifecycle for the netlink
>> datapath by hooking the upcall tracepoint in kernel, the flow put USDT,
>> and
>> the revaldiator USDT, letting us watch as flows are added and removed
>> from
>> the kernel datapath.
>>
>> This change only enables this information via USDT probe, so it won't be
>> possible to access this information any other way (see:
>> Documentation/topics/usdt-probes.rst).
>>
>> Also included is a script (utilities/usdt-scripts/flow_reval_monitor.py)
>> which serves as a demonstration of how the new USDT probe might be used
>> going forward.
>>
>> Signed-off-by: Kevin Sprague
>> Co-authored-by: Aaron Conole
>> Signed-off-by: Aaron Conole
>
> Thanks for following this up Aaron! See comments on this patch
> below. I have no additional comments on patch 2.
>
> Cheers,
>
> Eelco
>
>
>> ---
>> Documentation/topics/usdt-probes.rst | 1 +
>> ofproto/ofproto-dpif-upcall.c| 42 +-
>> utilities/automake.mk| 3 +
>> utilities/usdt-scripts/flow_reval_monitor.py | 653 +++
>> 4 files changed, 693 insertions(+), 6 deletions(-)
>> create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py
>>
>> diff --git a/Documentation/topics/usdt-probes.rst
>> b/Documentation/topics/usdt-probes.rst
>> index e527f43bab..a8da9bb1f7 100644
>> --- a/Documentation/topics/usdt-probes.rst
>> +++ b/Documentation/topics/usdt-probes.rst
>> @@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``:
>> - dpif_recv:recv_upcall
>> - main:poll_block
>> - main:run_start
>> +- revalidate:flow_result
>> - revalidate_ukey\_\_:entry
>> - revalidate_ukey\_\_:exit
>> - udpif_revalidator:start_dump
>
> You are missing the specific flow_result result section. This is from the
> previous patch:
D'oh! Thanks for catching it. I'll re-add it.
> @@ -358,6 +360,27 @@ See also the ``main:run_start`` probe above.
> - ``utilities/usdt-scripts/bridge_loop.bt``
>
>
> +probe revalidate:flow_result
> +~
> +
> +**Description**:
> +This probe is triggered when the revalidator decides whether or not to
> +revalidate a flow. ``reason`` is an enum that denotes that either the
> flow
> +is being kept, or the reason why the flow is being deleted. The
> +``flow_reval_monitor.py`` script uses this probe to notify users when
> flows
> +matching user-provided criteria are deleted.
> +
> +**Arguments**:
> +
> +- *arg0*: ``(enum flow_del_reason) reason``
> +- *arg1*: ``(struct udpif *) udpif``
> +- *arg2*: ``(struct udpif_key *) ukey``
> +
> +**Script references**:
> +
> +- ``utilities/usdt-scripts/flow_reval_monitor.py``
> +
> +
> Adding your own probes
> --
>
>> diff --git a/ofproto/ofproto-dpif-upcall.c
>> b/ofproto/ofproto-dpif-upcall.c
>> index b5cbeed878..97d75833f7 100644
>> --- a/ofproto/ofproto-dpif-upcall.c
>> +++ b/ofproto/ofproto-dpif-upcall.c
>> @@ -269,6 +269,18 @@ enum ukey_state {
>> };
>> #define N_UKEY_STATES (UKEY_DELETED + 1)
>>
>> +enum flow_del_reason {
>> +FDR_REVALIDATE = 0, /* The flow was revalidated. */
>
> It was called FDR_FLOW_LIVE before, which might make more
> sense. As the flow is just NOT deleted. It might or might not have
> been revalidated. Thoughts?
I think it had to have been revalidated if we emit the reason, because
we only emit the reason code after revalidation. IE: there are many
places where
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
Adrian Moreno writes:
> On 2/1/24 10:02, Eelco Chaudron wrote:
>> On 31 Jan 2024, at 18:03, Aaron Conole wrote:
>>
>>> Eelco Chaudron writes:
>>>
On 25 Jan 2024, at 21:55, Aaron Conole wrote:
> From: Kevin Sprague
>
> During normal operations, it is useful to understand when a particular
> flow
> gets removed from the system. This can be useful when debugging
> performance
> issues tied to ofproto flow changes, trying to determine deployed traffic
> patterns, or while debugging dynamic systems where ports come and go.
>
> Prior to this change, there was a lack of visibility around flow
> expiration.
> The existing debugging infrastructure could tell us when a flow was added
> to
> the datapath, but not when it was removed or why.
>
> This change introduces a USDT probe at the point where the revalidator
> determines that the flow should be removed. Additionally, we track the
> reason for the flow eviction and provide that information as well. With
> this change, we can track the complete flow lifecycle for the netlink
> datapath by hooking the upcall tracepoint in kernel, the flow put USDT,
> and
> the revaldiator USDT, letting us watch as flows are added and removed from
> the kernel datapath.
>
> This change only enables this information via USDT probe, so it won't be
> possible to access this information any other way (see:
> Documentation/topics/usdt-probes.rst).
>
> Also included is a script (utilities/usdt-scripts/flow_reval_monitor.py)
> which serves as a demonstration of how the new USDT probe might be used
> going forward.
>
> Signed-off-by: Kevin Sprague
> Co-authored-by: Aaron Conole
> Signed-off-by: Aaron Conole
Thanks for following this up Aaron! See comments on this patch
> below. I have no additional comments on patch 2.
Cheers,
Eelco
> ---
> Documentation/topics/usdt-probes.rst | 1 +
> ofproto/ofproto-dpif-upcall.c| 42 +-
> utilities/automake.mk| 3 +
> utilities/usdt-scripts/flow_reval_monitor.py | 653 +++
> 4 files changed, 693 insertions(+), 6 deletions(-)
> create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py
>
> diff --git a/Documentation/topics/usdt-probes.rst
> b/Documentation/topics/usdt-probes.rst
> index e527f43bab..a8da9bb1f7 100644
> --- a/Documentation/topics/usdt-probes.rst
> +++ b/Documentation/topics/usdt-probes.rst
> @@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``:
> - dpif_recv:recv_upcall
> - main:poll_block
> - main:run_start
> +- revalidate:flow_result
> - revalidate_ukey\_\_:entry
> - revalidate_ukey\_\_:exit
> - udpif_revalidator:start_dump
You are missing the specific flow_result result section. This is from the
previous patch:
>>>
>>> D'oh! Thanks for catching it. I'll re-add it.
>>>
@@ -358,6 +360,27 @@ See also the ``main:run_start`` probe above.
- ``utilities/usdt-scripts/bridge_loop.bt``
+probe revalidate:flow_result
+~
+
+**Description**:
+This probe is triggered when the revalidator decides whether or not to
+revalidate a flow. ``reason`` is an enum that denotes that either the flow
+is being kept, or the reason why the flow is being deleted. The
+``flow_reval_monitor.py`` script uses this probe to notify users when
flows
+matching user-provided criteria are deleted.
+
+**Arguments**:
+
+- *arg0*: ``(enum flow_del_reason) reason``
+- *arg1*: ``(struct udpif *) udpif``
+- *arg2*: ``(struct udpif_key *) ukey``
+
+**Script references**:
+
+- ``utilities/usdt-scripts/flow_reval_monitor.py``
+
+
Adding your own probes
--
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index b5cbeed878..97d75833f7 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -269,6 +269,18 @@ enum ukey_state {
> };
> #define N_UKEY_STATES (UKEY_DELETED + 1)
>
> +enum flow_del_reason {
> +FDR_REVALIDATE = 0, /* The flow was revalidated. */
It was called FDR_FLOW_LIVE before, which might make more
> sense. As the flow is just NOT deleted. It might or might not have
> been revalidated. Thoughts?
>>>
>>> I think it had to have been revalidated if we emit the reason, because
>>> we only emit the reason code after revalidation. IE: there are many
>>> places where we skip revalidation but the flow stays live - and we don't
>>> emit reasons in those cases.
>>>
>>> So at least for this patch, it MUST have been revalidated. But maybe in
>>> the future, we would wan
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
Eelco Chaudron writes:
> On 1 Feb 2024, at 18:28, Aaron Conole wrote:
>
>> Eelco Chaudron writes:
>>
>>> On 31 Jan 2024, at 18:03, Aaron Conole wrote:
>>>
Eelco Chaudron writes:
> On 25 Jan 2024, at 21:55, Aaron Conole wrote:
>
>> From: Kevin Sprague
>>
>> During normal operations, it is useful to understand when a particular
>> flow
>> gets removed from the system. This can be useful when debugging
>> performance
>> issues tied to ofproto flow changes, trying to determine deployed traffic
>> patterns, or while debugging dynamic systems where ports come and go.
>>
>> Prior to this change, there was a lack of visibility around flow
>> expiration.
>> The existing debugging infrastructure could tell us when a flow was
>> added to
>> the datapath, but not when it was removed or why.
>>
>> This change introduces a USDT probe at the point where the revalidator
>> determines that the flow should be removed. Additionally, we track the
>> reason for the flow eviction and provide that information as well. With
>> this change, we can track the complete flow lifecycle for the netlink
>> datapath by hooking the upcall tracepoint in kernel, the flow put USDT,
>> and
>> the revaldiator USDT, letting us watch as flows are added and removed
>> from
>> the kernel datapath.
>>
>> This change only enables this information via USDT probe, so it won't be
>> possible to access this information any other way (see:
>> Documentation/topics/usdt-probes.rst).
>>
>> Also included is a script (utilities/usdt-scripts/flow_reval_monitor.py)
>> which serves as a demonstration of how the new USDT probe might be used
>> going forward.
>>
>> Signed-off-by: Kevin Sprague
>> Co-authored-by: Aaron Conole
>> Signed-off-by: Aaron Conole
>
> Thanks for following this up Aaron! See comments on this patch below. I
> have no additional comments on patch 2.
>
> Cheers,
>
> Eelco
>
>
>> ---
>> Documentation/topics/usdt-probes.rst | 1 +
>> ofproto/ofproto-dpif-upcall.c| 42 +-
>> utilities/automake.mk| 3 +
>> utilities/usdt-scripts/flow_reval_monitor.py | 653 +++
>> 4 files changed, 693 insertions(+), 6 deletions(-)
>> create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py
>>
>> diff --git a/Documentation/topics/usdt-probes.rst
>> b/Documentation/topics/usdt-probes.rst
>> index e527f43bab..a8da9bb1f7 100644
>> --- a/Documentation/topics/usdt-probes.rst
>> +++ b/Documentation/topics/usdt-probes.rst
>> @@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``:
>> - dpif_recv:recv_upcall
>> - main:poll_block
>> - main:run_start
>> +- revalidate:flow_result
>> - revalidate_ukey\_\_:entry
>> - revalidate_ukey\_\_:exit
>> - udpif_revalidator:start_dump
>
> You are missing the specific flow_result result section. This is from the
> previous patch:
D'oh! Thanks for catching it. I'll re-add it.
> @@ -358,6 +360,27 @@ See also the ``main:run_start`` probe above.
> - ``utilities/usdt-scripts/bridge_loop.bt``
>
>
> +probe revalidate:flow_result
> +~
> +
> +**Description**:
> +This probe is triggered when the revalidator decides whether or not to
> +revalidate a flow. ``reason`` is an enum that denotes that either the
> flow
> +is being kept, or the reason why the flow is being deleted. The
> +``flow_reval_monitor.py`` script uses this probe to notify users when
> flows
> +matching user-provided criteria are deleted.
> +
> +**Arguments**:
> +
> +- *arg0*: ``(enum flow_del_reason) reason``
> +- *arg1*: ``(struct udpif *) udpif``
> +- *arg2*: ``(struct udpif_key *) ukey``
> +
> +**Script references**:
> +
> +- ``utilities/usdt-scripts/flow_reval_monitor.py``
> +
> +
> Adding your own probes
> --
>
>> diff --git a/ofproto/ofproto-dpif-upcall.c
>> b/ofproto/ofproto-dpif-upcall.c
>> index b5cbeed878..97d75833f7 100644
>> --- a/ofproto/ofproto-dpif-upcall.c
>> +++ b/ofproto/ofproto-dpif-upcall.c
>> @@ -269,6 +269,18 @@ enum ukey_state {
>> };
>> #define N_UKEY_STATES (UKEY_DELETED + 1)
>>
>> +enum flow_del_reason {
>> +FDR_REVALIDATE = 0, /* The flow was revalidated. */
>
> It was called FDR_FLOW_LIVE before, which might make more sense. As the
> flow is just NOT deleted. It might or might not have been revalidated.
> Thoughts?
I think it had to have been revalidated if we emit the reason, because
we only emit the reason code after revalidation. IE: there are many
places where we skip revalidation b
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
On 2 Feb 2024, at 11:31, Adrian Moreno wrote:
> On 2/1/24 10:02, Eelco Chaudron wrote:
>>
>>
>> On 31 Jan 2024, at 18:03, Aaron Conole wrote:
>>
>>> Eelco Chaudron writes:
>>>
On 25 Jan 2024, at 21:55, Aaron Conole wrote:
> From: Kevin Sprague
>
> During normal operations, it is useful to understand when a particular
> flow
> gets removed from the system. This can be useful when debugging
> performance
> issues tied to ofproto flow changes, trying to determine deployed traffic
> patterns, or while debugging dynamic systems where ports come and go.
>
> Prior to this change, there was a lack of visibility around flow
> expiration.
> The existing debugging infrastructure could tell us when a flow was added
> to
> the datapath, but not when it was removed or why.
>
> This change introduces a USDT probe at the point where the revalidator
> determines that the flow should be removed. Additionally, we track the
> reason for the flow eviction and provide that information as well. With
> this change, we can track the complete flow lifecycle for the netlink
> datapath by hooking the upcall tracepoint in kernel, the flow put USDT,
> and
> the revaldiator USDT, letting us watch as flows are added and removed from
> the kernel datapath.
>
> This change only enables this information via USDT probe, so it won't be
> possible to access this information any other way (see:
> Documentation/topics/usdt-probes.rst).
>
> Also included is a script (utilities/usdt-scripts/flow_reval_monitor.py)
> which serves as a demonstration of how the new USDT probe might be used
> going forward.
>
> Signed-off-by: Kevin Sprague
> Co-authored-by: Aaron Conole
> Signed-off-by: Aaron Conole
Thanks for following this up Aaron! See comments on this patch below. I
have no additional comments on patch 2.
Cheers,
Eelco
> ---
> Documentation/topics/usdt-probes.rst | 1 +
> ofproto/ofproto-dpif-upcall.c| 42 +-
> utilities/automake.mk| 3 +
> utilities/usdt-scripts/flow_reval_monitor.py | 653 +++
> 4 files changed, 693 insertions(+), 6 deletions(-)
> create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py
>
> diff --git a/Documentation/topics/usdt-probes.rst
> b/Documentation/topics/usdt-probes.rst
> index e527f43bab..a8da9bb1f7 100644
> --- a/Documentation/topics/usdt-probes.rst
> +++ b/Documentation/topics/usdt-probes.rst
> @@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``:
> - dpif_recv:recv_upcall
> - main:poll_block
> - main:run_start
> +- revalidate:flow_result
> - revalidate_ukey\_\_:entry
> - revalidate_ukey\_\_:exit
> - udpif_revalidator:start_dump
You are missing the specific flow_result result section. This is from the
previous patch:
>>>
>>> D'oh! Thanks for catching it. I'll re-add it.
>>>
@@ -358,6 +360,27 @@ See also the ``main:run_start`` probe above.
- ``utilities/usdt-scripts/bridge_loop.bt``
+probe revalidate:flow_result
+~
+
+**Description**:
+This probe is triggered when the revalidator decides whether or not to
+revalidate a flow. ``reason`` is an enum that denotes that either the flow
+is being kept, or the reason why the flow is being deleted. The
+``flow_reval_monitor.py`` script uses this probe to notify users when
flows
+matching user-provided criteria are deleted.
+
+**Arguments**:
+
+- *arg0*: ``(enum flow_del_reason) reason``
+- *arg1*: ``(struct udpif *) udpif``
+- *arg2*: ``(struct udpif_key *) ukey``
+
+**Script references**:
+
+- ``utilities/usdt-scripts/flow_reval_monitor.py``
+
+
Adding your own probes
--
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index b5cbeed878..97d75833f7 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -269,6 +269,18 @@ enum ukey_state {
> };
> #define N_UKEY_STATES (UKEY_DELETED + 1)
>
> +enum flow_del_reason {
> +FDR_REVALIDATE = 0, /* The flow was revalidated. */
It was called FDR_FLOW_LIVE before, which might make more sense. As the
flow is just NOT deleted. It might or might not have been revalidated.
Thoughts?
>>>
>>> I think it had to have been revalidated if we emit the reason, because
>>> we only emit the reason code after revalidation. IE: there are many
>>> places where we skip revalidation but the flow stays live - and we don't
>>> emit reasons in those cases.
>>>
>>> So at least for this patch, it MUST have been revalidated.
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
On 2/1/24 10:02, Eelco Chaudron wrote:
On 31 Jan 2024, at 18:03, Aaron Conole wrote:
Eelco Chaudron writes:
On 25 Jan 2024, at 21:55, Aaron Conole wrote:
From: Kevin Sprague
During normal operations, it is useful to understand when a particular flow
gets removed from the system. This can be useful when debugging performance
issues tied to ofproto flow changes, trying to determine deployed traffic
patterns, or while debugging dynamic systems where ports come and go.
Prior to this change, there was a lack of visibility around flow expiration.
The existing debugging infrastructure could tell us when a flow was added to
the datapath, but not when it was removed or why.
This change introduces a USDT probe at the point where the revalidator
determines that the flow should be removed. Additionally, we track the
reason for the flow eviction and provide that information as well. With
this change, we can track the complete flow lifecycle for the netlink
datapath by hooking the upcall tracepoint in kernel, the flow put USDT, and
the revaldiator USDT, letting us watch as flows are added and removed from
the kernel datapath.
This change only enables this information via USDT probe, so it won't be
possible to access this information any other way (see:
Documentation/topics/usdt-probes.rst).
Also included is a script (utilities/usdt-scripts/flow_reval_monitor.py)
which serves as a demonstration of how the new USDT probe might be used
going forward.
Signed-off-by: Kevin Sprague
Co-authored-by: Aaron Conole
Signed-off-by: Aaron Conole
Thanks for following this up Aaron! See comments on this patch below. I have no
additional comments on patch 2.
Cheers,
Eelco
---
Documentation/topics/usdt-probes.rst | 1 +
ofproto/ofproto-dpif-upcall.c| 42 +-
utilities/automake.mk| 3 +
utilities/usdt-scripts/flow_reval_monitor.py | 653 +++
4 files changed, 693 insertions(+), 6 deletions(-)
create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py
diff --git a/Documentation/topics/usdt-probes.rst
b/Documentation/topics/usdt-probes.rst
index e527f43bab..a8da9bb1f7 100644
--- a/Documentation/topics/usdt-probes.rst
+++ b/Documentation/topics/usdt-probes.rst
@@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``:
- dpif_recv:recv_upcall
- main:poll_block
- main:run_start
+- revalidate:flow_result
- revalidate_ukey\_\_:entry
- revalidate_ukey\_\_:exit
- udpif_revalidator:start_dump
You are missing the specific flow_result result section. This is from the
previous patch:
D'oh! Thanks for catching it. I'll re-add it.
@@ -358,6 +360,27 @@ See also the ``main:run_start`` probe above.
- ``utilities/usdt-scripts/bridge_loop.bt``
+probe revalidate:flow_result
+~
+
+**Description**:
+This probe is triggered when the revalidator decides whether or not to
+revalidate a flow. ``reason`` is an enum that denotes that either the flow
+is being kept, or the reason why the flow is being deleted. The
+``flow_reval_monitor.py`` script uses this probe to notify users when flows
+matching user-provided criteria are deleted.
+
+**Arguments**:
+
+- *arg0*: ``(enum flow_del_reason) reason``
+- *arg1*: ``(struct udpif *) udpif``
+- *arg2*: ``(struct udpif_key *) ukey``
+
+**Script references**:
+
+- ``utilities/usdt-scripts/flow_reval_monitor.py``
+
+
Adding your own probes
--
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index b5cbeed878..97d75833f7 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -269,6 +269,18 @@ enum ukey_state {
};
#define N_UKEY_STATES (UKEY_DELETED + 1)
+enum flow_del_reason {
+FDR_REVALIDATE = 0, /* The flow was revalidated. */
It was called FDR_FLOW_LIVE before, which might make more sense. As the flow is
just NOT deleted. It might or might not have been revalidated. Thoughts?
I think it had to have been revalidated if we emit the reason, because
we only emit the reason code after revalidation. IE: there are many
places where we skip revalidation but the flow stays live - and we don't
emit reasons in those cases.
So at least for this patch, it MUST have been revalidated. But maybe in
the future, we would want to catch cases where the flow hasn't been. In
that case, it makes sense to add the FDR_FLOW_LIVE at that time - I
think.
Maybe you disagree?
Well, it depends on how you define revalidation, it might only have updated the
counters. i.e. it all depends on ‘bool need_revalidate = ukey->reval_seq !=
reval_seq;’ in revalidate_ukey(). That was why I opted for a more general name.
+FDR_FLOW_IDLE, /* The flow went unused and was deleted. */
+FDR_TOO_EXPENSIVE, /* The flow was too expensive to revalidate. */
+FDR_FLOW_WILDCARDED,/* The flow needed a narrower wildcard mask. */
+FDR_BAD_ODP_FIT,/* The flow had a bad ODP
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
On 1 Feb 2024, at 18:28, Aaron Conole wrote:
> Eelco Chaudron writes:
>
>> On 31 Jan 2024, at 18:03, Aaron Conole wrote:
>>
>>> Eelco Chaudron writes:
>>>
On 25 Jan 2024, at 21:55, Aaron Conole wrote:
> From: Kevin Sprague
>
> During normal operations, it is useful to understand when a particular
> flow
> gets removed from the system. This can be useful when debugging
> performance
> issues tied to ofproto flow changes, trying to determine deployed traffic
> patterns, or while debugging dynamic systems where ports come and go.
>
> Prior to this change, there was a lack of visibility around flow
> expiration.
> The existing debugging infrastructure could tell us when a flow was added
> to
> the datapath, but not when it was removed or why.
>
> This change introduces a USDT probe at the point where the revalidator
> determines that the flow should be removed. Additionally, we track the
> reason for the flow eviction and provide that information as well. With
> this change, we can track the complete flow lifecycle for the netlink
> datapath by hooking the upcall tracepoint in kernel, the flow put USDT,
> and
> the revaldiator USDT, letting us watch as flows are added and removed from
> the kernel datapath.
>
> This change only enables this information via USDT probe, so it won't be
> possible to access this information any other way (see:
> Documentation/topics/usdt-probes.rst).
>
> Also included is a script (utilities/usdt-scripts/flow_reval_monitor.py)
> which serves as a demonstration of how the new USDT probe might be used
> going forward.
>
> Signed-off-by: Kevin Sprague
> Co-authored-by: Aaron Conole
> Signed-off-by: Aaron Conole
Thanks for following this up Aaron! See comments on this patch below. I
have no additional comments on patch 2.
Cheers,
Eelco
> ---
> Documentation/topics/usdt-probes.rst | 1 +
> ofproto/ofproto-dpif-upcall.c| 42 +-
> utilities/automake.mk| 3 +
> utilities/usdt-scripts/flow_reval_monitor.py | 653 +++
> 4 files changed, 693 insertions(+), 6 deletions(-)
> create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py
>
> diff --git a/Documentation/topics/usdt-probes.rst
> b/Documentation/topics/usdt-probes.rst
> index e527f43bab..a8da9bb1f7 100644
> --- a/Documentation/topics/usdt-probes.rst
> +++ b/Documentation/topics/usdt-probes.rst
> @@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``:
> - dpif_recv:recv_upcall
> - main:poll_block
> - main:run_start
> +- revalidate:flow_result
> - revalidate_ukey\_\_:entry
> - revalidate_ukey\_\_:exit
> - udpif_revalidator:start_dump
You are missing the specific flow_result result section. This is from the
previous patch:
>>>
>>> D'oh! Thanks for catching it. I'll re-add it.
>>>
@@ -358,6 +360,27 @@ See also the ``main:run_start`` probe above.
- ``utilities/usdt-scripts/bridge_loop.bt``
+probe revalidate:flow_result
+~
+
+**Description**:
+This probe is triggered when the revalidator decides whether or not to
+revalidate a flow. ``reason`` is an enum that denotes that either the flow
+is being kept, or the reason why the flow is being deleted. The
+``flow_reval_monitor.py`` script uses this probe to notify users when
flows
+matching user-provided criteria are deleted.
+
+**Arguments**:
+
+- *arg0*: ``(enum flow_del_reason) reason``
+- *arg1*: ``(struct udpif *) udpif``
+- *arg2*: ``(struct udpif_key *) ukey``
+
+**Script references**:
+
+- ``utilities/usdt-scripts/flow_reval_monitor.py``
+
+
Adding your own probes
--
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index b5cbeed878..97d75833f7 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -269,6 +269,18 @@ enum ukey_state {
> };
> #define N_UKEY_STATES (UKEY_DELETED + 1)
>
> +enum flow_del_reason {
> +FDR_REVALIDATE = 0, /* The flow was revalidated. */
It was called FDR_FLOW_LIVE before, which might make more sense. As the
flow is just NOT deleted. It might or might not have been revalidated.
Thoughts?
>>>
>>> I think it had to have been revalidated if we emit the reason, because
>>> we only emit the reason code after revalidation. IE: there are many
>>> places where we skip revalidation but the flow stays live - and we don't
>>> emit reasons in those cases.
>>>
>>> So at least for this patch, it MUST have been revalidated. But maybe in
>>> the future, we woul
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
Eelco Chaudron writes:
> On 31 Jan 2024, at 18:03, Aaron Conole wrote:
>
>> Eelco Chaudron writes:
>>
>>> On 25 Jan 2024, at 21:55, Aaron Conole wrote:
>>>
From: Kevin Sprague
During normal operations, it is useful to understand when a particular flow
gets removed from the system. This can be useful when debugging performance
issues tied to ofproto flow changes, trying to determine deployed traffic
patterns, or while debugging dynamic systems where ports come and go.
Prior to this change, there was a lack of visibility around flow
expiration.
The existing debugging infrastructure could tell us when a flow was added
to
the datapath, but not when it was removed or why.
This change introduces a USDT probe at the point where the revalidator
determines that the flow should be removed. Additionally, we track the
reason for the flow eviction and provide that information as well. With
this change, we can track the complete flow lifecycle for the netlink
datapath by hooking the upcall tracepoint in kernel, the flow put USDT, and
the revaldiator USDT, letting us watch as flows are added and removed from
the kernel datapath.
This change only enables this information via USDT probe, so it won't be
possible to access this information any other way (see:
Documentation/topics/usdt-probes.rst).
Also included is a script (utilities/usdt-scripts/flow_reval_monitor.py)
which serves as a demonstration of how the new USDT probe might be used
going forward.
Signed-off-by: Kevin Sprague
Co-authored-by: Aaron Conole
Signed-off-by: Aaron Conole
>>>
>>> Thanks for following this up Aaron! See comments on this patch below. I
>>> have no additional comments on patch 2.
>>>
>>> Cheers,
>>>
>>> Eelco
>>>
>>>
---
Documentation/topics/usdt-probes.rst | 1 +
ofproto/ofproto-dpif-upcall.c| 42 +-
utilities/automake.mk| 3 +
utilities/usdt-scripts/flow_reval_monitor.py | 653 +++
4 files changed, 693 insertions(+), 6 deletions(-)
create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py
diff --git a/Documentation/topics/usdt-probes.rst
b/Documentation/topics/usdt-probes.rst
index e527f43bab..a8da9bb1f7 100644
--- a/Documentation/topics/usdt-probes.rst
+++ b/Documentation/topics/usdt-probes.rst
@@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``:
- dpif_recv:recv_upcall
- main:poll_block
- main:run_start
+- revalidate:flow_result
- revalidate_ukey\_\_:entry
- revalidate_ukey\_\_:exit
- udpif_revalidator:start_dump
>>>
>>> You are missing the specific flow_result result section. This is from the
>>> previous patch:
>>
>> D'oh! Thanks for catching it. I'll re-add it.
>>
>>> @@ -358,6 +360,27 @@ See also the ``main:run_start`` probe above.
>>> - ``utilities/usdt-scripts/bridge_loop.bt``
>>>
>>>
>>> +probe revalidate:flow_result
>>> +~
>>> +
>>> +**Description**:
>>> +This probe is triggered when the revalidator decides whether or not to
>>> +revalidate a flow. ``reason`` is an enum that denotes that either the flow
>>> +is being kept, or the reason why the flow is being deleted. The
>>> +``flow_reval_monitor.py`` script uses this probe to notify users when flows
>>> +matching user-provided criteria are deleted.
>>> +
>>> +**Arguments**:
>>> +
>>> +- *arg0*: ``(enum flow_del_reason) reason``
>>> +- *arg1*: ``(struct udpif *) udpif``
>>> +- *arg2*: ``(struct udpif_key *) ukey``
>>> +
>>> +**Script references**:
>>> +
>>> +- ``utilities/usdt-scripts/flow_reval_monitor.py``
>>> +
>>> +
>>> Adding your own probes
>>> --
>>>
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index b5cbeed878..97d75833f7 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -269,6 +269,18 @@ enum ukey_state {
};
#define N_UKEY_STATES (UKEY_DELETED + 1)
+enum flow_del_reason {
+FDR_REVALIDATE = 0, /* The flow was revalidated. */
>>>
>>> It was called FDR_FLOW_LIVE before, which might make more sense. As the
>>> flow is just NOT deleted. It might or might not have been revalidated.
>>> Thoughts?
>>
>> I think it had to have been revalidated if we emit the reason, because
>> we only emit the reason code after revalidation. IE: there are many
>> places where we skip revalidation but the flow stays live - and we don't
>> emit reasons in those cases.
>>
>> So at least for this patch, it MUST have been revalidated. But maybe in
>> the future, we would want to catch cases where the flow hasn't been. In
>> that case, it makes sense to add the FDR_FLOW_LIVE at that time - I
>> think.
>>
>> Maybe you disagree?
>
> Well, it depends on how you define re
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
On 31 Jan 2024, at 18:03, Aaron Conole wrote:
> Eelco Chaudron writes:
>
>> On 25 Jan 2024, at 21:55, Aaron Conole wrote:
>>
>>> From: Kevin Sprague
>>>
>>> During normal operations, it is useful to understand when a particular flow
>>> gets removed from the system. This can be useful when debugging performance
>>> issues tied to ofproto flow changes, trying to determine deployed traffic
>>> patterns, or while debugging dynamic systems where ports come and go.
>>>
>>> Prior to this change, there was a lack of visibility around flow expiration.
>>> The existing debugging infrastructure could tell us when a flow was added to
>>> the datapath, but not when it was removed or why.
>>>
>>> This change introduces a USDT probe at the point where the revalidator
>>> determines that the flow should be removed. Additionally, we track the
>>> reason for the flow eviction and provide that information as well. With
>>> this change, we can track the complete flow lifecycle for the netlink
>>> datapath by hooking the upcall tracepoint in kernel, the flow put USDT, and
>>> the revaldiator USDT, letting us watch as flows are added and removed from
>>> the kernel datapath.
>>>
>>> This change only enables this information via USDT probe, so it won't be
>>> possible to access this information any other way (see:
>>> Documentation/topics/usdt-probes.rst).
>>>
>>> Also included is a script (utilities/usdt-scripts/flow_reval_monitor.py)
>>> which serves as a demonstration of how the new USDT probe might be used
>>> going forward.
>>>
>>> Signed-off-by: Kevin Sprague
>>> Co-authored-by: Aaron Conole
>>> Signed-off-by: Aaron Conole
>>
>> Thanks for following this up Aaron! See comments on this patch below. I have
>> no additional comments on patch 2.
>>
>> Cheers,
>>
>> Eelco
>>
>>
>>> ---
>>> Documentation/topics/usdt-probes.rst | 1 +
>>> ofproto/ofproto-dpif-upcall.c| 42 +-
>>> utilities/automake.mk| 3 +
>>> utilities/usdt-scripts/flow_reval_monitor.py | 653 +++
>>> 4 files changed, 693 insertions(+), 6 deletions(-)
>>> create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py
>>>
>>> diff --git a/Documentation/topics/usdt-probes.rst
>>> b/Documentation/topics/usdt-probes.rst
>>> index e527f43bab..a8da9bb1f7 100644
>>> --- a/Documentation/topics/usdt-probes.rst
>>> +++ b/Documentation/topics/usdt-probes.rst
>>> @@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``:
>>> - dpif_recv:recv_upcall
>>> - main:poll_block
>>> - main:run_start
>>> +- revalidate:flow_result
>>> - revalidate_ukey\_\_:entry
>>> - revalidate_ukey\_\_:exit
>>> - udpif_revalidator:start_dump
>>
>> You are missing the specific flow_result result section. This is from the
>> previous patch:
>
> D'oh! Thanks for catching it. I'll re-add it.
>
>> @@ -358,6 +360,27 @@ See also the ``main:run_start`` probe above.
>> - ``utilities/usdt-scripts/bridge_loop.bt``
>>
>>
>> +probe revalidate:flow_result
>> +~
>> +
>> +**Description**:
>> +This probe is triggered when the revalidator decides whether or not to
>> +revalidate a flow. ``reason`` is an enum that denotes that either the flow
>> +is being kept, or the reason why the flow is being deleted. The
>> +``flow_reval_monitor.py`` script uses this probe to notify users when flows
>> +matching user-provided criteria are deleted.
>> +
>> +**Arguments**:
>> +
>> +- *arg0*: ``(enum flow_del_reason) reason``
>> +- *arg1*: ``(struct udpif *) udpif``
>> +- *arg2*: ``(struct udpif_key *) ukey``
>> +
>> +**Script references**:
>> +
>> +- ``utilities/usdt-scripts/flow_reval_monitor.py``
>> +
>> +
>> Adding your own probes
>> --
>>
>>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>>> index b5cbeed878..97d75833f7 100644
>>> --- a/ofproto/ofproto-dpif-upcall.c
>>> +++ b/ofproto/ofproto-dpif-upcall.c
>>> @@ -269,6 +269,18 @@ enum ukey_state {
>>> };
>>> #define N_UKEY_STATES (UKEY_DELETED + 1)
>>>
>>> +enum flow_del_reason {
>>> +FDR_REVALIDATE = 0, /* The flow was revalidated. */
>>
>> It was called FDR_FLOW_LIVE before, which might make more sense. As the flow
>> is just NOT deleted. It might or might not have been revalidated. Thoughts?
>
> I think it had to have been revalidated if we emit the reason, because
> we only emit the reason code after revalidation. IE: there are many
> places where we skip revalidation but the flow stays live - and we don't
> emit reasons in those cases.
>
> So at least for this patch, it MUST have been revalidated. But maybe in
> the future, we would want to catch cases where the flow hasn't been. In
> that case, it makes sense to add the FDR_FLOW_LIVE at that time - I
> think.
>
> Maybe you disagree?
Well, it depends on how you define revalidation, it might only have updated the
counters. i.e. it all depends on ‘bool need_revalidate = ukey->reval_seq !=
reval_seq;’ in revalidate_ukey(). That was why I
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
Eelco Chaudron writes:
> On 25 Jan 2024, at 21:55, Aaron Conole wrote:
>
>> From: Kevin Sprague
>>
>> During normal operations, it is useful to understand when a particular flow
>> gets removed from the system. This can be useful when debugging performance
>> issues tied to ofproto flow changes, trying to determine deployed traffic
>> patterns, or while debugging dynamic systems where ports come and go.
>>
>> Prior to this change, there was a lack of visibility around flow expiration.
>> The existing debugging infrastructure could tell us when a flow was added to
>> the datapath, but not when it was removed or why.
>>
>> This change introduces a USDT probe at the point where the revalidator
>> determines that the flow should be removed. Additionally, we track the
>> reason for the flow eviction and provide that information as well. With
>> this change, we can track the complete flow lifecycle for the netlink
>> datapath by hooking the upcall tracepoint in kernel, the flow put USDT, and
>> the revaldiator USDT, letting us watch as flows are added and removed from
>> the kernel datapath.
>>
>> This change only enables this information via USDT probe, so it won't be
>> possible to access this information any other way (see:
>> Documentation/topics/usdt-probes.rst).
>>
>> Also included is a script (utilities/usdt-scripts/flow_reval_monitor.py)
>> which serves as a demonstration of how the new USDT probe might be used
>> going forward.
>>
>> Signed-off-by: Kevin Sprague
>> Co-authored-by: Aaron Conole
>> Signed-off-by: Aaron Conole
>
> Thanks for following this up Aaron! See comments on this patch below. I have
> no additional comments on patch 2.
>
> Cheers,
>
> Eelco
>
>
>> ---
>> Documentation/topics/usdt-probes.rst | 1 +
>> ofproto/ofproto-dpif-upcall.c| 42 +-
>> utilities/automake.mk| 3 +
>> utilities/usdt-scripts/flow_reval_monitor.py | 653 +++
>> 4 files changed, 693 insertions(+), 6 deletions(-)
>> create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py
>>
>> diff --git a/Documentation/topics/usdt-probes.rst
>> b/Documentation/topics/usdt-probes.rst
>> index e527f43bab..a8da9bb1f7 100644
>> --- a/Documentation/topics/usdt-probes.rst
>> +++ b/Documentation/topics/usdt-probes.rst
>> @@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``:
>> - dpif_recv:recv_upcall
>> - main:poll_block
>> - main:run_start
>> +- revalidate:flow_result
>> - revalidate_ukey\_\_:entry
>> - revalidate_ukey\_\_:exit
>> - udpif_revalidator:start_dump
>
> You are missing the specific flow_result result section. This is from the
> previous patch:
D'oh! Thanks for catching it. I'll re-add it.
> @@ -358,6 +360,27 @@ See also the ``main:run_start`` probe above.
> - ``utilities/usdt-scripts/bridge_loop.bt``
>
>
> +probe revalidate:flow_result
> +~
> +
> +**Description**:
> +This probe is triggered when the revalidator decides whether or not to
> +revalidate a flow. ``reason`` is an enum that denotes that either the flow
> +is being kept, or the reason why the flow is being deleted. The
> +``flow_reval_monitor.py`` script uses this probe to notify users when flows
> +matching user-provided criteria are deleted.
> +
> +**Arguments**:
> +
> +- *arg0*: ``(enum flow_del_reason) reason``
> +- *arg1*: ``(struct udpif *) udpif``
> +- *arg2*: ``(struct udpif_key *) ukey``
> +
> +**Script references**:
> +
> +- ``utilities/usdt-scripts/flow_reval_monitor.py``
> +
> +
> Adding your own probes
> --
>
>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>> index b5cbeed878..97d75833f7 100644
>> --- a/ofproto/ofproto-dpif-upcall.c
>> +++ b/ofproto/ofproto-dpif-upcall.c
>> @@ -269,6 +269,18 @@ enum ukey_state {
>> };
>> #define N_UKEY_STATES (UKEY_DELETED + 1)
>>
>> +enum flow_del_reason {
>> +FDR_REVALIDATE = 0, /* The flow was revalidated. */
>
> It was called FDR_FLOW_LIVE before, which might make more sense. As the flow
> is just NOT deleted. It might or might not have been revalidated. Thoughts?
I think it had to have been revalidated if we emit the reason, because
we only emit the reason code after revalidation. IE: there are many
places where we skip revalidation but the flow stays live - and we don't
emit reasons in those cases.
So at least for this patch, it MUST have been revalidated. But maybe in
the future, we would want to catch cases where the flow hasn't been. In
that case, it makes sense to add the FDR_FLOW_LIVE at that time - I
think.
Maybe you disagree?
>> +FDR_FLOW_IDLE, /* The flow went unused and was deleted. */
>> +FDR_TOO_EXPENSIVE, /* The flow was too expensive to revalidate. */
>> +FDR_FLOW_WILDCARDED,/* The flow needed a narrower wildcard mask. */
>> +FDR_BAD_ODP_FIT,/* The flow had a bad ODP flow fit. */
>> +FDR_NO_OFPROTO, /* The flow didn't have an associated ofproto.
>>
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
On 25 Jan 2024, at 21:55, Aaron Conole wrote:
> From: Kevin Sprague
>
> During normal operations, it is useful to understand when a particular flow
> gets removed from the system. This can be useful when debugging performance
> issues tied to ofproto flow changes, trying to determine deployed traffic
> patterns, or while debugging dynamic systems where ports come and go.
>
> Prior to this change, there was a lack of visibility around flow expiration.
> The existing debugging infrastructure could tell us when a flow was added to
> the datapath, but not when it was removed or why.
>
> This change introduces a USDT probe at the point where the revalidator
> determines that the flow should be removed. Additionally, we track the
> reason for the flow eviction and provide that information as well. With
> this change, we can track the complete flow lifecycle for the netlink
> datapath by hooking the upcall tracepoint in kernel, the flow put USDT, and
> the revaldiator USDT, letting us watch as flows are added and removed from
> the kernel datapath.
>
> This change only enables this information via USDT probe, so it won't be
> possible to access this information any other way (see:
> Documentation/topics/usdt-probes.rst).
>
> Also included is a script (utilities/usdt-scripts/flow_reval_monitor.py)
> which serves as a demonstration of how the new USDT probe might be used
> going forward.
>
> Signed-off-by: Kevin Sprague
> Co-authored-by: Aaron Conole
> Signed-off-by: Aaron Conole
Thanks for following this up Aaron! See comments on this patch below. I have no
additional comments on patch 2.
Cheers,
Eelco
> ---
> Documentation/topics/usdt-probes.rst | 1 +
> ofproto/ofproto-dpif-upcall.c| 42 +-
> utilities/automake.mk| 3 +
> utilities/usdt-scripts/flow_reval_monitor.py | 653 +++
> 4 files changed, 693 insertions(+), 6 deletions(-)
> create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py
>
> diff --git a/Documentation/topics/usdt-probes.rst
> b/Documentation/topics/usdt-probes.rst
> index e527f43bab..a8da9bb1f7 100644
> --- a/Documentation/topics/usdt-probes.rst
> +++ b/Documentation/topics/usdt-probes.rst
> @@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``:
> - dpif_recv:recv_upcall
> - main:poll_block
> - main:run_start
> +- revalidate:flow_result
> - revalidate_ukey\_\_:entry
> - revalidate_ukey\_\_:exit
> - udpif_revalidator:start_dump
You are missing the specific flow_result result section. This is from the
previous patch:
@@ -358,6 +360,27 @@ See also the ``main:run_start`` probe above.
- ``utilities/usdt-scripts/bridge_loop.bt``
+probe revalidate:flow_result
+~
+
+**Description**:
+This probe is triggered when the revalidator decides whether or not to
+revalidate a flow. ``reason`` is an enum that denotes that either the flow
+is being kept, or the reason why the flow is being deleted. The
+``flow_reval_monitor.py`` script uses this probe to notify users when flows
+matching user-provided criteria are deleted.
+
+**Arguments**:
+
+- *arg0*: ``(enum flow_del_reason) reason``
+- *arg1*: ``(struct udpif *) udpif``
+- *arg2*: ``(struct udpif_key *) ukey``
+
+**Script references**:
+
+- ``utilities/usdt-scripts/flow_reval_monitor.py``
+
+
Adding your own probes
--
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index b5cbeed878..97d75833f7 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -269,6 +269,18 @@ enum ukey_state {
> };
> #define N_UKEY_STATES (UKEY_DELETED + 1)
>
> +enum flow_del_reason {
> +FDR_REVALIDATE = 0, /* The flow was revalidated. */
It was called FDR_FLOW_LIVE before, which might make more sense. As the flow is
just NOT deleted. It might or might not have been revalidated. Thoughts?
> +FDR_FLOW_IDLE, /* The flow went unused and was deleted. */
> +FDR_TOO_EXPENSIVE, /* The flow was too expensive to revalidate. */
> +FDR_FLOW_WILDCARDED,/* The flow needed a narrower wildcard mask. */
> +FDR_BAD_ODP_FIT,/* The flow had a bad ODP flow fit. */
> +FDR_NO_OFPROTO, /* The flow didn't have an associated ofproto. */
> +FDR_XLATION_ERROR, /* There was an error translating the flow. */
> +FDR_AVOID_CACHING, /* Flow deleted to avoid caching. */
> +FDR_FLOW_LIMIT, /* All flows being killed. */
Looking at the comment from Han on FDR_PURGE, and this patch needing another
spin, we should probably add it.
> +};
> +
> /* 'udpif_key's are responsible for tracking the little bit of state udpif
> * needs to do flow expiration which can't be pulled directly from the
> * datapath. They may be created by any handler or revalidator thread at any
> @@ -2272,7 +2284,8 @@ populate_xcache(struct udpif *udpif, struct udpif_key
> *ukey,
> static enum reval_result
> revalidate_ukey__(str
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
On Fri, Jan 26, 2024 at 10:29:27AM -0800, Han Zhou wrote:
> On Fri, Jan 26, 2024 at 10:26 AM Han Zhou wrote:
> >
> >
> >
> > On Fri, Jan 26, 2024 at 7:53 AM Aaron Conole wrote:
> > >
> > > Han Zhou writes:
> > >
> > > > On Thu, Jan 25, 2024 at 12:55 PM Aaron Conole
> wrote:
> > > >>
> > > >> From: Kevin Sprague
> > > >>
> > > >> During normal operations, it is useful to understand when a
> particular flow
> > > >> gets removed from the system. This can be useful when debugging
> performance
> > > >> issues tied to ofproto flow changes, trying to determine deployed
> traffic
> > > >> patterns, or while debugging dynamic systems where ports come and go.
> > > >>
> > > >> Prior to this change, there was a lack of visibility around flow
> expiration.
> > > >> The existing debugging infrastructure could tell us when a flow was
> added to
> > > >> the datapath, but not when it was removed or why.
> > > >>
> > > >> This change introduces a USDT probe at the point where the
> revalidator
> > > >> determines that the flow should be removed. Additionally, we track
> the
> > > >> reason for the flow eviction and provide that information as well.
> With
> > > >> this change, we can track the complete flow lifecycle for the netlink
> > > >> datapath by hooking the upcall tracepoint in kernel, the flow put
> USDT, and
> > > >> the revaldiator USDT, letting us watch as flows are added and
> removed from
> > > >> the kernel datapath.
> > > >>
> > > >> This change only enables this information via USDT probe, so it
> won't be
> > > >> possible to access this information any other way (see:
> > > >> Documentation/topics/usdt-probes.rst).
> > > >>
> > > >> Also included is a script
> (utilities/usdt-scripts/flow_reval_monitor.py)
> > > >> which serves as a demonstration of how the new USDT probe might be
> used
> > > >> going forward.
> > > >>
> > > >> Signed-off-by: Kevin Sprague
> > > >> Co-authored-by: Aaron Conole
> > > >> Signed-off-by: Aaron Conole
> > > >
> > > > Thanks Aaron for taking care of this patch. I saw you resolved most
> of my comments for the v6 of the original patch:
> > > >
> https://mail.openvswitch.org/pipermail/ovs-dev/2023-January/401220.html
> > > >
> > > > Butit seems my last comment was missed:
> > > > ===
> > > >
> > > > I do notice a counter in my patch doesn't have a
> > > > counterpart in this patch. In revalidator_sweep__(), I have:
> > > > if (purge) {
> > > > result = UKEY_DELETE;
> > > > +COVERAGE_INC(upcall_flow_del_purge);
> > > >
> > > > Would it be good to add one (e.g. FDR_PURGE) here, too?
> > > >
> > > > ===
> > > > Could you check if this can be added?
> > > > If this is merged I can rebase my patch on top of this.
> > >
> > > Sorry I didn't reply to this.
> > >
> > > I'm not sure it makes sense to add the probe for purge, specifically as
> > > the purge is only done in two cases:
> > >
> > > 1. The threads are being stopped (which should never occur after
> > >initialization unless the vswitchd is being stopped / killed)
> > >
> > > 2. An admin runs a command to purge the revalidators (which isn't a
> > >recommended procedure as it can cause lots of really weird side
> > >effects and we only use it as a debug tool).
> > >
> > > Did I understand the case enough? I didn't reread the patch you're
> > > proposing, so I might be misunderstanding something.
> >
> > I believe your understanding is correct. However, I think it would be
> good to cover all the reasons of DP flow deletion, and "purge" seems to be
> the only case missing now.
> > Although purge is less likely to happen in production, it is still
> possible, probably in some weird scenarios. Would it be good to add for
> completeness, if there is no harm?
> >
> > Thanks,
> > Han
> >
> That being said, I don't have a strong opinion on this. So I give my ack in
> either case:
> Acked-by: Han Zhou
Perhaps we can treat this as a follow-up item?
Acked-by: Simon Horman
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
On Fri, Jan 26, 2024 at 10:26 AM Han Zhou wrote:
>
>
>
> On Fri, Jan 26, 2024 at 7:53 AM Aaron Conole wrote:
> >
> > Han Zhou writes:
> >
> > > On Thu, Jan 25, 2024 at 12:55 PM Aaron Conole
wrote:
> > >>
> > >> From: Kevin Sprague
> > >>
> > >> During normal operations, it is useful to understand when a
particular flow
> > >> gets removed from the system. This can be useful when debugging
performance
> > >> issues tied to ofproto flow changes, trying to determine deployed
traffic
> > >> patterns, or while debugging dynamic systems where ports come and go.
> > >>
> > >> Prior to this change, there was a lack of visibility around flow
expiration.
> > >> The existing debugging infrastructure could tell us when a flow was
added to
> > >> the datapath, but not when it was removed or why.
> > >>
> > >> This change introduces a USDT probe at the point where the
revalidator
> > >> determines that the flow should be removed. Additionally, we track
the
> > >> reason for the flow eviction and provide that information as well.
With
> > >> this change, we can track the complete flow lifecycle for the netlink
> > >> datapath by hooking the upcall tracepoint in kernel, the flow put
USDT, and
> > >> the revaldiator USDT, letting us watch as flows are added and
removed from
> > >> the kernel datapath.
> > >>
> > >> This change only enables this information via USDT probe, so it
won't be
> > >> possible to access this information any other way (see:
> > >> Documentation/topics/usdt-probes.rst).
> > >>
> > >> Also included is a script
(utilities/usdt-scripts/flow_reval_monitor.py)
> > >> which serves as a demonstration of how the new USDT probe might be
used
> > >> going forward.
> > >>
> > >> Signed-off-by: Kevin Sprague
> > >> Co-authored-by: Aaron Conole
> > >> Signed-off-by: Aaron Conole
> > >
> > > Thanks Aaron for taking care of this patch. I saw you resolved most
of my comments for the v6 of the original patch:
> > >
https://mail.openvswitch.org/pipermail/ovs-dev/2023-January/401220.html
> > >
> > > Butit seems my last comment was missed:
> > > ===
> > >
> > > I do notice a counter in my patch doesn't have a
> > > counterpart in this patch. In revalidator_sweep__(), I have:
> > > if (purge) {
> > > result = UKEY_DELETE;
> > > +COVERAGE_INC(upcall_flow_del_purge);
> > >
> > > Would it be good to add one (e.g. FDR_PURGE) here, too?
> > >
> > > ===
> > > Could you check if this can be added?
> > > If this is merged I can rebase my patch on top of this.
> >
> > Sorry I didn't reply to this.
> >
> > I'm not sure it makes sense to add the probe for purge, specifically as
> > the purge is only done in two cases:
> >
> > 1. The threads are being stopped (which should never occur after
> >initialization unless the vswitchd is being stopped / killed)
> >
> > 2. An admin runs a command to purge the revalidators (which isn't a
> >recommended procedure as it can cause lots of really weird side
> >effects and we only use it as a debug tool).
> >
> > Did I understand the case enough? I didn't reread the patch you're
> > proposing, so I might be misunderstanding something.
>
> I believe your understanding is correct. However, I think it would be
good to cover all the reasons of DP flow deletion, and "purge" seems to be
the only case missing now.
> Although purge is less likely to happen in production, it is still
possible, probably in some weird scenarios. Would it be good to add for
completeness, if there is no harm?
>
> Thanks,
> Han
>
That being said, I don't have a strong opinion on this. So I give my ack in
either case:
Acked-by: Han Zhou
> >
> > > Thanks,
> > > Han
> > >
> > >>
> > >> ---
> > >> Documentation/topics/usdt-probes.rst | 1 +
> > >> ofproto/ofproto-dpif-upcall.c| 42 +-
> > >> utilities/automake.mk| 3 +
> > >> utilities/usdt-scripts/flow_reval_monitor.py | 653
+++
> > >> 4 files changed, 693 insertions(+), 6 deletions(-)
> > >> create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py
> > >>
> > >> diff --git a/Documentation/topics/usdt-probes.rst
b/Documentation/topics/usdt-probes.rst
> > >> index e527f43bab..a8da9bb1f7 100644
> > >> --- a/Documentation/topics/usdt-probes.rst
> > >> +++ b/Documentation/topics/usdt-probes.rst
> > >> @@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``:
> > >> - dpif_recv:recv_upcall
> > >> - main:poll_block
> > >> - main:run_start
> > >> +- revalidate:flow_result
> > >> - revalidate_ukey\_\_:entry
> > >> - revalidate_ukey\_\_:exit
> > >> - udpif_revalidator:start_dump
> > >> diff --git a/ofproto/ofproto-dpif-upcall.c
b/ofproto/ofproto-dpif-upcall.c
> > >> index b5cbeed878..97d75833f7 100644
> > >> --- a/ofproto/ofproto-dpif-upcall.c
> > >> +++ b/ofproto/ofproto-dpif-upcall.c
> > >> @@ -269,6 +269,18 @@ enum ukey_state {
> > >> };
> > >> #define N_UKEY_STATES (UKEY_DELETED + 1)
> > >>
> > >> +enum flow_del_reason {
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
On Fri, Jan 26, 2024 at 7:53 AM Aaron Conole wrote:
>
> Han Zhou writes:
>
> > On Thu, Jan 25, 2024 at 12:55 PM Aaron Conole
wrote:
> >>
> >> From: Kevin Sprague
> >>
> >> During normal operations, it is useful to understand when a particular
flow
> >> gets removed from the system. This can be useful when debugging
performance
> >> issues tied to ofproto flow changes, trying to determine deployed
traffic
> >> patterns, or while debugging dynamic systems where ports come and go.
> >>
> >> Prior to this change, there was a lack of visibility around flow
expiration.
> >> The existing debugging infrastructure could tell us when a flow was
added to
> >> the datapath, but not when it was removed or why.
> >>
> >> This change introduces a USDT probe at the point where the revalidator
> >> determines that the flow should be removed. Additionally, we track the
> >> reason for the flow eviction and provide that information as well.
With
> >> this change, we can track the complete flow lifecycle for the netlink
> >> datapath by hooking the upcall tracepoint in kernel, the flow put
USDT, and
> >> the revaldiator USDT, letting us watch as flows are added and removed
from
> >> the kernel datapath.
> >>
> >> This change only enables this information via USDT probe, so it won't
be
> >> possible to access this information any other way (see:
> >> Documentation/topics/usdt-probes.rst).
> >>
> >> Also included is a script
(utilities/usdt-scripts/flow_reval_monitor.py)
> >> which serves as a demonstration of how the new USDT probe might be used
> >> going forward.
> >>
> >> Signed-off-by: Kevin Sprague
> >> Co-authored-by: Aaron Conole
> >> Signed-off-by: Aaron Conole
> >
> > Thanks Aaron for taking care of this patch. I saw you resolved most of
my comments for the v6 of the original patch:
> > https://mail.openvswitch.org/pipermail/ovs-dev/2023-January/401220.html
> >
> > Butit seems my last comment was missed:
> > ===
> >
> > I do notice a counter in my patch doesn't have a
> > counterpart in this patch. In revalidator_sweep__(), I have:
> > if (purge) {
> > result = UKEY_DELETE;
> > +COVERAGE_INC(upcall_flow_del_purge);
> >
> > Would it be good to add one (e.g. FDR_PURGE) here, too?
> >
> > ===
> > Could you check if this can be added?
> > If this is merged I can rebase my patch on top of this.
>
> Sorry I didn't reply to this.
>
> I'm not sure it makes sense to add the probe for purge, specifically as
> the purge is only done in two cases:
>
> 1. The threads are being stopped (which should never occur after
>initialization unless the vswitchd is being stopped / killed)
>
> 2. An admin runs a command to purge the revalidators (which isn't a
>recommended procedure as it can cause lots of really weird side
>effects and we only use it as a debug tool).
>
> Did I understand the case enough? I didn't reread the patch you're
> proposing, so I might be misunderstanding something.
I believe your understanding is correct. However, I think it would be good
to cover all the reasons of DP flow deletion, and "purge" seems to be the
only case missing now.
Although purge is less likely to happen in production, it is still
possible, probably in some weird scenarios. Would it be good to add for
completeness, if there is no harm?
Thanks,
Han
>
> > Thanks,
> > Han
> >
> >>
> >> ---
> >> Documentation/topics/usdt-probes.rst | 1 +
> >> ofproto/ofproto-dpif-upcall.c| 42 +-
> >> utilities/automake.mk| 3 +
> >> utilities/usdt-scripts/flow_reval_monitor.py | 653 +++
> >> 4 files changed, 693 insertions(+), 6 deletions(-)
> >> create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py
> >>
> >> diff --git a/Documentation/topics/usdt-probes.rst
b/Documentation/topics/usdt-probes.rst
> >> index e527f43bab..a8da9bb1f7 100644
> >> --- a/Documentation/topics/usdt-probes.rst
> >> +++ b/Documentation/topics/usdt-probes.rst
> >> @@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``:
> >> - dpif_recv:recv_upcall
> >> - main:poll_block
> >> - main:run_start
> >> +- revalidate:flow_result
> >> - revalidate_ukey\_\_:entry
> >> - revalidate_ukey\_\_:exit
> >> - udpif_revalidator:start_dump
> >> diff --git a/ofproto/ofproto-dpif-upcall.c
b/ofproto/ofproto-dpif-upcall.c
> >> index b5cbeed878..97d75833f7 100644
> >> --- a/ofproto/ofproto-dpif-upcall.c
> >> +++ b/ofproto/ofproto-dpif-upcall.c
> >> @@ -269,6 +269,18 @@ enum ukey_state {
> >> };
> >> #define N_UKEY_STATES (UKEY_DELETED + 1)
> >>
> >> +enum flow_del_reason {
> >> +FDR_REVALIDATE = 0, /* The flow was revalidated. */
> >> +FDR_FLOW_IDLE, /* The flow went unused and was deleted.
*/
> >> +FDR_TOO_EXPENSIVE, /* The flow was too expensive to
revalidate. */
> >> +FDR_FLOW_WILDCARDED,/* The flow needed a narrower wildcard
mask. */
> >> +FDR_BAD_ODP_FIT,/* The flow had a bad ODP flow fit. */
>
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
Han Zhou writes:
> On Thu, Jan 25, 2024 at 12:55 PM Aaron Conole wrote:
>>
>> From: Kevin Sprague
>>
>> During normal operations, it is useful to understand when a particular flow
>> gets removed from the system. This can be useful when debugging performance
>> issues tied to ofproto flow changes, trying to determine deployed traffic
>> patterns, or while debugging dynamic systems where ports come and go.
>>
>> Prior to this change, there was a lack of visibility around flow expiration.
>> The existing debugging infrastructure could tell us when a flow was added to
>> the datapath, but not when it was removed or why.
>>
>> This change introduces a USDT probe at the point where the revalidator
>> determines that the flow should be removed. Additionally, we track the
>> reason for the flow eviction and provide that information as well. With
>> this change, we can track the complete flow lifecycle for the netlink
>> datapath by hooking the upcall tracepoint in kernel, the flow put USDT, and
>> the revaldiator USDT, letting us watch as flows are added and removed from
>> the kernel datapath.
>>
>> This change only enables this information via USDT probe, so it won't be
>> possible to access this information any other way (see:
>> Documentation/topics/usdt-probes.rst).
>>
>> Also included is a script (utilities/usdt-scripts/flow_reval_monitor.py)
>> which serves as a demonstration of how the new USDT probe might be used
>> going forward.
>>
>> Signed-off-by: Kevin Sprague
>> Co-authored-by: Aaron Conole
>> Signed-off-by: Aaron Conole
>
> Thanks Aaron for taking care of this patch. I saw you resolved most of my
> comments for the v6 of the original patch:
> https://mail.openvswitch.org/pipermail/ovs-dev/2023-January/401220.html
>
> Butit seems my last comment was missed:
> ===
>
> I do notice a counter in my patch doesn't have a
> counterpart in this patch. In revalidator_sweep__(), I have:
> if (purge) {
> result = UKEY_DELETE;
> +COVERAGE_INC(upcall_flow_del_purge);
>
> Would it be good to add one (e.g. FDR_PURGE) here, too?
>
> ===
> Could you check if this can be added?
> If this is merged I can rebase my patch on top of this.
Sorry I didn't reply to this.
I'm not sure it makes sense to add the probe for purge, specifically as
the purge is only done in two cases:
1. The threads are being stopped (which should never occur after
initialization unless the vswitchd is being stopped / killed)
2. An admin runs a command to purge the revalidators (which isn't a
recommended procedure as it can cause lots of really weird side
effects and we only use it as a debug tool).
Did I understand the case enough? I didn't reread the patch you're
proposing, so I might be misunderstanding something.
> Thanks,
> Han
>
>>
>> ---
>> Documentation/topics/usdt-probes.rst | 1 +
>> ofproto/ofproto-dpif-upcall.c| 42 +-
>> utilities/automake.mk| 3 +
>> utilities/usdt-scripts/flow_reval_monitor.py | 653 +++
>> 4 files changed, 693 insertions(+), 6 deletions(-)
>> create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py
>>
>> diff --git a/Documentation/topics/usdt-probes.rst
>> b/Documentation/topics/usdt-probes.rst
>> index e527f43bab..a8da9bb1f7 100644
>> --- a/Documentation/topics/usdt-probes.rst
>> +++ b/Documentation/topics/usdt-probes.rst
>> @@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``:
>> - dpif_recv:recv_upcall
>> - main:poll_block
>> - main:run_start
>> +- revalidate:flow_result
>> - revalidate_ukey\_\_:entry
>> - revalidate_ukey\_\_:exit
>> - udpif_revalidator:start_dump
>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>> index b5cbeed878..97d75833f7 100644
>> --- a/ofproto/ofproto-dpif-upcall.c
>> +++ b/ofproto/ofproto-dpif-upcall.c
>> @@ -269,6 +269,18 @@ enum ukey_state {
>> };
>> #define N_UKEY_STATES (UKEY_DELETED + 1)
>>
>> +enum flow_del_reason {
>> +FDR_REVALIDATE = 0, /* The flow was revalidated. */
>> +FDR_FLOW_IDLE, /* The flow went unused and was deleted. */
>> +FDR_TOO_EXPENSIVE, /* The flow was too expensive to revalidate. */
>> +FDR_FLOW_WILDCARDED,/* The flow needed a narrower wildcard mask. */
>> +FDR_BAD_ODP_FIT,/* The flow had a bad ODP flow fit. */
>> +FDR_NO_OFPROTO, /* The flow didn't have an associated ofproto.
>> */
>> +FDR_XLATION_ERROR, /* There was an error translating the flow. */
>> +FDR_AVOID_CACHING, /* Flow deleted to avoid caching. */
>> +FDR_FLOW_LIMIT, /* All flows being killed. */
>> +};
>> +
>> /* 'udpif_key's are responsible for tracking the little bit of state udpif
>> * needs to do flow expiration which can't be pulled directly from the
>> * datapath. They may be created by any handler or revalidator thread at
>> any
>> @@ -2272,7 +2284,8 @@ populate_xcache(struct udpif *udpif, stru
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
On Thu, Jan 25, 2024 at 12:55 PM Aaron Conole wrote:
>
> From: Kevin Sprague
>
> During normal operations, it is useful to understand when a particular
flow
> gets removed from the system. This can be useful when debugging
performance
> issues tied to ofproto flow changes, trying to determine deployed traffic
> patterns, or while debugging dynamic systems where ports come and go.
>
> Prior to this change, there was a lack of visibility around flow
expiration.
> The existing debugging infrastructure could tell us when a flow was added
to
> the datapath, but not when it was removed or why.
>
> This change introduces a USDT probe at the point where the revalidator
> determines that the flow should be removed. Additionally, we track the
> reason for the flow eviction and provide that information as well. With
> this change, we can track the complete flow lifecycle for the netlink
> datapath by hooking the upcall tracepoint in kernel, the flow put USDT,
and
> the revaldiator USDT, letting us watch as flows are added and removed from
> the kernel datapath.
>
> This change only enables this information via USDT probe, so it won't be
> possible to access this information any other way (see:
> Documentation/topics/usdt-probes.rst).
>
> Also included is a script (utilities/usdt-scripts/flow_reval_monitor.py)
> which serves as a demonstration of how the new USDT probe might be used
> going forward.
>
> Signed-off-by: Kevin Sprague
> Co-authored-by: Aaron Conole
> Signed-off-by: Aaron Conole
Thanks Aaron for taking care of this patch. I saw you resolved most of my
comments for the v6 of the original patch:
https://mail.openvswitch.org/pipermail/ovs-dev/2023-January/401220.html
Butit seems my last comment was missed:
===
I do notice a counter in my patch doesn't have a
counterpart in this patch. In revalidator_sweep__(), I have:
if (purge) {
result = UKEY_DELETE;
+COVERAGE_INC(upcall_flow_del_purge);
Would it be good to add one (e.g. FDR_PURGE) here, too?
===
Could you check if this can be added?
If this is merged I can rebase my patch on top of this.
Thanks,
Han
>
> ---
> Documentation/topics/usdt-probes.rst | 1 +
> ofproto/ofproto-dpif-upcall.c| 42 +-
> utilities/automake.mk| 3 +
> utilities/usdt-scripts/flow_reval_monitor.py | 653 +++
> 4 files changed, 693 insertions(+), 6 deletions(-)
> create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py
>
> diff --git a/Documentation/topics/usdt-probes.rst
b/Documentation/topics/usdt-probes.rst
> index e527f43bab..a8da9bb1f7 100644
> --- a/Documentation/topics/usdt-probes.rst
> +++ b/Documentation/topics/usdt-probes.rst
> @@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``:
> - dpif_recv:recv_upcall
> - main:poll_block
> - main:run_start
> +- revalidate:flow_result
> - revalidate_ukey\_\_:entry
> - revalidate_ukey\_\_:exit
> - udpif_revalidator:start_dump
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index b5cbeed878..97d75833f7 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -269,6 +269,18 @@ enum ukey_state {
> };
> #define N_UKEY_STATES (UKEY_DELETED + 1)
>
> +enum flow_del_reason {
> +FDR_REVALIDATE = 0, /* The flow was revalidated. */
> +FDR_FLOW_IDLE, /* The flow went unused and was deleted. */
> +FDR_TOO_EXPENSIVE, /* The flow was too expensive to revalidate.
*/
> +FDR_FLOW_WILDCARDED,/* The flow needed a narrower wildcard mask.
*/
> +FDR_BAD_ODP_FIT,/* The flow had a bad ODP flow fit. */
> +FDR_NO_OFPROTO, /* The flow didn't have an associated
ofproto. */
> +FDR_XLATION_ERROR, /* There was an error translating the flow.
*/
> +FDR_AVOID_CACHING, /* Flow deleted to avoid caching. */
> +FDR_FLOW_LIMIT, /* All flows being killed. */
> +};
> +
> /* 'udpif_key's are responsible for tracking the little bit of state
udpif
> * needs to do flow expiration which can't be pulled directly from the
> * datapath. They may be created by any handler or revalidator thread
at any
> @@ -2272,7 +2284,8 @@ populate_xcache(struct udpif *udpif, struct
udpif_key *ukey,
> static enum reval_result
> revalidate_ukey__(struct udpif *udpif, const struct udpif_key *ukey,
>uint16_t tcp_flags, struct ofpbuf *odp_actions,
> - struct recirc_refs *recircs, struct xlate_cache
*xcache)
> + struct recirc_refs *recircs, struct xlate_cache
*xcache,
> + enum flow_del_reason *reason)
> {
> struct xlate_out *xoutp;
> struct netflow *netflow;
> @@ -2293,11 +2306,13 @@ revalidate_ukey__(struct udpif *udpif, const
struct udpif_key *ukey,
> netflow = NULL;
>
> if (xlate_ukey(udpif, ukey, tcp_flags, &ctx)) {
> +*reason = FDR_XLATION_ERROR;
> goto exit;
> }
> xoutp =
[ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
From: Kevin Sprague
During normal operations, it is useful to understand when a particular flow
gets removed from the system. This can be useful when debugging performance
issues tied to ofproto flow changes, trying to determine deployed traffic
patterns, or while debugging dynamic systems where ports come and go.
Prior to this change, there was a lack of visibility around flow expiration.
The existing debugging infrastructure could tell us when a flow was added to
the datapath, but not when it was removed or why.
This change introduces a USDT probe at the point where the revalidator
determines that the flow should be removed. Additionally, we track the
reason for the flow eviction and provide that information as well. With
this change, we can track the complete flow lifecycle for the netlink
datapath by hooking the upcall tracepoint in kernel, the flow put USDT, and
the revaldiator USDT, letting us watch as flows are added and removed from
the kernel datapath.
This change only enables this information via USDT probe, so it won't be
possible to access this information any other way (see:
Documentation/topics/usdt-probes.rst).
Also included is a script (utilities/usdt-scripts/flow_reval_monitor.py)
which serves as a demonstration of how the new USDT probe might be used
going forward.
Signed-off-by: Kevin Sprague
Co-authored-by: Aaron Conole
Signed-off-by: Aaron Conole
---
Documentation/topics/usdt-probes.rst | 1 +
ofproto/ofproto-dpif-upcall.c| 42 +-
utilities/automake.mk| 3 +
utilities/usdt-scripts/flow_reval_monitor.py | 653 +++
4 files changed, 693 insertions(+), 6 deletions(-)
create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py
diff --git a/Documentation/topics/usdt-probes.rst
b/Documentation/topics/usdt-probes.rst
index e527f43bab..a8da9bb1f7 100644
--- a/Documentation/topics/usdt-probes.rst
+++ b/Documentation/topics/usdt-probes.rst
@@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``:
- dpif_recv:recv_upcall
- main:poll_block
- main:run_start
+- revalidate:flow_result
- revalidate_ukey\_\_:entry
- revalidate_ukey\_\_:exit
- udpif_revalidator:start_dump
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index b5cbeed878..97d75833f7 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -269,6 +269,18 @@ enum ukey_state {
};
#define N_UKEY_STATES (UKEY_DELETED + 1)
+enum flow_del_reason {
+FDR_REVALIDATE = 0, /* The flow was revalidated. */
+FDR_FLOW_IDLE, /* The flow went unused and was deleted. */
+FDR_TOO_EXPENSIVE, /* The flow was too expensive to revalidate. */
+FDR_FLOW_WILDCARDED,/* The flow needed a narrower wildcard mask. */
+FDR_BAD_ODP_FIT,/* The flow had a bad ODP flow fit. */
+FDR_NO_OFPROTO, /* The flow didn't have an associated ofproto. */
+FDR_XLATION_ERROR, /* There was an error translating the flow. */
+FDR_AVOID_CACHING, /* Flow deleted to avoid caching. */
+FDR_FLOW_LIMIT, /* All flows being killed. */
+};
+
/* 'udpif_key's are responsible for tracking the little bit of state udpif
* needs to do flow expiration which can't be pulled directly from the
* datapath. They may be created by any handler or revalidator thread at any
@@ -2272,7 +2284,8 @@ populate_xcache(struct udpif *udpif, struct udpif_key
*ukey,
static enum reval_result
revalidate_ukey__(struct udpif *udpif, const struct udpif_key *ukey,
uint16_t tcp_flags, struct ofpbuf *odp_actions,
- struct recirc_refs *recircs, struct xlate_cache *xcache)
+ struct recirc_refs *recircs, struct xlate_cache *xcache,
+ enum flow_del_reason *reason)
{
struct xlate_out *xoutp;
struct netflow *netflow;
@@ -2293,11 +2306,13 @@ revalidate_ukey__(struct udpif *udpif, const struct
udpif_key *ukey,
netflow = NULL;
if (xlate_ukey(udpif, ukey, tcp_flags, &ctx)) {
+*reason = FDR_XLATION_ERROR;
goto exit;
}
xoutp = &ctx.xout;
if (xoutp->avoid_caching) {
+*reason = FDR_AVOID_CACHING;
goto exit;
}
@@ -2311,6 +2326,7 @@ revalidate_ukey__(struct udpif *udpif, const struct
udpif_key *ukey,
ofpbuf_clear(odp_actions);
if (!ofproto) {
+*reason = FDR_NO_OFPROTO;
goto exit;
}
@@ -2322,6 +2338,7 @@ revalidate_ukey__(struct udpif *udpif, const struct
udpif_key *ukey,
if (odp_flow_key_to_mask(ukey->mask, ukey->mask_len, &dp_mask, &ctx.flow,
NULL)
== ODP_FIT_ERROR) {
+*reason = FDR_BAD_ODP_FIT;
goto exit;
}
@@ -2331,6 +2348,7 @@ revalidate_ukey__(struct udpif *udpif, const struct
udpif_key *ukey,
* down. Note that we do not know if the datapath has ignored any of the
* wildcarded bits, so we may be overly conservati
