On 10/31/2019 3:07 PM, Wang, Haiyue wrote:
>> -----Original Message-----
>> From: Yigit, Ferruh <ferruh.yi...@intel.com>
>> Sent: Thursday, October 31, 2019 22:58
>> To: Wang, Haiyue <haiyue.w...@intel.com>; Jerin Jacob <jerinjac...@gmail.com>
>> Cc: Thomas Monjalon <tho...@monjalon.net>; dpdk-dev <dev@dpdk.org>; Ye,
>> Xiaolong
>> <xiaolong...@intel.com>; Kinsella, Ray <ray.kinse...@intel.com>; Iremonger,
>> Bernard
>> <bernard.iremon...@intel.com>; Sun, Chenmin <chenmin....@intel.com>; Andrew
>> Rybchenko
>> <arybche...@solarflare.com>; Slava Ovsiienko <viachesl...@mellanox.com>;
>> Stephen Hemminger
>> <step...@networkplumber.org>; David Marchand <david.march...@redhat.com>;
>> Jerin Jacob
>> <jer...@marvell.com>
>> Subject: Re: [dpdk-dev] [PATCH v4 1/4] ethdev: add the API for getting burst
>> mode information
>>
>> On 10/31/2019 11:16 AM, Wang, Haiyue wrote:
>>>> -----Original Message-----
>>>> From: Jerin Jacob <jerinjac...@gmail.com>
>>>> Sent: Thursday, October 31, 2019 18:46
>>>> To: Wang, Haiyue <haiyue.w...@intel.com>
>>>> Cc: Yigit, Ferruh <ferruh.yi...@intel.com>; Thomas Monjalon
>>>> <tho...@monjalon.net>; dpdk-dev
>>>> <dev@dpdk.org>; Ye, Xiaolong <xiaolong...@intel.com>; Kinsella, Ray
>>>> <ray.kinse...@intel.com>;
>>>> Iremonger, Bernard <bernard.iremon...@intel.com>; Sun, Chenmin
>>>> <chenmin....@intel.com>; Andrew
>>>> Rybchenko <arybche...@solarflare.com>; Slava Ovsiienko
>>>> <viachesl...@mellanox.com>; Stephen
>> Hemminger
>>>> <step...@networkplumber.org>; David Marchand <david.march...@redhat.com>;
>>>> Jerin Jacob
>>>> <jer...@marvell.com>
>>>> Subject: Re: [dpdk-dev] [PATCH v4 1/4] ethdev: add the API for getting
>>>> burst mode information
>>>>
>>>>>> 'rte_eth_burst_mode_option_name()' can get "struct rte_eth_burst_mode" as
>>>>>> parameter and convert the 'options' to string and combine into single
>>>>>> string as
>>>>>> a helper function to the applications.
>>>>>>
>>>>>
>>>>> Change:
>>>>> const char *
>>>>> rte_eth_burst_mode_option_name(uint64_t option)
>>>>>
>>>>> to:
>>>>> int
>>>>> rte_eth_burst_mode_option_name(struct rte_eth_burst_mode *mode, char
>>>>> *str) ?
>>>>
>>>>
>>>> Since we are not ready to _remove_ flags in public API and rc2 time is
>>>> ticking, probably the following the change
>>>> would be enough. IMO, This API can be used only for logging purpose, I
>>>> don't want to spend too
>>>> many cycles on this discussion. I am leaving the decision to ethdev
>>>> maintainers to accommodate
>>>> the specifics of adding a string-based alternate options scheme.
>>>>
>>>
>>> Thanks, Jerin.
>>>
>>>>
>>>> [master][dpdk.org] $ git diff
>>>> diff --git a/lib/librte_ethdev/rte_ethdev.h
>>>> b/lib/librte_ethdev/rte_ethdev.h
>>>> index c36c1b631..2f9d2c0a7 100644
>>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>>> @@ -1272,8 +1272,11 @@ enum rte_eth_burst_mode_option {
>>>> * Ethernet device RX/TX queue packet burst mode information structure.
>>>> * Used to retrieve information about packet burst mode setting.
>>>> */
>>>> +#define RTE_ETH_BURST_MODE_ALT_OPT_SIZE 128
>>
>> Should we make it bigger to prevent ABI break just because of this later?
>> Like
>> 512 or bigger?
>>
>
> Use 1024 ?
>
>>>> +
>>>> struct rte_eth_burst_mode {
>>>> uint64_t options;
>>>> + char alternate_options[RTE_ETH_BURST_MODE_ALT_OPT_SIZE];
>>>> };
>>>
>>> +1
>>>
>>
>> +1
>>
>> It is not as flexible as getting the size from the PMD but much simpler, both
>> for the user and PMD.
>>
>> And what about dropping the 'rte_eth_burst_mode_option_name()', if
>> 'rte_eth_tx_burst_mode_get()' converts the 'options' to text
>> ('alternate_options') as Jerin suggested, we can drop it.
>>
>> So only difference from what Jerin suggested will be keeping 'uint64_t
>> options'
>> public for known/limited standardized options, which is currently only for
>> vectorization information.
>
> Since 'struct rte_eth_burst_mode' will be public, how about keep current
> design for
> rte_eth_rx/tx_burst_mode_get() ?
What do you mean keep current design, not having a string field in "struct
rte_eth_burst_mode"?
>
> But change 'const char *rte_eth_burst_mode_option_name(uint64_t option)' to
> 'int rte_eth_burst_mode_name(struct rte_eth_burst_mode *mode, char *name)' ?
> When 'name' is NULL, return the buffer size will be used. If non-NULL, format
> all the options. This will reduce the twice PMDs calls:
> dev->dev_ops->rx/tx_burst_mode_get
> And leave the filled *mode for application.
>
we need "dev_ops->rx/tx_burst_mode_get" to get information from PMDs, except
from 'uint64_t options' the provided information already as string, if the
ethdev APIs for these devops converts 'uint64_t options' to string and append to
'alternate_options', application will already have all the string, so won't need
this 'rte_eth_burst_mode_option_name()' API anymore.
