[ovs-dev,v17] 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

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>
Acked-by: Eelco Chaudron <echaudro@redhat.com
---
v17 (Ilya's comments)
 1. comments for xlate_error
 2. define xlate_error in ifndef __KERNEL__
 3. move declaration of xlate_error after OVS_TUNNEL_KEY_ATTR_MAX
 4. comment change for DROP action
 5. Add in datapath capability

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 |  24 +++
 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                      |  33 +++-
 ofproto/ofproto-dpif-xlate.h                      |  13 --
 ofproto/ofproto-dpif.c                            |  10 ++
 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 +-
 vswitchd/vswitch.xml                              |   5 +
 20 files changed, 457 insertions(+), 24 deletions(-)
 create mode 100644 tests/drop-stats.at

On 06.12.2019 09:49, 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.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>
> Acked-by: Eelco Chaudron <echaudro@redhat.com
> ---

Thanks for a new version.  I really hope that v18 will be the last one.
This patch needs some minor rebase on top of current master.
6 more comments inline (some of them are really minor, but I decided to
point to them as you'll re-spin the patch anyway).  Please, fix them and
send v18.

Best regards, Ilya Maximets.

> v17 (Ilya's comments)
>  1. comments for xlate_error
>  2. define xlate_error in ifndef __KERNEL__
>  3. move declaration of xlate_error after OVS_TUNNEL_KEY_ATTR_MAX
>  4. comment change for DROP action
>  5. Add in datapath capability
> 
> 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 |  24 +++
>  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                      |  33 +++-
>  ofproto/ofproto-dpif-xlate.h                      |  13 --
>  ofproto/ofproto-dpif.c                            |  10 ++
>  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 +-
>  vswitchd/vswitch.xml                              |   5 +
>  20 files changed, 457 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..8006724 100644
> --- a/datapath/linux/compat/include/linux/openvswitch.h
> +++ b/datapath/linux/compat/include/linux/openvswitch.h
> @@ -407,6 +407,28 @@ enum ovs_tunnel_key_attr {
>  #define OVS_TUNNEL_KEY_ATTR_MAX (__OVS_TUNNEL_KEY_ATTR_MAX - 1)
>  
>  /**
> + * enum xlate_error -  Different types of error during translation
> + */
> +
> +#ifndef __KERNEL__
> +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,
> +};
> +#endif


Over and over again, kernel coding style, please.


> +
> +/**
>   * enum ovs_frag_type - IPv4 and IPv6 fragment type
>   * @OVS_FRAG_TYPE_NONE: Packet is not a fragment.
>   * @OVS_FRAG_TYPE_FIRST: Packet is a fragment with offset 0.
> @@ -962,6 +984,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 +1017,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,          /* u32 xlate_error. */
>  #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_));

This is not a tunnel push error.  We literally executed all the actions
without errors and that is not our fault that there are no more actions
after tnl_push.  We're just saving some time avoiding actual execution
of a pointless action.  So, this should not be accounted as error, and
definitely not as a tunnel push error.

>              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_));
> +

Empty line not needed here.

>          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 *);
> +

New empty line is not needed here.

