DPDK patches and discussions
 help / color / mirror / Atom feed
From: Bruce Richardson <bruce.richardson@intel.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>
Cc: Chengchang Tang <tangchengchang@huawei.com>,
	dev@dpdk.org, linuxarm@huawei.com, thomas@monjalon.net,
	arybchenko@solarflare.com, wenzhuo.lu@intel.com,
	maryam.tahhan@intel.com
Subject: Re: [dpdk-dev] [PATCH v3 1/4] ethdev: add a field for rxq info structure
Date: Fri, 4 Sep 2020 16:30:40 +0100
Message-ID: <20200904153040.GD305@bricha3-MOBL.ger.corp.intel.com> (raw)
In-Reply-To: <161ed434-fe6b-c471-c505-cfb2c9739f00@intel.com>

On Fri, Sep 04, 2020 at 03:25:26PM +0100, Ferruh Yigit wrote:
> On 9/3/2020 4:35 PM, Bruce Richardson wrote:
> > On Sat, Aug 29, 2020 at 03:13:16PM +0800, Chengchang Tang wrote:
> >> Add a field named rx_buf_size in rte_eth_rxq_info to indicate the buffer
> >> size used in receiving packets for HW.
> >>
> >> In this way, upper-layer users can get this information by calling
> >> rte_eth_rx_queue_info_get.
> >>
> >> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> >> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> >> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> >> ---
> >>  lib/librte_ethdev/rte_ethdev.h | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> >> index 70295d7..9fed5cb 100644
> >> --- a/lib/librte_ethdev/rte_ethdev.h
> >> +++ b/lib/librte_ethdev/rte_ethdev.h
> >> @@ -1420,6 +1420,8 @@ struct rte_eth_rxq_info {
> >>  	struct rte_eth_rxconf conf; /**< queue config parameters. */
> >>  	uint8_t scattered_rx;       /**< scattered packets RX supported. */
> >>  	uint16_t nb_desc;           /**< configured number of RXDs. */
> >> +	/**< buffer size used for hardware when receive packets. */
> >> +	uint16_t rx_buf_size;
> >>  } __rte_cache_min_aligned;
> >>
> > Since this is breaking the ABI, this looks like the perfect opportunity to
> > add in a qinfo_size parameter to rte_eth_rx_queue_info_get() call which
> > allows ABI sanity-checking. Also, if passed through to the individual
> > drivers, allows them to make ABI determinations since driver functions
> > cannot be versioned, i.e. the driver info function cannot know whether it
> > has been called by queue_info_v21 or queue_info_v22.
> > 
> 
> Current approach we have is to detect the ABI breakage before release and either
> block it or use ABI versioning.
> What you suggest to add runtime checks assumes application can call old and new
> APIs, which is not our case and not sure what this runtime check adds.
> 
> The option to use runtime check between library and driver can be needed if
> library and driver from different DPDK versions can be used together, I think
> enabling this can bring more complexity and better to use all components of a
> DPDK release together.

Allow me to explain a bit further. For example, we add a new field to the
end of the configuration struct or the info struct, which changes the size
of the structure. We use function versioning to allow old and new ethdev
config APIs. The new API function just works passing the structure through
to the driver API. However, the old API implementation has a problem,
because it has just been passed a structure that is too small for use by
the driver functions, so it can't just pass it along. Therefore, the code
has two choices (that I am aware of):

1. allocate a new structure of the correct size, and call the driver API
with that. Once it returns, copy the fields over to the old structure to
return to the user.
2. create a new function pointer in the ops array for legacy compatibility
and have each driver implement a function to work with the old structure.

The alternative scheme is just to pass around the structure size. The
configure ethdev API takes the pointer and size, and does not need changing
if it does not use any new fields, just passes them through to the driver
configure API. Similarly there, any drivers that don't access the new
fields don't need changing, while those that do access them just need to
check the struct size before doing so to see what version of the structure
has been received.

> 
> And I think this kind of runtime checks should be discussed for all or none
> APIs, instead of having them only for some APIs.

Yes, I agree with that.
Incidentally, this same idea was covered in the context of system calls at
the linux plumbers conf:
https://linuxplumbersconf.org/event/7/contributions/657/attachments/639/1159/extensible_syscalls.pdf

Regards,
/Bruce

  parent reply	other threads:[~2020-09-04 15:30 UTC|newest]

