[ovs-dev] [PATCH] ofproto-dpif: Do not give stats to rules bypassed by "drop" frag policy.

Ben Pfaff blp at nicira.com
Wed Jun 5 17:16:14 UTC 2013


Thanks.

I applied the following incremental so that dropped fragments are not
reported as flow table lookups or matches (based on Jesse's commit
7e741ffb84c30 "ofproto-dpif: Don't count misses in OpenFlow table
stats." that was applied in the meantime) and will push this to master
momentarily.  I'll consider backporting, but if it's a lot of work
then I may skip it.

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 300ff95..096f0f2 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1771,7 +1771,7 @@ get_tables(struct ofproto *ofproto_, struct ofp12_table_stats *ots)
 {
     struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
     struct dpif_dp_stats s;
-    uint64_t n_miss, n_no_pkt_in, n_bytes;
+    uint64_t n_miss, n_no_pkt_in, n_bytes, n_dropped_frags;
     uint64_t n_lookup;
 
     strcpy(ots->name, "classifier");
@@ -1779,8 +1779,9 @@ get_tables(struct ofproto *ofproto_, struct ofp12_table_stats *ots)
     dpif_get_dp_stats(ofproto->backer->dpif, &s);
     rule_get_stats(&ofproto->miss_rule->up, &n_miss, &n_bytes);
     rule_get_stats(&ofproto->no_packet_in_rule->up, &n_no_pkt_in, &n_bytes);
+    rule_get_stats(&ofproto->drop_frags_rule->up, &n_dropped_frags, &n_bytes);
 
-    n_lookup = s.n_hit + s.n_missed;
+    n_lookup = s.n_hit + s.n_missed - n_dropped_frags;
     ots->lookup_count = htonll(n_lookup);
     ots->matched_count = htonll(n_lookup - n_miss - n_no_pkt_in);
 }


On Mon, Jun 03, 2013 at 07:28:44PM -0700, Ethan Jackson wrote:
> Acked-by: Ethan Jackson <ethan at nicira.com>
> 
> On Tue, May 28, 2013 at 1:16 PM, Ben Pfaff <blp at nicira.com> wrote:
> > When the OFPC_FRAG_DROP policy is in effect, IP fragments are supposed to
> > be dropped before they reach the flow table.  Open vSwitch properly dropped
> > IP fragments in this case, but still accounted them to the packet and byte
> > counters for the flow that they would have hit if the OFPC_FRAG_NX_MATCh
> > policy had been in effect.
> >
> > Reported-by: love you <thunder.love07 at gmail.com>
> > Signed-off-by: Ben Pfaff <blp at nicira.com>
> > ---
> >  ofproto/ofproto-dpif.c |   20 ++++++++++++++++----
> >  tests/ofproto-dpif.at  |    9 +++++++--
> >  2 files changed, 23 insertions(+), 6 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index 280fd57..17af5c6 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -698,6 +698,7 @@ struct ofproto_dpif {
> >      /* Special OpenFlow rules. */
> >      struct rule_dpif *miss_rule; /* Sends flow table misses to controller. */
> >      struct rule_dpif *no_packet_in_rule; /* Drops flow table misses. */
> > +    struct rule_dpif *drop_frags_rule; /* Used in OFPC_FRAG_DROP mode. */
> >
> >      /* Statistics. */
> >      uint64_t n_matches;
> > @@ -1505,6 +1506,12 @@ add_internal_flows(struct ofproto_dpif *ofproto)
> >      ofpbuf_clear(&ofpacts);
> >      error = add_internal_flow(ofproto, id++, &ofpacts,
> >                                &ofproto->no_packet_in_rule);
> > +    if (error) {
> > +        return error;
> > +    }
> > +
> > +    error = add_internal_flow(ofproto, id++, &ofpacts,
> > +                              &ofproto->drop_frags_rule);
> >      return error;
> >  }
> >
> > @@ -5611,20 +5618,22 @@ rule_dpif_lookup__(struct ofproto_dpif *ofproto, const struct flow *flow,
> >  {
> >      struct cls_rule *cls_rule;
> >      struct classifier *cls;
> > +    bool frag;
> >
> >      if (table_id >= N_TABLES) {
> >          return NULL;
> >      }
> >
> >      cls = &ofproto->up.tables[table_id].cls;
> > -    if (flow->nw_frag & FLOW_NW_FRAG_ANY
> > -        && ofproto->up.frag_handling == OFPC_FRAG_NORMAL) {
> > -        /* For OFPC_NORMAL frag_handling, we must pretend that transport ports
> > -         * are unavailable. */
> > +    frag = (flow->nw_frag & FLOW_NW_FRAG_ANY) != 0;
> > +    if (frag && ofproto->up.frag_handling == OFPC_FRAG_NORMAL) {
> > +        /* We must pretend that transport ports are unavailable. */
> >          struct flow ofpc_normal_flow = *flow;
> >          ofpc_normal_flow.tp_src = htons(0);
> >          ofpc_normal_flow.tp_dst = htons(0);
> >          cls_rule = classifier_lookup(cls, &ofpc_normal_flow);
> > +    } else if (frag && ofproto->up.frag_handling == OFPC_FRAG_DROP) {
> > +        cls_rule = &ofproto->drop_frags_rule->up.cr;
> >      } else {
> >          cls_rule = classifier_lookup(cls, flow);
> >      }
> > @@ -8262,6 +8271,9 @@ ofproto_trace(struct ofproto_dpif *ofproto, const struct flow *flow,
> >      } else if (rule == ofproto->no_packet_in_rule) {
> >          ds_put_cstr(ds, "\nNo match, packets dropped because "
> >                      "OFPPC_NO_PACKET_IN is set on in_port.\n");
> > +    } else if (rule == ofproto->drop_frags_rule) {
> > +        ds_put_cstr(ds, "\nPackets dropped because they are IP fragments "
> > +                    "and the fragment handling mode is \"drop\".\n");
> >      }
> >
> >      if (rule) {
> > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> > index ff007cd..7fc896f 100644
> > --- a/tests/ofproto-dpif.at
> > +++ b/tests/ofproto-dpif.at
> > @@ -804,9 +804,14 @@ do
> >    AT_CHECK([ovs-ofctl set-frags br0 $mode])
> >    for type in no first later; do
> >      eval flow=\$${type}_flow exp_output=\$$type
> > +    printf "\n%s\n" "----$mode $type-----"
> >      AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow"], [0], [stdout])
> > -    AT_CHECK_UNQUOTED([tail -1 stdout], [0], [Datapath actions: $exp_output
> > -])
> > +    : > expout
> > +    if test $mode = drop && test $type != no; then
> > +        echo 'Packets dropped because they are IP fragments and the fragment handling mode is "drop".' >> expout
> > +    fi
> > +    echo "Datapath actions: $exp_output" >> expout
> > +    AT_CHECK([grep 'IP fragments' stdout; tail -1 stdout], [0], [expout])
> >    done
> >  done
> >  OVS_VSWITCHD_STOP
> > --
> > 1.7.2.5
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev



More information about the dev mailing list