>  
>  /* 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);

Please, add a single space after ':'.

> +   }
> +}
>  
>  /* 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_batch_size(batch)

> +            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..222c954 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,21 @@ exit:
>              ofpbuf_clear(xin->odp_actions);
>          }
>      }
> +
> +    /* Install drop action if datapath supports explicit drop action. */
> +    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..7d580dc 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);
> @@ -5550,6 +5558,8 @@ get_datapath_cap(const char *datapath_type, struct smap *cap)
>  
>      smap_add(cap, "check_pkt_len", s.check_pkt_len ? "true" : "false");
>      smap_add(cap, "ct_timeout", s.ct_timeout ? "true" : "false");
> +    smap_add(cap, "explicit_drop_action",
> +             s.explicit_drop_action ? "true" :"false");
>  }
>  
>  /* Gets timeout policy name in 'backer' based on 'zone', 'dl_type' and
> 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")
> +
>  
>  /* Stores the various features which the corresponding backer supports. */
>  struct dpif_backer_support {
> @@ -382,4 +386,6 @@ bool ofproto_dpif_ct_zone_timeout_policy_get_name(
>      const struct dpif_backer *backer, uint16_t zone, uint16_t dl_type,
>      uint8_t nw_proto, char **tp_name, bool *unwildcard);
>  
> +bool ovs_explicit_drop_action_supported(struct ofproto_dpif *);
> +
>  #endif /* ofproto-dpif.h */
> diff --git a/tests/automake.mk b/tests/automake.mk
> index 4bf8f00..c9362ab 100644
> --- a/tests/automake.mk
> +++ b/tests/automake.mk
> @@ -105,7 +105,8 @@ TESTSUITE_AT = \
>  	tests/auto-attach.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 ef521dd..0aeb4e7 100644
> --- a/tests/dpif-netdev.at
> +++ b/tests/dpif-netdev.at
> @@ -349,6 +349,14 @@ 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
> +
> +AT_CHECK([
> +ovs-appctl coverage/read-counter datapath_drop_meter
> +], [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/drop-stats.at b/tests/drop-stats.at
> new file mode 100644
> index 0000000..23df977
> --- /dev/null
> +++ b/tests/drop-stats.at
> @@ -0,0 +1,190 @@
> +AT_BANNER([drop-stats])
> +
> +AT_SETUP([drop-stats - cli tests])
> +
> +OVS_VSWITCHD_START([dnl
> +    set bridge br0 datapath_type=dummy \
> +        protocols=OpenFlow10,OpenFlow13,OpenFlow14,OpenFlow15 -- \
> +    add-port br0 p1 -- set Interface p1 type=dummy ofport_request=1])
> +
> +AT_DATA([flows.txt], [dnl
> +table=0,in_port=1,actions=drop
> +])
> +
> +AT_CHECK([
> +    ovs-ofctl del-flows br0
> +    ovs-ofctl -Oopenflow13 add-flows br0 flows.txt
> +    ovs-ofctl -Oopenflow13 dump-flows br0 | ofctl_strip | sort | grep actions ], [0], [dnl
> + in_port=1 actions=drop
> +])
> +
> +AT_CHECK([
> +    ovs-appctl netdev-dummy/receive p1 'in_port(1),packet_type(ns=0,id=0),eth(src=3a:6d:d2:09:9c:ab,dst=1e:2c:e9:2a:66:9e),ipv4(src=192.168.10.10,dst=192.168.10.30,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
> +    ovs-appctl netdev-dummy/receive p1 'in_port(1),packet_type(ns=0,id=0),eth(src=3a:6d:d2:09:9c:ab,dst=1e:2c:e9:2a:66:9e),ipv4(src=192.168.10.10,dst=192.168.10.30,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
> +    ovs-appctl netdev-dummy/receive p1 'in_port(1),packet_type(ns=0,id=0),eth(src=3a:6d:d2:09:9c:ab,dst=1e:2c:e9:2a:66:9e),ipv4(src=192.168.10.10,dst=192.168.10.30,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
> +], [0], [ignore])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-flows | sed 's/used:[[0-9]].[[0-9]]*s/used:0.0/' | sort], [0], [flow-dump from non-dpdk interfaces:
> +recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), packets:2, bytes:212, used:0.0, actions:drop
> +])
> +
> +ovs-appctl time/warp 5000
> +
> +AT_CHECK([
> +ovs-appctl coverage/read-counter drop_action_of_pipeline
> +], [0], [dnl
> +3
> +])
> +
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
> +AT_SETUP([drop-stats - pipeline and recurssion drops])
> +
> +OVS_VSWITCHD_START([dnl
> +    set bridge br0 datapath_type=dummy \
> +        protocols=OpenFlow10,OpenFlow13,OpenFlow14,OpenFlow15 -- \
> +    add-port br0 p1 -- set Interface p1 type=dummy ofport_request=1 -- \
> +    add-port br0 p2 -- set Interface p2 type=dummy ofport_request=2])
> +
> +AT_DATA([flows.txt], [dnl
> +table=0,in_port=1,actions=drop
> +])
> +
> +AT_CHECK([
> +    ovs-ofctl del-flows br0
> +    ovs-ofctl -Oopenflow13 add-flows br0 flows.txt
> +    ovs-ofctl -Oopenflow13 dump-flows br0 | ofctl_strip | sort | grep actions ], [0], [dnl
> + in_port=1 actions=drop
> +])
> +
> +AT_CHECK([
> +    ovs-appctl netdev-dummy/receive p1 'in_port(1),packet_type(ns=0,id=0),eth(src=3a:6d:d2:09:9c:ab,dst=1e:2c:e9:2a:66:9e),ipv4(src=192.168.10.10,dst=192.168.10.30,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
> +], [0], [ignore])
> +
> +ovs-appctl time/warp 5000
> +
> +AT_CHECK([
> +ovs-appctl coverage/read-counter drop_action_of_pipeline
> +], [0], [dnl
> +1
> +])
> +
> +
> +AT_DATA([flows.txt], [dnl
> +table=0, in_port=1, actions=goto_table:1
> +table=1, in_port=1, actions=goto_table:2
> +table=2, in_port=1, actions=resubmit(,1)
> +])
> +
> +AT_CHECK([
> +    ovs-ofctl del-flows br0
> +    ovs-ofctl -Oopenflow13 add-flows br0 flows.txt
> +    ovs-ofctl -Oopenflow13 dump-flows br0 | ofctl_strip | sort | grep actions ], [0], [ignore])
> +
> +AT_CHECK([
> +    ovs-appctl netdev-dummy/receive p1 'in_port(1),packet_type(ns=0,id=0),eth(src=3a:6d:d2:09:9c:ab,dst=1e:2c:e9:2a:66:9e),ipv4(src=192.168.10.10,dst=192.168.10.30,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
> +], [0], [ignore])
> +
> +ovs-appctl time/warp 5000
> +
> +AT_CHECK([
> +ovs-appctl coverage/read-counter drop_action_recursion_too_deep
> +], [0], [dnl
> +1
> +])
> +
> +
> +OVS_VSWITCHD_STOP(["/|WARN|/d"])
> +AT_CLEANUP
> +
> +AT_SETUP([drop-stats - too many resubmit])
> +OVS_VSWITCHD_START
> +add_of_ports br0 1
> +(for i in `seq 1 64`; do
> +     j=`expr $i + 1`
> +     echo "in_port=$i, actions=resubmit:$j, resubmit:$j, local"
> + done
> + echo "in_port=65, actions=local") > flows.txt
> +
> +AT_CHECK([
> +    ovs-ofctl del-flows br0
> +    ovs-ofctl -Oopenflow13 add-flows br0 flows.txt ], [0], [ignore])
> +
> +ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x1234)'
> +
> +ovs-appctl time/warp 5000
> +
> +AT_CHECK([
> +ovs-appctl coverage/read-counter drop_action_too_many_resubmit
> +], [0], [dnl
> +1
> +])
> +
> +OVS_VSWITCHD_STOP(["/|WARN|/d"])
> +AT_CLEANUP
> +
> +
> +AT_SETUP([drop-stats - stack too deep])
> +OVS_VSWITCHD_START
> +add_of_ports br0 1
> +(for i in `seq 1 12`; do
> +     j=`expr $i + 1`
> +     echo "in_port=$i, actions=resubmit:$j, resubmit:$j, local"
> + done
> + push="push:NXM_NX_REG0[[]]"
> + echo "in_port=13, actions=$push,$push,$push,$push,$push,$push,$push,$push") > flows
> +
> +AT_CHECK([ovs-ofctl add-flows br0 flows])
> +
> +ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x1234)'
> +
> +ovs-appctl time/warp 5000
> +
> +AT_CHECK([
> +ovs-appctl coverage/read-counter drop_action_stack_too_deep
> +], [0], [dnl
> +1
> +])
> +
> +
> +OVS_VSWITCHD_STOP(["/resubmits yielded over 64 kB of stack/d"])
> +AT_CLEANUP
> +
> +AT_SETUP([drop-stats - too many mpls labels])
> +
> +OVS_VSWITCHD_START([dnl
> +    set bridge br0 datapath_type=dummy \
> +        protocols=OpenFlow10,OpenFlow13,OpenFlow14,OpenFlow15 -- \
> +    add-port br0 p1 -- set Interface p1 type=dummy ofport_request=1 -- \
> +    add-port br0 p2 -- set Interface p2 type=dummy ofport_request=2])
> +
> +AT_DATA([flows.txt], [dnl
> +table=0, in_port=1, actions=push_mpls:0x8847, resubmit:3
> +table=0, in_port=3, actions=push_mpls:0x8847, set_field:10->mpls_label, set_field:15->mpls_label, resubmit:4
> +table=0, in_port=4, actions=push_mpls:0x8847, set_field:11->mpls_label, resubmit:5
> +table=0, in_port=5, actions=push_mpls:0x8847, set_field:12->mpls_label, resubmit:6
> +table=0, in_port=6, actions=push_mpls:0x8847, set_field:13->mpls_label, output:2
> +])
> +
> +AT_CHECK([
> +    ovs-ofctl del-flows br0
> +    ovs-ofctl -Oopenflow13 add-flows br0 flows.txt
> +])
> +
> +AT_CHECK([
> +    ovs-appctl netdev-dummy/receive p1 'in_port(1),packet_type(ns=0,id=0),eth(src=3a:6d:d2:09:9c:ab,dst=1e:2c:e9:2a:66:9e),ipv4(src=192.168.10.10,dst=192.168.10.30,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
> +], [0], [ignore])
> +
> +ovs-appctl time/warp 5000
> +
> +AT_CHECK([
> +ovs-appctl coverage/read-counter drop_action_too_many_mpls_labels
> +], [0], [dnl
> +1
> +])
> +
> +
> +OVS_VSWITCHD_STOP(["/|WARN|/d"])
> +AT_CLEANUP
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 49326c5..1ba396a 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -9425,7 +9425,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 e759123..7369991 100644
> --- a/tests/testsuite.at
> +++ b/tests/testsuite.at
> @@ -76,3 +76,4 @@ m4_include([tests/auto-attach.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..b92c23f 100644
> --- a/tests/tunnel-push-pop.at
> +++ b/tests/tunnel-push-pop.at
> @@ -447,6 +447,27 @@ 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 5000
> +
> +AT_CHECK([
> +ovs-appctl coverage/read-counter datapath_drop_tunnel_pop_error
> +], [0], [dnl
> +1
> +])
> +
> +AT_CHECK([ovs-appctl netdev-dummy/receive p0 'aa55aa550000001b213cab6408004503007079464000402fba600101025c0101025820000800000001c845000054ba200000400184861e0000011e00000200004227e75400030af3195500000000f265010000000000101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f3031323334353637'])
> +
> +ovs-appctl time/warp 5000
> +
> +AT_CHECK([
> +ovs-appctl coverage/read-counter drop_action_congestion
> +], [0], [dnl
> +1
> +])
> +
> +
>  dnl Check GREL3 only accepts non-fragmented packets?
>  AT_CHECK([ovs-appctl netdev-dummy/receive p0 'aa55aa550000001b213cab6408004500007e79464000402fba550101025c0101025820000800000001c8fe71d883724fbeb6f4e1494a080045000054ba200000400184861e0000011e00000200004227e75400030af3195500000000f265010000000000101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f3031323334353637'])
>  
> @@ -455,7 +476,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
> @@ -478,7 +499,7 @@ AT_CHECK([ovs-ofctl dump-ports int-br | grep 'port  5'], [0], [dnl
>    port  5: rx pkts=1, bytes=98, drop=?, errs=?, frame=?, over=?, crc=?
>  ])
>  AT_CHECK([ovs-appctl dpif/dump-flows int-br | grep 'in_port(6081)'], [0], [dnl
> -tunnel(tun_id=0x7b,src=1.1.2.92,dst=1.1.2.88,geneve({class=0xffff,type=0x80,len=4,0xa/0xf}{class=0xffff,type=0,len=4}),flags(-df-csum+key)),recirc_id(0),in_port(6081),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:userspace(pid=0,controller(reason=1,dont_send=0,continuation=0,recirc_id=3,rule_cookie=0,controller_id=0,max_len=65535))
> +tunnel(tun_id=0x7b,src=1.1.2.92,dst=1.1.2.88,geneve({class=0xffff,type=0x80,len=4,0xa/0xf}{class=0xffff,type=0,len=4}),flags(-df-csum+key)),recirc_id(0),in_port(6081),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:userspace(pid=0,controller(reason=1,dont_send=0,continuation=0,recirc_id=2,rule_cookie=0,controller_id=0,max_len=65535))
>  ])
>  
>  ovs-appctl time/warp 10000
> @@ -510,7 +531,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 faffb41..ce000a2 100644
> --- a/tests/tunnel.at
> +++ b/tests/tunnel.at
> @@ -102,8 +102,9 @@ 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],
> -  [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
> +AT_CHECK([tail -3 stdout], [0],
> +  [Final flow: unchanged
> +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
>  ])
>  OVS_VSWITCHD_STOP(["/dropping tunnel packet marked ECN CE but is not ECN capable/d"])
> @@ -193,6 +194,17 @@ 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'])
> +
> +ovs-appctl time/warp 5000
> +
> +AT_CHECK([
> +ovs-appctl coverage/read-counter datapath_drop_invalid_port
> +], [0], [dnl
> +1
> +])
> +
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
>  
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index 09e7bdf..14f73cc 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -5917,6 +5917,11 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \
>          timeout policies based on connection tracking zones, as configured
>          through the <code>CT_Timeout_Policy</code> table.
>        </column>
> +      <column name="capabilities" key="explicit_drop_action"
> +              type='{"type": "boolean"}'>
> +        True if the datapath supports OVS_ACTION_ATTR_DROP.  If false,
> +        explicit drop action will not be sent to the datapath.
> +      </column>
>      </group>
>  
>      <group title="Common Columns">
>

Thanks for the review Ilya,

For the below comment 


> @@ -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_));
This is not a tunnel push error.  We literally executed all the actions without errors and that is not our fault that there are no more actions after tnl_push.  We're just saving some time avoiding actual execution of a pointless action.  So, this should not be accounted as error, and definitely not as a tunnel push error.

