On Tue, Mar 20, 2018 at 8:24 PM, Ferruh Yigit <ferruh.yi...@intel.com> wrote:
> On 3/16/2018 1:54 PM, Shreyansh Jain wrote:
>> On Thu, Mar 15, 2018 at 8:27 PM, Ferruh Yigit <ferruh.yi...@intel.com> wrote:
[...]
>>> Hi Remy, Shreyansh,
>>>
>>> What do you think about using a variable name consistent with existing
>>> "default_[rt]xconf" in dev_info?
>>
>> It just turned out to be much more complex than I initially thought :)
>> Is this what the above conversation merging at (for Rx, as example):
>>
>> 1. 'default_rx_size_conf' is added in rte_eth_dev_info (and this
>> includes I/O params like burst size, besides configure time nb_queue,
>> nb_desc etc). Driver would return these values filled in when
>> info_get() is called.
>>
>> 2a. If an application needs the defaults, it would perform info_get()
>> and get the values. then, use the values in configuration APIs
>> (rx_queue_setup for nb_rx_desc, eth_dev_dev_configure for
>> nb_rx_queues).
>> For rx_burst calls, it would use the burst_size fields obtained from
>> info_get().
>> This is good enough for configuration and datapath (rx_burst).
>>
>> OR, another case
>>
>> 2b. Application wants to use default vaules provided by driver without
>> calling info_get. In which case, it would call
>> rx_queue_setup(nb_rx_desc=0..) or eth_dev_configure(nb_rx_queue=0,
>> nb_tx_queue=0). The implementation would query the value from
>> 'default_rx_size_conf' through info_get() and use those values.
>> Though, in this case, rte_eth_rx_burst(burst=0) might not work for
>> picking up the default within rte_ethdev.h.
>
> In Bruce's suggestion where ethdev keep defaults is changed.
> Initial suggestion was rte_eth_dev_info_get() filling default data, now
> defaults
> will be defined in functions like rte_eth_rx_queue_setup().
>
> This is a little different from filling defaults in rte_eth_dev_info_get():
> - Application can know where the defaults are coming from because dev_info
> fields are only modified by PMD. Application still prefer to use ethdev
> defaults.
>
> - The default values in ethdev library provided in function related to that
> data, instead of separate rte_eth_dev_info_get() function.
It seems we both are on same page (almost) - just that I couldn't
articulate my comments properly earlier, maybe.
rte_eth_dev_info_get is only a method to get defaults set by PMDs.
dev_info_get is not setting defaults by itself. I get this.
>
>
> What application can do:
> - Application can call rte_eth_dev_info_get() and can learn if PMD provided
> defaults or not.
> - If PMD doesn't provided any default values application can prefer to use
> application defined values. This may be an option for the application looking
> for most optimized values.
> - Although PMD doesn't provide any defaults, application still can use
> defaults
> provided by ethdev by providing '0' as arguments.
Yes, agree - and only comment I added previously in this case is that
this is not applicable for burst APIs. So, optimal [rt]x burst size
cannot be 'defaulted' to EAL layer. Other values like ring size, queue
count can be delegated to EAL for overwriting if passed as '0'.
>
>
> So how related ethdev functions will be updated:
> if argument != 0
> use argument
> else
> dev_info_get()
> if dev_info->argument != 0
> use dev_info->argument
> else
> use function_prov
Perfect, but only for eth_dev_configure and eth_[rt]x_queue_setup functions -
and that is OK with me.
>
>>
>> :Four observations:
>> A). For burst size (or any other I/O time value added in future),
>> values would have to be explicitly used by application - always. If
>> value reported by info_get() is '0' (see (B) below), application to
>> use its own judgement. No default override by lib_eal.
>> IMO, This is good enough assumption.
>
> This is no more true after Bruce's comment.
> If application provides any values it will overwrite everything else,
> application has the final word.
> But application may prefer to use provided default values.
I am not sure what has changed with Bruce's comment - but I agree with
what you are stating.
>
>>
>> B). '0' as an indicator for 'no-default-value-available-from-driver'
>> is still an open point. It is good enough for current proposed
>> parameters, but may be a valid numerical value in future.
>> IMO, this can be ignored for now.
>
> Agree that we can ignore it for now.
>
>>
>> C) Unlike the original proposal, this would add two separate members
>> to rte_eth_dev_info - one each for Rx and Tx. They both are still
>> expected to be populated through the info_get() implementation but not
>> by lib_eal.
>> IMO, doesn't matter.
>
> There won't be new members, which ones are you talking about?
original proposal: (ignore change of names, please)
rte_eth_dev_preferred_info {
rx_burst_size
tx_burst_size
rx_ring_size
tx_ring_size
...
}
And this is what I think last few comments intended:
rte_eth_rxpreferred {
...
rx_burst_size
rx_ring_size
...
}
rte_eth_txpreferred {
...
tx_burst_size
tx_ring_size
...
}
both the above added rte_eth_dev_info{}
This is what I meant when I stated "...this would add two separate
members to rte_eth_dev_info - one each for Rx and Tx..."
[...]
-
Shreyansh
