From patchwork Fri Mar 10 19:06:13 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Joe Stringer X-Patchwork-Id: 737542 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3vfxZV73L8z9s7K for ; Sat, 11 Mar 2017 06:06:42 +1100 (AEDT) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 71ED8B2E; Fri, 10 Mar 2017 19:06:40 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id 200D5904 for ; Fri, 10 Mar 2017 19:06:39 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net [217.70.183.197]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 8B13416E for ; Fri, 10 Mar 2017 19:06:38 +0000 (UTC) Received: from mfilter19-d.gandi.net (mfilter19-d.gandi.net [217.70.178.147]) by relay5-d.mail.gandi.net (Postfix) with ESMTP id 5499341C087 for ; Fri, 10 Mar 2017 20:06:37 +0100 (CET) X-Virus-Scanned: Debian amavisd-new at mfilter19-d.gandi.net Received: from relay5-d.mail.gandi.net ([IPv6:::ffff:217.70.183.197]) by mfilter19-d.gandi.net (mfilter19-d.gandi.net [::ffff:10.0.15.180]) (amavisd-new, port 10024) with ESMTP id 7C-zAxH6IrC0 for ; Fri, 10 Mar 2017 20:06:35 +0100 (CET) X-Originating-IP: 209.85.220.177 Received: from mail-qk0-f177.google.com (mail-qk0-f177.google.com [209.85.220.177]) (Authenticated sender: joe@ovn.org) by relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 8DAD741C08D for ; Fri, 10 Mar 2017 20:06:35 +0100 (CET) Received: by mail-qk0-f177.google.com with SMTP id v125so178278278qkh.2 for ; Fri, 10 Mar 2017 11:06:35 -0800 (PST) X-Gm-Message-State: AMke39lNjG5cNpB4pM+JS4i+pvf8U5RrMTmszmI/MbaghXyyvcVlQZ2hQc186MINBrODIqyITenZ9tYfMdzS/A== X-Received: by 10.237.49.36 with SMTP id 33mr20995170qtg.117.1489172794244; Fri, 10 Mar 2017 11:06:34 -0800 (PST) MIME-Version: 1.0 Received: by 10.12.163.229 with HTTP; Fri, 10 Mar 2017 11:06:13 -0800 (PST) In-Reply-To: <1489083889-64842-1-git-send-email-yihung.wei@gmail.com> References: <1489083889-64842-1-git-send-email-yihung.wei@gmail.com> From: Joe Stringer Date: Fri, 10 Mar 2017 11:06:13 -0800 X-Gmail-Original-Message-ID: Message-ID: To: Yi-Hung Wei X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW, RCVD_IN_SORBS_SPAM autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Cc: ovs dev Subject: Re: [ovs-dev] [PATCH v2 1/2] nx-match: Use vl_mff_map to parse match field. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org On 9 March 2017 at 10:24, Yi-Hung Wei wrote: > vl_mff_map is introduced in commit 04f48a68c428 ("ofp-actions: Fix variable > length meta-flow OXMs") to account variable length mf_field, and it is used > to decode variable length mf_field in ofp_action. In this patch, vl_mff_map > is further used to decode the variable length match field as well. > > Signed-off-by: Yi-Hung Wei Thanks for v2, I had a bit of trouble applying to master but I've reviewed below anyway. I think that Jarno's changes yesterday conflict, so you'll need to rebase before submitting the next version. > @@ -594,16 +598,21 @@ nx_pull_match__(struct ofpbuf *b, unsigned int match_len, bool strict, > * are valid pointers, then stores the cookie and mask in them if 'b' contains > * a "NXM_NX_COOKIE*" match. Otherwise, stores 0 in both. > * > + * 'vl_mff_map" is an optional parameter that is used to derive variable length > + * mf_fields in flow match. If it is not provided, the default mf_fields with > + * maximum length will be used. > + * Would the below be more accurate and informative? Or is it too specific? 'vl_mff_map' is an optional parameter that is used to validate the length of variable length mf_fields in 'match'. If it is not provided, the default mf_fields with maximum length will be used. I guess there's a couple of places where this comment exists that would benefit from this. > * Fails with an error upon encountering an unknown NXM header. > * > * Returns 0 if successful, otherwise an OpenFlow error code. */ > enum ofperr > nx_pull_match(struct ofpbuf *b, unsigned int match_len, struct match *match, > ovs_be64 *cookie, ovs_be64 *cookie_mask, > - const struct tun_table *tun_table) > + const struct tun_table *tun_table, > + const struct vl_mff_map *vl_mff_map) > { > return nx_pull_match__(b, match_len, true, match, cookie, cookie_mask, > - tun_table); > + tun_table, vl_mff_map); > } > > /* Behaves the same as nx_pull_match(), but skips over unknown NXM headers, > @@ -612,15 +621,17 @@ enum ofperr > nx_pull_match_loose(struct ofpbuf *b, unsigned int match_len, > struct match *match, > ovs_be64 *cookie, ovs_be64 *cookie_mask, > - const struct tun_table *tun_table) > + const struct tun_table *tun_table, > + const struct vl_mff_map *vl_mff_map) > { > return nx_pull_match__(b, match_len, false, match, cookie, cookie_mask, > - tun_table); > + tun_table, vl_mff_map); > } Now that I look at the comment above these *_loose() functions, I wonder - should the vl_mff_map ever be specified for these versions? If I follow correctly, these loose functions are useful for decoupling controller and switch support for fields. For example, if the switch sends up something in a slightly different format than what the controller expects, the controller is able to basically skip over the unknown bits but still process the rest of the message. One case might be a packet_in message from the switch to the controller. The switch would provide packet and its view of the parsed packet to the controller, but the controller doesn't have to work with the same knowledge of all packet fields as what the switch understands. Should OXM lengths be allowed to be flexible when decoding in a controller? Well, I guess that the controller should be responsible for ensuring that it configures the TLV map correctly ahead of time.. so if the controller already has this knowledge, then it's not unreasonable for it to keep its own view of the TLV maps in sync with the switch. On the flip side, we enforce that the controller must maintain the same vll mff map as what the switch has. I discussed this briefly with Jarno (thanks!), and figured it would be easy enough to try this out.. } When I ran the OVS testsuite with this applied, all the tests ran successfully so it seems to me that it would be safe not to add the extra vl_mff_map argument to these *_loose() functions. It would be good if you could later on submit a separate patch to remove tun_table argument from these as well. diff --git a/lib/nx-match.c b/lib/nx-match.c index 1a0f0de594e7..f1e020a766c6 100644 --- a/lib/nx-match.c +++ b/lib/nx-match.c @@ -624,6 +624,8 @@ nx_pull_match_loose(struct ofpbuf *b, unsigned int match_len, const struct tun_table *tun_table, const struct vl_mff_map *vl_mff_map) { + ovs_assert(!tun_table); + ovs_assert(!vl_mff_map); return nx_pull_match__(b, match_len, false, match, cookie, cookie_mask, tun_table, vl_mff_map);