I agree we need a better name .Does datapath_drop_incomplete_dp_action sound good or do you have any other suggestions in mind?

Regards & Thanks
Anju

T
-----Original Message-----
From: Ilya Maximets <i.maximets@ovn.org> 
Sent: Friday, December 13, 2019 9:57 PM
To: Anju Thomas <anju.thomas@ericsson.com>; dev@openvswitch.org
Cc: i.maximets@ovn.org; echaudro@redhat.com; Rohith Basavaraja <rohith.basavaraja@gmail.com>; Keshav Gupta <keshugupta1@gmail.com>
Subject: Re: [PATCH v17] Improved Packet Drop Statistics in OVS

On 06.12.2019 09:49, 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.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>
> Acked-by: Eelco Chaudron <echaudro@redhat.com
> ---

Thanks for a new version.  I really hope that v18 will be the last one.
This patch needs some minor rebase on top of current master.
6 more comments inline (some of them are really minor, but I decided to point to them as you'll re-spin the patch anyway).  Please, fix them and send v18.

Best regards, Ilya Maximets.

> v17 (Ilya's comments)
>  1. comments for xlate_error
>  2. define xlate_error in ifndef __KERNEL__  3. move declaration of 
> xlate_error after OVS_TUNNEL_KEY_ATTR_MAX  4. comment change for DROP 
> action  5. Add in datapath capability
> 
> 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 |  24 +++
>  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                      |  33 +++-
>  ofproto/ofproto-dpif-xlate.h                      |  13 --
>  ofproto/ofproto-dpif.c                            |  10 ++
>  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 +-
>  vswitchd/vswitch.xml                              |   5 +
>  20 files changed, 457 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..8006724 100644
> --- a/datapath/linux/compat/include/linux/openvswitch.h
> +++ b/datapath/linux/compat/include/linux/openvswitch.h
> @@ -407,6 +407,28 @@ enum ovs_tunnel_key_attr {  #define 
> OVS_TUNNEL_KEY_ATTR_MAX (__OVS_TUNNEL_KEY_ATTR_MAX - 1)
>  
>  /**
> + * enum xlate_error -  Different types of error during translation */
> +
> +#ifndef __KERNEL__
> +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,
> +};
> +#endif


Over and over again, kernel coding style, please.


> +
> +/**
>   * enum ovs_frag_type - IPv4 and IPv6 fragment type
>   * @OVS_FRAG_TYPE_NONE: Packet is not a fragment.
>   * @OVS_FRAG_TYPE_FIRST: Packet is a fragment with offset 0.
> @@ -962,6 +984,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 +1017,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,          /* u32 xlate_error. */
>  #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_));

This is not a tunnel push error.  We literally executed all the actions without errors and that is not our fault that there are no more actions after tnl_push.  We're just saving some time avoiding actual execution of a pointless action.  So, this should not be accounted as error, and definitely not as a tunnel push error.

>              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_));
> +

