From patchwork Fri Mar 3 21:44:06 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Joe Stringer X-Patchwork-Id: 735244 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 3vZjPz697Kz9s3v for ; Sat, 4 Mar 2017 08:44:39 +1100 (AEDT) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id DA1F1F3B; Fri, 3 Mar 2017 21:44:15 +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 30A2EF29 for ; Fri, 3 Mar 2017 21:44:14 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net [217.70.183.198]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 4B3151B8 for ; Fri, 3 Mar 2017 21:44:12 +0000 (UTC) Received: from mfilter20-d.gandi.net (mfilter20-d.gandi.net [217.70.178.148]) by relay6-d.mail.gandi.net (Postfix) with ESMTP id D7570FB87E for ; Fri, 3 Mar 2017 22:44:10 +0100 (CET) X-Virus-Scanned: Debian amavisd-new at mfilter20-d.gandi.net Received: from relay6-d.mail.gandi.net ([IPv6:::ffff:217.70.183.198]) by mfilter20-d.gandi.net (mfilter20-d.gandi.net [::ffff:10.0.15.180]) (amavisd-new, port 10024) with ESMTP id hA0mO7B60ntk for ; Fri, 3 Mar 2017 22:44:08 +0100 (CET) X-Originating-IP: 209.85.220.172 Received: from mail-qk0-f172.google.com (mail-qk0-f172.google.com [209.85.220.172]) (Authenticated sender: joe@ovn.org) by relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 320F4FB87D for ; Fri, 3 Mar 2017 22:44:08 +0100 (CET) Received: by mail-qk0-f172.google.com with SMTP id n127so198394600qkf.0 for ; Fri, 03 Mar 2017 13:44:08 -0800 (PST) X-Gm-Message-State: AMke39mSG/Q5y9jSPiApNqCsOz6IretuYwuxWnkaV9Ohk8b/uAQx5YhMTIcWvbUavJX8vmwJLvJizo16lCQ/RA== X-Received: by 10.55.188.66 with SMTP id m63mr4958531qkf.278.1488577447018; Fri, 03 Mar 2017 13:44:07 -0800 (PST) MIME-Version: 1.0 Received: by 10.12.163.229 with HTTP; Fri, 3 Mar 2017 13:44:06 -0800 (PST) Received: by 10.12.163.229 with HTTP; Fri, 3 Mar 2017 13:44:06 -0800 (PST) In-Reply-To: <9BC052EF-974F-4CE8-9437-CD6114A9664B@ovn.org> References: <1488331058-40038-1-git-send-email-jarno@ovn.org> <1488331058-40038-10-git-send-email-jarno@ovn.org> <9BC052EF-974F-4CE8-9437-CD6114A9664B@ovn.org> From: Joe Stringer Date: Fri, 3 Mar 2017 13:44:06 -0800 X-Gmail-Original-Message-ID: Message-ID: To: Jarno Rajahalme X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,HTML_MESSAGE, 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 X-Content-Filtered-By: Mailman/MimeDel 2.1.12 Cc: ovs dev Subject: Re: [ovs-dev] [PATCH v2 09/22] datapath: Refactor labels initialization. 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 3/03/2017 10:37, "Jarno Rajahalme" wrote: On Mar 2, 2017, at 5:26 PM, Joe Stringer wrote: On 28 February 2017 at 17:17, Jarno Rajahalme wrote: Upstream commit: Refactoring conntrack labels initialization makes changes in later patches easier to review. Signed-off-by: Jarno Rajahalme Acked-by: Pravin B Shelar Acked-by: Joe Stringer Signed-off-by: David S. Miller Signed-off-by: Jarno Rajahalme --- datapath/conntrack.c | 113 ++++++++++++++++++++++++++++++ --------------------- 1 file changed, 66 insertions(+), 47 deletions(-) if (ct->mark != new_mark) { ct->mark = new_mark; @@ -270,56 +263,71 @@ static int ovs_ct_set_mark(struct sk_buff *skb, struct sw_flow_key *key, #endif } -static int ovs_ct_set_labels(struct sk_buff *skb, struct sw_flow_key *key, - const struct ovs_key_ct_labels *labels, - const struct ovs_key_ct_labels *mask) +static struct nf_conn_labels *ovs_ct_get_conn_labels(struct nf_conn *ct) { - enum ip_conntrack_info ctinfo; struct nf_conn_labels *cl; - struct nf_conn *ct; - - /* The connection could be invalid, in which case set_label is no-op.*/ - ct = nf_ct_get(skb, &ctinfo); - if (!ct) - return 0; cl = nf_ct_labels_find(ct); if (!cl) { nf_ct_labels_ext_add(ct); cl = nf_ct_labels_find(ct); } + if (cl && ovs_ct_get_labels_len(cl) < OVS_CT_LABELS_LEN) + return NULL; The above two lines were not introduced in the upstream code. Do you intend to introduce them? Should have mentioned this in a commit message or in a comment. The inclusion of this test is intentional, and the rationale is that it might be possible that the kernel is configured with too little space for labels. However, it is possible that the way OVS kernel module initializes the number of words in labels for older kernels already takes care of this, do you have a take on this? When we compile the out of tree module for a particular kernel, this information should be available. I don't think that we try to support compiling against one kernel with one definition of the labels length, then allow that same module to run on another kernel with a different definition. So it should be fine to omit so long as there are still the compile time checks. Jarno For my current working tree for review/build/test, I will drop these lines. + return cl; +} + +/* Initialize labels for a new, yet to be committed conntrack entry. Note that + * since the new connection is not yet confirmed, and thus no-one else has + * access to it's labels, we simply write them over. + */ +static int ovs_ct_init_labels(struct nf_conn *ct, struct sw_flow_key *key, + const struct ovs_key_ct_labels *labels, + const struct ovs_key_ct_labels *mask) +{ + struct nf_conn_labels *cl; + u32 *dst; + int i; - if (!cl || ovs_ct_get_labels_len(cl) < OVS_CT_LABELS_LEN) + cl = ovs_ct_get_conn_labels(ct); + if (!cl) return -ENOSPC; - if (nf_ct_is_confirmed(ct)) { - /* Triggers a change event, which makes sense only for - * confirmed connections. - */ - int err = nf_connlabels_replace(ct, labels->ct_labels_32, - mask->ct_labels_32, - OVS_CT_LABELS_LEN_32); - if (err) - return err; - } else { - u32 *dst = (u32 *)cl->bits; - const u32 *msk = mask->ct_labels_32; - const u32 *lbl = labels->ct_labels_32; - int i; + dst = (u32 *)cl->bits; + for (i = 0; i < OVS_CT_LABELS_LEN_32; i++) + dst[i] = (dst[i] & ~mask->ct_labels_32[i]) | + (labels->ct_labels_32[i] & mask->ct_labels_32[i]); - /* No-one else has access to the non-confirmed entry, copy - * labels over, keeping any bits we are not explicitly setting. - */ - for (i = 0; i < OVS_CT_LABELS_LEN_32; i++) - dst[i] = (dst[i] & ~msk[i]) | (lbl[i] & msk[i]); + /* Labels are included in the IPCTNL_MSG_CT_NEW event only if the + * IPCT_LABEL bit it set in the event cache. + */ + nf_conntrack_event_cache(IPCT_LABEL, ct); - /* Labels are included in the IPCTNL_MSG_CT_NEW event only if - * the IPCT_LABEL bit it set in the event cache. - */ - nf_conntrack_event_cache(IPCT_LABEL, ct); - } + memcpy(&key->ct.labels, cl->bits, OVS_CT_LABELS_LEN); + + return 0; +} + +static int ovs_ct_set_labels(struct nf_conn *ct, struct sw_flow_key *key, + const struct ovs_key_ct_labels *labels, + const struct ovs_key_ct_labels *mask) +{ + struct nf_conn_labels *cl; + int err; + + cl = ovs_ct_get_conn_labels(ct); + if (!cl) + return -ENOSPC; + + err = nf_connlabels_replace(ct, labels->ct_labels_32, + mask->ct_labels_32, + OVS_CT_LABELS_LEN_32); + if (err) + return err; + + memcpy(&key->ct.labels, cl->bits, OVS_CT_LABELS_LEN); - ovs_ct_get_labels(ct, &key->ct.labels); return 0; } @@ -926,25 +934,36 @@ static int ovs_ct_commit(struct net *net, struct sw_flow_key *key, const struct ovs_conntrack_info *info, struct sk_buff *skb) { + enum ip_conntrack_info ctinfo; + struct nf_conn *ct; int err; err = __ovs_ct_lookup(net, key, info, skb); if (err) return err; + /* The connection could be invalid, in which case this is a no-op.*/ + ct = nf_ct_get(skb, &ctinfo); + if (!ct) + return 0; + /* Apply changes before confirming the connection so that the initial * conntrack NEW netlink event carries the values given in the CT * action. */ if (info->mark.mask) { - err = ovs_ct_set_mark(skb, key, info->mark.value, + err = ovs_ct_set_mark(ct, key, info->mark.value, info->mark.mask); if (err) return err; } if (labels_nonzero(&info->labels.mask)) { - err = ovs_ct_set_labels(skb, key, &info->labels.value, - &info->labels.mask); + if (!nf_ct_is_confirmed(ct)) + err = ovs_ct_init_labels(ct, key, &info->labels.value, + &info->labels.mask); + else + err = ovs_ct_set_labels(ct, key, &info->labels.value, + &info->labels.mask); if (err) return err; } -- 2.1.4 diff --git a/datapath/conntrack.c b/datapath/conntrack.c index dacf34c..adc4315 100644 --- a/datapath/conntrack.c +++ b/datapath/conntrack.c @@ -243,19 +243,12 @@ int ovs_ct_put_key(const struct sw_flow_key *key, struct sk_buff *skb) return 0; } -static int ovs_ct_set_mark(struct sk_buff *skb, struct sw_flow_key *key, +static int ovs_ct_set_mark(struct nf_conn *ct, struct sw_flow_key *key, u32 ct_mark, u32 mask) { #if IS_ENABLED(CONFIG_NF_CONNTRACK_MARK) - enum ip_conntrack_info ctinfo; - struct nf_conn *ct; u32 new_mark; - /* The connection could be invalid, in which case set_mark is no-op. */ - ct = nf_ct_get(skb, &ctinfo); - if (!ct) - return 0; - new_mark = ct_mark | (ct->mark & ~(mask));