I'm OK with using XLATE_*, but it shouldn't be reported as an error to the caller of xlate_actions(). I guess that you can find a different internal mechanism, or you can just suppress the error before returning to the caller.
On Thu, Jul 18, 2019 at 03:59:21AM +0000, Anju Thomas wrote: > Hi Ben, > As per your comments in v6 that I started using XLATE_* as drop reason to be > passed to the datapath. Should I revert back to that method of passing the > drop reason? > > Regards > Anju > > -----Original Message----- > From: Ben Pfaff <[email protected]> > Sent: Wednesday, July 17, 2019 11:38 PM > To: Anju Thomas <[email protected]> > Subject: Re: [ovs-dev] [PATCH v11] Improved Packet Drop Statistics in OVS > > On Wed, Jul 17, 2019 at 09:19:06AM +0000, Anju Thomas wrote: > > 1. 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. > > > > I missed XLATE_FORWARDING_DISABLED in this patch. However it is added > > in the below area > > > > > > /* We've let OFPP_NORMAL and the learning action look at the > > * packet, so cancel all actions and freezing if forwarding is > > * disabled. */ > > if (in_port && (!xport_stp_forward_state(in_port) || > > !xport_rstp_forward_state(in_port))) { > > ctx.odp_actions->size = sample_actions_len; > > ctx_cancel_freeze(&ctx); > > ofpbuf_clear(&ctx.action_set); > > ctx.error = XLATE_FORWARDING_DISABLED; > > } > > > > if (!ctx.freezing) { > > xlate_action_set(&ctx); > > } > > if (ctx.freezing) { > > finish_freezing(&ctx); > > } > > } else if (ecn_drop) { > > ctx.error = XLATE_CONGESTION_DROP; > > } > > > > > > > > Regarding the validity, In these cases I am going to be passing a drop > action to the datapath now . > > > > if (xin->odp_actions && !xin->odp_actions->size && > > ovs_explicit_drop_action_supported(ctx.xbridge->ofproto)) { > > put_drop_action(xin->odp_actions, ctx.error); > > } > > > > > > Do what reason should I pass in case it is drop due to forwarding > disabled or congestion drop? I would still like to account for these drops > in datapath . Can you give me any suggestions ? > > I'd just use some way to signal the drop reason to that code other than > ctx.error. > To: Ben Pfaff <[email protected]> > Cc: [email protected], Keshav Gupta <[email protected]> > Subject: RE: [ovs-dev] [PATCH v6] Improved Packet Drop Statistics in OVS > X-Mailer: Microsoft Outlook 16.0 > > > > -----Original Message----- > From: Ben Pfaff <[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.) > > 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. > > 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. > > 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