Thread overview: 59+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-18 12:35 [dpdk-dev] [RFC] ethdev: add a field for rte_eth_rxq_info Chengchang Tang
2020-08-26  1:57 ` [dpdk-dev] [PATCH 0/3] add RX buffer size " Chengchang Tang
2020-08-26  1:57   ` [dpdk-dev] [PATCH 1/3] ethdev: add a field " Chengchang Tang
2020-08-26  1:57   ` [dpdk-dev] [PATCH 2/3] app/testpmd: Add RX buffer size display in queue info querry Chengchang Tang
2020-08-26  1:57   ` [dpdk-dev] [PATCH 3/3] net/hns3: add RX buffer size to rx qinfo querry Chengchang Tang
2020-08-26  7:12 ` [dpdk-dev] [PATCH v2 0/3] add Rx buffer size for rxq info structure Chengchang Tang
2020-08-26  7:12   ` [dpdk-dev] [PATCH v2 1/3] ethdev: add a field " Chengchang Tang
2020-08-26  7:29     ` Wei Hu (Xavier)
2020-08-26  7:12   ` [dpdk-dev] [PATCH v2 2/3] app/testpmd: add Rx buffer size display in queue info query Chengchang Tang
2020-08-26  7:28     ` Wei Hu (Xavier)
2020-08-26 14:42     ` Stephen Hemminger
2020-08-29  6:48       ` Chengchang Tang
2020-08-26  7:12   ` [dpdk-dev] [PATCH v2 3/3] net/hns3: add Rx buffer size to Rx qinfo query Chengchang Tang
2020-08-26  7:20     ` Wei Hu (Xavier)
2020-08-29  7:13 ` [dpdk-dev] [PATCH v3 0/4] add Rx buffer size for rxq info structure Chengchang Tang
2020-08-29  7:13   ` [dpdk-dev] [PATCH v3 1/4] ethdev: add a field " Chengchang Tang
2020-09-01 15:33     ` Matan Azrad
2020-09-02  3:52       ` Chengchang Tang
2020-09-02  7:19         ` Matan Azrad
2020-09-02 10:01           ` Chengchang Tang
2020-09-02 10:30             ` Matan Azrad
2020-09-03  1:48               ` Chengchang Tang
2020-09-06 13:45                 ` Matan Azrad
2020-09-07  7:47                   ` Chengchang Tang
2020-09-07  8:28                     ` Matan Azrad
2020-09-07 12:06                       ` Chengchang Tang
2020-09-07 13:02                         ` Matan Azrad
2020-09-03 15:00           ` Ferruh Yigit
2020-09-03 14:55         ` Ferruh Yigit
2020-09-03 15:01     ` Ferruh Yigit
2020-09-04  1:43       ` Chengchang Tang
2020-09-03 15:35     ` Bruce Richardson
2020-09-04 14:25       ` Ferruh Yigit
2020-09-04 15:14         ` Bruce Richardson
2020-09-04 15:30         ` Bruce Richardson [this message]
2020-08-29  7:13   ` [dpdk-dev] [PATCH v3 2/4] app/testpmd: add Rx buffer size display in queue info query Chengchang Tang
2020-08-29  7:13   ` [dpdk-dev] [PATCH v3 3/4] app/procinfo: add Rx buffer size to --show-port Chengchang Tang
2020-08-29  7:13   ` [dpdk-dev] [PATCH v3 4/4] net/hns3: add Rx buffer size to Rx qinfo query Chengchang Tang
2020-09-05  9:07 ` [dpdk-dev] [PATCH v4 0/5] add Rx buffer size for rxq info structure Chengchang Tang
2020-09-05  9:07   ` [dpdk-dev] [PATCH v4 1/5] ethdev: add a field " Chengchang Tang
2020-09-05 16:50     ` Stephen Hemminger
2020-09-05  9:07   ` [dpdk-dev] [PATCH v4 2/5] app/testpmd: add Rx buffer size display in queue info query Chengchang Tang
2020-09-18  8:54     ` Ferruh Yigit
2020-09-20  8:47       ` Chengchang Tang
2020-09-05  9:07   ` [dpdk-dev] [PATCH v4 3/5] app/procinfo: add Rx buffer size to --show-port Chengchang Tang
2020-09-05 16:59     ` Stephen Hemminger
2020-09-07  9:14       ` Chengchang Tang
2020-09-18 22:11         ` Stephen Hemminger
2020-09-21  2:06           ` Chengchang Tang
2020-09-21 11:26             ` Ferruh Yigit
2020-09-05  9:07   ` [dpdk-dev] [PATCH v4 4/5] net/hns3: add Rx buffer size to Rx qinfo query Chengchang Tang
2020-09-05  9:07   ` [dpdk-dev] [PATCH v4 5/5] doc: remove rxq info structure deprecation notice Chengchang Tang
2020-09-05 16:33     ` Thomas Monjalon
2020-09-07  9:12       ` Chengchang Tang
2020-09-21 13:22 ` [dpdk-dev] [PATCH v5 0/2] add Rx buffer size for rxq info structure Chengchang Tang
2020-09-21 13:22   ` [dpdk-dev] [PATCH v5 1/2] ethdev: add a field " Chengchang Tang
2020-09-21 14:18     ` Ferruh Yigit
2020-09-21 13:22   ` [dpdk-dev] [PATCH v5 2/2] net/hns3: add Rx buffer size to Rx qinfo query Chengchang Tang
2020-09-21 14:19   ` [dpdk-dev] [PATCH v5 0/2] add Rx buffer size for rxq info structure Ferruh Yigit

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200904153040.GD305@bricha3-MOBL.ger.corp.intel.com \
    --to=bruce.richardson@intel.com \
    --cc=arybchenko@solarflare.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=linuxarm@huawei.com \
    --cc=maryam.tahhan@intel.com \
    --cc=tangchengchang@huawei.com \
    --cc=thomas@monjalon.net \
    --cc=wenzhuo.lu@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git