DPDK patches and discussions
 help / color / mirror / Atom feed
From: Ruifeng Wang <Ruifeng.Wang@arm.com>
To: "Medvedkin, Vladimir" <vladimir.medvedkin@intel.com>,
	Jan Viktorin <viktorin@rehivetech.com>,
	"jerinj@marvell.com" <jerinj@marvell.com>,
	Bruce Richardson <bruce.richardson@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
	"hemant.agrawal@nxp.com" <hemant.agrawal@nxp.com>,
	Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>,
	nd <nd@arm.com>, nd <nd@arm.com>
Subject: Re: [dpdk-dev] [RFC PATCH] lpm: add sve support for lookup on Arm platform
Date: Wed, 6 Jan 2021 10:11:51 +0000
Message-ID: <VI1PR0802MB23517B185E4DB39F9E23ED119ED00@VI1PR0802MB2351.eurprd08.prod.outlook.com> (raw)
In-Reply-To: <cd38b092-db1b-4a49-d00a-d17a85c41a34@intel.com>

> -----Original Message-----
> From: Medvedkin, Vladimir <vladimir.medvedkin@intel.com>
> Sent: Tuesday, January 5, 2021 11:44 PM
> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; Jan Viktorin
> <viktorin@rehivetech.com>; jerinj@marvell.com; Bruce Richardson
> <bruce.richardson@intel.com>
> Cc: dev@dpdk.org; hemant.agrawal@nxp.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
> Subject: Re: [RFC PATCH] lpm: add sve support for lookup on Arm platform
> 
> Hi Ruifeng,
> 
> Thanks for the patch, see comments below

Hi Vladimir,
Thank you for your review.

