Re: [PATCH v6 6/8] ethdev: add common devargs parser

From: "Gaëtan Rivet" <gaetan.rivet@6wind.com>
To: Declan Doherty <declan.doherty@intel.com>
Cc: dev@dpdk.org, Ferruh Yigit <ferruh.yigit@intel.com>,
	Thomas Monjalon <thomas@monjalon.net>,
	Mohammad Abdul Awal <mohammad.abdul.awal@intel.com>,
	Remy Horton <remy.horton@intel.com>,
	Yuanhan Liu <yliu@fridaylinux.org>
Subject: Re: [PATCH v6 6/8] ethdev: add common devargs parser
Date: Thu, 29 Mar 2018 14:12:50 +0200	[thread overview]
Message-ID: <20180329121250.ibbrgpf4m4gyqquy@bidouze.vm.6wind.com> (raw)
In-Reply-To: <20180328135433.20203-7-declan.doherty@intel.com>

Hi,

On Wed, Mar 28, 2018 at 02:54:31PM +0100, Declan Doherty wrote:
> From: Remy Horton <remy.horton@intel.com>
> 
> Introduces a new structure, rte_eth_devargs, to support generic
> ethdev arguments common across NET PMDs, with a new API
> rte_eth_devargs_parse API to support PMD parsing these arguments.
> 

Here is the future layout of rte_devargs:

   1. The rte_class introduced by [1], should be expanded with a "find_device"
      function, equivalent to that of the rte_bus interface.

   2. Class and Bus should export a match function as well as a parse
      function. The match function would be used for device querying,
      parsing would serve for device declaration.

   3. The match function is already implemented by [1].
      The parse function for bus already exists and should now
      be expanded to classes as well. Its expected input should
      change to be a list of kvargs.

   4. Accompanying those changes, the rte_devargs lib would
      now divide the device string in the three layers identified,
      use the class parse function to identify the intended class, and
      be able to feed each layers with its proper input.

This way, this API should be generic to all layers.

Now, this work in underway but takes time.
The current patch I think is an attempt to go in the right direction,
but in the end is only a compromise between the simple way and the
generic way.

Instead of having rte_eth_devargs_parse, you could have had a simple
rte_eth_representor_set(uint16_t *pid_list, size_t len);

That would have set the proper info within the rte_eth_dev_data. The port_id
list would have been parsed by your PMD by reading the representor
option.

The current version, that feeds directly the devargs to the eth layer,
makes conflicts inevitable (with PMDs having potential representor as
their parameter, or for future ether parameters such as "mac" that will
conflicts with current existing PMD parameters).

I would say that this implementation should be simple at first, for
the current work on representor. If the generic API is ready for this
release, then we might integrate afterward.

[1]: https://dpdk.org/ml/archives/dev/2018-March/092891.html

> Signed-off-by: Remy Horton <remy.horton@intel.com>
> Signed-off-by: Declan Doherty <declan.doherty@intel.com>
> ---

Regards,
-- 
Gaëtan Rivet
6WIND