| Message ID | 1548791582-8424-1-git-send-email-anju.thomas@ericsson.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,PATCHv7,1/3] Improved Packet Drop Statistics in OVS | expand |
Hi Ben/Ilya, I have addressed the comments in the below patch. Can you tell me if this is fine? Regards Anju -----Original Message----- From: Anju Thomas [mailto:anju.thomas@ericsson.com] Sent: Tuesday, January 29, 2019 5:21 PM To: dev@openvswitch.org Cc: Anju Thomas <anju.thomas@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-data-plane-in-ovs-82280329 Co-authored-by: Rohith Basavaraja <rohith.basavaraja@gmail.com> Co-authored-by: Keshav Gupta <keshugupta1@gmail.com> Signed-off-by: Anju Thomas <anju.thomas@ericsson.com> Signed-off-by: Rohith Basavaraja <rohith.basavaraja@gmail.com> Signed-off-by: Keshav Gupta <keshugupta1@gmail.com> --- 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, #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" 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); } 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); + } 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); 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); + 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); + } +} + /* 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; + 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); + 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" 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; } - 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"; + case XLATE_FORWARDING_DISABLED: + return "Forwarding is disabled"; + } return "Unknown error"; } + 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)) { + 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 *); + 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 *); + 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 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(frag=no), actions:meter(0),7 recirc_id(0),in_port(2),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=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: //'` - 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(vid=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@ovs-dummy: put[[modify]] -dpif|DBG|skb_priority(0/0),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_ -dpif|DBG|mark(0/0),ct_label(0/0),recirc_id(0),dp_hash(0/0),in_port(1),p -dpif|DBG|acket_type(ns=0,id=0),eth(src=50:54:00:00:00:09/00:00:00:00:00 -dpif|DBG|:00,dst=50:54:00:00:00:0a/00:00:00:00:00:00),eth_type(0x1234) +dpif|DBG|dummy@ovs-dummy: put[[modify]] +dpif|DBG|skb_priority(0/0),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_ +dpif|DBG|mark(0/0),ct_label(0/0),recirc_id(0),dp_hash(0/0),in_port(1),p +dpif|DBG|acket_type(ns=0,id=0),eth(src=50:54:00:00:00:09/00:00:00:00:00 +dpif|DBG|:00,dst=50:54:00:00:00:0a/00:00:00:00:00:00),eth_type(0x1234), +dpif|DBG|actions:drop dpif|DBG|dummy@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:0a/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(vid=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]) 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 +'aa55aa550000001b213cab6408004500007079464000402fba600101025c0101025820 +000800000001c845000054ba200000400184861e0000011e00000200004227e75400030 +af3195500000000f265010000000000101112131415161718191a1b1c1d1e1f20212223 +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 +'aa55aa550000001b213cab6408004503007079464000402fba600101025c0101025820 +000800000001c845000054ba200000400184861e0000011e00000200004227e75400030 +af3195500000000f265010000000000101112131415161718191a1b1c1d1e1f20212223 +2425262728292a2b2c2d2e2f3031323334353637']) + +ovs-appctl time/warp 1200 +ovs-appctl time/warp 1200 + +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 'aa55aa550000001b213cab6408004500007e79464000402fba550101025c0101025820000800000001c8fe71d883724fbeb6f4e1494a080045000054ba200000400184861e0000011e00000200004227e75400030af3195500000000f265010000000000101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f3031323334353637']) @@ -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(src=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))),set(skb_mark(0x2)),1 ]) + +AT_CHECK([ovs-appctl netdev-dummy/receive p2 +'aa55aa550001f8bc124434b6080045000054ba20000040018486010103580101037001 +004227e75400030af3195500000000f265010000000000101112131415161718191a1b1 +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
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@ericsson.com] > Sent: Tuesday, January 29, 2019 5:21 PM > To: dev@openvswitch.org > Cc: Anju Thomas <anju.thomas@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-data-plane-in-ovs-82280329 > > Co-authored-by: Rohith Basavaraja <rohith.basavaraja@gmail.com> > Co-authored-by: Keshav Gupta <keshugupta1@gmail.com> > Signed-off-by: Anju Thomas <anju.thomas@ericsson.com> > Signed-off-by: Rohith Basavaraja <rohith.basavaraja@gmail.com> > Signed-off-by: Keshav Gupta <keshugupta1@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(frag=no), actions:meter(0),7 recirc_id(0),in_port(2),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=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(vid=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@ovs-dummy: put[[modify]] > -dpif|DBG|skb_priority(0/0),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_ > -dpif|DBG|mark(0/0),ct_label(0/0),recirc_id(0),dp_hash(0/0),in_port(1),p > -dpif|DBG|acket_type(ns=0,id=0),eth(src=50:54:00:00:00:09/00:00:00:00:00 > -dpif|DBG|:00,dst=50:54:00:00:00:0a/00:00:00:00:00:00),eth_type(0x1234) > +dpif|DBG|dummy@ovs-dummy: put[[modify]] > +dpif|DBG|skb_priority(0/0),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_ > +dpif|DBG|mark(0/0),ct_label(0/0),recirc_id(0),dp_hash(0/0),in_port(1),p > +dpif|DBG|acket_type(ns=0,id=0),eth(src=50:54:00:00:00:09/00:00:00:00:00 > +dpif|DBG|:00,dst=50:54:00:00:00:0a/00:00:00:00:00:00),eth_type(0x1234), > +dpif|DBG|actions:drop > dpif|DBG|dummy@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:0a/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(vid=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 > +'aa55aa550000001b213cab6408004500007079464000402fba600101025c0101025820 > +000800000001c845000054ba200000400184861e0000011e00000200004227e75400030 > +af3195500000000f265010000000000101112131415161718191a1b1c1d1e1f20212223 > +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 > +'aa55aa550000001b213cab6408004503007079464000402fba600101025c0101025820 > +000800000001c845000054ba200000400184861e0000011e00000200004227e75400030 > +af3195500000000f265010000000000101112131415161718191a1b1c1d1e1f20212223 > +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 'aa55aa550000001b213cab6408004500007e79464000402fba550101025c0101025820000800000001c8fe71d883724fbeb6f4e1494a080045000054ba200000400184861e0000011e00000200004227e75400030af3195500000000f265010000000000101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f3031323334353637']) > > @@ -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(src=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))),set(skb_mark(0x2)),1 > ]) > + > +AT_CHECK([ovs-appctl netdev-dummy/receive p2 > +'aa55aa550001f8bc124434b6080045000054ba20000040018486010103580101037001 > +004227e75400030af3195500000000f265010000000000101112131415161718191a1b1 > +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 >
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? Regards Anju -----Original Message----- From: Ilya Maximets [mailto:i.maximets@samsung.com] Sent: Wednesday, February 06, 2019 8:48 PM To: Anju Thomas <anju.thomas@ericsson.com>; Ben Pfaff <blp@ovn.org> Cc: dev@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@ericsson.com] > Sent: Tuesday, January 29, 2019 5:21 PM > To: dev@openvswitch.org > Cc: Anju Thomas <anju.thomas@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@gmail.com> > Co-authored-by: Keshav Gupta <keshugupta1@gmail.com> > Signed-off-by: Anju Thomas <anju.thomas@ericsson.com> > Signed-off-by: Rohith Basavaraja <rohith.basavaraja@gmail.com> > Signed-off-by: Keshav Gupta <keshugupta1@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@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@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@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 >
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@samsung.com] > Sent: Wednesday, February 06, 2019 8:48 PM > To: Anju Thomas <anju.thomas@ericsson.com>; Ben Pfaff <blp@ovn.org> > Cc: dev@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@ericsson.com] >> Sent: Tuesday, January 29, 2019 5:21 PM >> To: dev@openvswitch.org >> Cc: Anju Thomas <anju.thomas@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@gmail.com> >> Co-authored-by: Keshav Gupta <keshugupta1@gmail.com> >> Signed-off-by: Anju Thomas <anju.thomas@ericsson.com> >> Signed-off-by: Rohith Basavaraja <rohith.basavaraja@gmail.com> >> Signed-off-by: Keshav Gupta <keshugupta1@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@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@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@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 >>
Thanks Ilya. I have sent the updated patch. Regards Anju -----Original Message----- From: Ilya Maximets [mailto:i.maximets@samsung.com] Sent: Thursday, February 07, 2019 7:52 PM To: Anju Thomas <anju.thomas@ericsson.com>; Ben Pfaff <blp@ovn.org> Cc: dev@openvswitch.org Subject: Re: [PATCHv7 1/3] Improved Packet Drop Statistics in OVS 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@samsung.com] > Sent: Wednesday, February 06, 2019 8:48 PM > To: Anju Thomas <anju.thomas@ericsson.com>; Ben Pfaff <blp@ovn.org> > Cc: dev@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@ericsson.com] >> Sent: Tuesday, January 29, 2019 5:21 PM >> To: dev@openvswitch.org >> Cc: Anju Thomas <anju.thomas@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@gmail.com> >> Co-authored-by: Keshav Gupta <keshugupta1@gmail.com> >> Signed-off-by: Anju Thomas <anju.thomas@ericsson.com> >> Signed-off-by: Rohith Basavaraja <rohith.basavaraja@gmail.com> >> Signed-off-by: Keshav Gupta <keshugupta1@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@ovs-dummy: put[[modify]] >> -dpif|DBG|skb_priority(0/0),skb_mark(0/0),ct_state(0/0),ct_zone(0/0), >> -dpif|DBG|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|) >> -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(0x123 >> -dpif|DBG|4 >> -dpif|DBG|) >> +dpif|DBG|dummy@ovs-dummy: put[[modify]] >> +dpif|DBG|skb_priority(0/0),skb_mark(0/0),ct_state(0/0),ct_zone(0/0), >> +dpif|DBG|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|) >> +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(0x123 >> +dpif|DBG|4 >> +dpif|DBG|), >> +dpif|DBG|actions:drop >> dpif|DBG|dummy@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 >> +'aa55aa550000001b213cab6408004500007079464000402fba600101025c0101025 >> +8 >> +20 >> +000800000001c845000054ba200000400184861e0000011e00000200004227e75400 >> +0 >> +30 >> +af3195500000000f265010000000000101112131415161718191a1b1c1d1e1f20212 >> +2 >> +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 >> +'aa55aa550000001b213cab6408004503007079464000402fba600101025c0101025 >> +8 >> +20 >> +000800000001c845000054ba200000400184861e0000011e00000200004227e75400 >> +0 >> +30 >> +af3195500000000f265010000000000101112131415161718191a1b1c1d1e1f20212 >> +2 >> +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 >> 'aa55aa550000001b213cab6408004500007e79464000402fba550101025c01010258 >> 2 >> 0000800000001c8fe71d883724fbeb6f4e1494a080045000054ba200000400184861e >> 0 >> 000011e00000200004227e75400030af3195500000000f26501000000000010111213 >> 1 >> 415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f30313233343536 >> 3 >> 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),et >> h >> (src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(s >> r >> 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 >> +'aa55aa550001f8bc124434b6080045000054ba20000040018486010103580101037 >> +0 >> +01 >> +004227e75400030af3195500000000f265010000000000101112131415161718191a >> +1 >> +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 >>
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, #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" 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); } 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); + } 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); 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); + 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); + } +} + /* 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; + 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); + 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" 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; } - 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"; + case XLATE_FORWARDING_DISABLED: + return "Forwarding is disabled"; + } return "Unknown error"; } + 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)) { + 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 *); + 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 *); + 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 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(frag=no), actions:meter(0),7 recirc_id(0),in_port(2),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=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: //'` - 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(vid=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@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:0a/00:00:00:00:00:00),eth_type(0x1234) +dpif|DBG|dummy@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:0a/00:00:00:00:00:00),eth_type(0x1234), actions:drop dpif|DBG|dummy@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:0a/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(vid=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]) 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 'aa55aa550000001b213cab6408004500007079464000402fba600101025c0101025820000800000001c845000054ba200000400184861e0000011e00000200004227e75400030af3195500000000f265010000000000101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f3031323334353637']) + +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 'aa55aa550000001b213cab6408004503007079464000402fba600101025c0101025820000800000001c845000054ba200000400184861e0000011e00000200004227e75400030af3195500000000f265010000000000101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f3031323334353637']) + +ovs-appctl time/warp 1200 +ovs-appctl time/warp 1200 + +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 'aa55aa550000001b213cab6408004500007e79464000402fba550101025c0101025820000800000001c8fe71d883724fbeb6f4e1494a080045000054ba200000400184861e0000011e00000200004227e75400030af3195500000000f265010000000000101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f3031323334353637']) @@ -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(src=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))),set(skb_mark(0x2)),1 ]) + +AT_CHECK([ovs-appctl netdev-dummy/receive p2 'aa55aa550001f8bc124434b6080045000054ba20000040018486010103580101037001004227e75400030af3195500000000f265010000000000101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f3031323334353637']) +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