> 
> On 18/12/2020 10:12, Ruifeng Wang wrote:
> > Added new path to do lpm4 lookup by using scalable vector extension.
> > The SVE path will be selected if compiler has flag SVE set.
> >
> > Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > ---
> >   lib/librte_eal/arm/include/rte_vect.h |  3 +
> >   lib/librte_lpm/meson.build            |  2 +-
> >   lib/librte_lpm/rte_lpm.h              |  4 ++
> >   lib/librte_lpm/rte_lpm_sve.h          | 83 +++++++++++++++++++++++++++
> >   4 files changed, 91 insertions(+), 1 deletion(-)
> >   create mode 100644 lib/librte_lpm/rte_lpm_sve.h
> >
> > diff --git a/lib/librte_eal/arm/include/rte_vect.h
> > b/lib/librte_eal/arm/include/rte_vect.h
> > index a739e6e66..093e9122a 100644
> > --- a/lib/librte_eal/arm/include/rte_vect.h
> > +++ b/lib/librte_eal/arm/include/rte_vect.h
> > @@ -9,6 +9,9 @@
> >   #include "generic/rte_vect.h"
> >   #include "rte_debug.h"
> >   #include "arm_neon.h"
> > +#ifdef __ARM_FEATURE_SVE
> > +#include <arm_sve.h>
> > +#endif
> >
> >   #ifdef __cplusplus
> >   extern "C" {
> > diff --git a/lib/librte_lpm/meson.build b/lib/librte_lpm/meson.build
> > index 6cfc083c5..f93c86640 100644
> > --- a/lib/librte_lpm/meson.build
> > +++ b/lib/librte_lpm/meson.build
> > @@ -5,6 +5,6 @@ sources = files('rte_lpm.c', 'rte_lpm6.c')
> >   headers = files('rte_lpm.h', 'rte_lpm6.h')
> >   # since header files have different names, we can install all vector headers
> >   # without worrying about which architecture we actually need
> > -headers += files('rte_lpm_altivec.h', 'rte_lpm_neon.h',
> > 'rte_lpm_sse.h')
> > +headers += files('rte_lpm_altivec.h', 'rte_lpm_neon.h',
> > +'rte_lpm_sse.h', 'rte_lpm_sve.h')
> >   deps += ['hash']
> >   deps += ['rcu']
> > diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h index
> > 1afe55cdc..28b57683b 100644
> > --- a/lib/librte_lpm/rte_lpm.h
> > +++ b/lib/librte_lpm/rte_lpm.h
> > @@ -402,7 +402,11 @@ rte_lpm_lookupx4(const struct rte_lpm *lpm,
> xmm_t ip, uint32_t hop[4],
> >   	uint32_t defv);
> >
> >   #if defined(RTE_ARCH_ARM)
> > +#ifdef __ARM_FEATURE_SVE
> > +#include "rte_lpm_sve.h"
> > +#else
> >   #include "rte_lpm_neon.h"
> > +#endif
> >   #elif defined(RTE_ARCH_PPC_64)
> >   #include "rte_lpm_altivec.h"
> >   #else
> > diff --git a/lib/librte_lpm/rte_lpm_sve.h
> > b/lib/librte_lpm/rte_lpm_sve.h new file mode 100644 index
> > 000000000..86576ec52
> > --- /dev/null
> > +++ b/lib/librte_lpm/rte_lpm_sve.h
> > @@ -0,0 +1,83 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2020 Arm Limited
> > + */
> > +
> > +#ifndef _RTE_LPM_SVE_H_
> > +#define _RTE_LPM_SVE_H_
> > +
> > +#include <rte_vect.h>
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +__rte_internal
> > +static void
> > +__rte_lpm_lookup_vec(const struct rte_lpm *lpm, const uint32_t *ips,
> > +		uint32_t *__rte_restrict next_hops, const uint32_t n) {
> > +	uint32_t i = 0;
> > +	svuint32_t v_ip, v_idx, v_tbl24, v_tbl8, v_hop;
> > +	svuint32_t v_mask_xv, v_mask_v, v_mask_hop;
> > +	svbool_t pg = svwhilelt_b32(i, n);
> > +	svbool_t pv;
> > +
> > +	do {
> > +		v_ip = svld1(pg, &ips[i]);
> > +		/* Get indices for tbl24[] */
> > +		v_idx = svlsr_x(pg, v_ip, 8);
> > +		/* Extract values from tbl24[] */
> > +		v_tbl24 = svld1_gather_index(pg, (const uint32_t *)lpm-
> >tbl24,
> > +						v_idx);
> > +
> > +		/* Create mask with valid set */
> > +		v_mask_v = svdup_u32_z(pg, RTE_LPM_LOOKUP_SUCCESS);
> > +		/* Create mask with valid and valid_group set */
> > +		v_mask_xv = svdup_u32_z(pg,
> RTE_LPM_VALID_EXT_ENTRY_BITMASK);
> > +		/* Create predicate for tbl24 entries: (valid && !valid_group)
> */
> > +		pv = svcmpeq(pg, svand_z(pg, v_tbl24, v_mask_xv),
> v_mask_v);
> > +		/* Create mask for next_hop in table entry */
> > +		v_mask_hop = svdup_u32_z(pg, 0x00ffffff);
> > +		/* Extract next_hop and write back */
> > +		v_hop = svand_x(pv, v_tbl24, v_mask_hop);
> > +		svst1(pv, &next_hops[i], v_hop);
> > +
> > +		/* Update predicate for tbl24 entries: (valid && valid_group)
> */
> > +		pv = svcmpeq(pg, svand_z(pg, v_tbl24, v_mask_xv),
> v_mask_xv);
> > +		/* Compute tbl8 index */
> > +		v_idx = svand_x(pv, v_tbl24, svdup_u32_z(pv, 0xff));
> 
> Loos like here should be
> v_idx = svand_x(pv, v_tbl24, svdup_u32_z(pv, 0xffffff)); because we are
> using 24 bits to keep tbl8 group.

Yes, the mask should be 0xffffff.

Also noticed there is common issue in all vector lookup implementations (NEON/SSE/ALTIVEC).
I'll correct this and fix other vector implementations in next version.

> 
> 
> > +		v_idx = svmul_x(pv, v_idx,
> RTE_LPM_TBL8_GROUP_NUM_ENTRIES);
> > +		v_idx = svadd_x(pv, svand_x(pv, v_ip, svdup_u32_z(pv,
> 0xff)),
> > +				v_idx);
> > +		/* Extract values from tbl8[] */
> > +		v_tbl8 = svld1_gather_index(pv, (const uint32_t *)lpm->tbl8,
> > +						v_idx);
> > +		/* Update predicate for tbl8 entries: (valid) */
> > +		pv = svcmpeq(pv, svand_z(pv, v_tbl8, v_mask_v), v_mask_v);
> > +		/* Extract next_hop and write back */
> > +		v_hop = svand_x(pv, v_tbl8, v_mask_hop);
> > +		svst1(pv, &next_hops[i], v_hop);
> 
> I'm not an expert, but probably it would be better to merge two stores
> (svst1) into a single one?

I think we can keep current implementation.
In most cases, tbl24 will be not expanded. Then SVE predicate for tbl8 processing will be zero.
So operations on tbl8 will be null operations.
I think it is better not to mix the two stores (from tbl24 and from tbl8).
> 
> > +
> > +		i += svlen(v_ip);
> > +		pg = svwhilelt_b32(i, n);
> 
> Isn't it better to move the predicate calculation to the beginning of the loop
> and just do {} while (i < n)?

Yes, that also works.
I think checking on SVE predicates is the suggested way to do vector length agnostic loop.
It is more generic and flexible.
> 
> > +	} while (svptest_any(svptrue_b32(), pg)); }
> > +
> > +static inline void
> > +rte_lpm_lookupx4(const struct rte_lpm *lpm, xmm_t ip, uint32_t hop[4],
> > +		uint32_t defv)
> > +{
> > +	uint32_t i, ips[4];
> > +
> > +	vst1q_s32((int32_t *)ips, ip);
> > +	for (i = 0; i < 4; i++)
> > +		hop[i] = defv;
> > +
> > +	__rte_lpm_lookup_vec(lpm, ips, hop, 4); }
> > +
> > +#ifdef __cplusplus
> > +}
> > +#endif
> > +
> > +#endif /* _RTE_LPM_SVE_H_ */
> >
> 
> --
> Regards,
> Vladimir

  reply	other threads:[~2021-01-06 10:12 UTC|newest]

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-18 10:12 Ruifeng Wang
2021-01-05 15:44 ` Medvedkin, Vladimir
2021-01-06 10:11   ` Ruifeng Wang [this message]
2021-01-08  8:25 ` [dpdk-dev] [PATCH v2 0/5] lpm lookup with sve support Ruifeng Wang
2021-01-08  8:25   ` [dpdk-dev] [PATCH v2 1/5] lpm: add sve support for lookup on Arm platform Ruifeng Wang
2021-01-13 18:54     ` Medvedkin, Vladimir
2021-01-08  8:25   ` [dpdk-dev] [PATCH v2 2/5] net/hns3: fix build with sve enabled Ruifeng Wang
2021-01-09  0:06     ` Honnappa Nagarahalli
2021-01-09  2:11       ` oulijun
2021-01-11  2:39         ` Ruifeng Wang
2021-01-11 13:38           ` Honnappa Nagarahalli
2021-01-09  2:15     ` oulijun
2021-01-11  2:27       ` Ruifeng Wang
2021-01-08  8:25   ` [dpdk-dev] [PATCH v2 3/5] net/octeontx: " Ruifeng Wang
2021-01-08  8:25   ` [dpdk-dev] [PATCH v2 4/5] common/octeontx2: " Ruifeng Wang
2021-01-08 10:29     ` [dpdk-dev] [EXT] " Pavan Nikhilesh Bhagavatula
2021-01-11  9:51       ` Ruifeng Wang
2021-01-08  8:25   ` [dpdk-dev] [PATCH v2 5/5] config: add Arm Neoverse N2 Ruifeng Wang
2021-01-08 23:58     ` Honnappa Nagarahalli
2021-01-11  3:01       ` Ruifeng Wang
2021-01-11  3:09         ` Jerin Jacob
2021-01-11  8:32           ` Ruifeng Wang
2021-01-11 13:58             ` Honnappa Nagarahalli
2021-01-12  2:57 ` [dpdk-dev] [PATCH v3 0/5] lpm lookup with sve support Ruifeng Wang
2021-01-12  2:57   ` [dpdk-dev] [PATCH v3 1/5] lpm: add sve support for lookup on Arm platform Ruifeng Wang
2021-01-13 15:58     ` David Marchand
2021-01-27 13:04     ` David Marchand
2021-01-27 21:03       ` Honnappa Nagarahalli
2021-01-28  8:03         ` David Marchand
2021-01-28 12:24           ` Honnappa Nagarahalli
2021-01-28  5:47       ` Ruifeng Wang
2021-01-12  2:57   ` [dpdk-dev] [PATCH v3 2/5] net/hns3: fix build with sve enabled Ruifeng Wang
2021-01-13  2:16     ` Honnappa Nagarahalli
2021-01-12  2:57   ` [dpdk-dev] [PATCH v3 3/5] net/octeontx: " Ruifeng Wang
2021-01-12  4:39     ` Jerin Jacob
2021-01-12  2:57   ` [dpdk-dev] [PATCH v3 4/5] common/octeontx2: " Ruifeng Wang
2021-01-12  4:38     ` Jerin Jacob
2021-01-12  2:57   ` [dpdk-dev] [PATCH v3 5/5] config: add Arm Neoverse N2 Ruifeng Wang
2021-01-12  4:44     ` Jerin Jacob
2021-01-13  2:08     ` Honnappa Nagarahalli
2021-01-14 15:18   ` [dpdk-dev] [PATCH v3 0/5] lpm lookup with sve support David Marchand
2021-01-14 15:40     ` David Marchand
2021-01-15  7:02       ` Ruifeng Wang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=VI1PR0802MB23517B185E4DB39F9E23ED119ED00@VI1PR0802MB2351.eurprd08.prod.outlook.com \
    --to=ruifeng.wang@arm.com \
    --cc=Honnappa.Nagarahalli@arm.com \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=hemant.agrawal@nxp.com \
    --cc=jerinj@marvell.com \
    --cc=nd@arm.com \
    --cc=viktorin@rehivetech.com \
    --cc=vladimir.medvedkin@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git