On 22.08.2018 19:55, Ferruh Yigit wrote:
On 8/14/2018 1:57 AM, Lu, Wenzhuo wrote:
Hi Andrew,
-----Original Message-----
From: Andrew Rybchenko [mailto:arybche...@solarflare.com]
Sent: Monday, August 13, 2018 4:39 PM
To: Lu, Wenzhuo <wenzhuo...@intel.com>; Thomas Monjalon
<tho...@monjalon.net>; Yigit, Ferruh <ferruh.yi...@intel.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
On 13.08.2018 05:50, Lu, Wenzhuo wrote:
Hi Thomas,
-----Original Message-----
From: Thomas Monjalon [mailto:tho...@monjalon.net]
Sent: Wednesday, August 1, 2018 11:37 PM
To: Lu, Wenzhuo <wenzhuo...@intel.com>; Andrew Rybchenko
<arybche...@solarflare.com>; Yigit, Ferruh <ferruh.yi...@intel.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
16/07/2018 03:58, Lu, Wenzhuo:
Hi Andrew,
-----Original Message-----
From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Lu, Wenzhuo
Sent: Monday, July 16, 2018 9:08 AM
To: Andrew Rybchenko <arybche...@solarflare.com>; dev@dpdk.org
Cc: Yigit, Ferruh <ferruh.yi...@intel.com>; Thomas Monjalon
<tho...@monjalon.net>
Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
Hi Andrew,
-----Original Message-----
From: Andrew Rybchenko [mailto:arybche...@solarflare.com]
Sent: Friday, July 13, 2018 4:03 PM
To: Lu, Wenzhuo <wenzhuo...@intel.com>; dev@dpdk.org
Cc: Yigit, Ferruh <ferruh.yi...@intel.com>; Thomas Monjalon
<tho...@monjalon.net>
Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
Hi, Wenzhuo,
I'm sorry, but I have more even harder questions than the previous
one.
This questions are rather generic and mainly to ethdev maintainers.
On 13.07.2018 05:42, Wenzhuo Lu wrote:
The device information cannot be gotten correctly before the
configuration is set. Because on some NICs the information has
dependence on the configuration.
Thinking about it I have the following question. Is it valid
behaviour of the dev_info if it changes after configuration?
I always thought that the primary goal of the dev_info is to
provide information to app about device capabilities to allow app
configure device and queues correctly. Now we see the case when
dev_info changes on configure. May be it is acceptable, but it is
really suspicious. If we accept it, it should be documented.
May be dev_info should be split into parts: part which is
persistent and part which may depend on device configuration.
As I remember, the similar discussion has happened :) I've raised
the similar suggestion like this. But we don’t make it happen.
The reason is, you see, this is the rte layer's behavior. So the
user doesn't have to know it. From APP's PoV, it inputs the
configuration, it calls this API "rte_eth_dev_configure". It
doesn't know the configuration is copied before getting the info or not.
So, to my opinion, we can still keep the behavior. We only need to
split it into parts when we do see the case that cannot make it.
Maybe I talked too much about the patch. Think about it again. Your
comments is about how to use the APIs, rte_eth_dev_info_get,
rte_eth_dev_configure. To my opinion, rte_eth_dev_info_get is just to
get the info. It can be called anywhere, before configuration or
after. It's reasonable the info changes with the configuration changing.
But we do have something missing, like, rte_eth_dev_capability_get
which
should be stable. APP can use this API to get the necessary info
before configuration.
A question, maybe a little divergent thinking, that APP should have
some
intelligence to handle the capability automatically. So getting the
capability is not so good and effective, looks like we still need the human
involvement.
Maybe that the reason currently we suppose APP know the capability
from the paper copies, examples...
I am not sure to understand all the sentences.
But I agree that we should take a decision about the stability of these
infos.
Either infos cannot change after probing, or we must document that
the app must request infos regularly (when?).
Sorry, I missed this mail.
I have the concern that different NICs have different behavior. One info
can be stable on a NIC but dynamic on another. Considering this, we may
better not splitting the rte_eth_dev_info_get to 2 APIs. And comparing with
handling this in rte layer, maybe we can let every NIC has its own decision.
I have an idea. Maybe we can add a parameter for potential dynamic
fields. Like, Changing uint16_t nb_rx_queues; to struct nb_rx_queues {
uint16_t value; bool stable; }
May be it is just very bad example, but as I understand nb_rx_queues is
mainly required to configure the device properly. Or should app configure,
get new value, reconfigure again, get new value and so on and stop when
previous is equal to the new one. Yes, I dramatise and it sounds really bad.
In any case it would over-complicate interface and no single app will do it
correctly.
I think you're talking about max_rx_queues. APP can get that info before
configuration. Then configure rx queue number which is not larger than it.
That's enough.
nb_rx_queues should be the number which is configured by APP and how many
queues are actually used. To my opinion, it's mainly used by the GUI to show
the value to human being.
BTW, max_rx_queues could be an good example that shows that some parameters are
stable on some NICs but not on other NICs.
Take Intel NICs for example (I don’t familiar with others.), normally
max_rx_queues is stable on PF. But on VF, as the max number is decided by PF,
it could be dynamic. When VF starts, it can get an default value from PF. If it
not enough, it can request a larger one from PF. If the number works, VF can
get a new number.
"struct rte_eth_dev_info" is a little overloaded, it has:
- static info, like *device
- device limitations, max_*, *_lim
- device capabilities, *_capa
- suggested configurations, default_*conf
- device configuration, nb_[r/t]x_queues
- other, switch_info
There is a concern that some values are dynamic, but this is not new, for
example nb_rx/tx_queues can be changed by rte_eth_dev_rx/tx_queue_config() API
and rte_eth_dev_info() output will be changed.
The example looks different to me. It is explicit changes directly
requested by the application. So, it is not a surprise that it changes.
For this patch suggested configuration changes based on some other config values
looks ok as concept.
So I think we can say after every configuration related API dev info can be
changed.
I think that saying that any configuration changes may result in any
changes in dev_info is hardly helpful. I'd suggest to be more specific.
Yes, it is harder and will have bugs, but at least it is helpful.
But rte_eth_dev_info_get() has been called within rte_eth_dev_configure()
creating a cyclic dependency, this is forcing copy the user provided config
before rte_eth_dev_info().
This case the concern of copying user provided config to device config in early
stages cause inconsistent data in error case, this is valid concern and
unfortunately already an issue with the current implementation.
What would you think keep the logic in this patch but improve it with save and
restore existing device config for error cases?
Stable dev_info is simple. If there are real cases when something can't be
stable (and may be recommended Rx/Tx ring sizes is good example, it should
at least documented in dev_info structure description or may be moved to
separate API.
By default, the stable is false. Then every NIC can maintain its own
behavior.
Some fileds that must be stable can be left unchanged, like, driver_name,
max_rx_queues.
As this patch is just reversing a bad commit to fix a bug, if my idea sounds
good or worth discussing, I can send another RFC mail for it.
