Hi Thomas,

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

2016-06-23 01:04, Lu, Wenzhuo:

quoted

From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]

quoted

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

quoted

+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().

quoted

quoted

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.

quoted

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().

quoted

quoted

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.

quoted

quoted

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.

quoted

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.

quoted

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
:)

We probably need more opinions.
I think it is not a good idea to introduce a new API only to workaround another
one and keep confusion in place.
A similar approach which looks better is to introduce a new API which will partly
replace the old one and will remain a good one when the old API will be
completely removed.
In other words, we should introduce a good API for flow steering as soon as
possible and deprecate rte_eth_dev_configure().

I think you're right. The workaround can make things confusing. Better to introduce a new API to replace rte_eth_dev_configure.