[dpdk-dev] [PATCH v2 01/25] ethdev: introduce generic flow API

[Zhao1, Wei]

Hi, Adrien

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

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil at 6wind.com]
> Sent: Wednesday, November 1, 2017 1:46 AM
> To: Zhao1, Wei <wei.zhao1 at intel.com>
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 01/25] ethdev: introduce generic flow API
> 
> Hi Wei,
> 
> Sorry for the late answer, see below.
> 
> 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?
> 
> The original idea was to provide the ability to share a single rss_conf structure
> between many flow rules and avoid redundant allocations.
> 
> It's based on the fact applications currently use a single RSS context for
> everything, they can easily make rss_conf point the global context found in
> struct rte_eth_dev.
> 
> Currently the testpmd flow command is incomplete, it doesn't support the
> configuration of this field and always provides NULL to PMDs instead. This is
> not documented in the flow API and may crash PMDs.
> 
> 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.
> 
> That's a problem that needs to be addressed in testpmd.
> 
> > 2. And also why the" const" is not need? We need a const ?How can we
> config this parameter?
> 
> It means the allocation is managed outside of rte_flow, where the data may
> or may not be const, it's up to the application. The pointer stored in this
> structure is only a copy.
> 
> 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.
> 
> It's an API issue I'd like to address by embedding the rss_conf parameters
> directly in rte_flow_action_rss instead of doing so through a pointer.
> 
> > 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 ?
> 
> Basically you need to specify an additional "end" keyword to terminate the
> queues list (tab completion shows it):
> 
>  flow create 0 ingress pattern eth / tcp / end actions rss queues 0 1 2 end /
> end
> 
> There are other design flaws with the RSS action definition:
> 
> - RSS hash algorithm to use is missing, PMDs must rely on global parameters.
> - The C99-style flexible queues array is a super pain to manage and is not
>   supported by C++. I want to make it a normal pointer (the same applies to
>   the RAW pattern item).
> 
> 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.
> 
> --
> Adrien Mazarguil
> 6WIND