On 9 Jul 2024, at 21:31, Adrián Moreno wrote:
> On Tue, Jul 09, 2024 at 11:46:09AM GMT, Eelco Chaudron wrote:
>> On 7 Jul 2024, at 22:09, Adrian Moreno wrote:
>>
>>> The decision to add or not the explicit drop action is currently based
>>> on whether the resulting dp action list is empty or not.
>>>
>>> This is OK for most of the cases but if per-flow IPFIX/sFlow sampling
>>> is enabled on the bridge, it doesn't work as expected.
>>>
>>> Fix this by taking into account the size of these sampling actions.
>>
>> Same comments as on the previous patch.
>>
>>> Fixes: a13a0209750c ("userspace: Improved packet drop statistics.")
>>> Cc: anju.tho...@ericsson.com
>>> Signed-off-by: Adrian Moreno <amore...@redhat.com>
>>> ---
>>> ofproto/ofproto-dpif-xlate.c | 5 ++--
>>> tests/drop-stats.at | 44 ++++++++++++++++++++++++++++++++++++
>>> tests/ofproto-dpif.at | 4 ++--
>>> 3 files changed, 49 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>>> index 094fe5d72..323a58cbf 100644
>>> --- a/ofproto/ofproto-dpif-xlate.c
>>> +++ b/ofproto/ofproto-dpif-xlate.c
>>> @@ -8153,6 +8153,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out
>>> *xout)
>>> uint64_t action_set_stub[1024 / 8];
>>> uint64_t frozen_actions_stub[1024 / 8];
>>> uint64_t actions_stub[256 / 8];
>>> + size_t sample_actions_len = 0;
>>> struct ofpbuf scratch_actions = OFPBUF_STUB_INITIALIZER(actions_stub);
>>> struct xlate_ctx ctx = {
>>> .xin = xin,
>>> @@ -8412,7 +8413,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out
>>> *xout)
>>> user_cookie_offset = compose_sflow_action(&ctx);
>>> compose_ipfix_action(&ctx, ODPP_NONE);
>>> }
>>> - size_t sample_actions_len = ctx.odp_actions->size;
>>> + sample_actions_len = ctx.odp_actions->size;
>>> bool ecn_drop = !tnl_process_ecn(flow);
>>>
>>> if (!ecn_drop
>>> @@ -8575,7 +8576,7 @@ exit:
>>> }
>>>
>>> /* Install drop action if datapath supports explicit drop action. */
>>> - if (xin->odp_actions && !xin->odp_actions->size &&
>>> + if (xin->odp_actions && xin->odp_actions->size == sample_actions_len &&
>>> ovs_explicit_drop_action_supported(ctx.xbridge->ofproto)) {
>>> put_drop_action(xin->odp_actions, ctx.error);
>>
>> See other patch, do we need ctx.error here?
>
> Here we *do* need it.
>
> This is where it was originally placed to flag xlate errors. We are at
> the end of xlate_actions main function and the one place where we have
> gathered ctx.error.
>
> The change here is not to modify how the drop is added to the action
> list. What this change does is change the condition upon which it's
> added. Before we were checking if the action list was empty under the
> assumption that if do_xlate_actions does not fill in any action, it's a
> drop.
>
> However, if per-bridge IPFIX (or sFlow) is enabled, their corresponding
> actions are added _before_ calling do_xlate_actions. That's why this
> patch only makes the check compare the size of the actions with the one
> it had before calling do_xlate_actions.
>
> Calling do_xlate_actions can yield errors and we should definitely flag
> them.
You are right, I was too quick adding the comment :(
>>
>>> }
>>> diff --git a/tests/drop-stats.at b/tests/drop-stats.at
>>> index 44c5cc35b..31782ac52 100644
>>> --- a/tests/drop-stats.at
>>> +++ b/tests/drop-stats.at
>>> @@ -192,6 +192,50 @@ ovs-appctl coverage/read-counter
>>> drop_action_too_many_mpls_labels
>>> OVS_VSWITCHD_STOP(["/|WARN|/d"])
>>> AT_CLEANUP
>>>
>>> +AT_SETUP([drop-stats - sampling])
>>
>> Should this maybe be 'bridge sampling'?
>>
>
> Ack.
>
>>> +
>>> +OVS_VSWITCHD_START([dnl
>>> + set bridge br0 datapath_type=dummy \
>>> + protocols=OpenFlow10,OpenFlow13,OpenFlow14,OpenFlow15 -- \
>>> + add-port br0 p1 -- set Interface p1 type=dummy ofport_request=1])
>>> +
>>> +AT_DATA([flows.txt], [dnl
>>> +table=0,in_port=1,actions=drop
>>> +])
>>> +
>>> +AT_CHECK([
>>> + ovs-ofctl del-flows br0
>>> + ovs-ofctl -Oopenflow13 add-flows br0 flows.txt
>>> + ovs-ofctl -Oopenflow13 dump-flows br0 | ofctl_strip | sort | grep
>>> actions ], [0], [dnl
>>> + in_port=1 actions=drop
>>> +])
>>> +
>>> +AT_CHECK([ovs-vsctl -- set bridge br0 ipfix=@fix -- \
>>> + --id=@fix create ipfix targets=\"127.0.0.1:4739\" \
>>> + sampling=1],
>>> + [0], [ignore])
>>> +
>>> +AT_CHECK([
>>> + ovs-appctl netdev-dummy/receive p1
>>> 'in_port(1),packet_type(ns=0,id=0),eth(src=3a:6d:d2:09:9c:ab,dst=1e:2c:e9:2a:66:9e),ipv4(src=192.168.10.10,dst=192.168.10.30,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
>>> + ovs-appctl netdev-dummy/receive p1
>>> 'in_port(1),packet_type(ns=0,id=0),eth(src=3a:6d:d2:09:9c:ab,dst=1e:2c:e9:2a:66:9e),ipv4(src=192.168.10.10,dst=192.168.10.30,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
>>> + ovs-appctl netdev-dummy/receive p1
>>> 'in_port(1),packet_type(ns=0,id=0),eth(src=3a:6d:d2:09:9c:ab,dst=1e:2c:e9:2a:66:9e),ipv4(src=192.168.10.10,dst=192.168.10.30,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
>>> +], [0], [ignore])
>>>
>>
>> For loop?
>>
>
> Ack.
>
>>> +AT_CHECK([ovs-appctl dpctl/dump-flows | sed
>>> 's/used:[[0-9]].[[0-9]]*s/used:0.0/' | sort], [0], [flow-dump from the main
>>> thread:
>>> +recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no),
>>> packets:2, bytes:212, used:0.0,
>>> actions:userspace(pid=0,ipfix(output_port=4294967295)),drop
>>> +])
>>> +
>>> +ovs-appctl time/warp 5000
>>> +
>>> +AT_CHECK([
>>> +ovs-appctl coverage/read-counter drop_action_of_pipeline
>>> +], [0], [dnl
>>> +3
>>> +])
>>> +
>>> +OVS_VSWITCHD_STOP(["/sending to collector failed/d"])
>>> +
>>> +AT_CLEANUP
>>> AT_SETUP([drop-stats - sampling action])
>>>
>>> OVS_VSWITCHD_START([dnl
>>> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
>>> index 701b8a851..ba8f3b69c 100644
>>> --- a/tests/ofproto-dpif.at
>>> +++ b/tests/ofproto-dpif.at
>>> @@ -8082,7 +8082,7 @@ for i in `seq 1 3`; do
>>> 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 the main thread:
>>> -packets:2, bytes:68, used:0.001s,
>>> actions:userspace(pid=0,ipfix(output_port=4294967295))
>>> +packets:2, bytes:68, used:0.001s,
>>> actions:userspace(pid=0,ipfix(output_port=4294967295)),drop
>>> ])
>>>
>>> AT_CHECK([ovs-appctl revalidator/purge])
>>> @@ -8118,7 +8118,7 @@ for i in `seq 1 3`; do
>>> 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 the main thread:
>>> -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:sample(sample=100.0%,actions(meter(0),userspace(pid=0,ipfix(output_port=4294967295)))),drop
>>> ])
>>>
>>> dnl Remove the IPFIX configuration.
>>> --
>>> 2.45.2
>>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
