From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.5 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 95851C282C7 for ; Sat, 26 Jan 2019 15:52:09 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 6BE9721903 for ; Sat, 26 Jan 2019 15:52:09 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726106AbfAZPwI (ORCPT ); Sat, 26 Jan 2019 10:52:08 -0500 Received: from mx1.redhat.com ([209.132.183.28]:59984 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726070AbfAZPwI (ORCPT ); Sat, 26 Jan 2019 10:52:08 -0500 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id CF27D1556B; Sat, 26 Jan 2019 15:52:06 +0000 (UTC) Received: from localhost.localdomain (ovpn-116-20.gru2.redhat.com [10.97.116.20]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 328D81750E; Sat, 26 Jan 2019 15:52:03 +0000 (UTC) Received: by localhost.localdomain (Postfix, from userid 1000) id 5349B180C35; Sat, 26 Jan 2019 13:52:01 -0200 (-02) Date: Sat, 26 Jan 2019 13:52:01 -0200 From: Marcelo Ricardo Leitner To: Simon Horman Cc: Guy Shattah , Aaron Conole , John Hurley , Justin Pettit , Gregory Rose , Eelco Chaudron , Flavio Leitner , Florian Westphal , Jiri Pirko , Rashid Khan , Sushil Kulkarni , Andy Gospodarek , Roi Dayan , Yossi Kuperman , Or Gerlitz , Rony Efraim , "davem@davemloft.net" , netdev@vger.kernel.org Subject: Re: [RFC PATCH 2/6] net/sched: flower: add support for matching on ConnTrack Message-ID: <20190126155201.GI10660@localhost.localdomain> References: <6c976cc538f1f565b74bd2c750639af91a93adc1.1548285996.git.mleitner@redhat.com> <20190125133711.f3caew4d7osr5czg@netronome.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190125133711.f3caew4d7osr5czg@netronome.com> User-Agent: Mutt/1.10.1 (2018-07-13) X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Sat, 26 Jan 2019 15:52:07 +0000 (UTC) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Fri, Jan 25, 2019 at 02:37:13PM +0100, Simon Horman wrote: > Hi Marcelo, > > On Fri, Jan 25, 2019 at 12:32:31AM -0200, Marcelo Ricardo Leitner wrote: > > Hook on flow dissector's new interface on ConnTrack from previous patch. > > > > Signed-off-by: Marcelo Ricardo Leitner > > --- > > include/uapi/linux/pkt_cls.h | 9 +++++++++ > > net/sched/cls_flower.c | 33 +++++++++++++++++++++++++++++++++ > > 2 files changed, 42 insertions(+) > > > > diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h > > index 95d0db2a8350dffb1dd20816591f3b179913fb2e..ba1f3bc01b2fdfd810e37a2b3853a1da1f838acf 100644 > > --- a/include/uapi/linux/pkt_cls.h > > +++ b/include/uapi/linux/pkt_cls.h > > @@ -490,6 +490,15 @@ enum { > > TCA_FLOWER_KEY_PORT_DST_MIN, /* be16 */ > > TCA_FLOWER_KEY_PORT_DST_MAX, /* be16 */ > > > > + TCA_FLOWER_KEY_CT_ZONE, /* u16 */ > > + TCA_FLOWER_KEY_CT_ZONE_MASK, /* u16 */ > > + TCA_FLOWER_KEY_CT_STATE, /* u8 */ > > + TCA_FLOWER_KEY_CT_STATE_MASK, /* u8 */ > > With the corresponding flow dissector patch this API is > exposing the contents of an instance of enum ip_conntrack_info > as an ABI for conntrack state. > > I believe (after getting similar review for my geneve options macthing > patches for flower) that this exposes implementation details as an ABI > to a degree that is not desirable. > > My suggested would be to define, say in the form of named bits, > an ABI, that describes the state information that is exposed. > These bits may not correspond directly to the implementation of > ip_conntrack_info. > > I think there should also be some consideration of if a mask makes > sense for the state as, f.e. in the implementation of enum > ip_conntrack_info not all bit combinations are valid. Right. ct_state must be handled differently. For conntrack it is a linear enum and as we want to be able to OR match, we will have to convert the states in a bitfield as you were saying or so. I don't think the representation above wouldn't change, though: we have 8 bits wrapped under a u8. What would change is how we deal with it. If iproute tc is able to parse the cmdline and set a corresponding bit for each state, the flower-side of flow dissector here should be mostly fine (need to consider the invalid bits as you mentioned, as part of sanity checking). Then just need to change on how flow dissector is reading ct_state from the packet. Is your comment only related to ct_state or other fields too? I'm thinking only ct_state. > > > + TCA_FLOWER_KEY_CT_MARK, /* u32 */ > > + TCA_FLOWER_KEY_CT_MARK_MASK, /* u32 */ > > + TCA_FLOWER_KEY_CT_LABEL, /* 128 bits */ > > + TCA_FLOWER_KEY_CT_LABEL_MASK, /* 128 bits */ > > + > > __TCA_FLOWER_MAX, > > }; > > ... >