Re: [dpdk-dev] [PATCH v4 1/4] ethdev: add the API for getting burst mode information

[Wang, Haiyue]

> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yi...@intel.com>
> Sent: Thursday, October 31, 2019 23:30
> 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 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.

Got it now. Thanks!