Hi Ben, Thank you for the comments. Replies inline
Regards Anju -----Original Message----- From: Ben Pfaff [mailto:[email protected]] Sent: Friday, January 18, 2019 11:45 PM To: Anju Thomas <[email protected]> Cc: [email protected]; Keshav Gupta <[email protected]> Subject: Re: [ovs-dev] [PATCH v6] Improved Packet Drop Statistics in OVS On Thu, Jan 17, 2019 at 04:49:20AM +0000, 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. Thanks for the patch! I agree with your motivations--it is useful to understand why packets are dropped. I have some comments to add to Ilya's. It looks like the drop actions that this formats in format_odp_drop_action() can't necessarily be parsed by odp_actions_from_string(). Usually we expect this. (Probably the syntax should be adjusted to make parsing more straightforward.) <Anju> odp_actions_from string is used to convert string to action for parsing .But we are using the drop action reason only fo display. We do not want to provide an option for its parsing. Is my understanding correct. It looks like xlate_error maps one-to-one to drop reasons (except why is XLATE_FORWARDING_DISABLED mapped to OVS_DROP_REASON_MAX?), so do we really want different enumerations? Mapping back and forth is a bit of a slog, and there's already a way to translate xlate_errors to strings. <Anju> .We just kept it separate for better code readability and also that XLATE_** error were more in the ofproto layer and we wanted something in the datapath . If you believe that that is not justified compared to the slogging then I will modify this . What do you say ? This exports coverage_mutex but I don't see why since nothing new uses it. Actually I think all the changes to coverage.[ch] are unneeded. <Anju> You are right. I will change this This adds and removes a number of blank lines, I don't see the value in that. Thanks, Ben. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
