| Message ID | 20230417162813.2242181-1-david.marchand@redhat.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [ovs-dev] system-offloads-traffic: Remove tc ingress pps check on meter offload. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | fail | test: fail |
On Mon, Apr 17, 2023 at 06:28:13PM +0200, David Marchand wrote: > Caught during some code review. > The incriminated commit had put an unneeded check on tc ingress support > for the meter offloading test. > > Note: SUPPORT_TC_INGRESS_PPS had been reworked in the commit 5f0fdf5e2c2e > ("test: Move check for tc ingress pps support to test script."). > > Fixes: 5660b89a309d ("dpif-netlink: Offloading meter to tc police action") > Signed-off-by: David Marchand <david.marchand@redhat.com> Hi David, I am slightly confused by this. 1. The test in question is for hardware offload of metering, which will use the TC datapath. 2. The test creates a meter with pktps (PPS) rate limiting, which will lead to the creation of a TC police action with a PPS rate. 3. SUPPORT_TC_INGRESS_PPS is intended to check if TC police action with PPS rate is supported by the kernel. I feel that I am missing something obvious here. > --- > tests/system-offloads-traffic.at | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at > index da18597cd8..df43caa9e5 100644 > --- a/tests/system-offloads-traffic.at > +++ b/tests/system-offloads-traffic.at > @@ -240,7 +240,6 @@ AT_CLEANUP > > AT_SETUP([offloads - check interface meter offloading - offloads enabled]) > AT_KEYWORDS([offload-meter]) > -AT_SKIP_IF([test $SUPPORT_TC_INGRESS_PPS = "no"]) > AT_SKIP_IF([test $HAVE_NC = "no"]) > OVS_TRAFFIC_VSWITCHD_START([], [], [-- set Open_vSwitch . other_config:hw-offload=true]) > > -- > 2.39.2 >
On Tue, Apr 18, 2023 at 10:20 AM Simon Horman <simon.horman@corigine.com> wrote: > > On Mon, Apr 17, 2023 at 06:28:13PM +0200, David Marchand wrote: > > Caught during some code review. > > The incriminated commit had put an unneeded check on tc ingress support > > for the meter offloading test. > > > > Note: SUPPORT_TC_INGRESS_PPS had been reworked in the commit 5f0fdf5e2c2e > > ("test: Move check for tc ingress pps support to test script."). > > > > Fixes: 5660b89a309d ("dpif-netlink: Offloading meter to tc police action") > > Signed-off-by: David Marchand <david.marchand@redhat.com> > > Hi David, > > I am slightly confused by this. Well, I am a bit lost in those tests :-). > > 1. The test in question is for hardware offload of metering, > which will use the TC datapath. > 2. The test creates a meter with pktps (PPS) rate limiting, > which will lead to the creation of a TC police action with a PPS rate. I had not made a link with tc.. thanks for forcing me to look better. > 3. SUPPORT_TC_INGRESS_PPS is intended to check if > TC police action with PPS rate is supported by the kernel. Well, back to this topic then. SUPPORT_TC_INGRESS_PPS does not exist anymore. So I guess we should at least fix it as CHECK_TC_INGRESS_PPS?
On Tue, Apr 18, 2023 at 10:34:22AM +0200, David Marchand wrote: > On Tue, Apr 18, 2023 at 10:20 AM Simon Horman <simon.horman@corigine.com> wrote: > > > > On Mon, Apr 17, 2023 at 06:28:13PM +0200, David Marchand wrote: > > > Caught during some code review. > > > The incriminated commit had put an unneeded check on tc ingress support > > > for the meter offloading test. > > > > > > Note: SUPPORT_TC_INGRESS_PPS had been reworked in the commit 5f0fdf5e2c2e > > > ("test: Move check for tc ingress pps support to test script."). > > > > > > Fixes: 5660b89a309d ("dpif-netlink: Offloading meter to tc police action") > > > Signed-off-by: David Marchand <david.marchand@redhat.com> > > > > Hi David, > > > > I am slightly confused by this. > > Well, I am a bit lost in those tests :-). > > > > > 1. The test in question is for hardware offload of metering, > > which will use the TC datapath. > > 2. The test creates a meter with pktps (PPS) rate limiting, > > which will lead to the creation of a TC police action with a PPS rate. > > I had not made a link with tc.. thanks for forcing me to look better. > > > > 3. SUPPORT_TC_INGRESS_PPS is intended to check if > > TC police action with PPS rate is supported by the kernel. > > Well, back to this topic then. > SUPPORT_TC_INGRESS_PPS does not exist anymore. > So I guess we should at least fix it as CHECK_TC_INGRESS_PPS? See, I knew I was missing something obvious. Yes, I think that update is needed. It looks like I missed it when writing 5f0fdf5e2c2e. And I think it should be that commit in the Fixes tag for your patch.
On Tue, Apr 18, 2023 at 10:53 AM Simon Horman <simon.horman@corigine.com> wrote: > > > 3. SUPPORT_TC_INGRESS_PPS is intended to check if > > > TC police action with PPS rate is supported by the kernel. > > > > Well, back to this topic then. > > SUPPORT_TC_INGRESS_PPS does not exist anymore. > > So I guess we should at least fix it as CHECK_TC_INGRESS_PPS? > > See, I knew I was missing something obvious. > > Yes, I think that update is needed. > It looks like I missed it when writing 5f0fdf5e2c2e. > And I think it should be that commit in the Fixes tag for your patch. I just sent a new patch, and marked this current one as Rejected. Thanks Simon and sorry for the confusion.
diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at index da18597cd8..df43caa9e5 100644 --- a/tests/system-offloads-traffic.at +++ b/tests/system-offloads-traffic.at @@ -240,7 +240,6 @@ AT_CLEANUP AT_SETUP([offloads - check interface meter offloading - offloads enabled]) AT_KEYWORDS([offload-meter]) -AT_SKIP_IF([test $SUPPORT_TC_INGRESS_PPS = "no"]) AT_SKIP_IF([test $HAVE_NC = "no"]) OVS_TRAFFIC_VSWITCHD_START([], [], [-- set Open_vSwitch . other_config:hw-offload=true])
Caught during some code review. The incriminated commit had put an unneeded check on tc ingress support for the meter offloading test. Note: SUPPORT_TC_INGRESS_PPS had been reworked in the commit 5f0fdf5e2c2e ("test: Move check for tc ingress pps support to test script."). Fixes: 5660b89a309d ("dpif-netlink: Offloading meter to tc police action") Signed-off-by: David Marchand <david.marchand@redhat.com> --- tests/system-offloads-traffic.at | 1 - 1 file changed, 1 deletion(-)