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

[Adrien Mazarguil]

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