From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Zhao1, Wei" <wei.zhao1@intel.com>
Subject: Re: [PATCH v2 01/25] ethdev: introduce generic flow API
Date: Tue, 7 Nov 2017 06:56:01 +0000
Message-ID: <A2573D2ACFCADC41BB3BE09C6DE313CA07C73A6C@PGSMSX103.gar.corp.intel.com>
References: <cover.1479309719.git.adrien.mazarguil@6wind.com>
 <cover.1481903839.git.adrien.mazarguil@6wind.com>
 <f8e97c46b8002ae6d4061e17abd8ff836b8c7cc6.1481903839.git.adrien.mazarguil@6wind.com>
 <A2573D2ACFCADC41BB3BE09C6DE313CA07C70341@PGSMSX103.gar.corp.intel.com>
 <20171031174551.GV26782@6wind.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Cc: "dev@dpdk.org" <dev@dpdk.org>
To: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Return-path: <dev-bounces@dpdk.org>
Received: from mga03.intel.com (mga03.intel.com [134.134.136.65])
 by dpdk.org (Postfix) with ESMTP id F3AE01B3B8
 for <dev@dpdk.org>; Tue,  7 Nov 2017 07:56:04 +0100 (CET)
In-Reply-To: <20171031174551.GV26782@6wind.com>
Content-Language: en-US
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

Hi, Adrien

Thank you for your so details answer!
Let me study your mail first then maybe I will supply some details=20
when I implement PMD code of MOVING rss  to rte_flow for ixgbe.

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Wednesday, November 1, 2017 1:46 AM
> To: Zhao1, Wei <wei.zhao1@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 01/25] ethdev: introduce generic flow A=
PI
>=20
> Hi Wei,
>=20
> Sorry for the late answer, see below.
>=20
> On Mon, Oct 23, 2017 at 08:53:49AM +0000, Zhao1, Wei wrote:
> <snip>
> > +/**
> > + * RTE_FLOW_ACTION_TYPE_RSS
> > + *
> > + * Similar to QUEUE, except RSS is additionally performed on packets
> > +to
> > + * spread them among several queues according to the provided
> parameters.
> > + *
> > + * Note: RSS hash result is normally stored in the hash.rss mbuf
> > +field,
> > + * however it conflicts with the MARK action as they share the same
> > + * space. When both actions are specified, the RSS hash is discarded
> > +and
> > + * PKT_RX_RSS_HASH is not set in ol_flags. MARK has priority. The
> > +mbuf
> > + * structure should eventually evolve to store both.
> > + *
> > + * Terminating by default.
> > + */
> > +struct rte_flow_action_rss {
> > +	const struct rte_eth_rss_conf *rss_conf; /**< RSS parameters. */
> > +	uint16_t num; /**< Number of entries in queue[]. */
> > +	uint16_t queue[]; /**< Queues indices to use. */ };
> > +
> >
> > I am plan for moving rss to rte_flow.
> > May I ask you some question for this struct of rte_flow_action_rss.
> > 1. why do you use pointer mode for rss_conf, why not use " struct
> rte_eth_rss_conf  rss_conf "?
> > we need to fill these rss info which get from CLI to this struct, if we=
 use the
> pointer mode, how can we fill in these info?
>=20
> The original idea was to provide the ability to share a single rss_conf s=
tructure
> between many flow rules and avoid redundant allocations.
>=20
> It's based on the fact applications currently use a single RSS context fo=
r
> everything, they can easily make rss_conf point the global context found =
in
> struct rte_eth_dev.
>=20
> Currently the testpmd flow command is incomplete, it doesn't support the
> configuration of this field and always provides NULL to PMDs instead. Thi=
s is
> not documented in the flow API and may crash PMDs.
>=20
> For the time being, a PMD can interpret NULL as a kind of default global
> rss_conf parameters, as is done in both mlx5 and mlx4 PMDs.
>=20
> That's a problem that needs to be addressed in testpmd.
>=20
> > 2. And also why the" const" is not need? We need a const ?How can we
> config this parameter?
>=20
> It means the allocation is managed outside of rte_flow, where the data ma=
y
> or may not be const, it's up to the application. The pointer stored in th=
is
> structure is only a copy.
>=20
> Whether the pointed data remains allocated once the flow rule is created =
is
> unspecified. A PMD cannot assume anything and has to copy these
> parameters if needed later.
>=20
> It's an API issue I'd like to address by embedding the rss_conf parameter=
s
> directly in rte_flow_action_rss instead of doing so through a pointer.
>=20
> > 3. what is your expect mode for CLI command for rss config? When I type
> in :
> > " flow create 0 pattern eth / tcp / end actions rss queues queue 0 /end=
 "
> > Or " flow create 0 pattern eth / tcp / end actions rss queues {0 1 2} /=
end "
> > I get " Bad arguments ".
> >
> > So, the right CLI command is ?
>=20
> Basically you need to specify an additional "end" keyword to terminate th=
e
> queues list (tab completion shows it):
>=20
>  flow create 0 ingress pattern eth / tcp / end actions rss queues 0 1 2 e=
nd /
> end
>=20
> There are other design flaws with the RSS action definition:
>=20
> - RSS hash algorithm to use is missing, PMDs must rely on global paramete=
rs.
> - The C99-style flexible queues array is a super pain to manage and is no=
t
>   supported by C++. I want to make it a normal pointer (the same applies =
to
>   the RAW pattern item).
>=20
> These changes are planned for the next overhaul of rte_flow, I'll submit =
a
> RFC for them as soon as I get the chance.
>=20
> --
> Adrien Mazarguil
> 6WIND