Empty line not needed here.

>          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 *);
> +

New empty line is not needed here.

>  
>  /* 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);

Please, add a single space after ':'.

> +   }
> +}
>  
>  /* 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_batch_size(batch)

> +            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..222c954 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,21 @@ exit:
>              ofpbuf_clear(xin->odp_actions);
>          }
>      }
> +
> +    /* Install drop action if datapath supports explicit drop action. */
> +    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..7d580dc 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); @@ 
> -5550,6 +5558,8 @@ get_datapath_cap(const char *datapath_type, struct 
> smap *cap)
>  
>      smap_add(cap, "check_pkt_len", s.check_pkt_len ? "true" : "false");
>      smap_add(cap, "ct_timeout", s.ct_timeout ? "true" : "false");
> +    smap_add(cap, "explicit_drop_action",
> +             s.explicit_drop_action ? "true" :"false");
>  }
>  
>  /* Gets timeout policy name in 'backer' based on 'zone', 'dl_type' 
> and 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")
> +
>  
>  /* Stores the various features which the corresponding backer 
> supports. */  struct dpif_backer_support { @@ -382,4 +386,6 @@ bool 
> ofproto_dpif_ct_zone_timeout_policy_get_name(
>      const struct dpif_backer *backer, uint16_t zone, uint16_t dl_type,
>      uint8_t nw_proto, char **tp_name, bool *unwildcard);
>  
> +bool ovs_explicit_drop_action_supported(struct ofproto_dpif *);
> +
>  #endif /* ofproto-dpif.h */
> diff --git a/tests/automake.mk b/tests/automake.mk index 
> 4bf8f00..c9362ab 100644
> --- a/tests/automake.mk
> +++ b/tests/automake.mk
> @@ -105,7 +105,8 @@ TESTSUITE_AT = \
>  	tests/auto-attach.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 
> ef521dd..0aeb4e7 100644
> --- a/tests/dpif-netdev.at
> +++ b/tests/dpif-netdev.at
> @@ -349,6 +349,14 @@ 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
> +
> +AT_CHECK([
> +ovs-appctl coverage/read-counter datapath_drop_meter ], [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/drop-stats.at 
> b/tests/drop-stats.at new file mode 100644 index 0000000..23df977
> --- /dev/null
> +++ b/tests/drop-stats.at
> @@ -0,0 +1,190 @@
> +AT_BANNER([drop-stats])
> +
> +AT_SETUP([drop-stats - cli tests])
> +
> +OVS_VSWITCHD_START([dnl
> +    set bridge br0 datapath_type=dummy \
> +        protocols=OpenFlow10,OpenFlow13,OpenFlow14,OpenFlow15 -- \
> +    add-port br0 p1 -- set Interface p1 type=dummy ofport_request=1])
> +
> +AT_DATA([flows.txt], [dnl
> +table=0,in_port=1,actions=drop
> +])
> +
> +AT_CHECK([
> +    ovs-ofctl del-flows br0
> +    ovs-ofctl -Oopenflow13 add-flows br0 flows.txt
> +    ovs-ofctl -Oopenflow13 dump-flows br0 | ofctl_strip | sort | grep 
> +actions ], [0], [dnl
> + in_port=1 actions=drop
> +])
> +
> +AT_CHECK([
> +    ovs-appctl netdev-dummy/receive p1 'in_port(1),packet_type(ns=0,id=0),eth(src=3a:6d:d2:09:9c:ab,dst=1e:2c:e9:2a:66:9e),ipv4(src=192.168.10.10,dst=192.168.10.30,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
> +    ovs-appctl netdev-dummy/receive p1 'in_port(1),packet_type(ns=0,id=0),eth(src=3a:6d:d2:09:9c:ab,dst=1e:2c:e9:2a:66:9e),ipv4(src=192.168.10.10,dst=192.168.10.30,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
> +    ovs-appctl netdev-dummy/receive p1 'in_port(1),packet_type(ns=0,id=0),eth(src=3a:6d:d2:09:9c:ab,dst=1e:2c:e9:2a:66:9e),ipv4(src=192.168.10.10,dst=192.168.10.30,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
> +], [0], [ignore])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-flows | sed 's/used:[[0-9]].[[0-9]]*s/used:0.0/' | sort], [0], [flow-dump from non-dpdk interfaces:
> +recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(
> +frag=no), packets:2, bytes:212, used:0.0, actions:drop
> +])
> +
> +ovs-appctl time/warp 5000
> +
> +AT_CHECK([
> +ovs-appctl coverage/read-counter drop_action_of_pipeline ], [0], [dnl
> +3
> +])
> +
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
> +AT_SETUP([drop-stats - pipeline and recurssion drops])
> +
> +OVS_VSWITCHD_START([dnl
> +    set bridge br0 datapath_type=dummy \
> +        protocols=OpenFlow10,OpenFlow13,OpenFlow14,OpenFlow15 -- \
> +    add-port br0 p1 -- set Interface p1 type=dummy ofport_request=1 -- \
> +    add-port br0 p2 -- set Interface p2 type=dummy ofport_request=2])
> +
> +AT_DATA([flows.txt], [dnl
> +table=0,in_port=1,actions=drop
> +])
> +
> +AT_CHECK([
> +    ovs-ofctl del-flows br0
> +    ovs-ofctl -Oopenflow13 add-flows br0 flows.txt
> +    ovs-ofctl -Oopenflow13 dump-flows br0 | ofctl_strip | sort | grep 
> +actions ], [0], [dnl
> + in_port=1 actions=drop
> +])
> +
> +AT_CHECK([
> +    ovs-appctl netdev-dummy/receive p1 'in_port(1),packet_type(ns=0,id=0),eth(src=3a:6d:d2:09:9c:ab,dst=1e:2c:e9:2a:66:9e),ipv4(src=192.168.10.10,dst=192.168.10.30,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
> +], [0], [ignore])
> +
> +ovs-appctl time/warp 5000
> +
> +AT_CHECK([
> +ovs-appctl coverage/read-counter drop_action_of_pipeline ], [0], [dnl
> +1
> +])
> +
> +
> +AT_DATA([flows.txt], [dnl
> +table=0, in_port=1, actions=goto_table:1 table=1, in_port=1, 
> +actions=goto_table:2 table=2, in_port=1, actions=resubmit(,1)
> +])
> +
> +AT_CHECK([
> +    ovs-ofctl del-flows br0
> +    ovs-ofctl -Oopenflow13 add-flows br0 flows.txt
> +    ovs-ofctl -Oopenflow13 dump-flows br0 | ofctl_strip | sort | grep 
> +actions ], [0], [ignore])
> +
> +AT_CHECK([
> +    ovs-appctl netdev-dummy/receive p1 'in_port(1),packet_type(ns=0,id=0),eth(src=3a:6d:d2:09:9c:ab,dst=1e:2c:e9:2a:66:9e),ipv4(src=192.168.10.10,dst=192.168.10.30,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
> +], [0], [ignore])
> +
> +ovs-appctl time/warp 5000
> +
> +AT_CHECK([
> +ovs-appctl coverage/read-counter drop_action_recursion_too_deep ], 
> +[0], [dnl
> +1
> +])
> +
> +
> +OVS_VSWITCHD_STOP(["/|WARN|/d"])
> +AT_CLEANUP
> +
> +AT_SETUP([drop-stats - too many resubmit]) OVS_VSWITCHD_START 
> +add_of_ports br0 1 (for i in `seq 1 64`; do
> +     j=`expr $i + 1`
> +     echo "in_port=$i, actions=resubmit:$j, resubmit:$j, local"
> + done
> + echo "in_port=65, actions=local") > flows.txt
> +
> +AT_CHECK([
> +    ovs-ofctl del-flows br0
> +    ovs-ofctl -Oopenflow13 add-flows br0 flows.txt ], [0], [ignore])
> +
> +ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x1234)'
> +
> +ovs-appctl time/warp 5000
> +
> +AT_CHECK([
> +ovs-appctl coverage/read-counter drop_action_too_many_resubmit ], 
> +[0], [dnl
> +1
> +])
> +
> +OVS_VSWITCHD_STOP(["/|WARN|/d"])
> +AT_CLEANUP
> +
> +
> +AT_SETUP([drop-stats - stack too deep]) OVS_VSWITCHD_START 
> +add_of_ports br0 1 (for i in `seq 1 12`; do
> +     j=`expr $i + 1`
> +     echo "in_port=$i, actions=resubmit:$j, resubmit:$j, local"
> + done
> + push="push:NXM_NX_REG0[[]]"
> + echo "in_port=13, 
> +actions=$push,$push,$push,$push,$push,$push,$push,$push") > flows
> +
> +AT_CHECK([ovs-ofctl add-flows br0 flows])
> +
> +ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x1234)'
> +
> +ovs-appctl time/warp 5000
> +
> +AT_CHECK([
> +ovs-appctl coverage/read-counter drop_action_stack_too_deep ], [0], 
> +[dnl
> +1
> +])
> +
> +
> +OVS_VSWITCHD_STOP(["/resubmits yielded over 64 kB of stack/d"]) 
> +AT_CLEANUP
> +
> +AT_SETUP([drop-stats - too many mpls labels])
> +
> +OVS_VSWITCHD_START([dnl
> +    set bridge br0 datapath_type=dummy \
> +        protocols=OpenFlow10,OpenFlow13,OpenFlow14,OpenFlow15 -- \
> +    add-port br0 p1 -- set Interface p1 type=dummy ofport_request=1 -- \
> +    add-port br0 p2 -- set Interface p2 type=dummy ofport_request=2])
> +
> +AT_DATA([flows.txt], [dnl
> +table=0, in_port=1, actions=push_mpls:0x8847, resubmit:3 table=0, 
> +in_port=3, actions=push_mpls:0x8847, set_field:10->mpls_label, 
> +set_field:15->mpls_label, resubmit:4 table=0, in_port=4, 
> +actions=push_mpls:0x8847, set_field:11->mpls_label, resubmit:5 
> +table=0, in_port=5, actions=push_mpls:0x8847, 
> +set_field:12->mpls_label, resubmit:6 table=0, in_port=6, 
> +actions=push_mpls:0x8847, set_field:13->mpls_label, output:2
> +])
> +
> +AT_CHECK([
> +    ovs-ofctl del-flows br0
> +    ovs-ofctl -Oopenflow13 add-flows br0 flows.txt
> +])
> +
> +AT_CHECK([
> +    ovs-appctl netdev-dummy/receive p1 'in_port(1),packet_type(ns=0,id=0),eth(src=3a:6d:d2:09:9c:ab,dst=1e:2c:e9:2a:66:9e),ipv4(src=192.168.10.10,dst=192.168.10.30,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
> +], [0], [ignore])
> +
> +ovs-appctl time/warp 5000
> +
> +AT_CHECK([
> +ovs-appctl coverage/read-counter drop_action_too_many_mpls_labels ], 
> +[0], [dnl
> +1
> +])
> +
> +
> +OVS_VSWITCHD_STOP(["/|WARN|/d"])
> +AT_CLEANUP
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index 
> 49326c5..1ba396a 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -9425,7 +9425,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_mark(0/0),ct_label(0/0),recirc_id(0),dp_hash(0/0),in_port(
> -dpif|DBG|1),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:09/00:00:00
> -dpif|DBG|:00:00:00,dst=50:54:00:00:00:0a/00:00:00:00:00:00),eth_type(
> -dpif|DBG|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),c
> +dpif|DBG|t_mark(0/0),ct_label(0/0),recirc_id(0),dp_hash(0/0),in_port(
> +dpif|DBG|1),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:09/00:00:00
> +dpif|DBG|:00:00:00,dst=50:54:00:00:00:0a/00:00:00:00:00:00),eth_type(
> +dpif|DBG|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: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 
> e759123..7369991 100644
> --- a/tests/testsuite.at
> +++ b/tests/testsuite.at
> @@ -76,3 +76,4 @@ m4_include([tests/auto-attach.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..b92c23f 100644
> --- a/tests/tunnel-push-pop.at
> +++ b/tests/tunnel-push-pop.at
> @@ -447,6 +447,27 @@ 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
> +20000800000001c845000054ba200000400184861e0000011e00000200004227e7540
> +0030af3195500000000f265010000000000101112131415161718191a1b1c1d1e1f20
> +2122232425262728292a2b2c2d2e2f3031323334353637'])
> +
> +ovs-appctl time/warp 5000
> +
> +AT_CHECK([
> +ovs-appctl coverage/read-counter datapath_drop_tunnel_pop_error ], 
> +[0], [dnl
> +1
> +])
> +
> +AT_CHECK([ovs-appctl netdev-dummy/receive p0 
> +'aa55aa550000001b213cab6408004503007079464000402fba600101025c01010258
> +20000800000001c845000054ba200000400184861e0000011e00000200004227e7540
> +0030af3195500000000f265010000000000101112131415161718191a1b1c1d1e1f20
> +2122232425262728292a2b2c2d2e2f3031323334353637'])
> +
> +ovs-appctl time/warp 5000
> +
> +AT_CHECK([
> +ovs-appctl coverage/read-counter drop_action_congestion ], [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 +476,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 @@ -478,7 
> +499,7 @@ AT_CHECK([ovs-ofctl dump-ports int-br | grep 'port  5'], [0], [dnl
>    port  5: rx pkts=1, bytes=98, drop=?, errs=?, frame=?, over=?, crc=?
>  ])
>  AT_CHECK([ovs-appctl dpif/dump-flows int-br | grep 'in_port(6081)'], 
> [0], [dnl 
> -tunnel(tun_id=0x7b,src=1.1.2.92,dst=1.1.2.88,geneve({class=0xffff,typ
> e=0x80,len=4,0xa/0xf}{class=0xffff,type=0,len=4}),flags(-df-csum+key))
> ,recirc_id(0),in_port(6081),packet_type(ns=0,id=0),eth_type(0x0800),ip
> v4(frag=no), packets:0, bytes:0, used:never, 
> actions:userspace(pid=0,controller(reason=1,dont_send=0,continuation=0
> ,recirc_id=3,rule_cookie=0,controller_id=0,max_len=65535))
> +tunnel(tun_id=0x7b,src=1.1.2.92,dst=1.1.2.88,geneve({class=0xffff,typ
> +e=0x80,len=4,0xa/0xf}{class=0xffff,type=0,len=4}),flags(-df-csum+key)
> +),recirc_id(0),in_port(6081),packet_type(ns=0,id=0),eth_type(0x0800),
> +ipv4(frag=no), packets:0, bytes:0, used:never, 
> +actions:userspace(pid=0,controller(reason=1,dont_send=0,continuation=
> +0,recirc_id=2,rule_cookie=0,controller_id=0,max_len=65535))
>  ])
>  
>  ovs-appctl time/warp 10000
> @@ -510,7 +531,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 faffb41..ce000a2 100644
> --- a/tests/tunnel.at
> +++ b/tests/tunnel.at
> @@ -102,8 +102,9 @@ 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],
> -  [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
> +AT_CHECK([tail -3 stdout], [0],
> +  [Final flow: unchanged
> +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
>  ])
>  OVS_VSWITCHD_STOP(["/dropping tunnel packet marked ECN CE but is not 
> ECN capable/d"]) @@ -193,6 +194,17 @@ 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
> +01004227e75400030af3195500000000f265010000000000101112131415161718191
> +a1b1c1d1e1f202122232425262728292a2b2c2d2e2f3031323334353637'])
> +
> +ovs-appctl time/warp 5000
> +
> +AT_CHECK([
> +ovs-appctl coverage/read-counter datapath_drop_invalid_port ], [0], 
> +[dnl
> +1
> +])
> +
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
>  
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml index 
> 09e7bdf..14f73cc 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -5917,6 +5917,11 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \
>          timeout policies based on connection tracking zones, as configured
>          through the <code>CT_Timeout_Policy</code> table.
>        </column>
> +      <column name="capabilities" key="explicit_drop_action"
> +              type='{"type": "boolean"}'>
> +        True if the datapath supports OVS_ACTION_ATTR_DROP.  If false,
> +        explicit drop action will not be sent to the datapath.
> +      </column>
>      </group>
>  
>      <group title="Common Columns">
>

On 16.12.2019 05:07, Anju Thomas wrote:
> Thanks for the review Ilya,
> 
> For the below comment 
> 
> 
>> @@ -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_));
> This is not a tunnel push error.  We literally executed all the actions without errors and that is not our fault that there are no more actions after tnl_push.  We're just saving some time avoiding actual execution of a pointless action.  So, this should not be accounted as error, and definitely not as a tunnel push error.
> 
> I agree we need a better name .Does datapath_drop_incomplete_dp_action sound good or do you have any other suggestions in mind?

datapath_drop_last_action_tnl_push ?

And we actually could just avoid counting in this case because:

1. It's a normal scenario of action execution.  If the code looked like this:

    case OVS_ACTION_ATTR_TUNNEL_PUSH:
        dp_packet_batch_apply_cutlen(packets_);
        push_tnl_action(pmd, a, packets_)
        break;

  you most probably wouldn't noticed this case because it's perfectly normal.

2. We're not tracking similar cases for all other actions.  What if push_vlan,
   set_ipv4_src or meter is the last action?

The only reason why we have a spacial case with big comment for tunnel push
is that we had dp_packet leak here and we don't really want to execute heavy
push action if we're going to drop these packets anyway.

Best regards, Ilya Maximets.