On Fri, Jul 07, 2017 at 02:16:12AM +0000, Yang, Yi Y wrote: > 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.
Thanks for verifying. I'll post it along with my layer3-rtnetlink series. Hopefully in the next few hours. I'm NACKing this patch by Zoltan. It's turned out to be more complex than anticipated and I don't think it'll solve the real problem. > > -----Original Message----- > From: Eric Garver [mailto:e...@erig.me] > Sent: Friday, July 7, 2017 3:44 AM > To: Zoltán Balogh <zoltan.bal...@ericsson.com> > Cc: Yang, Yi Y <yi.y.y...@intel.com>; 'd...@openvswitch.org' > <d...@openvswitch.org> > 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 > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
