[ovs-dev] [PATCH v12] Improved Packet Drop Statistics in OVS

Ilya Maximets i.maximets at samsung.com
Thu Sep 5 08:36:52 UTC 2019


Not a code review. Just a few notes about patch formatting.

Best regards, Ilya Maximets.

On 25.07.2019 15:16, 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
> 
> v11->v12 Addresses comments from Ben Pfaff

Above line should go after the '---' line as it's not useful in a git log.

> 
> Co-authored-by: Rohith Basavaraja <rohith.basavaraja at gmail.com>
> Co-authored-by: Keshav Gupta <keshugupta1 at gmail.com>
> Signed-off-by: Anju Thomas <anju.thomas at ericsson.com>
> Signed-off-by: Rohith Basavaraja <rohith.basavaraja at gmail.com>
> Signed-off-by: Keshav Gupta <keshugupta1 at gmail.com>
> ---

Version history should be here and it'll be good if it will be a bit
more detailed, i.e. what really changed in this and previous versions.
Like this:

Version 13:
  * Implemented something.
  * Fixed something.

Version 12:
  * ...

Without history it's hard to track changes especially for long living
or big patches.

Same applicable to other patches on a mail-list.

One more hint is that it's better to add relevant reviewers to CC list
while sending patches. This could speed up review process as not everyone
subscribed to all the messages from mail-list.

>  datapath/linux/compat/include/linux/openvswitch.h |   1 +
>  lib/dpif-netdev.c                                 |  46 ++++-
>  lib/dpif.c                                        |   7 +
>  lib/dpif.h                                        |   4 +
>  lib/odp-execute.c                                 |  79 ++++++++
>  lib/odp-util.c                                    |   9 +
>  ofproto/ofproto-dpif-ipfix.c                      |   1 +
>  ofproto/ofproto-dpif-sflow.c                      |   1 +
>  ofproto/ofproto-dpif-xlate.c                      |  40 ++++-
>  ofproto/ofproto-dpif-xlate.h                      |   3 +
>  ofproto/ofproto-dpif.c                            |   8 +
>  ofproto/ofproto-dpif.h                            |   8 +-
>  tests/automake.mk                                 |   3 +-
>  tests/dpif-netdev.at                              |   8 +
>  tests/drop-stats.at                               | 210 ++++++++++++++++++++++
>  tests/ofproto-dpif.at                             |   2 +-
>  tests/tunnel-push-pop.at                          |  29 ++-
>  tests/tunnel.at                                   |  16 +-
>  18 files changed, 464 insertions(+), 11 deletions(-)
>  create mode 100644 tests/drop-stats.at


More information about the dev mailing list