On Wed, Oct 08, 2014 at 03:53:02PM -0700, Jarno Rajahalme wrote:
> It is pointless to check special handling for fragments multiple times
> for each packet. As the nw_frags field is not writeable, it suffices
> to check them once, before the first table lookup.
>
> Also, previously a fragment could have been dropped before ingress
> stats were updated. This patch changes the drop to happen after
> ingress stats have been updated.
>
> Signed-off-by: Jarno Rajahalme <[email protected]>
I spent a little time thinking about the following hunk. I think that
at this point, ctx.base_flow and ctx.flow always have the same values
for these fields, so really we're just making sure that they stay in
sync. Can we copy them unconditionally?
> @@ -4229,6 +4228,11 @@ xlate_actions(struct xlate_in *xin, struct xlate_out
> *xout)
> !xin->skip_wildcards ? wc : NULL,
> &rule, ctx.xin->xcache != NULL,
> ctx.xin->resubmit_stats);
> + /* These may have been cleared due to frags handling. */
> + if (!(flow->tp_src | flow->tp_dst)) {
> + ctx.base_flow.tp_src = flow->tp_src;
> + ctx.base_flow.tp_dst = flow->tp_dst;
> + }
> if (ctx.xin->resubmit_stats) {
> rule_dpif_credit_stats(rule, ctx.xin->resubmit_stats);
> }
It also occurs to me that we probably don't test this case. I wonder
what happens if one tries to set the source or dest port for a frag.
Bad things could happen, especially if the datapath doesn't support
masked sets. Maybe we need a different strategy for OF frag handling.
(For that reason I'm a little uncomfortable with this patch at this
point.)
Regarding the following XXX comment in rule_dpif_lookup(), by "earlier"
do you mean in older versions of OVS? At first I read it as "we already
added stats to table 0, these are duplicates" but I think that's wrong.
So, it might be nice to clarify the comment.
> + /* Must be OFPC_FRAG_DROP (we don't have OFPC_FRAG_REASM).
> + * Use the drop_frags_rule (which cannot disappear). */
> + *rule = ofproto->drop_frags_rule;
> +
> + /* XXX: Earlier the stats went to table 0. */
> + if (stats) {
> + struct oftable *tbl = &ofproto->up.tables[TBL_INTERNAL];
> + unsigned long orig;
> +
> + atomic_add_relaxed(&tbl->n_matched, stats->n_packets, &orig);
> + }
> + if (take_ref) {
> + rule_dpif_ref(*rule);
> + }
> + return TBL_INTERNAL;
> + }
> + }
I think that this started out with noticing that the check of
ofproto->up.frag_handling occasionally showed up in profiles. That
makes me wonder whether a simpler fix would be, in
rule_dpif_lookup_in_table(), to check for flow->nw_frag &
FLOW_NW_FRAG_ANY first (because flow->nw_frag is presumably in-cache)
before checking ofproto->up.frag_handling.
struct flow ofpc_normal_flow;
if (ofproto->up.frag_handling != OFPC_FRAG_NX_MATCH) {
/* We always unwildcard dl_type and nw_frag (for IP), so they
* need not be unwildcarded here. */
if (flow->nw_frag & FLOW_NW_FRAG_ANY) {
if (ofproto->up.frag_handling == OFPC_FRAG_NORMAL) {
/* We must pretend that transport ports are unavailable. */
ofpc_normal_flow = *flow;
ofpc_normal_flow.tp_src = htons(0);
ofpc_normal_flow.tp_dst = htons(0);
flow = &ofpc_normal_flow;
} else {
/* Must be OFPC_FRAG_DROP (we don't have OFPC_FRAG_REASM).
* Use the drop_frags_rule (which cannot disappear). */
cls_rule = &ofproto->drop_frags_rule->up.cr;
rule = rule_dpif_cast(rule_from_cls_rule(cls_rule));
if (take_ref) {
rule_dpif_ref(rule);
}
return rule;
}
}
}
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
