[ovs-dev,action,upcall,meters,4/5] ofproto: Meter sample action when configured.

Submitted by andy zhou on April 14, 2017, 7:46 p.m.

Details

Message ID 1492199182-4479-5-git-send-email-azhou@ovn.org
State Superseded
Headers show

Commit Message

andy zhou April 14, 2017, 7:46 p.m.
When slowpath meter is configured, add meter action when translate
sample action.

Signed-off-by: Andy Zhou <azhou@ovn.org>
---
 ofproto/ofproto-dpif-xlate.c |  9 +++++++++
 tests/ofproto-dpif.at        | 14 ++++++++++++++
 2 files changed, 23 insertions(+)

Comments

Jarno Rajahalme April 27, 2017, 10:42 p.m.
This breaks the test "ofproto-dpif - Bridge IPFIX sanity check” (currently test #1128), which appears to be the tests case that is being modified.

More comments below.

> On Apr 14, 2017, at 12:46 PM, Andy Zhou <azhou@ovn.org> wrote:
> 
> When slowpath meter is configured, add meter action when translate
> sample action.
> 
> Signed-off-by: Andy Zhou <azhou@ovn.org>
> ---
> ofproto/ofproto-dpif-xlate.c |  9 +++++++++
> tests/ofproto-dpif.at        | 14 ++++++++++++++
> 2 files changed, 23 insertions(+)
> 
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index a24aef9a43a1..52e0d3f1b0bb 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -2861,6 +2861,15 @@ compose_sample_action(struct xlate_ctx *ctx,
>                                              OVS_SAMPLE_ATTR_ACTIONS);
>     }
> 
> +    /* If the slow path meter is configured by the controller,
> +     * Insert a meter action before the user space action.   */
> +    struct ofproto *ofproto = &ctx->xin->ofproto->up;
> +    uint32_t meter_id = ofproto->slowpath_meter_id;
> +
> +    if (meter_id != UINT32_MAX) {
> +        nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_METER, meter_id);
> +    }
> +
>     odp_port_t odp_port = ofp_port_to_odp_port(
>         ctx->xbridge, ctx->xin->flow.in_port.ofp_port);
>     uint32_t pid = dpif_port_get_pid(ctx->xbridge->dpif, odp_port,
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 0c2ea384b422..3c3037b16548 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -6491,6 +6491,20 @@ flow-dump from non-dpdk interfaces:
> packets:2, bytes:68, used:0.001s, actions:userspace(pid=0,ipfix(output_port=4294967295))
> ])
> 
> +AT_CHECK([ovs-appctl revalidator/purge])
> +dnl
> +dnl Add a slowpath meter. The userspace action should be metered.
> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=slowpath pktps burst stats bands=type=drop rate=3 burst_size=1'])
> +
> +dnl Send some packets that should be sampled and metered.
> +for i in `seq 1 3`; do
> +    AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800)'])
> +done
> +AT_CHECK([ovs-appctl dpctl/dump-flows | sed 's/.*\(packets:\)/\1/' | sed 's/used:[[0-9]].[[0-9]]*s/used:0.001s/'], [0], [dnl
> +flow-dump from non-dpdk interfaces:
> +packets:2, bytes:68, used:0.001s, actions:sample(sample=100.0%,actions(meter(0),userspace(pid=0,ipfix(output_port=4294967295))))
> +])
> +

This is the test failure:

-packets:2, bytes:68, used:0.001s, actions:sample(sample=100.0%,actions(meter(0),userspace(pid=0,ipfix(output_port=4294967295))))
+packets:2, bytes:68, used:0.001s, actions:meter(0),userspace(pid=0,ipfix(output_port=4294967295))

Applied to current master the sample envelope is not being inserted when probability is 100%. However, when using a meter the sample envelope is needed in all cases, as if the meter drops the packet, we still need to continue execution if there are further actions after the sample action.


