[ovs-dev] [PATCHv7 1/3] Improved Packet Drop Statistics in OVS
Ilya Maximets
i.maximets at samsung.com
Thu Feb 7 14:22:28 UTC 2019
On 07.02.2019 17:11, Anju Thomas wrote:
> Thanks for comments Ilya. Just one question regarding the steal flag :-
>
>> + case OVS_ACTION_ATTR_DROP: {
>> + const enum xlate_error *drop_reason = nl_attr_get(a);
>> + if (*drop_reason < XLATE_MAX) {
>> + dp_update_drop_action_counter(*drop_reason, batch->count);
>> + }
>> + dp_packet_delete_batch(batch, steal);
>
> I'm not sure if we need to honor 'steal' while performing the DROP action, because packets will not be dropped if 'steal == false'.
>
> <Anju> What if there is the drop is within a clone action and there is another output action.In that case , we would not want to delete the batch . Right?
If the drop action is within a clone action, it will drop cloned packets.
i.e. "actions=clone(drop),output:N" should clone packets, drop cloned packets
and send original packets to port N.
"actions=drop,output:N" should not send anything to N, because all packets
should be dropped while executing drop action. This list of actions makes
no practical sense, though.
>
> Regards
> Anju
>
> -----Original Message-----
> From: Ilya Maximets [mailto:i.maximets at samsung.com]
> Sent: Wednesday, February 06, 2019 8:48 PM
> To: Anju Thomas <anju.thomas at ericsson.com>; Ben Pfaff <blp at ovn.org>
> Cc: dev at openvswitch.org
> Subject: Re: [PATCHv7 1/3] Improved Packet Drop Statistics in OVS
>
> Hi.
> See comments inline.
>
> Best regards, Ilya Maximets.
>
> On 06.02.2019 7:01, Anju Thomas wrote:
>>
>> Hi Ben/Ilya,
>>
>> I have addressed the comments in the below patch. Can you tell me if
>> this is fin> Regards Anju -----Original Message-----
>> From: Anju Thomas [mailto:anju.thomas at ericsson.com]
>> Sent: Tuesday, January 29, 2019 5:21 PM
>> To: dev at openvswitch.org
>> Cc: Anju Thomas <anju.thomas at ericsson.com>
>> Subject: [PATCHv7 1/3] Improved Packet Drop Statistics in OVS
>>
>> Currently OVS maintains explicit packet drop/error counters only on port
>> level. Packets that are dropped as part of normal OpenFlow processing are
>> counted in flow stats of “drop” flows or as table misses in table stats.
>> These can only be interpreted by controllers that know the semantics of
>> the configured OpenFlow pipeline. Without that knowledge, it is impossible
>> for an OVS user to obtain e.g. the total number of packets dropped due to
>> OpenFlow rules.
>>
>> Furthermore, there are numerous other reasons for which packets can be
>> dropped by OVS slow path that are not related to the OpenFlow pipeline.
>> The generated datapath flow entries include a drop action to avoid further
>> expensive upcalls to the slow path, but subsequent packets dropped by the
>> datapath are not accounted anywhere.
>>
>> Finally, the datapath itself drops packets in certain error situations.
>> Also, these drops are today not accounted for.
>>
>> This makes it difficult for OVS users to monitor packet drop in an OVS
>> instance and to alert a management system in case of a unexpected increase
>> of such drops. Also OVS trouble-shooters face difficulties in analysing
>> packet drops.
>>
>> With this patch we implement following changes to address the issues
>> mentioned above.
>>
>> 1. Identify and account all the silent packet drop scenarios
>>
>> 2. Display these drops in ovs-appctl coverage/show
>>
>> A detailed presentation on this was presented at OvS conference 2017 and
>> link for the corresponding presentation is available at:
>>
>>
>> https://www.slideshare.net/LF_OpenvSwitch/lfovs17troubleshooting-the-d
>> ata-plane-in-ovs-82280329
>>
>> Co-authored-by: Rohith Basavaraja <rohith.basavaraja at gmail.com>
>> Co-authored-by: Keshav Gupta <keshugupta1 at gmail.com>
>> Signed-off-by: Anju Thomas <anju.thomas at ericsson.com>
>> Signed-off-by: Rohith Basavaraja <rohith.basavaraja at gmail.com>
>> Signed-off-by: Keshav Gupta <keshugupta1 at gmail.com>
>
>
> You still have this strange shift to the right by 3 spaces in commit-message.
>
>> ---
>> datapath/linux/compat/include/linux/openvswitch.h | 1 +
>> lib/dpif-netdev.c | 39 ++++++++++-
>> lib/dpif.c | 7 ++
>> lib/dpif.h | 3 +
>> lib/netdev-dpdk.c | 4 ++
>> lib/odp-execute.c | 81 +++++++++++++++++++++++
>> lib/odp-execute.h | 2 +
>> lib/odp-util.c | 10 ++-
>> ofproto/ofproto-dpif-ipfix.c | 1 +
>> ofproto/ofproto-dpif-sflow.c | 1 +
>> ofproto/ofproto-dpif-xlate.c | 31 +++++++++
>> ofproto/ofproto-dpif-xlate.h | 4 ++
>> ofproto/ofproto-dpif.c | 8 +++
>> ofproto/ofproto-dpif.h | 8 ++-
>> tests/automake.mk | 3 +-
>> tests/dpif-netdev.at | 10 +++
>> tests/ofproto-dpif.at | 4 +-
>> tests/testsuite.at | 1 +
>> tests/tunnel-push-pop.at | 28 +++++++-
>> tests/tunnel.at | 14 +++-
>> 20 files changed, 248 insertions(+), 12 deletions(-)
>>
>> diff --git a/datapath/linux/compat/include/linux/openvswitch.h
>> b/datapath/linux/compat/include/linux/openvswitch.h
>> index 9b087f1..92db378 100644
>> --- a/datapath/linux/compat/include/linux/openvswitch.h
>> +++ b/datapath/linux/compat/include/linux/openvswitch.h
>> @@ -938,6 +938,7 @@ enum ovs_action_attr {
>> OVS_ACTION_ATTR_POP_NSH, /* No argument. */
>> OVS_ACTION_ATTR_METER, /* u32 meter number. */
>> OVS_ACTION_ATTR_CLONE, /* Nested OVS_CLONE_ATTR_*. */
>> + OVS_ACTION_ATTR_DROP,
>
> Comment about argument type required.
>
>>
>> #ifndef __KERNEL__
>> OVS_ACTION_ATTR_TUNNEL_PUSH, /* struct ovs_action_push_tnl*/
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index
>> 0f57e3f..c726463 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -77,6 +77,7 @@
>> #include "unixctl.h"
>> #include "util.h"
>> #include "uuid.h"
>> +#include "ofproto/ofproto-dpif-xlate.h"
>
> This header not needed there.
>
>>
>> VLOG_DEFINE_THIS_MODULE(dpif_netdev);
>>
>> @@ -100,6 +101,17 @@ enum { MAX_METERS = 65536 }; /* Maximum number of meters. */
>> enum { MAX_BANDS = 8 }; /* Maximum number of bands / meter. */
>> enum { N_METER_LOCKS = 64 }; /* Maximum number of meters. */
>>
>> +COVERAGE_DEFINE(datapath_drop_meter);
>> +COVERAGE_DEFINE(datapath_drop_upcall_error);
>> +COVERAGE_DEFINE(datapath_drop_lock_error);
>> +COVERAGE_DEFINE(datapath_drop_userspace_action_error);
>> +COVERAGE_DEFINE(datapath_drop_tunnel_push_error);
>> +COVERAGE_DEFINE(datapath_drop_tunnel_pop_error);
>> +COVERAGE_DEFINE(datapath_drop_recirc_error);
>> +COVERAGE_DEFINE(datapath_drop_invalid_port);
>> +COVERAGE_DEFINE(datapath_drop_invalid_tnl_port);
>> +COVERAGE_DEFINE(datapath_drop_rx_invalid_packet);
>> +
>> /* Protects against changes to 'dp_netdevs'. */ static struct
>> ovs_mutex dp_netdev_mutex = OVS_MUTEX_INITIALIZER;
>>
>> @@ -5643,7 +5655,7 @@ dp_netdev_run_meter(struct dp_netdev *dp, struct dp_packet_batch *packets_,
>> band = &meter->bands[exceeded_band[j]];
>> band->packet_count += 1;
>> band->byte_count += dp_packet_size(packet);
>> -
>> + COVERAGE_INC(datapath_drop_meter);
>> dp_packet_delete(packet);
>> } else {
>> /* Meter accepts packet. */ @@ -6399,6 +6411,7 @@
>> dfc_processing(struct dp_netdev_pmd_thread *pmd,
>>
>> if (OVS_UNLIKELY(dp_packet_size(packet) < ETH_HEADER_LEN)) {
>> dp_packet_delete(packet);
>> + COVERAGE_INC(datapath_drop_rx_invalid_packet);
>> continue;
>> }
>>
>> @@ -6525,6 +6538,7 @@ handle_packet_upcall(struct dp_netdev_pmd_thread *pmd,
>> put_actions);
>> if (OVS_UNLIKELY(error && error != ENOSPC)) {
>> dp_packet_delete(packet);
>> + COVERAGE_INC(datapath_drop_upcall_error);
>> return error;
>> }
>>
>> @@ -6656,6 +6670,7 @@ fast_path_processing(struct dp_netdev_pmd_thread *pmd,
>> DP_PACKET_BATCH_FOR_EACH (i, packet, packets_) {
>> if (OVS_UNLIKELY(!rules[i])) {
>> dp_packet_delete(packet);
>> + COVERAGE_INC(datapath_drop_lock_error);
>> upcall_fail_cnt++;
>> }
>> }
>> @@ -6925,6 +6940,7 @@ dp_execute_userspace_action(struct dp_netdev_pmd_thread *pmd,
>> actions->data, actions->size);
>> } else if (should_steal) {
>> dp_packet_delete(packet);
>> + COVERAGE_INC(datapath_drop_userspace_action_error);
>> }
>> }
>>
>> @@ -6939,6 +6955,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch *packets_,
>> struct dp_netdev *dp = pmd->dp;
>> int type = nl_attr_type(a);
>> struct tx_port *p;
>> + uint32_t packet_count, packet_dropped;
>>
>> switch ((enum ovs_action_attr)type) {
>> case OVS_ACTION_ATTR_OUTPUT:
>> @@ -6980,6 +6997,8 @@ dp_execute_cb(void *aux_, struct dp_packet_batch *packets_,
>> dp_packet_batch_add(&p->output_pkts, packet);
>> }
>> return;
>> + } else {
>> + COVERAGE_ADD(datapath_drop_invalid_port,
>> + packets_->count);
>
> Please, use 'dp_packet_batch_size(packets_)' instead of direct access to 'count'.
> Same for all the places below.
>
>> }
>> break;
>>
>> @@ -6989,10 +7008,13 @@ dp_execute_cb(void *aux_, struct dp_packet_batch *packets_,
>> * the ownership of these packets. Thus, we can avoid performing
>> * the action, because the caller will not use the result anyway.
>> * Just break to free the batch. */
>> + COVERAGE_ADD(datapath_drop_tunnel_push_error,
>> + packets_->count);
>> break;
>> }
>> dp_packet_batch_apply_cutlen(packets_);
>> - push_tnl_action(pmd, a, packets_);
>> + if (push_tnl_action(pmd, a, packets_)) {
>> + COVERAGE_ADD(datapath_drop_tunnel_push_error,
>> + packets_->count);
>
> 'push_tnl_action' deletes the packet batch on failure. So, 'packets_->count'
> is always zero here.
>
>> + }
>> return;
>>
>> case OVS_ACTION_ATTR_TUNNEL_POP:
>> @@ -7012,7 +7034,13 @@ dp_execute_cb(void *aux_, struct
>> dp_packet_batch *packets_,
>>
>> dp_packet_batch_apply_cutlen(packets_);
>>
>> + packet_count = packets_->count;
>> netdev_pop_header(p->port->netdev, packets_);
>> + packet_dropped = packet_count - packets_->count;
>> + if (packet_dropped) {
>> + COVERAGE_ADD(datapath_drop_tunnel_pop_error,
>> + packet_dropped);
>> + }
>> if (dp_packet_batch_is_empty(packets_)) {
>> return;
>> }
>> @@ -7027,6 +7055,9 @@ dp_execute_cb(void *aux_, struct dp_packet_batch *packets_,
>> (*depth)--;
>> return;
>> }
>> + COVERAGE_ADD(datapath_drop_invalid_tnl_port, packets_->count);
>> + } else {
>> + COVERAGE_ADD(datapath_drop_recirc_error,
>> + packets_->count);
>> }
>> break;
>>
>> @@ -7071,6 +7102,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch
>> *packets_,
>>
>> return;
>> }
>> + COVERAGE_ADD(datapath_drop_lock_error, packets_->count);
>> break;
>>
>> case OVS_ACTION_ATTR_RECIRC:
>> @@ -7093,7 +7125,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch
>> *packets_,
>>
>> return;
>> }
>> -
>> + COVERAGE_ADD(datapath_drop_recirc_error, packets_->count);
>> VLOG_WARN("Packet dropped. Max recirculation depth exceeded.");
>> break;
>>
>> @@ -7246,6 +7278,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch *packets_,
>> case OVS_ACTION_ATTR_PUSH_NSH:
>> case OVS_ACTION_ATTR_POP_NSH:
>> case OVS_ACTION_ATTR_CT_CLEAR:
>> + case OVS_ACTION_ATTR_DROP:
>> case __OVS_ACTION_ATTR_MAX:
>> OVS_NOT_REACHED();
>> }
>> diff --git a/lib/dpif.c b/lib/dpif.c
>> index e35f111..abdb679 100644
>> --- a/lib/dpif.c
>> +++ b/lib/dpif.c
>> @@ -1274,6 +1274,7 @@ dpif_execute_helper_cb(void *aux_, struct dp_packet_batch *packets_,
>> case OVS_ACTION_ATTR_POP_NSH:
>> case OVS_ACTION_ATTR_CT_CLEAR:
>> case OVS_ACTION_ATTR_UNSPEC:
>> + case OVS_ACTION_ATTR_DROP:
>> case __OVS_ACTION_ATTR_MAX:
>> OVS_NOT_REACHED();
>> }
>> @@ -1879,6 +1880,12 @@ dpif_supports_tnl_push_pop(const struct dpif *dpif)
>> return dpif_is_netdev(dpif);
>> }
>>
>> +bool
>> +dpif_supports_explicit_drop_action(const struct dpif *dpif) {
>> + return dpif_is_netdev(dpif);
>> +}
>> +
>> /* Meters */
>> void
>> dpif_meter_get_features(const struct dpif *dpif, diff --git
>> a/lib/dpif.h b/lib/dpif.h index 475d5a6..e799da8 100644
>> --- a/lib/dpif.h
>> +++ b/lib/dpif.h
>> @@ -888,6 +888,9 @@ int dpif_get_pmds_for_port(const struct dpif *
>> dpif, odp_port_t port_no,
>>
>> char *dpif_get_dp_version(const struct dpif *); bool
>> dpif_supports_tnl_push_pop(const struct dpif *);
>> +bool dpif_supports_explicit_drop_action(const struct dpif *); int
>> +dpif_show_drop_stats_support(struct dpif *dpif, bool detail,
>> + struct ds *reply);
>>
>> /* Log functions. */
>> struct vlog_module;
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
>> 4bf0ca9..4a61a5c 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -2458,6 +2458,10 @@ netdev_dpdk_send__(struct netdev_dpdk *dev, int qid,
>> bool concurrent_txq) {
>> if (OVS_UNLIKELY(!(dev->flags & NETDEV_UP))) {
>> + int batch_cnt = dp_packet_batch_size(batch);
>> + rte_spinlock_lock(&dev->stats_lock);
>> + dev->stats.tx_dropped += batch_cnt;
>> + rte_spinlock_unlock(&dev->stats_lock);
>
> As I already wrote, above change should be sent as a separate patch.
> It's not related to this patch and should be possibly backported to some previous branches.
>
>> dp_packet_delete_batch(batch, true);
>> return;
>> }
>> diff --git a/lib/odp-execute.c b/lib/odp-execute.c index
>> 3b6890e..b67c755 100644
>> --- a/lib/odp-execute.c
>> +++ b/lib/odp-execute.c
>> @@ -25,6 +25,7 @@
>> #include <stdlib.h>
>> #include <string.h>
>>
>> +#include "coverage.h"
>> #include "dp-packet.h"
>> #include "dpif.h"
>> #include "netlink.h"
>> @@ -36,6 +37,73 @@
>> #include "util.h"
>> #include "csum.h"
>> #include "conntrack.h"
>> +#include "ofproto/ofproto-dpif-xlate.h"
>> +#include "openvswitch/vlog.h"
>> +
>> +VLOG_DEFINE_THIS_MODULE(odp_execute)
>> +COVERAGE_DEFINE(dp_sample_error_drop);
>> +COVERAGE_DEFINE(dp_nsh_decap_error_drop);
>> +COVERAGE_DEFINE(drop_action_of_pipeline);
>> +COVERAGE_DEFINE(drop_action_bridge_not_found);
>> +COVERAGE_DEFINE(drop_action_recursion_too_deep);
>> +COVERAGE_DEFINE(drop_action_too_many_resubmit);
>> +COVERAGE_DEFINE(drop_action_stack_too_deep);
>> +COVERAGE_DEFINE(drop_action_no_recirculation_context);
>> +COVERAGE_DEFINE(drop_action_recirculation_conflict);
>> +COVERAGE_DEFINE(drop_action_too_many_mpls_labels);
>> +COVERAGE_DEFINE(drop_action_invalid_tunnel_metadata);
>> +COVERAGE_DEFINE(drop_action_unsupported_packet_type);
>> +COVERAGE_DEFINE(drop_action_congestion);
>> +COVERAGE_DEFINE(drop_action_forwarding_disabled);
>> +
>> +static void
>> +dp_update_drop_action_counter(enum xlate_error drop_reason,
>> + int delta) {
>> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>
> Empty line would be nice.
>
>> + switch (drop_reason) {
>> + case XLATE_OK:
>> + COVERAGE_ADD(drop_action_of_pipeline, delta);
>> + break;
>> + case XLATE_BRIDGE_NOT_FOUND:
>> + COVERAGE_ADD(drop_action_bridge_not_found, delta);
>> + break;
>> + case XLATE_RECURSION_TOO_DEEP:
>> + COVERAGE_ADD(drop_action_recursion_too_deep, delta);
>> + break;
>> + case XLATE_TOO_MANY_RESUBMITS:
>> + COVERAGE_ADD(drop_action_too_many_resubmit, delta);
>> + break;
>> + case XLATE_STACK_TOO_DEEP:
>> + COVERAGE_ADD(drop_action_stack_too_deep, delta);
>> + break;
>> + case XLATE_NO_RECIRCULATION_CONTEXT:
>> + COVERAGE_ADD(drop_action_no_recirculation_context, delta);
>> + break;
>> + case XLATE_RECIRCULATION_CONFLICT:
>> + COVERAGE_ADD(drop_action_recirculation_conflict, delta);
>> + break;
>> + case XLATE_TOO_MANY_MPLS_LABELS:
>> + COVERAGE_ADD(drop_action_too_many_mpls_labels, delta);
>> + break;
>> + case XLATE_INVALID_TUNNEL_METADATA:
>> + COVERAGE_ADD(drop_action_invalid_tunnel_metadata, delta);
>> + break;
>> + case XLATE_UNSUPPORTED_PACKET_TYPE:
>> + COVERAGE_ADD(drop_action_unsupported_packet_type, delta);
>> + break;
>> + case XLATE_CONGESTION_DROP:
>> + COVERAGE_ADD(drop_action_congestion, delta);
>> + break;
>> + case XLATE_FORWARDING_DISABLED:
>> + COVERAGE_ADD(drop_action_forwarding_disabled, delta);
>> + break;
>> + case XLATE_MAX:
>> + default:
>> + VLOG_ERR_RL(&rl,"Invalid Drop reason type:%d",drop_reason);
>
> Some spaces needed.
>
>> + }
>> +}
>> +
>>
>> /* Masked copy of an ethernet address. 'src' is already properly masked. */ static void @@ -589,6 +657,7 @@ odp_execute_sample(void *dp, struct dp_packet *packet, bool steal,
>> case OVS_SAMPLE_ATTR_PROBABILITY:
>> if (random_uint32() >= nl_attr_get_u32(a)) {
>> if (steal) {
>> + COVERAGE_ADD(dp_sample_error_drop, 1);
>> dp_packet_delete(packet);
>> }
>> return;
>> @@ -673,6 +742,7 @@ requires_datapath_assistance(const struct nlattr *a)
>> case OVS_ACTION_ATTR_PUSH_NSH:
>> case OVS_ACTION_ATTR_POP_NSH:
>> case OVS_ACTION_ATTR_CT_CLEAR:
>> + case OVS_ACTION_ATTR_DROP:
>> return false;
>>
>> case OVS_ACTION_ATTR_UNSPEC:
>> @@ -705,6 +775,7 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal,
>> const struct nlattr *a;
>> unsigned int left;
>>
>> +
>
> Not needed.
>
>> NL_ATTR_FOR_EACH_UNSAFE (a, left, actions, actions_len) {
>> int type = nl_attr_type(a);
>> bool last_action = (left <= NLA_ALIGN(a->nla_len)); @@ -889,6 +960,7 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal,
>> if (pop_nsh(packet)) {
>> dp_packet_batch_refill(batch, packet, i);
>> } else {
>> + COVERAGE_INC(dp_nsh_decap_error_drop);
>> dp_packet_delete(packet);
>> }
>> }
>> @@ -900,6 +972,15 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal,
>> }
>> break;
>>
>> + case OVS_ACTION_ATTR_DROP: {
>> + const enum xlate_error *drop_reason = nl_attr_get(a);
>> + if (*drop_reason < XLATE_MAX) {
>> + dp_update_drop_action_counter(*drop_reason, batch->count);
>> + }
>> + dp_packet_delete_batch(batch, steal);
>
> I'm not sure if we need to honor 'steal' while performing the DROP action, because packets will not be dropped if 'steal == false'.
>
>> + return;
>> + }
>> +
>> case OVS_ACTION_ATTR_OUTPUT:
>> case OVS_ACTION_ATTR_TUNNEL_PUSH:
>> case OVS_ACTION_ATTR_TUNNEL_POP:
>> diff --git a/lib/odp-execute.h b/lib/odp-execute.h index
>> a3578a5..c3e1815 100644
>> --- a/lib/odp-execute.h
>> +++ b/lib/odp-execute.h
>> @@ -22,6 +22,8 @@
>> #include <stddef.h>
>> #include <stdint.h>
>> #include "openvswitch/types.h"
>> +#include "ovs-atomic.h"
>> +#include "dpif.h"
>
> Above change seems redundant.
>
>>
>> struct nlattr;
>> struct dp_packet;
>> diff --git a/lib/odp-util.c b/lib/odp-util.c index 778c00e..ecf9668
>> 100644
>> --- a/lib/odp-util.c
>> +++ b/lib/odp-util.c
>> @@ -43,6 +43,7 @@
>> #include "uuid.h"
>> #include "openvswitch/vlog.h"
>> #include "openvswitch/match.h"
>> +#include "ofproto/ofproto-dpif-xlate.h"
>>
>> VLOG_DEFINE_THIS_MODULE(odp_util);
>>
>> @@ -131,6 +132,7 @@ odp_action_len(uint16_t type)
>> case OVS_ACTION_ATTR_CLONE: return ATTR_LEN_VARIABLE;
>> case OVS_ACTION_ATTR_PUSH_NSH: return ATTR_LEN_VARIABLE;
>> case OVS_ACTION_ATTR_POP_NSH: return 0;
>> + case OVS_ACTION_ATTR_DROP: return sizeof(enum xlate_error);
>>
>> case OVS_ACTION_ATTR_UNSPEC:
>> case __OVS_ACTION_ATTR_MAX:
>> @@ -1181,6 +1183,9 @@ format_odp_action(struct ds *ds, const struct nlattr *a,
>> case OVS_ACTION_ATTR_POP_NSH:
>> ds_put_cstr(ds, "pop_nsh()");
>> break;
>> + case OVS_ACTION_ATTR_DROP:
>> + ds_put_cstr(ds, "drop");
>> + break;
>> case OVS_ACTION_ATTR_UNSPEC:
>> case __OVS_ACTION_ATTR_MAX:
>> default:
>> @@ -2427,11 +2432,14 @@ odp_actions_from_string(const char *s, const struct simap *port_names,
>> struct ofpbuf *actions) {
>> size_t old_size;
>> + enum xlate_error drop_action;
>>
>> if (!strcasecmp(s, "drop")) {
>> + drop_action = XLATE_OK;
>> + nl_msg_put_unspec(actions, OVS_ACTION_ATTR_DROP,
>> + &drop_action, sizeof drop_action);
>> return 0;
>> }
>> -
>
> Please, keep this empty line.
>
>> old_size = actions->size;
>> for (;;) {
>> int retval;
>> diff --git a/ofproto/ofproto-dpif-ipfix.c
>> b/ofproto/ofproto-dpif-ipfix.c index 4029806..1d23a5a 100644
>> --- a/ofproto/ofproto-dpif-ipfix.c
>> +++ b/ofproto/ofproto-dpif-ipfix.c
>> @@ -3015,6 +3015,7 @@ dpif_ipfix_read_actions(const struct flow *flow,
>> case OVS_ACTION_ATTR_PUSH_NSH:
>> case OVS_ACTION_ATTR_POP_NSH:
>> case OVS_ACTION_ATTR_UNSPEC:
>> + case OVS_ACTION_ATTR_DROP:
>> case __OVS_ACTION_ATTR_MAX:
>> default:
>> break;
>> diff --git a/ofproto/ofproto-dpif-sflow.c
>> b/ofproto/ofproto-dpif-sflow.c index 7da3175..69ed7b8 100644
>> --- a/ofproto/ofproto-dpif-sflow.c
>> +++ b/ofproto/ofproto-dpif-sflow.c
>> @@ -1222,6 +1222,7 @@ dpif_sflow_read_actions(const struct flow *flow,
>> case OVS_ACTION_ATTR_PUSH_NSH:
>> case OVS_ACTION_ATTR_POP_NSH:
>> case OVS_ACTION_ATTR_UNSPEC:
>> + case OVS_ACTION_ATTR_DROP:
>> case __OVS_ACTION_ATTR_MAX:
>> default:
>> break;
>> diff --git a/ofproto/ofproto-dpif-xlate.c
>> b/ofproto/ofproto-dpif-xlate.c index 6c6df66..368f900 100644
>> --- a/ofproto/ofproto-dpif-xlate.c
>> +++ b/ofproto/ofproto-dpif-xlate.c
>> @@ -444,10 +444,16 @@ const char *xlate_strerror(enum xlate_error error)
>> return "Invalid tunnel metadata";
>> case XLATE_UNSUPPORTED_PACKET_TYPE:
>> return "Unsupported packet type";
>> + case XLATE_CONGESTION_DROP:
>> + return "CONGESTION DROP";
>
> I guess, this shouldn't be in uppercase.
>
>> + case XLATE_FORWARDING_DISABLED:
>> + return "Forwarding is disabled";
>> +
>
> Empty line not needed.
>
>> }
>> return "Unknown error";
>> }
>>
>> +
>
> Here too.
>
>> static void xlate_action_set(struct xlate_ctx *ctx); static void
>> xlate_commit_actions(struct xlate_ctx *ctx);
>>
>> @@ -5928,6 +5934,14 @@ put_ct_label(const struct flow *flow, struct
>> ofpbuf *odp_actions, }
>>
>> static void
>> +put_drop_action(struct ofpbuf *odp_actions, enum xlate_error error) {
>> + nl_msg_put_unspec(odp_actions, OVS_ACTION_ATTR_DROP,
>> + &error, sizeof error);
>> +
>> +}
>> +
>> +static void
>> put_ct_helper(struct xlate_ctx *ctx,
>> struct ofpbuf *odp_actions, struct ofpact_conntrack *ofc) { @@ -7390,6 +7404,10 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
>> }
>> size_t sample_actions_len = ctx.odp_actions->size;
>>
>> + if (!tnl_process_ecn(flow)) {
>
> This function is complex and has side effect. We shouldn't call it twice.
>
>> + ctx.error = XLATE_CONGESTION_DROP;
>> + }
>> +
>> if (tnl_process_ecn(flow)
>> && (!in_port || may_receive(in_port, &ctx))) {
>> const struct ofpact *ofpacts; @@ -7422,6 +7440,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
>> ctx.odp_actions->size = sample_actions_len;
>> ctx_cancel_freeze(&ctx);
>> ofpbuf_clear(&ctx.action_set);
>> + ctx.error = XLATE_FORWARDING_DISABLED;
>> }
>>
>> if (!ctx.freezing) {
>> @@ -7529,6 +7548,18 @@ exit:
>> ofpbuf_clear(xin->odp_actions);
>> }
>> }
>> +
>> + /*
>> + * If we are going to install "drop" action, check whether
>> + * datapath supports explicit "drop"action. If datapath
>> + * supports explicit "drop"action then install the "drop"
>> + * action containing the drop reason.
>> + */
>> + if (xin->odp_actions && !xin->odp_actions->size &&
>> + ovs_explicit_drop_action_supported(ctx.xbridge->ofproto)) {
>> + put_drop_action(xin->odp_actions, ctx.error);
>> + }
>> +
>> return ctx.error;
>> }
>>
>> diff --git a/ofproto/ofproto-dpif-xlate.h
>> b/ofproto/ofproto-dpif-xlate.h index 0a5a528..313dfb4 100644
>> --- a/ofproto/ofproto-dpif-xlate.h
>> +++ b/ofproto/ofproto-dpif-xlate.h
>> @@ -216,12 +216,16 @@ enum xlate_error {
>> XLATE_TOO_MANY_MPLS_LABELS,
>> XLATE_INVALID_TUNNEL_METADATA,
>> XLATE_UNSUPPORTED_PACKET_TYPE,
>> + XLATE_CONGESTION_DROP,
>> + XLATE_FORWARDING_DISABLED,
>> + XLATE_MAX,
>> };
>>
>> const char *xlate_strerror(enum xlate_error error);
>>
>> enum xlate_error xlate_actions(struct xlate_in *, struct xlate_out
>> *);
>>
>> +
>
> Not needed.
>
>> void xlate_in_init(struct xlate_in *, struct ofproto_dpif *, ovs_version_t,
>> const struct flow *, ofp_port_t in_port, struct rule_dpif *,
>> uint16_t tcp_flags, const struct dp_packet
>> *packet, diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> index 14fe6fc..aa4e6dd 100644
>> --- a/ofproto/ofproto-dpif.c
>> +++ b/ofproto/ofproto-dpif.c
>> @@ -827,6 +827,12 @@ ovs_native_tunneling_is_on(struct ofproto_dpif *ofproto)
>> && atomic_count_get(&ofproto->backer->tnl_count);
>> }
>>
>> +bool
>> +ovs_explicit_drop_action_supported(struct ofproto_dpif *ofproto) {
>> + return ofproto->backer->rt_support.explicit_drop_action;
>> +}
>> +
>> /* Tests whether 'backer''s datapath supports recirculation. Only newer
>> * datapaths support OVS_KEY_ATTR_RECIRC_ID in keys. We need to disable some
>> * features on older datapaths that don't support this feature.
>> @@ -1397,6 +1403,8 @@ check_support(struct dpif_backer *backer)
>> backer->rt_support.ct_eventmask = check_ct_eventmask(backer);
>> backer->rt_support.ct_clear = check_ct_clear(backer);
>> backer->rt_support.max_hash_alg = check_max_dp_hash_alg(backer);
>> + backer->rt_support.explicit_drop_action =
>> + dpif_supports_explicit_drop_action(backer->dpif);
>>
>> /* Flow fields. */
>> backer->rt_support.odp.ct_state = check_ct_state(backer); diff
>> --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h index
>> 1a404c8..9162ba0 100644
>> --- a/ofproto/ofproto-dpif.h
>> +++ b/ofproto/ofproto-dpif.h
>> @@ -106,6 +106,7 @@ struct rule_dpif *rule_dpif_lookup_from_table(struct ofproto_dpif *,
>> bool honor_table_miss,
>> struct xlate_cache *);
>>
>> +
>
> Not needed.
>
>> void rule_dpif_credit_stats(struct rule_dpif *,
>> const struct dpif_flow_stats *);
>>
>> @@ -192,7 +193,10 @@ struct group_dpif *group_dpif_lookup(struct ofproto_dpif *,
>> DPIF_SUPPORT_FIELD(bool, ct_clear, "Conntrack clear") \
>> \
>> /* Highest supported dp_hash algorithm. */ \
>> - DPIF_SUPPORT_FIELD(size_t, max_hash_alg, "Max dp_hash algorithm")
>> + DPIF_SUPPORT_FIELD(size_t, max_hash_alg, "Max dp_hash algorithm") \
>> + \
>> + /* True if the datapath supports explicit drop action. */ \
>> + DPIF_SUPPORT_FIELD(bool, explicit_drop_action, "Explicit Drop
>> + action")
>>
>> /* Stores the various features which the corresponding backer
>> supports. */ struct dpif_backer_support { @@ -361,4 +365,6 @@ int
>> ofproto_dpif_delete_internal_flow(struct ofproto_dpif *, struct match
>> *,
>>
>> bool ovs_native_tunneling_is_on(struct ofproto_dpif *);
>>
>> +bool ovs_explicit_drop_action_supported(struct ofproto_dpif *);
>> +
>> #endif /* ofproto-dpif.h */
>> diff --git a/tests/automake.mk b/tests/automake.mk index
>> 92d56b2..a4da75e 100644
>> --- a/tests/automake.mk
>> +++ b/tests/automake.mk
>> @@ -108,7 +108,8 @@ TESTSUITE_AT = \
>> tests/ovn-controller-vtep.at \
>> tests/mcast-snooping.at \
>> tests/packet-type-aware.at \
>> - tests/nsh.at
>> + tests/nsh.at \
>> + tests/drop-stats.at
>>
>> EXTRA_DIST += $(FUZZ_REGRESSION_TESTS) FUZZ_REGRESSION_TESTS = \
>> diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at index
>> 6915d43..29f7b25 100644
>> --- a/tests/dpif-netdev.at
>> +++ b/tests/dpif-netdev.at
>> @@ -281,6 +281,7 @@ type=drop rate=1 burst_size=2
>> ])
>>
>> ovs-appctl time/warp 5000
>> +sleep 1
>
> This sleep is not needed as you have another sleep below.
>
>> AT_CHECK([ovs-appctl netdev-dummy/receive p7 'in_port(7),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)' --len 60]) AT_CHECK([ovs-appctl netdev-dummy/receive p7 'in_port(7),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)' --len 60]) AT_CHECK([ovs-appctl netdev-dummy/receive p7 'in_port(7),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)' --len 60]) @@ -337,6 +338,15 @@ meter:2 flow_count:1 packet_in_count:10 byte_in_count:600 duration:0.0s bands:
>> 0: packet_count:5 byte_count:300
>> ])
>>
>> +ovs-appctl time/warp 5000
>> +sleep 1
>> +
>> +AT_CHECK([
>> +ovs-appctl coverage/show | grep "datapath_drop_meter" | cut -d':' -f2|sed 's/ //'
>> +], [0], [dnl
>> +14
>> +])
>> +
>> AT_CHECK([cat ovs-vswitchd.log | filter_flow_install |
>> strip_xout_keep_actions], [0], [dnl
>> recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(f
>> rag=no), actions:meter(0),7
>> recirc_id(0),in_port(2),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(f
>> rag=no), actions:8 diff --git a/tests/ofproto-dpif.at
>> b/tests/ofproto-dpif.at index ded2ef0..685a9c0 100644
>> --- a/tests/ofproto-dpif.at
>> +++ b/tests/ofproto-dpif.at
>> @@ -3601,10 +3601,8 @@ do
>>
>> echo "----------------------------------------------------------------------"
>> echo "in_port=$in_port vlan=$vlan pcp=$pcp"
>> -
>> AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow"], [0], [stdout])
>> actual=`tail -1 stdout | sed 's/Datapath actions: //'`
>> -
>
> This change not needed.
>
>> AT_CHECK([ovs-dpctl normalize-actions "$flow" "$expected"], [0], [stdout])
>> mv stdout expout
>> AT_CHECK([ovs-dpctl normalize-actions "$flow" "$actual"], [0],
>> [expout]) @@ -9384,7 +9382,7 @@
>> recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x8100),vlan(v
>> id=99,pcp=
>> # are wildcarded.
>> AT_CHECK([grep '\(modify\)\|\(flow_add\)' ovs-vswitchd.log |
>> strip_ufid ], [0], [dnl
>> dpif_netdev|DBG|flow_add:
>> recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x1234),
>> actions:100
>> -dpif|DBG|dummy at ovs-dummy: put[[modify]]
>> -dpif|DBG|skb_priority(0/0),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),c
>> -dpif|DBG|t_
>> -dpif|DBG|mark(0/0),ct_label(0/0),recirc_id(0),dp_hash(0/0),in_port(1)
>> -dpif|DBG|,p
>> -dpif|DBG|acket_type(ns=0,id=0),eth(src=50:54:00:00:00:09/00:00:00:00:
>> -dpif|DBG|00
>> -dpif|DBG|:00,dst=50:54:00:00:00:0a/00:00:00:00:00:00),eth_type(0x1234
>> -dpif|DBG|)
>> +dpif|DBG|dummy at ovs-dummy: put[[modify]]
>> +dpif|DBG|skb_priority(0/0),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),c
>> +dpif|DBG|t_
>> +dpif|DBG|mark(0/0),ct_label(0/0),recirc_id(0),dp_hash(0/0),in_port(1)
>> +dpif|DBG|,p
>> +dpif|DBG|acket_type(ns=0,id=0),eth(src=50:54:00:00:00:09/00:00:00:00:
>> +dpif|DBG|00
>> +dpif|DBG|:00,dst=50:54:00:00:00:0a/00:00:00:00:00:00),eth_type(0x1234
>> +dpif|DBG|),
>> +dpif|DBG|actions:drop
>> dpif|DBG|dummy at ovs-dummy: put[[modify]]
>> skb_priority(0/0),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0
>> ),ct_label(0/0),recirc_id(0),dp_hash(0/0),in_port(1),packet_type(ns=0,
>> id=0),eth(src=50:54:00:00:00:09/00:00:00:00:00:00,dst=50:54:00:00:00:0
>> a/00:00:00:00:00:00),eth_type(0x1234), actions:100
>> dpif_netdev|DBG|flow_add:
>> recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x8100),vlan(v
>> id=99,pcp=7/0x0),encap(eth_type(0x1234)), actions:drop
>> ])
>> diff --git a/tests/testsuite.at b/tests/testsuite.at index
>> b840dbf..922ba48 100644
>> --- a/tests/testsuite.at
>> +++ b/tests/testsuite.at
>> @@ -82,3 +82,4 @@ m4_include([tests/ovn-controller-vtep.at])
>> m4_include([tests/mcast-snooping.at])
>> m4_include([tests/packet-type-aware.at])
>> m4_include([tests/nsh.at])
>> +m4_include([tests/drop-stats.at])
>
> Where is this file ? It's not included to the patch.
>
>> diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at index
>> f717243..949c50a 100644
>> --- a/tests/tunnel-push-pop.at
>> +++ b/tests/tunnel-push-pop.at
>> @@ -447,6 +447,29 @@ AT_CHECK([ovs-ofctl dump-ports int-br | grep 'port 7'], [0], [dnl
>> port 7: rx pkts=3, bytes=252, drop=?, errs=?, frame=?, over=?, crc=?
>> ])
>>
>> +AT_CHECK([ovs-appctl netdev-dummy/receive p0
>> +'aa55aa550000001b213cab6408004500007079464000402fba600101025c01010258
>> +20
>> +000800000001c845000054ba200000400184861e0000011e00000200004227e754000
>> +30
>> +af3195500000000f265010000000000101112131415161718191a1b1c1d1e1f202122
>> +23
>> +2425262728292a2b2c2d2e2f3031323334353637'])
>> +
>> +ovs-appctl time/warp 1200
>> +
>> +
>> +AT_CHECK([
>> +ovs-appctl coverage/show | grep "datapath_drop_tunnel_pop_error" | cut -d':' -f2|sed 's/ //'
>> +], [0], [dnl
>> +1
>> +])
>> +
>> +sleep 1
>> +AT_CHECK([ovs-appctl netdev-dummy/receive p0
>> +'aa55aa550000001b213cab6408004503007079464000402fba600101025c01010258
>> +20
>> +000800000001c845000054ba200000400184861e0000011e00000200004227e754000
>> +30
>> +af3195500000000f265010000000000101112131415161718191a1b1c1d1e1f202122
>> +23
>> +2425262728292a2b2c2d2e2f3031323334353637'])
>> +
>> +ovs-appctl time/warp 1200
>> +ovs-appctl time/warp 1200
>
> Something strange with time warps in test files.
> COVERAGE_RUN_INTERVAL interval is 5 seconds. You need to wrap at least for
> 5 seconds to get counters. Also some sleep could be required for threads to clear the values.
>
>> +
>> +AT_CHECK([
>> +ovs-appctl coverage/show | grep "drop_action_congestion" | cut -d':' -f2|sed 's/ //'
>> +], [0], [dnl
>> +1
>> +])
>> +
>> dnl Check GREL3 only accepts non-fragmented packets?
>> AT_CHECK([ovs-appctl netdev-dummy/receive p0
>> 'aa55aa550000001b213cab6408004500007e79464000402fba550101025c010102582
>> 0000800000001c8fe71d883724fbeb6f4e1494a080045000054ba200000400184861e0
>> 000011e00000200004227e75400030af3195500000000f265010000000000101112131
>> 415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f303132333435363
>> 7'])
>>
>> @@ -455,7 +478,7 @@ ovs-appctl time/warp 1000
>>
>> AT_CHECK([ovs-ofctl dump-ports int-br | grep 'port [[37]]' | sort], [0], [dnl
>> port 3: rx pkts=3, bytes=294, drop=?, errs=?, frame=?, over=?, crc=?
>> - port 7: rx pkts=4, bytes=350, drop=?, errs=?, frame=?, over=?, crc=?
>> + port 7: rx pkts=5, bytes=434, drop=?, errs=?, frame=?, over=?, crc=?
>> ])
>>
>> dnl Check decapsulation of Geneve packet with options @@ -510,7 +533,8 @@ AT_CHECK([ovs-appctl tnl/ports/show |sort], [0], [dnl Listening ports:
>> ])
>>
>> -OVS_VSWITCHD_STOP
>> +OVS_VSWITCHD_STOP(["/dropping tunnel packet marked ECN CE but is not
>> +ECN capable/d /ip packet has invalid checksum/d"])
>> AT_CLEANUP
>>
>> AT_SETUP([tunnel_push_pop - packet_out]) diff --git a/tests/tunnel.at
>> b/tests/tunnel.at index 55fb1d3..8bb87e5 100644
>> --- a/tests/tunnel.at
>> +++ b/tests/tunnel.at
>> @@ -102,10 +102,12 @@ Datapath actions: set(ipv4(tos=0x3/0x3)),2
>>
>> dnl Tunnel CE and encapsulated packet Non-ECT AT_CHECK([ovs-appctl
>> ofproto/trace ovs-dummy
>> 'tunnel(src=1.1.1.1,dst=2.2.2.2,tos=0x3,ttl=64,flags()),in_port(1),eth
>> (src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(sr
>> c=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no),tcp(src=8,
>> dst=9)'], [0], [stdout]) -AT_CHECK([tail -2 stdout], [0],
>> +AT_CHECK([tail -3 stdout], [0],
>> [Megaflow:
>> recirc_id=0,eth,ip,tun_id=0,tun_src=1.1.1.1,tun_dst=2.2.2.2,tun_tos=3,
>> tun_flags=-df-csum-key,in_port=1,nw_ecn=0,nw_frag=no
>> Datapath actions: drop
>> +Translation failed (CONGESTION DROP), packet is dropped.
>> ])
>> +
>> OVS_VSWITCHD_STOP(["/dropping tunnel packet marked ECN CE but is not
>> ECN capable/d"]) AT_CLEANUP
>>
>> @@ -193,6 +195,16 @@ AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(2),eth(src=50:54:00:00:00:
>> AT_CHECK([tail -1 stdout], [0],
>> [Datapath actions:
>> set(tunnel(tun_id=0x5,src=2.2.2.2,dst=1.1.1.1,ttl=64,flags(df|key))),s
>> et(skb_mark(0x2)),1
>> ])
>> +
>> +AT_CHECK([ovs-appctl netdev-dummy/receive p2
>> +'aa55aa550001f8bc124434b6080045000054ba200000400184860101035801010370
>> +01
>> +004227e75400030af3195500000000f265010000000000101112131415161718191a1
>> +b1
>> +c1d1e1f202122232425262728292a2b2c2d2e2f3031323334353637'])
>> +sleep 2
>> +
>> +
>> +AT_CHECK([
>> +ovs-appctl coverage/show | grep "datapath_drop_invalid_port" | cut -d':' -f2|sed 's/ //'
>> +], [0], [dnl
>> +1
>> +])
>> OVS_VSWITCHD_STOP
>> AT_CLEANUP
>>
>> --
>> 1.9.1
>>
More information about the dev
mailing list