Hi Olivier,


On Monday 25 September 2017 08:37 AM, Olivier MATZ wrote:

Hi,

On Mon, Sep 11, 2017 at 08:48:37PM +0530, Santosh Shukla wrote:

quoted

Now that dpdk supports more than one mempool drivers and
each mempool driver works best for specific PMD, example:
- sw ring based mempool for Intel PMD drivers.
- dpaa2 HW mempool manager for dpaa2 PMD driver.
- fpa HW mempool manager for Octeontx PMD driver.

Application would like to know the best mempool handle
for any port.

Introducing rte_eth_dev_pools_ops_supported() API,
which allows PMD driver to advertise
his supported pools capability to the application.

Supported pools are categorized in below priority:-
- Best mempool handle for this port (Highest priority '0')
- Port supports this mempool handle (Priority '1')

Signed-off-by: Santosh Shukla <redacted>

[...]

+int
+rte_eth_dev_pools_ops_supported(uint8_t port_id, const char *pool)

pools -> pool?

ok.

quoted

+{
+	struct rte_eth_dev *dev;
+	const char *tmp;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+
+	if (pool == NULL)
+		return -EINVAL;
+
+	dev = &rte_eth_devices[port_id];
+
+	if (*dev->dev_ops->pools_ops_supported == NULL) {
+		tmp = rte_eal_mbuf_default_mempool_ops();
+		if (!strcmp(tmp, pool))
+			return 0;
+		else
+			return -ENOTSUP;

I don't understand why we are comparing with
rte_eal_mbuf_default_mempool_ops().

It means that the result of the function would be influenced
by the parameter given by the user.

But that will be only for ops not supported case and in that case,
function _must_ make sure that if inputted param is _default_ops_name
then function should return ops supported correct info (whether 
returning '0' : Best ops or '1': ops does support 
, this part is arguable.. meaning One can say that default_ops ='handle-name' is best possible handle Or 
one of handle which platform supports).

I think that a PMD that does not implement ->pools_ops_supported
should always return 1 (mempool is supported).

Return 1 says: PMD support this ops.. 

So if ops is not supported and func returns with 1, then which ops application will use?
If that ops is default_ops.. then How application will distinguish when to use default ops or
param ops?.. as because in both cases func will return with value 1.

The approach in the patch takes care of that condition and func will return -ENOTSUP 
if (ops not support || inputted param not matching with default ops) otherwise will return
0 or 1.

At application side; 
For error case: In case of -ENOTSUP, its upto application to use _default_ops or exit.
For good case: 0 or 1 case, func gaurantee that handle is either best handle for pool or pool supports
that handle.. However in your suggestion if ops not supported case returns 1 then application is not 
sure which ops to use.. default_ops Or input ops given to func.

make sense? 

quoted

+	}
+
+	return (*dev->dev_ops->pools_ops_supported)(dev, pool);
+}

diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 0adf3274a..d90029b1e 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h

@@ -1425,6 +1425,10 @@ typedef int (*eth_get_dcb_info)(struct rte_eth_dev *dev,
 				 struct rte_eth_dcb_info *dcb_info);
 /**< @internal Get dcb information on an Ethernet device */
 
+typedef int (*eth_pools_ops_supported_t)(struct rte_eth_dev *dev,
+						const char *pool);
+/**< @internal Get the supported pools for a port */
+

The comment should be something like:
Test if a port supports specific mempool ops.

quoted

 /**
  * @internal A structure containing the functions exported by an Ethernet driver.
  */

@@ -1544,6 +1548,8 @@ struct eth_dev_ops {
 
 	eth_tm_ops_get_t tm_ops_get;
 	/**< Get Traffic Management (TM) operations. */
+	eth_pools_ops_supported_t pools_ops_supported;
+	/**< Get the supported pools for a port */

Same

quoted

 };
 
 /**

@@ -4436,6 +4442,24 @@ int rte_eth_dev_adjust_nb_rx_tx_desc(uint8_t port_id,
 				     uint16_t *nb_rx_desc,
 				     uint16_t *nb_tx_desc);
 
+
+/**
+ * Get the supported pools for a port

Same

quoted

+ *
+ * @param port_id
+ *   Port identifier of the Ethernet device.
+ * @param [in] pool
+ *   The supported pool handle for this port.

The name of the pool operations to test

ok, v5.

quoted

+ *   Maximum length of pool handle name is RTE_MEMPOOL_OPS_NAMESIZE.

I don't think we should keep this

Ok

Thanks,
Olivier