Nice! I, for one, never understood the TOO_LITTLE stuff ... glad to see it clarified and fixed.
Ethan On Wed, Jan 24, 2018 at 11:40 AM, Ben Pfaff <[email protected]> wrote: > In the OVS architecture, when a datapath doesn't have a match for a packet, > it sends the packet and the flow that it extracted from it to userspace. > Userspace then examines the packet and the flow and compares them. > Commonly, the flow is the same as what userspace expects, given the packet, > but there are two other possibilities: > > - The flow lacks one or more fields that userspace expects to be there, > that is, the datapath doesn't understand or parse them but userspace > does. This is, for example, what would happen if current OVS > userspace, which understands and extracts TCP flags, were to be > paired with an older OVS kernel module, which does not. Internally > OVS uses the name ODP_FIT_TOO_LITTLE for this situation. > > - The flow includes fields that userspace does not know about, that is, > the datapath understands and parses them but userspace does not. > This is, for example, what would happen if an old OVS userspace that > does not understand or extract TCP flags, were to be paired with a > recent OVS kernel module that does. Internally, OVS uses the name > ODP_FIT_TOO_MUCH for this situation. > > The latter is not a big deal and OVS doesn't have to do much to cope with > it. > > The former is more of a problem. When the datapath can't match on all the > fields that OVS supports, it means that OVS can't safely install a flow at > all, other than one that directs packets to the slow path. Otherwise, if > OVS did install a flow, it could match a packet that does not match the > flow that OVS intended to match and could cause the wrong behavior. > > Somehow, this nuance was lost a long time. From about 2013 until today, > it seems that OVS has ignored ODP_FIT_TOO_LITTLE. Instead, it happily > installs a flow regardless of whether the datapath can actually fully match > it. I imagine that this is rarely a problem because most of the time > the datapath and userspace are well matched, but it is still an important > problem to fix. This commit fixes it, by forcing flows into the slow path > when the datapath cannot match specifically enough. > > CC: Ethan Jackson <[email protected]> > Fixes: e79a6c833e0d ("ofproto: Handle flow installation and eviction in > upcall.") > Reported-by: Huanle Han <[email protected]> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2018- > January/343665.html > Signed-off-by: Ben Pfaff <[email protected]> > --- > lib/odp-util.h | 5 +++-- > ofproto/ofproto-dpif-upcall.c | 16 +++++++++++++--- > 2 files changed, 16 insertions(+), 5 deletions(-) > > diff --git a/lib/odp-util.h b/lib/odp-util.h > index 0fcc593d2ace..1fad159db9fb 100644 > --- a/lib/odp-util.h > +++ b/lib/odp-util.h > @@ -37,14 +37,15 @@ struct simap; > struct pkt_metadata; > > #define SLOW_PATH_REASONS \ > - /* These reasons are mutually exclusive. */ \ > SPR(SLOW_CFM, "cfm", "Consists of CFM packets") \ > SPR(SLOW_BFD, "bfd", "Consists of BFD packets") \ > SPR(SLOW_LACP, "lacp", "Consists of LACP packets") \ > SPR(SLOW_STP, "stp", "Consists of STP packets") \ > SPR(SLOW_LLDP, "lldp", "Consists of LLDP packets") \ > SPR(SLOW_ACTION, "action", \ > - "Uses action(s) not supported by datapath") > + "Uses action(s) not supported by datapath") \ > + SPR(SLOW_MATCH, "match", \ > + "Datapath can't match specifically enough") > > /* Indexes for slow-path reasons. Client code uses "enum > slow_path_reason" > * values instead of these, these are just a way to construct those. */ > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index 035233d5adc8..5eb20f7cc236 100644 > --- a/ofproto/ofproto-dpif-upcall.c > +++ b/ofproto/ofproto-dpif-upcall.c > @@ -206,6 +206,7 @@ struct upcall { > * dpif-netdev. If a modification is absolutely necessary, a const > cast > * may be used with other datapaths. */ > const struct flow *flow; /* Parsed representation of the > packet. */ > + enum odp_key_fitness fitness; /* Fitness of 'flow' relative to ODP > key. */ > const ovs_u128 *ufid; /* Unique identifier for 'flow'. */ > unsigned pmd_id; /* Datapath poll mode driver id. */ > const struct dp_packet *packet; /* Packet associated with this > upcall. */ > @@ -787,8 +788,9 @@ recv_upcalls(struct handler *handler) > break; > } > > - if (odp_flow_key_to_flow(dupcall->key, dupcall->key_len, flow) > - == ODP_FIT_ERROR) { > + upcall->fitness = odp_flow_key_to_flow(dupcall->key, > dupcall->key_len, > + flow); > + if (upcall->fitness == ODP_FIT_ERROR) { > goto free_dupcall; > } > > @@ -1174,6 +1176,9 @@ upcall_xlate(struct udpif *udpif, struct upcall > *upcall, > > upcall->xout_initialized = true; > > + if (upcall->fitness == ODP_FIT_TOO_LITTLE) { > + upcall->xout.slow |= SLOW_MATCH; > + } > if (!upcall->xout.slow) { > ofpbuf_use_const(&upcall->put_actions, > odp_actions->data, odp_actions->size); > @@ -2029,10 +2034,12 @@ xlate_key(struct udpif *udpif, const struct nlattr > *key, unsigned int len, > { > struct ofproto_dpif *ofproto; > ofp_port_t ofp_in_port; > + enum odp_key_fitness fitness; > struct xlate_in xin; > int error; > > - if (odp_flow_key_to_flow(key, len, &ctx->flow) == ODP_FIT_ERROR) { > + fitness = odp_flow_key_to_flow(key, len, &ctx->flow); > + if (fitness == ODP_FIT_ERROR) { > return EINVAL; > } > > @@ -2051,6 +2058,9 @@ xlate_key(struct udpif *udpif, const struct nlattr > *key, unsigned int len, > } > xin.xcache = ctx->xcache; > xlate_actions(&xin, &ctx->xout); > + if (fitness == ODP_FIT_TOO_LITTLE) { > + ctx->xout.slow |= SLOW_MATCH; > + } > > return 0; > } > -- > 2.10.2 > > -- Ethan J. Jackson ejj.sh _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
