[ovs-dev] [PATCH v2 4/5] ofproto-dpif-upcall: Slow path flows that datapath can't fully match.
Ethan J. Jackson
ejj at eecs.berkeley.edu
Wed Jan 24 22:24:50 UTC 2018
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 <blp at ovn.org> 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 <ejj at eecs.berkeley.edu>
> Fixes: e79a6c833e0d ("ofproto: Handle flow installation and eviction in
> upcall.")
> Reported-by: Huanle Han <hanxueluo at gmail.com>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2018-
> January/343665.html
> Signed-off-by: Ben Pfaff <blp at ovn.org>
> ---
> 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
More information about the dev
mailing list