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

[Ben Pfaff]

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 <blp at ovn.org> 
> Sent: Wednesday, July 17, 2019 11:38 PM
> To: Anju Thomas <anju.thomas at ericsson.com>
> 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 <blp at ovn.org>
> Cc: dev at openvswitch.org, Keshav Gupta <keshugupta1 at gmail.com>
> Subject: RE: [ovs-dev] [PATCH v6] Improved Packet Drop Statistics in OVS
> X-Mailer: Microsoft Outlook 16.0
> 
> 
> 
> -----Original Message-----
> From: Ben Pfaff <blp at ovn.org> 
> Sent: Friday, January 18, 2019 11:45 PM
> To: Anju Thomas <anju.thomas at ericsson.com>
> Cc: dev at openvswitch.org; Keshav Gupta <keshugupta1 at gmail.com>
> 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.