On 9 Jul 2024, at 16:00, Adrián Moreno wrote:
> On Tue, Jul 09, 2024 at 11:46:06AM GMT, Eelco Chaudron wrote:
>> On 7 Jul 2024, at 22:09, Adrian Moreno wrote:
>>
>>> When an action set ends in a an OFP_SAMPLE action, it means the packet
>>> will be dropped and sampled. Make the drop explicit in this case so that
>>> drop statistics remain accurate.
>>>
>>> This could be done outside of the sample action, i.e: "sample(...),drop"
>>> but datapaths optimize sample actions that are found in the last
>>> position. So, taking into account that datapaths already report when the
>>> last sample probability fails, it is safe to put the drop inside the
>>> sample, i.e: "sample(...,drop)".
>>
>> Some style comments below, as this was discussed with Ilya, I let him comment
>> on the feature side.
>>
>>> Signed-off-by: Adrian Moreno <amore...@redhat.com>
>>> ---
>>> ofproto/ofproto-dpif-xlate.c | 16 +++++++--
>>> tests/drop-stats.at | 65 ++++++++++++++++++++++++++++++++++++
>>> tests/ofproto-dpif.at | 44 ++++++++++++++++++++++++
>>> 3 files changed, 123 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>>> index b9546dc5b..094fe5d72 100644
>>> --- a/ofproto/ofproto-dpif-xlate.c
>>> +++ b/ofproto/ofproto-dpif-xlate.c
>>> @@ -715,6 +715,7 @@ static void xlate_xbundle_copy(struct xbridge *, struct
>>> xbundle *);
>>> static void xlate_xport_copy(struct xbridge *, struct xbundle *,
>>> struct xport *);
>>> static void xlate_xcfg_free(struct xlate_cfg *);
>>> +static void put_drop_action(struct ofpbuf *, enum xlate_error);
>>>
>>> /* Tracing helpers. */
>>>
>>> @@ -3392,6 +3393,8 @@ struct sample_userspace_args {
>>> struct compose_sample_args {
>>> uint32_t probability; /* Number of packets out of
>>> * UINT32_MAX to sample. */
>>> + bool last; /* If it's the last action
>>> and a
>>> + * drop action must be
>>> added. */
>>
>> Should describing this means this is the last action not be enough?
>> Maybe also rename it to be last_action, so it's more clear what this means.
>>
>
> Sure "last_action" is pretty much self-explanatory.
>
>>> struct sample_userspace_args *userspace; /* Optional,
>>> * arguments for userspace.
>>> */
>>> struct sample_psample_args *psample; /* Optional,
>>> @@ -3456,6 +3459,11 @@ compose_sample_action(struct xlate_ctx *ctx,
>>> ovs_assert(res == 0);
>>> }
>>>
>>> + if (args->last &&
>>> + ovs_explicit_drop_action_supported(ctx->xbridge->ofproto)) {
>>> + put_drop_action(ctx->odp_actions, ctx->error);
>>
>> Any reason why we use ctx->error here? Should we just not use XLATE_OK?
>>
>
> I was trying to thing in a case where ctx->error would not be XLATE_OK
> but I couldn't. Nevertheless I thought if there is such case, we should
> definitely report the right error code, right?
Well, in case of an error all actions are deleted, and only an explicit drop
(or no) actions are added.
For the sample() action, the drop reason should always be ok, or else the
action should not be there. So I would prefer to make this clear by using
XLATE_OK.
>>> + }
>>> +
>>> if (is_sample) {
>>> nl_msg_end_nested(ctx->odp_actions, actions_offset);
>>> nl_msg_end_nested(ctx->odp_actions, sample_offset);
>>> @@ -3490,6 +3498,7 @@ compose_sflow_action(struct xlate_ctx *ctx)
>>>
>>> args.probability = dpif_sflow_get_probability(sflow);
>>> args.userspace = &userspace;
>>> + args.last = false;
>>>
>>> return compose_sample_action(ctx, &args);
>>> }
>>> @@ -3542,6 +3551,7 @@ compose_ipfix_action(struct xlate_ctx *ctx,
>>> odp_port_t output_odp_port)
>>>
>>> args.probability = dpif_ipfix_get_bridge_exporter_probability(ipfix);
>>> args.userspace = &userspace;
>>> + args.last = false;
>>>
>>> compose_sample_action(ctx, &args);
>>> }
>>> @@ -5974,7 +5984,8 @@ xlate_fill_ipfix_sample(struct xlate_ctx *ctx,
>>>
>>> static void
>>> xlate_sample_action(struct xlate_ctx *ctx,
>>> - const struct ofpact_sample *os)
>>> + const struct ofpact_sample *os,
>>> + bool last)
>>> {
>>> uint8_t cookie_buf[sizeof(os->obs_domain_id) +
>>> sizeof(os->obs_point_id)];
>>> struct dpif_lsample *lsample = ctx->xbridge->lsample;
>>> @@ -5991,6 +6002,7 @@ xlate_sample_action(struct xlate_ctx *ctx,
>>> * the same percentage. */
>>> compose_args.probability =
>>> ((uint32_t) os->probability << 16) | os->probability;
>>> + compose_args.last = last;
>>>
>>> if (ipfix) {
>>> xlate_fill_ipfix_sample(ctx, os, ipfix, &userspace);
>>> @@ -7726,7 +7738,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
>>> ofpacts_len,
>>> break;
>>>
>>> case OFPACT_SAMPLE:
>>> - xlate_sample_action(ctx, ofpact_get_SAMPLE(a));
>>> + xlate_sample_action(ctx, ofpact_get_SAMPLE(a), last);
>>> break;
>>>
>>> case OFPACT_CLONE:
>>> diff --git a/tests/drop-stats.at b/tests/drop-stats.at
>>> index 1d3af98da..44c5cc35b 100644
>>> --- a/tests/drop-stats.at
>>> +++ b/tests/drop-stats.at
>>> @@ -191,3 +191,68 @@ ovs-appctl coverage/read-counter
>>> drop_action_too_many_mpls_labels
>>>
>>> OVS_VSWITCHD_STOP(["/|WARN|/d"])
>>> AT_CLEANUP
>>> +
>>> +AT_SETUP([drop-stats - sampling action])
>>> +
>>> +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 -- \
>>> + add-port br0 p2 -- set Interface p2 type=dummy ofport_request=2])
>>> +
>>> +AT_DATA([flows.txt], [dnl
>>> +table=0,in_port=1,actions=sample(probability=65535,collector_set_id=1)
>>> +table=0,in_port=2,actions=sample(probability=32767,collector_set_id=1)
>>> +])
>>> +
>>> +AT_CHECK([
>>> + ovs-ofctl del-flows br0
>>> + ovs-ofctl -Oopenflow13 add-flows br0 flows.txt
>>> +])
>>> +
>>> +AT_CHECK([ovs-vsctl --id=@br0 get Bridge br0 \
>>> + -- --id=@ipfix create IPFIX targets=\"127.0.0.1:4739\"
>>> \
>>> + -- create Flow_Sample_Collector_Set id=1 bridge=@br0
>>> ipfix=@ipfix],
>>> + [0], [ignore])
>>
>> Maybe we can try this and other places to be more <80 friendly?
>>
>
> Ack.
>
>>> +
>>> +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])
>>
>> Maybe a 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,flow_sample(probability=65535,collector_set_id=1,obs_domain_id=0,obs_point_id=0,output_port=4294967295)),drop
>>> +])
>>> +
>>> +ovs-appctl time/warp 5000
>>> +
>>> +AT_CHECK([
>>> +ovs-appctl coverage/read-counter drop_action_of_pipeline
>>> +], [0], [dnl
>>> +3
>>> +])
>>> +
>>> +AT_CHECK([ovs-appctl dpctl/del-flows])
>>> +
>>> +AT_CHECK([
>>> + ovs-appctl netdev-dummy/receive p2
>>> 'in_port(2),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 p2
>>> 'in_port(2),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 p2
>>> 'in_port(2),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])
>>> +
>>> +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(2),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no),
>>> packets:2, bytes:212, used:0.0,
>>> actions:sample(sample=50.0%,actions(userspace(pid=0,flow_sample(probability=32767,collector_set_id=1,obs_domain_id=0,obs_point_id=0,output_port=4294967295)),drop))
>>> +])
>>> +
>>> +ovs-appctl time/warp 5000
>>> +
>>> +AT_CHECK([ovs-appctl coverage/read-counter drop_action_of_pipeline >
>>> explicit_drops])
>>> +AT_CHECK([ovs-appctl coverage/read-counter datapath_drop_sample_error >
>>> sample_drops])
>>> +
>>> +AT_CHECK([expr $(cat sample_drops) + $(cat explicit_drops)], [0], [dnl
>>> +6
>>> +])
>>> +
>>> +OVS_VSWITCHD_STOP(["/sending to collector failed/d"])
>>> +AT_CLEANUP
>>> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
>>> index 2a04977a7..701b8a851 100644
>>> --- a/tests/ofproto-dpif.at
>>> +++ b/tests/ofproto-dpif.at
>>> @@ -12282,3 +12282,47 @@ Datapath actions: EXPECTED_ACT
>>>
>>> OVS_VSWITCHD_STOP()
>>> AT_CLEANUP
>>> +
>>> +AT_SETUP([ofproto-dpif - Local sampling - drop])
>>> +OVS_VSWITCHD_START
>>> +add_of_ports br0 1 2
>>> +
>>> +AT_CHECK([ovs-appctl dpif/set-dp-features --force br0 psample true], [0],
>>> [ignore])
>>> +
>>> +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
>>> + -- create Flow_Sample_Collector_Set id=1 bridge=@br0
>>> local_group_id=42],
>>> + [0], [ignore])
>>> +
>>> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=slowpath pktps
>>> stats bands=type=drop rate=2'])
>>> +
>>> +AT_DATA([flows.txt], [dnl
>>> +in_port=1,
>>> actions=sample(probability=32767,collector_set_id=1,obs_domain_id=100,obs_point_id=200)
>>> +in_port=2,
>>> actions=sample(probability=65535,collector_set_id=1,obs_domain_id=100,obs_point_id=200)
>>> +])
>>> +
>>> +AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt])
>>> +
>>> +m4_define([TRACE_PKT], [m4_join([,],
>>> + [eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800)],
>>> + [ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no)],
>>> + [icmp(type=8,code=0)])])
>>> +
>>> +m4_define([EXPECTED_ACT1], [m4_join([],
>>> + [sample(sample=50.0%,actions(],
>>> + [psample(group=42,cookie=0x64000000c8),],
>>> + [drop],
>>> + [))],
>>> +)])
>>> +
>>> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1) TRACE_PKT'], [0],
>>> [stdout])
>>> +AT_CHECK([tail -1 stdout], [0], [dnl
>>> +Datapath actions: EXPECTED_ACT1
>>> +])
>>> +
>>> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(2) TRACE_PKT'], [0],
>>> [stdout])
>>> +AT_CHECK([tail -1 stdout], [0], [dnl
>>> +Datapath actions: psample(group=42,cookie=0x64000000c8),drop
>>> +])
>>> +
>>> +OVS_VSWITCHD_STOP()
>>> +AT_CLEANUP
>>> --
>>> 2.45.2
>>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
