Hi Thomas,

-----Original Message-----
From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
Sent: Thursday, June 23, 2016 1:02 AM
To: Lu, Wenzhuo
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v3] ethdev: fix DCB config issue on ixgbe

2016-05-06 05:33, Wenzhuo Lu:

quoted

An issue is found that DCB cannot be configured on ixgbe NICs. It's
said the TX queue number is not right.
On ixgbe the max TX queue number is not fixed, it depends on the
multi-queue mode. The API rte_eth_dev_configure should be used to
configure this mode. But the input of this API includes TX queue
number. The problem is before the mode is configured, we cannot decide
the TX queue number.

This patch adds an API to configure RX & TX multi-queue mode
separately. After the mode is configured, the max RX & TX queue number
is decided. Then we can set the appropriate RX & TX queue number.

[...]

quoted

+/**
+ * Set RX & TX multi_queue mode.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param rx_mq_mode
+ *   RX multi_queue mode.
+ * @param tx_mq_mode
+ *   TX multi_queue mode.
+ *
+ * @return
+ *   - (0) if successful.
+ *   - (-ENODEV) if port identifier is invalid.
+ */
+int
+rte_eth_dev_mq_mode_set(uint8_t port_id,
+			enum rte_eth_rx_mq_mode rx_mq_mode,
+			enum rte_eth_tx_mq_mode tx_mq_mode);

I've really tried to think about it and I think it is more or less a hack.
First, it is not explained in the doc when we should use
rte_eth_dev_mq_mode_set() instead of a simple call to rte_eth_dev_configure().
Second, I don't understand why having a function which configures the
"multiqueue modes" without configuring properly RSS/VMDq/DCB.
Last, it is said that rte_eth_dev_configure() "must be invoked first before any
other function in the Ethernet API".

Sorry, didn't notice this announcement.

My opinion is that the primary goal of rte_eth_dev_configure() was "Embedding
all configuration information in a single data structure"
but it is currently configuring only speed and some flow steering (only RSS,
VMDq, DCB and flow director).
This bug and the state of the ethdev API clearly shows that we must have one
function per feature (or group of features) and drop rte_eth_dev_configure().

You can argue it is a just a personal feeling and this comment comes late, but I
promise it is not easy to give a negative opinion because of design perspective.
I strongly feel we must stop workarounding the ethdev API issues and start really
fixing it.

Hope you understand and agree to work on a new API.

I have the same feeling with you. There's some problem with rte_eth_dev_configure. So this patch is a workaround more than a real fix.
But the problem is this API has already been used. What I think is could we take this workaround as a first step. It need not ask the APP to change too much.
Then we can discuss how could we rework on a new API or APIs. We all know the change in rte layer is not easy and need to be very careful :)