Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752542AbbHUAkd (ORCPT ); Thu, 20 Aug 2015 20:40:33 -0400 Received: from mail-yk0-f170.google.com ([209.85.160.170]:35913 "EHLO mail-yk0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751570AbbHUAkb (ORCPT ); Thu, 20 Aug 2015 20:40:31 -0400 MIME-Version: 1.0 In-Reply-To: References: <1439941188-19293-1-git-send-email-joestringer@nicira.com> <1439941188-19293-10-git-send-email-joestringer@nicira.com> From: Joe Stringer Date: Thu, 20 Aug 2015 17:40:10 -0700 Message-ID: Subject: Re: [PATCHv4 net-next 09/10] openvswitch: Allow matching on conntrack label To: Pravin Shelar Cc: netdev , LKML , pablo , Florian Westphal , Hannes Sowa , Thomas Graf , Justin Pettit , Jesse Gross Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3705 Lines: 77 On 20 August 2015 at 14:01, Pravin Shelar wrote: > On Thu, Aug 20, 2015 at 12:13 PM, Joe Stringer wrote: >> On 20 August 2015 at 08:45, Pravin Shelar wrote: >>> On Wed, Aug 19, 2015 at 4:04 PM, Joe Stringer wrote: >>>> Thanks for the review, >>>> >>>> On 19 August 2015 at 14:24, Pravin Shelar wrote: >>>>> On Tue, Aug 18, 2015 at 4:39 PM, Joe Stringer wrote: >>>>>> Allow matching and setting the conntrack label field. As with ct_mark, >>>>>> this is populated by executing the CT action, and is a writable field. >>>>>> Specifying a label and optional mask allows the label to be modified, >>>>>> which takes effect on the entry found by the lookup of the CT action. >>>>>> >>>>>> E.g.: actions:ct(zone=1,label=1) >>>>>> >>>>>> This will perform conntrack lookup in zone 1, then modify the label for >>>>>> that entry. The conntrack entry itself must be committed using the >>>>>> "commit" flag in the conntrack action flags for this change to persist. >>>>>> >>> >>>> >>>>>> return false; >>>>>> } >>>>>> @@ -508,8 +601,12 @@ void ovs_ct_free_action(const struct nlattr *a) >>>>>> >>>>>> void ovs_ct_init(struct net *net, struct ovs_ct_perdp_data *data) >>>>>> { >>>>>> + unsigned int n_bits = sizeof(struct ovs_key_ct_label) * BITS_PER_BYTE; >>>>>> + >>>>>> data->xt_v4 = !nf_ct_l3proto_try_module_get(PF_INET); >>>>>> data->xt_v6 = !nf_ct_l3proto_try_module_get(PF_INET6); >>>>>> + if (nf_connlabels_get(net, n_bits); >>>>>> + OVS_NLERR(true, "Failed to set connlabel length"); >>>>>> } >>>>>> >>>>> In case of error should we reject conntrack label actions? Otherwise >>>>> user will never see any error. But action could drop packets. >>>> >>>> I suspect that currently errors would be seen from ovs_ct_set_label(): >>>> >>>>>.......if (!cl || cl->words * sizeof(long) < OVS_CT_LABEL_LEN) >>>>>.......>.......return -ENOSPC; >>>> >>>> So, for cmd_execute, userspace would see this. For regular handling, >>>> pipeline processing would stop (so, drop). >>>> >>>> However, I agree it would be more friendly to have the attribute >>>> rejected up-front. Just means we'll pass the datapath all the way >>>> down: >>>> ovs_nla_get_match() >>>> --> ovs_key_from_nlattrs() >>>> --> metadata_from_nlattrs() >>>> --> ovs_ct_verify() >> >> Incidentally, we generally don't have the datapath by this point >> (ovs_nla_get_match()). There'd need to be a bit of rearranging in the >> ovs_flow_cmd_* functions, which would include holding the locks for >> longer. Given that the two most common cases are that either A) The >> kernel is configured with connlabel support, and built with support >> for at least 128 bits of label, or B) the kernel is configured without >> connlabel, and this is handled already in ovs_ct_verify(), I don't >> think it's worth making this particular change. > > Actually I do not see need for this to be per datapath property. > infact there is no need to have struct ovs_ct_perdp_data. > ovs_ct_init can be called from ovs-module init. > nf-connlabel bit length is per net-namespace property. So > nf_connlabels_get() should be called from ovs namespace init. This > way you can move xt_label flag to ovs_net. ovs_net can be accessed > from ovs_flow_cmd_* functions. That sounds like a tidier approach, I'll roll it into the next version. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/