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

[Ben Pfaff]

On Tue, Jul 16, 2019 at 08:21:37PM +0530, 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
> 
> A detailed presentation on this was presented at OvS conference 2017 and link for the corresponding presentation is available at:
> 
> https://www.slideshare.net/LF_OpenvSwitch/lfovs17troubleshooting-the-data-plane-in-ovs-82280329
> v11: addressed comments from Eelco and Gregory from v10
> 
> 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>

Thanks for the patch!

Please word-wrap the commit message at 75 columns, as suggested by
submitting-patches.rst.  The exact width is not that important, but many
of the above lines are clearly too long.

Because the new datapath action is currently userspace-only, its
enumeration should be in the #ifndef __KERNEL__ block in openvswitch.h.

It is a bad idea to use an enumeration type as part of an ABI like the
one in openvswitch.h, because compilers can be configured to use
different types for enums.  For example, GCC with -fshort-enums changes
the size of enum types.  The user should not usually do that, but it's
best for ABIs to be resilient, in my opinion.  (There's another possible
excuse in this case, since this is only for the userspace datapath
currently so that the enum is always being produced and consumed within
the same process, so this is more about future extension.)  So, instead
of something like this:

    static void
    put_drop_action(struct ofpbuf *odp_actions, enum xlate_error error)
    {
        nl_msg_put_unspec(odp_actions, OVS_ACTION_ATTR_DROP,
                          &error, sizeof error);

    }

I'd suggest using a u32, like this:

    static void
    put_drop_action(struct ofpbuf *odp_actions, enum xlate_error error)
    {
        nl_msg_put_u32(odp_actions, OVS_ACTION_ATTR_DROP, error);
    }

I'm uncomfortable with the two new errors XLATE_CONGESTION_DROP and
XLATE_FORWARDING_DISABLED as translation errors, since they aren't
translation errors and don't prevent translation from completing.

XLATE_FORWARDING_DISABLED doesn't seem to actually be used anywhere.

Thanks,

Ben.