From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id A84B0370 for ; Tue, 13 Dec 2016 18:27:36 +0100 (CET) Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga103.fm.intel.com with ESMTP; 13 Dec 2016 09:27:33 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,342,1477983600"; d="scan'208";a="41922100" Received: from irsmsx109.ger.corp.intel.com ([163.33.3.23]) by orsmga005.jf.intel.com with ESMTP; 13 Dec 2016 09:27:32 -0800 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.43]) by IRSMSX109.ger.corp.intel.com ([169.254.13.158]) with mapi id 14.03.0248.002; Tue, 13 Dec 2016 17:27:31 +0000 From: "Ananyev, Konstantin" To: Michal Miroslaw CC: "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH 04/13] acl: allow zero verdict Thread-Index: AQHSVN2ahB5rgtyRpkyle4teQhaCOKEFry9QgAA3/4CAAANbwIAADPcAgAADYICAABNDgIAAED0Q Date: Tue, 13 Dec 2016 17:27:31 +0000 Message-ID: <2601191342CEEE43887BDE71AB9772583F0E7567@irsmsx105.ger.corp.intel.com> References: <2601191342CEEE43887BDE71AB9772583F0E6E0B@irsmsx105.ger.corp.intel.com> <20161213135443.ovmlunbh67dr4tew@rere.qmqm.pl> <2601191342CEEE43887BDE71AB9772583F0E7008@irsmsx105.ger.corp.intel.com> <20161213145308.lqqnm6ivryjfxih7@rere.qmqm.pl> <2601191342CEEE43887BDE71AB9772583F0E736D@irsmsx105.ger.corp.intel.com> <20161213161409.ekbagsze5pcy2ppl@rere.qmqm.pl> In-Reply-To: <20161213161409.ekbagsze5pcy2ppl@rere.qmqm.pl> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="iso-8859-2" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH 04/13] acl: allow zero verdict X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 13 Dec 2016 17:27:37 -0000 > -----Original Message----- > From: Michal Miroslaw [mailto:mirq-linux@rere.qmqm.pl] > Sent: Tuesday, December 13, 2016 4:14 PM > To: Ananyev, Konstantin > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH 04/13] acl: allow zero verdict >=20 > On Tue, Dec 13, 2016 at 03:13:42PM +0000, Ananyev, Konstantin wrote: > [...] > > > > > > > Subject: [dpdk-dev] [PATCH 04/13] acl: allow zero verdict > > > > > > > > > > > > > > Signed-off-by: Micha=B3 Miros=B3aw > > > > > > > --- > > > > > > > lib/librte_acl/rte_acl.c | 3 +-- > > > > > > > lib/librte_acl/rte_acl.h | 2 -- > > > > > > > lib/librte_table/rte_table_acl.c | 2 +- > > > > > > > 3 files changed, 2 insertions(+), 5 deletions(-) > > > > > > > > > > > > > > diff --git a/lib/librte_acl/rte_acl.c b/lib/librte_acl/rte_ac= l.c > > > > > > > index 8b7e92c..d1f40be 100644 > > > > > > > --- a/lib/librte_acl/rte_acl.c > > > > > > > +++ b/lib/librte_acl/rte_acl.c > > > > > > > @@ -313,8 +313,7 @@ acl_check_rule(const struct rte_acl_rule_= data *rd) > > > > > > > if ((RTE_LEN2MASK(RTE_ACL_MAX_CATEGORIES, typeof(rd->catego= ry_mask)) & > > > > > > > rd->category_mask) =3D=3D 0 || > > > > > > > rd->priority > RTE_ACL_MAX_PRIORITY || > > > > > > > - rd->priority < RTE_ACL_MIN_PRIORITY || > > > > > > > - rd->userdata =3D=3D RTE_ACL_INVALID_USERDATA) > > > > > > > + rd->priority < RTE_ACL_MIN_PRIORITY) > > > > > > > return -EINVAL; > > > > > > > return 0; > > > > > > > } > > > > > > > > > > > > I am not sure, how it supposed to work properly? > > > > > > Zero value is reserved and ifnicates that no match were found f= or that input. > > > > > > > > > > This is actually in use by us. In our use we don't need to differ= entiate > > > > > matching a rule with zero verdict vs not matching a rule at all. = I also > > > > > have a patch that changes the value returned in non-matching case= , but > > > > > it's in "dirty hack" state, as of yet. > > > > > > > > With that chane rte_acl_classify() might produce invalid results. > > > > Even if you don't need it (I still don't understand how) , it doesn= 't mean other people > > > > don't need it either and it is ok to change it. > > > > > > > > > > > > > > The ACL code does not treat zero userdata specially, so this is o= nly > > > > > a policy choice and as such would be better to be made by the use= r. > > > > > > > > I believe it does. > > > > userdata=3D=3D0 is a reserved value. > > > > When rte_acl_clasify() returns 0 for that particular input, it mean= s 'no matches were found'. > > > > > > Dear Konstantin, > > > > > > Can you describe how the ACL code treats zero specially? I could not = find > > > anything, really. The only thing I found is that iff I use zero userd= ata > > > in a rule I won't be able to differentiate a case where it matched fr= om > > > a case where no rule matched. > > > > Yes, that's what I am talking about. > > > > > If I all my rules have non-zero userdata, > > > then this patch changes nothing. > > > > Ok, then why do you remove a code that does checking for invalid userda= ta=3D=3D0? > > That supposed to prevent user to setup invalid value by mistake. > > > > But if I have a table where 0 means drop > > > (default-drop policy) then being able to use zero userdata in DROP ru= les > > > makes the ACLs just that more useful. > > > > Ok, and what prevents you from do +1 to your policy values before > > you insert it into the ACL table and -1 after you retrieved it via rte_= acl_classify()? >=20 > The check is enforcing an assumption that all users want to distinguish > the cases whether any rule matched and whether no rules matched. Not all > users do, hence the assumption is invalid and this patch removes it. The check is based on the assumption that users might need to distinguish the situation when no rules were matched. To support that we need a reserved userdata value, which would mean NO_MATCH. >>From what I heard, most users do need this ability, those who don't can easily overcome it. >=20 > Yes, people can work around it by loosing 1 of 2^32 useful values and > convoluting their code. Yes, one of 2^32 values is reserved. Any reason why (2^32 - 1) values might not be enough? >=20 > You seem to argue that 0 is somehow an invalid value, but I can't find > anything in the ACL that would require it to be so. Could you point me > to the code in DPDK where this actually matters? It was a while, when I looked into ACL code in details, but as remember that's the only reason: we need some value to be reserved as NO_MATCH. Let say in build_trie() we set results to zero for rules with unused catego= ries: for (m =3D context->cfg.num_categories; 0 !=3D m--; ) { if (rule->f->data.category_mask & (1 << m)) { end->mrt->results[m] =3D rule->f->data.user= data; end->mrt->priority[m] =3D rule->f->data.pri= ority; } else { end->mrt->results[m] =3D 0; end->mrt->priority[m] =3D 0; } } Konstantin