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.j...@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
