On 26.11.2019 20:43, Ilya Maximets wrote:
> On 26.11.2019 9:59, Anju Thomas wrote:
>> 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
>>
>> Co-authored-by: Rohith Basavaraja <rohith.basavar...@gmail.com>
>> Co-authored-by: Keshav Gupta <keshugup...@gmail.com>
>> Signed-off-by: Anju Thomas <anju.tho...@ericsson.com>
>> Signed-off-by: Rohith Basavaraja <rohith.basavar...@gmail.com>
>> Signed-off-by: Keshav Gupta <keshugup...@gmail.com>
>> Acked-by: Eelco Chaudron <echau...@redhat.com
>
> Please, limit the line length in a commit message to 72 characters.
>
>> ---
>> v16 (Ilya and Eelco comments)
>> 1. remove old declaration of xlate_error in v15
>> 2. kernel file formatting in openvswitch.h for xlate_error
>> 3. remove drop_action from odp_actions_from_string
>>
>> v15(Ilya's comments)
>> 1. Description in openvswitch.h
>> 2. Move xlate_error to openvswitch.h to not include ofproto heeader in dp
>> 3. Return u32 for instead of size of enum
>> 4. Formatting
>> 5. Add drop-stats in testsuite.at
>> 6. Use coverage-read-counter
>> 7. Use flow instead of raw packet data wherever possible
>> 8. Change sleep to warp
>>
>> v14 (Eelco's comments)
>> 1. Remove definition of dpif_show_drop_stats_support
>> 2. Remove extra check for drop reason in odp_execute_actions
>>
>> v13(Eelco and Illya's comments)
>> 1. packet_dropped changed to packets_dropped
>> 2. Uses dp_packet_batch_size instead of packet->size
>> 3. Add version history
>>
>> v12(Ben's comments)
>> 1. new dp action in kernel block in openvswitch.h
>> 2. change xlate_error enum to be used as u32 3. resetting xlate error in
>> case of congestion drop
>> and forwarding disabled
>> ---
>> datapath/linux/compat/include/linux/openvswitch.h | 18 ++
>> lib/dpif-netdev.c | 47 +++++-
>> lib/dpif.c | 7 +
>> lib/dpif.h | 2 +
>> lib/odp-execute.c | 77 +++++++++
>> lib/odp-util.c | 5 +
>> ofproto/ofproto-dpif-ipfix.c | 1 +
>> ofproto/ofproto-dpif-sflow.c | 1 +
>> ofproto/ofproto-dpif-xlate.c | 38 ++++-
>> ofproto/ofproto-dpif-xlate.h | 13 --
>> ofproto/ofproto-dpif.c | 8 +
>> ofproto/ofproto-dpif.h | 8 +-
>> tests/automake.mk | 3 +-
>> tests/dpif-netdev.at | 8 +
>> tests/drop-stats.at | 190
>> ++++++++++++++++++++++
>> tests/ofproto-dpif.at | 2 +-
>> tests/testsuite.at | 1 +
>> tests/tunnel-push-pop.at | 28 +++-
>> tests/tunnel.at | 16 +-
>> 19 files changed, 449 insertions(+), 24 deletions(-)
>> create mode 100644 tests/drop-stats.at
>>
>> diff --git a/datapath/linux/compat/include/linux/openvswitch.h
>> b/datapath/linux/compat/include/linux/openvswitch.h
>> index 778827f..e974d2c 100644
>> --- a/datapath/linux/compat/include/linux/openvswitch.h
>> +++ b/datapath/linux/compat/include/linux/openvswitch.h
>> @@ -404,6 +404,22 @@ enum ovs_tunnel_key_attr {
>> __OVS_TUNNEL_KEY_ATTR_MAX
>> };
>>
>
> 1. Brief explanation of the purpose of this enum is required.
> Look at other enums as a reference. You may avoid describing
> enum members though.
>
> 2. This should be under '#ifndef __KERNEL__'.
>
> 3. Please move the declaration after 'OVS_TUNNEL_KEY_ATTR_MAX'.
> Don't split it from 'enum ovs_tunnel_key_attr'.
>
>> +enum xlate_error {
>> + XLATE_OK = 0,
>> + XLATE_BRIDGE_NOT_FOUND,
>> + XLATE_RECURSION_TOO_DEEP,
>> + XLATE_TOO_MANY_RESUBMITS,
>> + XLATE_STACK_TOO_DEEP,
>> + XLATE_NO_RECIRCULATION_CONTEXT,
>> + XLATE_RECIRCULATION_CONFLICT,
>> + XLATE_TOO_MANY_MPLS_LABELS,
>> + XLATE_INVALID_TUNNEL_METADATA,
>> + XLATE_UNSUPPORTED_PACKET_TYPE,
>> + XLATE_CONGESTION_DROP,
>> + XLATE_FORWARDING_DISABLED,
>> + XLATE_MAX,
>> +};
>> +
>> #define OVS_TUNNEL_KEY_ATTR_MAX (__OVS_TUNNEL_KEY_ATTR_MAX - 1)
>>
>> /**
>> @@ -962,6 +978,7 @@ struct check_pkt_len_arg {
>> * @OVS_ACTION_ATTR_CHECK_PKT_LEN: Check the packet length and execute a set
>> * of actions if greater than the specified packet length, else execute
>> * another set of actions.
>> + * @OVS_ACTION_ATTR_DROP: Explicit drop action.
>> */
>>
>> enum ovs_action_attr {
>> @@ -994,6 +1011,7 @@ enum ovs_action_attr {
>> #ifndef __KERNEL__
>> OVS_ACTION_ATTR_TUNNEL_PUSH, /* struct ovs_action_push_tnl*/
>> OVS_ACTION_ATTR_TUNNEL_POP, /* u32 port number. */
>> + OVS_ACTION_ATTR_DROP, /* explicit drop action. */
>
> Comment is not correct. It should describe the type of the argument.
> Say that it's u32 and point to the enum.
>
>> #endif
>> __OVS_ACTION_ATTR_MAX, /* Nothing past this will be accepted
>> * from userspace. */
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index 5142bad..8adeac5 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -102,6 +102,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;
>>
>> @@ -5753,7 +5764,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. */
>> @@ -6503,6 +6514,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;
>> }
>>
>> @@ -6623,6 +6635,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;
>> }
>>
>> @@ -6753,6 +6766,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++;
>> }
>> }
>> @@ -7022,6 +7036,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);
>> }
>> }
>>
>> @@ -7036,6 +7051,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, packets_dropped;
>>
>> switch ((enum ovs_action_attr)type) {
>> case OVS_ACTION_ATTR_OUTPUT:
>> @@ -7077,6 +7093,9 @@ 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,
>> + dp_packet_batch_size(packets_));
>> }
>> break;
>>
>> @@ -7086,10 +7105,16 @@ 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,
>> + dp_packet_batch_size(packets_));
>> break;
>> }
>> dp_packet_batch_apply_cutlen(packets_);
>> - push_tnl_action(pmd, a, packets_);
>> + packet_count = dp_packet_batch_size(packets_);
>> + if (push_tnl_action(pmd, a, packets_)) {
>> + COVERAGE_ADD(datapath_drop_tunnel_push_error,
>> + packet_count);
>> + }
>> return;
>>
>> case OVS_ACTION_ATTR_TUNNEL_POP:
>> @@ -7109,7 +7134,14 @@ dp_execute_cb(void *aux_, struct dp_packet_batch
>> *packets_,
>>
>> dp_packet_batch_apply_cutlen(packets_);
>>
>> + packet_count = dp_packet_batch_size(packets_);
>> netdev_pop_header(p->port->netdev, packets_);
>> + packets_dropped =
>> + packet_count - dp_packet_batch_size(packets_);
>> + if (packets_dropped) {
>> + COVERAGE_ADD(datapath_drop_tunnel_pop_error,
>> + packets_dropped);
>> + }
>> if (dp_packet_batch_is_empty(packets_)) {
>> return;
>> }
>> @@ -7124,6 +7156,11 @@ dp_execute_cb(void *aux_, struct dp_packet_batch
>> *packets_,
>> (*depth)--;
>> return;
>> }
>> + COVERAGE_ADD(datapath_drop_invalid_tnl_port,
>> + dp_packet_batch_size(packets_));
>> + } else {
>> + COVERAGE_ADD(datapath_drop_recirc_error,
>> + dp_packet_batch_size(packets_));
>> }
>> break;
>>
>> @@ -7168,6 +7205,9 @@ dp_execute_cb(void *aux_, struct dp_packet_batch
>> *packets_,
>>
>> return;
>> }
>> + COVERAGE_ADD(datapath_drop_lock_error,
>> + dp_packet_batch_size(packets_));
>> +
>> break;
>>
>> case OVS_ACTION_ATTR_RECIRC:
>> @@ -7191,6 +7231,8 @@ dp_execute_cb(void *aux_, struct dp_packet_batch
>> *packets_,
>> return;
>> }
>>
>> + COVERAGE_ADD(datapath_drop_recirc_error,
>> + dp_packet_batch_size(packets_));
>> VLOG_WARN("Packet dropped. Max recirculation depth exceeded.");
>> break;
>>
>> @@ -7348,6 +7390,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch
>> *packets_,
>> case OVS_ACTION_ATTR_POP_NSH:
>> case OVS_ACTION_ATTR_CT_CLEAR:
>> case OVS_ACTION_ATTR_CHECK_PKT_LEN:
>> + case OVS_ACTION_ATTR_DROP:
>> case __OVS_ACTION_ATTR_MAX:
>> OVS_NOT_REACHED();
>> }
>> diff --git a/lib/dpif.c b/lib/dpif.c
>> index c88b210..5a9ff46 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_CT_CLEAR:
>> case OVS_ACTION_ATTR_UNSPEC:
>> case OVS_ACTION_ATTR_CHECK_PKT_LEN:
>> + 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 c98513e..7cc2220 100644
>> --- a/lib/dpif.h
>> +++ b/lib/dpif.h
>> @@ -892,6 +892,8 @@ 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 *);
>> +
>>
>> /* Log functions. */
>> struct vlog_module;
>> diff --git a/lib/odp-execute.c b/lib/odp-execute.c
>> index 563ad1d..3d4ad9e 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,72 @@
>> #include "util.h"
>> #include "csum.h"
>> #include "conntrack.h"
>> +#include "openvswitch/vlog.h"
>> +
>> +VLOG_DEFINE_THIS_MODULE(odp_execute);
>> +COVERAGE_DEFINE(datapath_drop_sample_error);
>> +COVERAGE_DEFINE(datapath_drop_nsh_decap_error);
>> +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
>> @@ -621,6 +688,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_INC(datapath_drop_sample_error);
>> dp_packet_delete(packet);
>> }
>> return;
>> @@ -749,6 +817,7 @@ requires_datapath_assistance(const struct nlattr *a)
>> case OVS_ACTION_ATTR_POP_NSH:
>> case OVS_ACTION_ATTR_CT_CLEAR:
>> case OVS_ACTION_ATTR_CHECK_PKT_LEN:
>> + case OVS_ACTION_ATTR_DROP:
>> return false;
>>
>> case OVS_ACTION_ATTR_UNSPEC:
>> @@ -965,6 +1034,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(datapath_drop_nsh_decap_error);
>> dp_packet_delete(packet);
>> }
>> }
>> @@ -989,6 +1059,13 @@ 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);
>> +
>> + 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-util.c b/lib/odp-util.c
>> index b9600b4..9bda91f 100644
>> --- a/lib/odp-util.c
>> +++ b/lib/odp-util.c
>> @@ -141,6 +141,7 @@ odp_action_len(uint16_t type)
>> case OVS_ACTION_ATTR_PUSH_NSH: return ATTR_LEN_VARIABLE;
>> case OVS_ACTION_ATTR_POP_NSH: return 0;
>> case OVS_ACTION_ATTR_CHECK_PKT_LEN: return ATTR_LEN_VARIABLE;
>> + case OVS_ACTION_ATTR_DROP: return sizeof(uint32_t);
>>
>> case OVS_ACTION_ATTR_UNSPEC:
>> case __OVS_ACTION_ATTR_MAX:
>> @@ -1238,6 +1239,9 @@ format_odp_action(struct ds *ds, const struct nlattr
>> *a,
>> case OVS_ACTION_ATTR_CHECK_PKT_LEN:
>> format_odp_check_pkt_len_action(ds, a, portno_names);
>> break;
>> + case OVS_ACTION_ATTR_DROP:
>> + ds_put_cstr(ds, "drop");
>> + break;
>> case OVS_ACTION_ATTR_UNSPEC:
>> case __OVS_ACTION_ATTR_MAX:
>> default:
>> @@ -2575,6 +2579,7 @@ odp_actions_from_string(const char *s, const struct
>> simap *port_names,
>> size_t old_size;
>>
>> if (!strcasecmp(s, "drop")) {
>> + nl_msg_put_u32(actions, OVS_ACTION_ATTR_DROP, XLATE_OK);
>> return 0;
>> }
>>
>> diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
>> index b8bd1b8..b413768 100644
>> --- a/ofproto/ofproto-dpif-ipfix.c
>> +++ b/ofproto/ofproto-dpif-ipfix.c
>> @@ -3016,6 +3016,7 @@ dpif_ipfix_read_actions(const struct flow *flow,
>> case OVS_ACTION_ATTR_POP_NSH:
>> case OVS_ACTION_ATTR_CHECK_PKT_LEN:
>> 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 9abaab6..f9ea47a 100644
>> --- a/ofproto/ofproto-dpif-sflow.c
>> +++ b/ofproto/ofproto-dpif-sflow.c
>> @@ -1224,6 +1224,7 @@ dpif_sflow_read_actions(const struct flow *flow,
>> case OVS_ACTION_ATTR_POP_NSH:
>> case OVS_ACTION_ATTR_UNSPEC:
>> case OVS_ACTION_ATTR_CHECK_PKT_LEN:
>> + 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 cebae7a..18dfebe 100644
>> --- a/ofproto/ofproto-dpif-xlate.c
>> +++ b/ofproto/ofproto-dpif-xlate.c
>> @@ -445,6 +445,12 @@ 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";
>> + case XLATE_MAX:
>> + break;
>> }
>> return "Unknown error";
>> }
>> @@ -6002,6 +6008,12 @@ 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_u32(odp_actions, OVS_ACTION_ATTR_DROP, error);
>> +}
>> +
>> +static void
>> put_ct_helper(struct xlate_ctx *ctx,
>> struct ofpbuf *odp_actions, struct ofpact_conntrack *ofc)
>> {
>> @@ -7638,8 +7650,9 @@ xlate_actions(struct xlate_in *xin, struct xlate_out
>> *xout)
>> compose_ipfix_action(&ctx, ODPP_NONE);
>> }
>> size_t sample_actions_len = ctx.odp_actions->size;
>> + bool ecn_drop = !tnl_process_ecn(flow);
>>
>> - if (tnl_process_ecn(flow)
>> + if (!ecn_drop
>> && (!in_port || may_receive(in_port, &ctx))) {
>> const struct ofpact *ofpacts;
>> size_t ofpacts_len;
>> @@ -7671,6 +7684,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) {
>> @@ -7679,6 +7693,8 @@ xlate_actions(struct xlate_in *xin, struct xlate_out
>> *xout)
>> if (ctx.freezing) {
>> finish_freezing(&ctx);
>> }
>> + } else if (ecn_drop) {
>> + ctx.error = XLATE_CONGESTION_DROP;
>> }
>>
>> /* Output only fully processed packets. */
>> @@ -7784,6 +7800,26 @@ 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.
>
> Some spaces are missing above.
> Probably, better to rephrase:
>
> /* Install explicit drop action containing the drop reason if
> * datapath supports it. */
>
>> + */
>> + if (xin->odp_actions && !xin->odp_actions->size &&
>> + ovs_explicit_drop_action_supported(ctx.xbridge->ofproto)) {
>> + put_drop_action(xin->odp_actions, ctx.error);
>> + }
>> +
>> + /* Since congestion drop and forwarding drop are not exactly
>> + * translation error, we are resetting the translation error.
>> + */
>> + if (ctx.error == XLATE_CONGESTION_DROP ||
>> + ctx.error == XLATE_FORWARDING_DISABLED) {
>> + ctx.error = XLATE_OK;
>> + }
>> +
>> return ctx.error;
>> }
>>
>> diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
>> index f97c7c0..3426a27 100644
>> --- a/ofproto/ofproto-dpif-xlate.h
>> +++ b/ofproto/ofproto-dpif-xlate.h
>> @@ -206,19 +206,6 @@ int xlate_lookup(const struct dpif_backer *, const
>> struct flow *,
>> struct dpif_sflow **, struct netflow **,
>> ofp_port_t *ofp_in_port);
>>
>> -enum xlate_error {
>> - XLATE_OK = 0,
>> - XLATE_BRIDGE_NOT_FOUND,
>> - XLATE_RECURSION_TOO_DEEP,
>> - XLATE_TOO_MANY_RESUBMITS,
>> - XLATE_STACK_TOO_DEEP,
>> - XLATE_NO_RECIRCULATION_CONTEXT,
>> - XLATE_RECIRCULATION_CONFLICT,
>> - XLATE_TOO_MANY_MPLS_LABELS,
>> - XLATE_INVALID_TUNNEL_METADATA,
>> - XLATE_UNSUPPORTED_PACKET_TYPE,
>> -};
>> -
>> const char *xlate_strerror(enum xlate_error error);
>>
>> enum xlate_error xlate_actions(struct xlate_in *, struct xlate_out *);
>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> index 2cd786a..c948aba 100644
>> --- a/ofproto/ofproto-dpif.c
>> +++ b/ofproto/ofproto-dpif.c
>> @@ -860,6 +860,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.
>> @@ -1584,6 +1590,8 @@ check_support(struct dpif_backer *backer)
>> backer->rt_support.max_hash_alg = check_max_dp_hash_alg(backer);
>> backer->rt_support.check_pkt_len = check_check_pkt_len(backer);
>> backer->rt_support.ct_timeout = check_ct_timeout_policy(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 cd45363..bcaacef 100644
>> --- a/ofproto/ofproto-dpif.h
>> +++ b/ofproto/ofproto-dpif.h
>> @@ -199,7 +199,11 @@ struct group_dpif *group_dpif_lookup(struct
>> ofproto_dpif *,
>>
>> \
>> /* True if the datapath supports OVS_CT_ATTR_TIMEOUT in
>> \
>> * OVS_ACTION_ATTR_CT action. */
>> \
>> - DPIF_SUPPORT_FIELD(bool, ct_timeout, "Conntrack timeout policy")
>> + DPIF_SUPPORT_FIELD(bool, ct_timeout, "Conntrack timeout policy")
>> \
>> +
>> \
>> + /* True if the datapath supports explicit drop action. */
>> \
>> + DPIF_SUPPORT_FIELD(bool, explicit_drop_action, "Explicit Drop action")
>
> There is a new functionality to report datapath supported features.
> You need to add this new field there. See get_datapath_cap() callback.
This also needs to be documented in vswitchd/vswitch.xml.
See commit 27501802d09f ("ofproto-dpif: Expose datapath capability to ovsdb.")
for details.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