> dnl Remove the IPFIX configuration.
> AT_CHECK([ovs-vsctl clear bridge br0 ipfix])
> AT_CHECK([ovs-appctl revalidator/purge])
> -- 
> 1.8.3.1
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
andy zhou April 28, 2017, 8:54 a.m.
On Thu, Apr 27, 2017 at 3:42 PM, Jarno Rajahalme <jarno@ovn.org> wrote:
> This breaks the test "ofproto-dpif - Bridge IPFIX sanity check” (currently test #1128), which appears to be the tests case that is being modified.
Oops. As you have noticed with patch 5, I messed up in splitting those patches.
>
> More comments below.
>
>> On Apr 14, 2017, at 12:46 PM, Andy Zhou <azhou@ovn.org> wrote:
>>
>> When slowpath meter is configured, add meter action when translate
>> sample action.
>>
>> Signed-off-by: Andy Zhou <azhou@ovn.org>
>> ---
>> ofproto/ofproto-dpif-xlate.c |  9 +++++++++
>> tests/ofproto-dpif.at        | 14 ++++++++++++++
>> 2 files changed, 23 insertions(+)
>>
>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>> index a24aef9a43a1..52e0d3f1b0bb 100644
>> --- a/ofproto/ofproto-dpif-xlate.c
>> +++ b/ofproto/ofproto-dpif-xlate.c
>> @@ -2861,6 +2861,15 @@ compose_sample_action(struct xlate_ctx *ctx,
>>                                              OVS_SAMPLE_ATTR_ACTIONS);
>>     }
>>
>> +    /* If the slow path meter is configured by the controller,
>> +     * Insert a meter action before the user space action.   */
>> +    struct ofproto *ofproto = &ctx->xin->ofproto->up;
>> +    uint32_t meter_id = ofproto->slowpath_meter_id;
>> +
>> +    if (meter_id != UINT32_MAX) {
>> +        nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_METER, meter_id);
>> +    }
>> +
>>     odp_port_t odp_port = ofp_port_to_odp_port(
>>         ctx->xbridge, ctx->xin->flow.in_port.ofp_port);
>>     uint32_t pid = dpif_port_get_pid(ctx->xbridge->dpif, odp_port,
>> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
>> index 0c2ea384b422..3c3037b16548 100644
>> --- a/tests/ofproto-dpif.at
>> +++ b/tests/ofproto-dpif.at
>> @@ -6491,6 +6491,20 @@ flow-dump from non-dpdk interfaces:
>> packets:2, bytes:68, used:0.001s, actions:userspace(pid=0,ipfix(output_port=4294967295))
>> ])
>>
>> +AT_CHECK([ovs-appctl revalidator/purge])
>> +dnl
>> +dnl Add a slowpath meter. The userspace action should be metered.
>> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=slowpath pktps burst stats bands=type=drop rate=3 burst_size=1'])
>> +
>> +dnl Send some packets that should be sampled and metered.
>> +for i in `seq 1 3`; do
>> +    AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800)'])
>> +done
>> +AT_CHECK([ovs-appctl dpctl/dump-flows | sed 's/.*\(packets:\)/\1/' | sed 's/used:[[0-9]].[[0-9]]*s/used:0.001s/'], [0], [dnl
>> +flow-dump from non-dpdk interfaces:
>> +packets:2, bytes:68, used:0.001s, actions:sample(sample=100.0%,actions(meter(0),userspace(pid=0,ipfix(output_port=4294967295))))
>> +])
>> +
>
> This is the test failure:
>
> -packets:2, bytes:68, used:0.001s, actions:sample(sample=100.0%,actions(meter(0),userspace(pid=0,ipfix(output_port=4294967295))))
> +packets:2, bytes:68, used:0.001s, actions:meter(0),userspace(pid=0,ipfix(output_port=4294967295))
>
> Applied to current master the sample envelope is not being inserted when probability is 100%. However, when using a meter the sample envelope is needed in all cases, as if the meter drops the packet, we still need to continue execution if there are further actions after the sample action.
>
I agree, the test is correct, the logic is correct in patch 5, but not
here. Will fix.
>
>> dnl Remove the IPFIX configuration.
>> AT_CHECK([ovs-vsctl clear bridge br0 ipfix])
>> AT_CHECK([ovs-appctl revalidator/purge])
>> --
>> 1.8.3.1
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

Patch hide | download patch | download mbox

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index a24aef9a43a1..52e0d3f1b0bb 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2861,6 +2861,15 @@  compose_sample_action(struct xlate_ctx *ctx,
                                              OVS_SAMPLE_ATTR_ACTIONS);
     }
 
+    /* If the slow path meter is configured by the controller,
+     * Insert a meter action before the user space action.   */
+    struct ofproto *ofproto = &ctx->xin->ofproto->up;
+    uint32_t meter_id = ofproto->slowpath_meter_id;
+
+    if (meter_id != UINT32_MAX) {
+        nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_METER, meter_id);
+    }
+
     odp_port_t odp_port = ofp_port_to_odp_port(
         ctx->xbridge, ctx->xin->flow.in_port.ofp_port);
     uint32_t pid = dpif_port_get_pid(ctx->xbridge->dpif, odp_port,
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 0c2ea384b422..3c3037b16548 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -6491,6 +6491,20 @@  flow-dump from non-dpdk interfaces:
 packets:2, bytes:68, used:0.001s, actions:userspace(pid=0,ipfix(output_port=4294967295))
 ])
 
+AT_CHECK([ovs-appctl revalidator/purge])
+dnl
+dnl Add a slowpath meter. The userspace action should be metered.
+AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=slowpath pktps burst stats bands=type=drop rate=3 burst_size=1'])
+
+dnl Send some packets that should be sampled and metered.
+for i in `seq 1 3`; do
+    AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800)'])
+done
+AT_CHECK([ovs-appctl dpctl/dump-flows | sed 's/.*\(packets:\)/\1/' | sed 's/used:[[0-9]].[[0-9]]*s/used:0.001s/'], [0], [dnl
+flow-dump from non-dpdk interfaces:
+packets:2, bytes:68, used:0.001s, actions:sample(sample=100.0%,actions(meter(0),userspace(pid=0,ipfix(output_port=4294967295))))
+])
+
 dnl Remove the IPFIX configuration.
 AT_CHECK([ovs-vsctl clear bridge br0 ipfix])
 AT_CHECK([ovs-appctl revalidator/purge])