Eric, I verified this one, ptap unit tests and nsh tests passed without any change, so it seems it is better than the original one you did.
-----Original Message----- From: Eric Garver [mailto:[email protected]] Sent: Friday, July 7, 2017 3:44 AM To: Zoltán Balogh <[email protected]> Cc: Yang, Yi Y <[email protected]>; '[email protected]' <[email protected]> Subject: Re: [ovs-dev] [PATCH v2] dpif-netlink: convert packet_type netlink attribute On Thu, Jul 06, 2017 at 01:14:31PM +0000, Zoltán Balogh wrote: > Hi Eric, > > Thank you for clarifying. > > ... > [OVS_FLOW_ATTR_KEY] > ... > [OVS_KEY_ATTR_IPV4] = ... > ... > [OVS_KEY_ATTR_ETHERTYPE] = ... <--- the one inserted > > I have not found any API that would extend a nested attribute. > Maybe I'm wrong, but I thought 'buf' holds a single nested attribute. If this > is true, isn't possible to add the ethertype to it and increase its size? I gave it a try and realized it doesn't matter. During the upcall we pass back the same key that the kernel gave us, with the exception that we also fill in wildcard info. See upcall_xlate() and ukey_create_from_upcall(). This is why my original patch works. It caused odp_flow_key_from_mask() to fill the ETHERTYPE for the mask. So I think this can only be solved in one of two places: 1) odp_flow_key_from_mask(), as my original patch did. - This sets the wildcard for all dpifs, including userspace/netdev. 2) xlate_wc_init() and xlate_wc_finish(). - Here we can limit by dpif type. Neither which is particularly pleasing. I gave #2 a try. see patch below. It works for both check-kernel and check-system-userspace. It does not force eth_type() for userspace. What do you think? Eric. --->8--- diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 1f4fe1dd6725..f3074c78c4b2 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -6129,7 +6129,10 @@ xlate_wc_init(struct xlate_ctx *ctx) /* Some fields we consider to always be examined. */ WC_MASK_FIELD(ctx->wc, packet_type); WC_MASK_FIELD(ctx->wc, in_port); - if (is_ethernet(&ctx->xin->flow, NULL)) { + if (is_ethernet(&ctx->xin->flow, NULL) + || (pt_ns(ctx->xin->flow.packet_type) == OFPHTN_ETHERTYPE + && !strcmp(dpif_normalize_type(dpif_type(ctx->xbridge->dpif)), + "system"))) { WC_MASK_FIELD(ctx->wc, dl_type); } if (is_ip_any(&ctx->xin->flow)) { @@ -6163,7 +6166,12 @@ xlate_wc_finish(struct xlate_ctx *ctx) if (ctx->xin->upcall_flow->packet_type != htonl(PT_ETH)) { ctx->wc->masks.dl_dst = eth_addr_zero; ctx->wc->masks.dl_src = eth_addr_zero; - ctx->wc->masks.dl_type = 0; + /* Kernel uses ETHERTYPE to implicitly mean packet_type(1, x) */ + if (pt_ns(ctx->xin->upcall_flow->packet_type) != OFPHTN_ETHERTYPE + || strcmp(dpif_normalize_type(dpif_type(ctx->xbridge->dpif)), + "system")) { + ctx->wc->masks.dl_type = 0; + } } /* ICMPv4 and ICMPv6 have 8-bit "type" and "code" fields. struct flow _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
