[dpdk-dev,v2] doc: add preferred burst size support
Checks
Commit Message
rte_eth_rx_burst(..,nb_pkts) function has semantic that if return value
is smaller than requested, application can consider it end of packet
stream. Some hardware can only support smaller burst sizes which need
to be advertised. Similar is the case for Tx burst.
This patch adds deprecation notice for rte_eth_dev_info structure as
two new members, for preferred Rx and Tx burst size would be added -
impacting the size of the structure.
Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
---
* Refer: http://dpdk.org/dev/patchwork/patch/32112 for context
v2:
- fix spelling error in deprecation notice
doc/guides/rel_notes/deprecation.rst | 8 ++++++++
1 file changed, 8 insertions(+)
Comments
On 2/1/2018 6:18 PM, Shreyansh Jain wrote:
> rte_eth_rx_burst(..,nb_pkts) function has semantic that if return value
> is smaller than requested, application can consider it end of packet
> stream. Some hardware can only support smaller burst sizes which need
> to be advertised. Similar is the case for Tx burst.
>
> This patch adds deprecation notice for rte_eth_dev_info structure as
> two new members, for preferred Rx and Tx burst size would be added -
> impacting the size of the structure.
>
> Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
<snip>...
On 02/01/2018 03:48 PM, Shreyansh Jain wrote:
> rte_eth_rx_burst(..,nb_pkts) function has semantic that if return value
> is smaller than requested, application can consider it end of packet
> stream. Some hardware can only support smaller burst sizes which need
> to be advertised. Similar is the case for Tx burst.
>
> This patch adds deprecation notice for rte_eth_dev_info structure as
> two new members, for preferred Rx and Tx burst size would be added -
> impacting the size of the structure.
>
> Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
> ---
> * Refer: http://dpdk.org/dev/patchwork/patch/32112 for context
>
> v2:
> - fix spelling error in deprecation notice
>
> doc/guides/rel_notes/deprecation.rst | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index d59ad5988..fdc7656fa 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -59,3 +59,11 @@ Deprecation Notices
> be added between the producer and consumer structures. The size of the
> structure and the offset of the fields will remain the same on
> platforms with 64B cache line, but will change on other platforms.
> +
> +* ethdev: Currently, if the rte_eth_rx_burst() function returns a value less
> + than *nb_pkts*, the application will assume that no more packets are present.
> + Some of the hw queue based hardware can only support smaller burst for RX
> + and TX and thus break the expectation of the rx_burst API. Similar is the
> + case for TX burst. ``rte_eth_dev_info`` will be added with two new
> + parameters, ``uint16_t pref_rx_burst`` and ``uint16_t pref_tx_burst``,
> + for preferred RX and TX burst sizes, respectively.
Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
On Thu, Feb 01, 2018 at 06:18:23PM +0530, Shreyansh Jain wrote:
> rte_eth_rx_burst(..,nb_pkts) function has semantic that if return value
> is smaller than requested, application can consider it end of packet
> stream. Some hardware can only support smaller burst sizes which need
> to be advertised. Similar is the case for Tx burst.
>
> This patch adds deprecation notice for rte_eth_dev_info structure as
> two new members, for preferred Rx and Tx burst size would be added -
> impacting the size of the structure.
>
> Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
> ---
> * Refer: http://dpdk.org/dev/patchwork/patch/32112 for context
>
> v2:
> - fix spelling error in deprecation notice
>
> doc/guides/rel_notes/deprecation.rst | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index d59ad5988..fdc7656fa 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -59,3 +59,11 @@ Deprecation Notices
> be added between the producer and consumer structures. The size of the
> structure and the offset of the fields will remain the same on
> platforms with 64B cache line, but will change on other platforms.
> +
> +* ethdev: Currently, if the rte_eth_rx_burst() function returns a value less
> + than *nb_pkts*, the application will assume that no more packets are present.
> + Some of the hw queue based hardware can only support smaller burst for RX
> + and TX and thus break the expectation of the rx_burst API. Similar is the
> + case for TX burst. ``rte_eth_dev_info`` will be added with two new
> + parameters, ``uint16_t pref_rx_burst`` and ``uint16_t pref_tx_burst``,
> + for preferred RX and TX burst sizes, respectively.
> --
> 2.14.1
>
LTGM as far as it goes, but following discussion on this patch,
http://dpdk.org/ml/archives/dev/2018-January/089585.html
I think we might also want to add in parameters for "pref_tx_ring_sz"
and "pref_rx_ring_sz" too. While it is the case that, once the structure
is changed, we can make multiple additional changes, I think it might be
worth mentioning as many as we can for completeness.
Another point to consider, is whether we might want to add in a
sub-structure for "preferred_settings" to hold all these, rather than
just adding them as new fields. It might help with making names more
readable (though also longer).
struct {
uint16_t rx_burst;
uint16_t tx_burst;
uint16_t rx_ring_sz;
uint16_t tx_ring_sz;
} preferred_settings;
In any case, for this or subsequent versions:
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
/Bruce
On Thu, Feb 01, 2018 at 07:58:32PM +0530, Shreyansh Jain wrote:
> On Thursday 01 February 2018 06:57 PM, Bruce Richardson wrote:
> > On Thu, Feb 01, 2018 at 06:18:23PM +0530, Shreyansh Jain wrote:
> > > rte_eth_rx_burst(..,nb_pkts) function has semantic that if return value
> > > is smaller than requested, application can consider it end of packet
> > > stream. Some hardware can only support smaller burst sizes which need
> > > to be advertised. Similar is the case for Tx burst.
> > >
> > > This patch adds deprecation notice for rte_eth_dev_info structure as
> > > two new members, for preferred Rx and Tx burst size would be added -
> > > impacting the size of the structure.
> > >
> > > Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
> > > ---
> > > * Refer: http://dpdk.org/dev/patchwork/patch/32112 for context
> > >
> > > v2:
> > > - fix spelling error in deprecation notice
> > >
> > > doc/guides/rel_notes/deprecation.rst | 8 ++++++++
> > > 1 file changed, 8 insertions(+)
> > >
> > > diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> > > index d59ad5988..fdc7656fa 100644
> > > --- a/doc/guides/rel_notes/deprecation.rst
> > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > @@ -59,3 +59,11 @@ Deprecation Notices
> > > be added between the producer and consumer structures. The size of the
> > > structure and the offset of the fields will remain the same on
> > > platforms with 64B cache line, but will change on other platforms.
> > > +
> > > +* ethdev: Currently, if the rte_eth_rx_burst() function returns a value less
> > > + than *nb_pkts*, the application will assume that no more packets are present.
> > > + Some of the hw queue based hardware can only support smaller burst for RX
> > > + and TX and thus break the expectation of the rx_burst API. Similar is the
> > > + case for TX burst. ``rte_eth_dev_info`` will be added with two new
> > > + parameters, ``uint16_t pref_rx_burst`` and ``uint16_t pref_tx_burst``,
> > > + for preferred RX and TX burst sizes, respectively.
> > > --
> > > 2.14.1
> > >
> >
> > LTGM as far as it goes, but following discussion on this patch,
> > http://dpdk.org/ml/archives/dev/2018-January/089585.html
> > I think we might also want to add in parameters for "pref_tx_ring_sz"
> > and "pref_rx_ring_sz" too. While it is the case that, once the structure
> > is changed, we can make multiple additional changes, I think it might be
> > worth mentioning as many as we can for completeness.
> >
> > Another point to consider, is whether we might want to add in a
> > sub-structure for "preferred_settings" to hold all these, rather than
> > just adding them as new fields. It might help with making names more
> > readable (though also longer).
> >
> > struct {
> > uint16_t rx_burst;
> > uint16_t tx_burst;
> > uint16_t rx_ring_sz;
> > uint16_t tx_ring_sz;
> > } preferred_settings;
>
> This, and the point above that we can make multiple additional changes, is
> definitely a good idea. Though, 'preferred_setting' is long and has chances
> of spell mistakes in first go - what about just 'pref' or, 'pref_size' if
> only 4 mentioned above are part of this.
>
> For now I saw need for burst size because I hit that case. Ring size looks
> logical to me. We can have a look if more such toggles are required.
>
I actually don't like the abbreviation "pref", as it looks too much like
"perf" short for performance. As this is an initialization setting, I
also don't think having a longer name is that big of deal. How about
calling them "suggested" or "recommended" settings - both of which have
less fiddly spellings.
/Bruce
On Thursday 01 February 2018 06:57 PM, Bruce Richardson wrote:
> On Thu, Feb 01, 2018 at 06:18:23PM +0530, Shreyansh Jain wrote:
>> rte_eth_rx_burst(..,nb_pkts) function has semantic that if return value
>> is smaller than requested, application can consider it end of packet
>> stream. Some hardware can only support smaller burst sizes which need
>> to be advertised. Similar is the case for Tx burst.
>>
>> This patch adds deprecation notice for rte_eth_dev_info structure as
>> two new members, for preferred Rx and Tx burst size would be added -
>> impacting the size of the structure.
>>
>> Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
>> ---
>> * Refer: http://dpdk.org/dev/patchwork/patch/32112 for context
>>
>> v2:
>> - fix spelling error in deprecation notice
>>
>> doc/guides/rel_notes/deprecation.rst | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
>> index d59ad5988..fdc7656fa 100644
>> --- a/doc/guides/rel_notes/deprecation.rst
>> +++ b/doc/guides/rel_notes/deprecation.rst
>> @@ -59,3 +59,11 @@ Deprecation Notices
>> be added between the producer and consumer structures. The size of the
>> structure and the offset of the fields will remain the same on
>> platforms with 64B cache line, but will change on other platforms.
>> +
>> +* ethdev: Currently, if the rte_eth_rx_burst() function returns a value less
>> + than *nb_pkts*, the application will assume that no more packets are present.
>> + Some of the hw queue based hardware can only support smaller burst for RX
>> + and TX and thus break the expectation of the rx_burst API. Similar is the
>> + case for TX burst. ``rte_eth_dev_info`` will be added with two new
>> + parameters, ``uint16_t pref_rx_burst`` and ``uint16_t pref_tx_burst``,
>> + for preferred RX and TX burst sizes, respectively.
>> --
>> 2.14.1
>>
>
> LTGM as far as it goes, but following discussion on this patch,
> http://dpdk.org/ml/archives/dev/2018-January/089585.html
> I think we might also want to add in parameters for "pref_tx_ring_sz"
> and "pref_rx_ring_sz" too. While it is the case that, once the structure
> is changed, we can make multiple additional changes, I think it might be
> worth mentioning as many as we can for completeness.
>
> Another point to consider, is whether we might want to add in a
> sub-structure for "preferred_settings" to hold all these, rather than
> just adding them as new fields. It might help with making names more
> readable (though also longer).
>
> struct {
> uint16_t rx_burst;
> uint16_t tx_burst;
> uint16_t rx_ring_sz;
> uint16_t tx_ring_sz;
> } preferred_settings;
This, and the point above that we can make multiple additional changes,
is definitely a good idea. Though, 'preferred_setting' is long and has
chances of spell mistakes in first go - what about just 'pref' or,
'pref_size' if only 4 mentioned above are part of this.
For now I saw need for burst size because I hit that case. Ring size
looks logical to me. We can have a look if more such toggles are required.
>
> In any case, for this or subsequent versions:
>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
>
> /Bruce
>
Thanks.
Hi Bruce,
On Thursday 01 February 2018 07:49 PM, Bruce Richardson wrote:
> On Thu, Feb 01, 2018 at 07:58:32PM +0530, Shreyansh Jain wrote:
>> On Thursday 01 February 2018 06:57 PM, Bruce Richardson wrote:
>>> On Thu, Feb 01, 2018 at 06:18:23PM +0530, Shreyansh Jain wrote:
>>>> rte_eth_rx_burst(..,nb_pkts) function has semantic that if return value
>>>> is smaller than requested, application can consider it end of packet
>>>> stream. Some hardware can only support smaller burst sizes which need
>>>> to be advertised. Similar is the case for Tx burst.
>>>>
>>>> This patch adds deprecation notice for rte_eth_dev_info structure as
>>>> two new members, for preferred Rx and Tx burst size would be added -
>>>> impacting the size of the structure.
>>>>
>>>> Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
>>>> ---
[...]
>>>
>>> LTGM as far as it goes, but following discussion on this patch,
>>> http://dpdk.org/ml/archives/dev/2018-January/089585.html
>>> I think we might also want to add in parameters for "pref_tx_ring_sz"
>>> and "pref_rx_ring_sz" too. While it is the case that, once the structure
>>> is changed, we can make multiple additional changes, I think it might be
>>> worth mentioning as many as we can for completeness.
>>>
>>> Another point to consider, is whether we might want to add in a
>>> sub-structure for "preferred_settings" to hold all these, rather than
>>> just adding them as new fields. It might help with making names more
>>> readable (though also longer).
>>>
>>> struct {
>>> uint16_t rx_burst;
>>> uint16_t tx_burst;
>>> uint16_t rx_ring_sz;
>>> uint16_t tx_ring_sz;
>>> } preferred_settings;
>>
>> This, and the point above that we can make multiple additional changes, is
>> definitely a good idea. Though, 'preferred_setting' is long and has chances
>> of spell mistakes in first go - what about just 'pref' or, 'pref_size' if
>> only 4 mentioned above are part of this.
>>
>> For now I saw need for burst size because I hit that case. Ring size looks
>> logical to me. We can have a look if more such toggles are required.
>>
>
> I actually don't like the abbreviation "pref", as it looks too much like
> "perf" short for performance. As this is an initialization setting, I
> also don't think having a longer name is that big of deal. How about
> calling them "suggested" or "recommended" settings - both of which have
> less fiddly spellings.
>
> /Bruce
>
I get your point and on a second thought even I think 'preferred_*' is
better than other options (including mine). Only change I will do is
change to 'preferred_size' as all members are size only:
struct {
uint16_t rx_burst; /*< Rx Burst size */
uint16_t tx_burst; /*< Tx Burst size */
uint16_t rx_ring; /*< Rx ring size */
uint16_t tx_ring; /*< Tx ring size */
} preferred_size;
I will use your ACK but if you have objections to focusing on size only
in this structure - let me know. I will send across another patch and
put it as 'preferred_settings' and then we can discuss the naming during
implementation.
-
Shreyansh
@@ -59,3 +59,11 @@ Deprecation Notices
be added between the producer and consumer structures. The size of the
structure and the offset of the fields will remain the same on
platforms with 64B cache line, but will change on other platforms.
+
+* ethdev: Currently, if the rte_eth_rx_burst() function returns a value less
+ than *nb_pkts*, the application will assume that no more packets are present.
+ Some of the hw queue based hardware can only support smaller burst for RX
+ and TX and thus break the expectation of the rx_burst API. Similar is the
+ case for TX burst. ``rte_eth_dev_info`` will be added with two new
+ parameters, ``uint16_t pref_rx_burst`` and ``uint16_t pref_tx_burst``,
+ for preferred RX and TX burst sizes, respectively.