From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id F3004FE5 for ; Wed, 22 Feb 2017 14:41:15 +0100 (CET) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 22 Feb 2017 05:41:14 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.35,194,1484035200"; d="scan'208";a="936835597" Received: from irsmsx106.ger.corp.intel.com ([163.33.3.31]) by orsmga003.jf.intel.com with ESMTP; 22 Feb 2017 05:41:12 -0800 Received: from irsmsx103.ger.corp.intel.com ([169.254.3.77]) by IRSMSX106.ger.corp.intel.com ([169.254.8.197]) with mapi id 14.03.0248.002; Wed, 22 Feb 2017 13:41:12 +0000 From: "Richardson, Bruce" To: "dev@dpdk.org" , chunguang yang Thread-Topic: [dpdk-dev] [PATCH] lpm: rte_lpm_iterate() - iterate through the routes Thread-Index: AdKNERdHcq8LeBNJQP+J5cFFPFB0GAAABU3g Date: Wed, 22 Feb 2017 13:41:11 +0000 Message-ID: <59AF69C657FD0841A61C55336867B5B035B9C0E4@IRSMSX103.ger.corp.intel.com> References: <1480055491-137021-1-git-send-email-chunguang.yang@windriver.com> <20170222133929.GA15184@bricha3-MOBL3.ger.corp.intel.com> In-Reply-To: <20170222133929.GA15184@bricha3-MOBL3.ger.corp.intel.com> Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiM2E0OWU4ZWYtYjlkYi00YjNiLTg3ZjktZDViMzVlZDg0NjAzIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjIuMTEuMCIsIlRydXN0ZWRMYWJlbEhhc2giOiIyUjhOWkhycWl2V0hGZ3VOOTZuMlJKSzlRdHA4cmU1b0twRm9iN2ZycGVRPSJ9 x-ctpclassification: CTP_IC x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] lpm: rte_lpm_iterate() - iterate through the routes 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: Wed, 22 Feb 2017 13:41:16 -0000 CC: dev@dpdk.org. Missed that address when pulling from email archive. > -----Original Message----- > From: Bruce Richardson [mailto:bruce.richardson@intel.com] > Sent: Wednesday, February 22, 2017 1:39 PM > To: chunguang yang > Subject: Re: [dpdk-dev] [PATCH] lpm: rte_lpm_iterate() - iterate through > the routes >=20 > On Fri, Nov 25, 2016 at 01:31:31AM -0500, chunguang yang wrote: > > From: J?rgen Grahn > > > > In practice, there's a need to iterate through the entries of a > > rte_lpm, apart from the usual insert/delete/lookup operations. This > > is useful for debugging and perhaps for things like removing all > > entries referencing a certain nexthop. > > > > This patch implements this through rte_lpm_iterate(), which uses a > > cursor (or iterator) to keep track of the current position. Client > > code doesn't need to be aware of rte_lpm implementation details. > > > > Change-Id: I28ea3d7d92f318988444553ee2bb30b709bcb3b6 > > Signed-off-by: Jorgen Grahn > > Signed-off-by: alloc >=20 > Apologies for the late review, I missed this patch at the time and only > spotted it in patchwork now. >=20 > First off, there are a number of checkpatch issues flagged by the > automated scan. If you still want to continue with this patch for 17.05 > release, you should resubmit with those fixed. Other review comments > inline below too. >=20 > /Bruce >=20 > > --- > > lib/librte_lpm/Makefile | 4 +- > > lib/librte_lpm/rte_lpm_iterate.c | 81 > > ++++++++++++++++++++++++++++++++++++++++ > > lib/librte_lpm/rte_lpm_iterate.h | 56 +++++++++++++++++++++++++++ > > 3 files changed, 139 insertions(+), 2 deletions(-) create mode > > 100644 lib/librte_lpm/rte_lpm_iterate.c create mode 100644 > > lib/librte_lpm/rte_lpm_iterate.h > > > > diff --git a/lib/librte_lpm/Makefile b/lib/librte_lpm/Makefile index > > 3dc549d..c45da19 100644 > > --- a/lib/librte_lpm/Makefile > > +++ b/lib/librte_lpm/Makefile > > @@ -42,10 +42,10 @@ EXPORT_MAP :=3D rte_lpm_version.map LIBABIVER :=3D= 2 > > > > # all source are stored in SRCS-y > > -SRCS-$(CONFIG_RTE_LIBRTE_LPM) :=3D rte_lpm.c rte_lpm6.c > > +SRCS-$(CONFIG_RTE_LIBRTE_LPM) :=3D rte_lpm.c rte_lpm6.c > > +rte_lpm_iterate.c >=20 > I don't see any reason why this needs to be in a new file. Can you > consider merging it into the existing rte_lpm.c/.h files. > What about an implementation for IPv6? Any plans for an equivalent > implementation. >=20 > > > > # install this header file > > -SYMLINK-$(CONFIG_RTE_LIBRTE_LPM)-include :=3D rte_lpm.h rte_lpm6.h > > +SYMLINK-$(CONFIG_RTE_LIBRTE_LPM)-include :=3D rte_lpm.h rte_lpm6.h > > +rte_lpm_iterate.h > > > > ifneq ($(filter y,$(CONFIG_RTE_ARCH_ARM) $(CONFIG_RTE_ARCH_ARM64)),) > > SYMLINK-$(CONFIG_RTE_LIBRTE_LPM)-include +=3D rte_lpm_neon.h diff --git > > a/lib/librte_lpm/rte_lpm_iterate.c b/lib/librte_lpm/rte_lpm_iterate.c > > new file mode 100644 > > index 0000000..f643764 > > --- /dev/null > > +++ b/lib/librte_lpm/rte_lpm_iterate.c > > @@ -0,0 +1,81 @@ > > +/*- > > + * BSD LICENSE > > + * > > + * Copyright(c) 2014 J?rgen Grahn. All rights reserved. > > + * All rights reserved. > > + * > > + * Redistribution and use in source and binary forms, with or withou= t > > + * modification, are permitted provided that the following condition= s > > + * are met: > > + * > > + * * Redistributions of source code must retain the above copyrigh= t > > + * notice, this list of conditions and the following disclaimer. > > + * * Redistributions in binary form must reproduce the above > copyright > > + * notice, this list of conditions and the following disclaimer > in > > + * the documentation and/or other materials provided with the > > + * distribution. > > + * * Neither the name of Intel Corporation nor the names of its > > + * contributors may be used to endorse or promote products > derived > > + * from this software without specific prior written permission. > > + * > > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND > CONTRIBUTORS > > + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT > > + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS > FOR > > + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE > COPYRIGHT > > + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, > INCIDENTAL, > > + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT > > + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF > USE, > > + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON > ANY > > + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR > TORT > > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE > USE > > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH > DAMAGE. > > + */ > > +#include "rte_lpm_iterate.h" > > +#include "rte_lpm.h" > > + > > +#include > > + > > + > > +/** > > + * Iterate through the lpm, pulling out at most 'buflen' valid routes > > + * (less means we've hit the end). The cursor should be initialized > > + * to { 0, 0 } before the first call. > > + * > > + * The routes are partially sorted, by prefix length. Undefined > > + * results if the lpm is modified in parallel with or inbetween > > +calls, > > + * although the iteration will still terminate properly. > > + */ > > +unsigned > > +rte_lpm_iterate(struct rte_lpm_route* const buf, unsigned buflen, > > + const struct rte_lpm* lpm, > > + struct rte_lpm_cursor* const cursor) > For the lpm library functions, the lpm parameter is given first. I think > this should be the same, for consistency. >=20 > > +{ > > + struct rte_lpm_route* p =3D buf; > > + struct rte_lpm_route* const end =3D p + buflen; > > + > > + const struct rte_lpm_rule_info* const rinfo =3D lpm->rule_info; > > + const struct rte_lpm_rule* const rtbl =3D lpm->rules_tbl; > > + > > + unsigned d =3D cursor->d; > > + unsigned n =3D cursor->n; > > + > > + while(p!=3Dend) { > > + if(d=3D=3D32) break; > > + if(n>=3Drinfo[d].used_rules) { > > + d++; > > + n =3D 0; > > + continue; > > + } > > + const struct rte_lpm_rule rule =3D rtbl[rinfo[d].first_rule + > n]; > > + p->addr.s_addr =3D htonl(rule.ip); > > + p->plen =3D d+1; > > + p->nh =3D rule.next_hop; > > + p++; > > + n++; > > + } > > + > > + cursor->d =3D d; > > + cursor->n =3D n; > > + > > + return p - buf; > > +} >=20 > My impression from the description and the function title "iterate" was > that this would iterate through the lpm table itself, returning all ip > address and next hop matchings. Instead, it appears that this just return= s > the rules from the rules table. >=20 > Given this, I think the function name and behaviour might be better as > "rte_lpm_get_rules(lpm, lpm_rules_buffer, num_rules, start_idx)" > where up to num_rules as filled into lpm_rules_buffer, starting at rule > start_idx in the list. The return value should indicate the number of > rules that would be filled into lpm_rules_buffer if it had space. This is > a standard approach we use for situations like this - if retval < > num_rules, you have them all, otherwise you need to query again. If you > want, it's also easy to get all the rules in one go - just make a call > first with a zero-buffer size, and then use the return value to allocate = a > suitably-sized buffer and call a second time. >=20 > > diff --git a/lib/librte_lpm/rte_lpm_iterate.h > > b/lib/librte_lpm/rte_lpm_iterate.h > > new file mode 100644 > > index 0000000..25c7841 > > --- /dev/null > > +++ b/lib/librte_lpm/rte_lpm_iterate.h > > @@ -0,0 +1,56 @@ > > +/*- > > + * BSD LICENSE > > + * > > + * Copyright(c) 2014 J?rgen Grahn. All rights reserved. > > + * All rights reserved. > > + * > > + * Redistribution and use in source and binary forms, with or withou= t > > + * modification, are permitted provided that the following condition= s > > + * are met: > > + * > > + * * Redistributions of source code must retain the above copyrigh= t > > + * notice, this list of conditions and the following disclaimer. > > + * * Redistributions in binary form must reproduce the above > copyright > > + * notice, this list of conditions and the following disclaimer > in > > + * the documentation and/or other materials provided with the > > + * distribution. > > + * * Neither the name of Intel Corporation nor the names of its > > + * contributors may be used to endorse or promote products > derived > > + * from this software without specific prior written permission. > > + * > > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND > CONTRIBUTORS > > + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT > > + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS > FOR > > + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE > COPYRIGHT > > + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, > INCIDENTAL, > > + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT > > + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF > USE, > > + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON > ANY > > + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR > TORT > > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE > USE > > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH > DAMAGE. > > + */ > > +#ifndef _RTE_LPM_ITERATE_H_ > > +#define _RTE_LPM_ITERATE_H_ > > + > > +#include > > +#include > > + > > +struct rte_lpm; > > + > > +struct rte_lpm_cursor { > > + unsigned d; > > + unsigned n; > > +}; >=20 > While I don't think we need a "cursor" structure - see my proposed API > above, if we do have one, I think it should be made opaque with an API to > initialize it. >=20 > > + > > +struct rte_lpm_route { > > + struct in_addr addr; > > + uint8_t plen; > > + uint8_t nh; > > +}; > > + > > +unsigned rte_lpm_iterate(struct rte_lpm_route* buf, unsigned buflen, > > + const struct rte_lpm* lpm, > > + struct rte_lpm_cursor* cursor); > > + > > +#endif > > -- > > 2.7.4 > >