Hi Eric, Doing the kernel datapath tweak in the ofproto translation logic certainly works but should only be our last resort as it is so ugly.
Zoltan will send you an corrected/simplified version of our patch in the next hour or so. It intercepts the flow and mask netlink keys before transmission to the kernel to remove packet_type and insert ether_type if needed. We believe this should do the trick. Please check if it works and would be OK for inclusion. Thanks, Jan > -----Original Message----- > From: ovs-dev-boun...@openvswitch.org > [mailto:ovs-dev-boun...@openvswitch.org] On Behalf Of Eric Garver > Sent: Friday, 07 July, 2017 14:44 > To: Yang, Yi Y <yi.y.y...@intel.com> > Cc: 'd...@openvswitch.org' <d...@openvswitch.org> > Subject: Re: [ovs-dev] [PATCH v2] dpif-netlink: convert packet_type netlink > attribute > > 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 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
