Re: [dpdk-dev] [PATCH v8] ethdev: check Rx/Tx offloads

["Dai, Wei" <wei.dai@intel.com>]

Hi, Andrew
See my in-line comments as below.

From: Andrew Rybchenko [mailto:arybchenko@solarflare.com] 
Sent: Tuesday, May 8, 2018 7:37 PM
To: Dai, Wei <wei.dai@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>; thomas@monjalon.net; Doherty, Declan <declan.doherty@intel.com>; linville@tuxdriver.com; mw@semihalf.com; mk@semihalf.com; gtzalik@amazon.com; evgenys@amazon.com; ravi1.kumar@amd.com; shepard.siegel@atomicrules.com; ed.czeck@atomicrules.com; john.miller@atomicrules.com; ajit.khaparde@broadcom.com; somnath.kotur@broadcom.com; jerin.jacob@caviumnetworks.com; maciej.czekaj@caviumnetworks.com; shijith.thotton@cavium.com; ssrinivasan@cavium.com; santosh.shukla@caviumnetworks.com; rahul.lakkireddy@chelsio.com; ohndale@cisco.com; hyonkim@cisco.com; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin <konstantin.ananyev@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Wang, Xiao W <xiao.w.wang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; tdu@semihalf.com; dima@marvell.com; nsamsono@marvell.com; jianbo.liu@arm.com; adrien.mazarguil@6wind.com; nelio.laranjeiro@6wind.com; yskoh@mellanox.com; matan@mellanox.com; vido@cesnet.cz; alejandro.lucero@netronome.com; emant.agrawal@nxp.com; shreyansh.jain@nxp.com; hemant.agrawal@nxp.com; harish.patil@cavium.com; rasesh.mody@cavium.com; asesh.mody@cavium.com; shahed.shaikh@cavium.com; yongwang@vmware.com; maxime.coquelin@redhat.com; mtetsuyah@gmail.com; Bie, Tiwei <tiwei.bie@intel.com>; Legacy, Allain (Wind River) <allain.legacy@windriver.com>; Peters, Matt (Wind River) <matt.peters@windriver.com>; pascal.mazon@6wind.com; Richardson, Bruce <bruce.richardson@intel.com>; gaetan.rivet@6wind.com; Singh, Jasvinder <jasvinder.singh@intel.com>; Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
Cc: dev@dpdk.org; Ivan Malov <Ivan.Malov@oktetlabs.ru>
Subject: Re: [PATCH v8] ethdev: check Rx/Tx offloads

On 05/08/2018 01:05 PM, Wei Dai wrote:
This patch check if a input requested offloading is valid or not.
Any reuqested offloading must be supported in the device capabilities.
Any offloading is disabled by default if it is not set in the parameter
dev_conf->[rt]xmode.offloads to rte_eth_dev_configure( ) and
[rt]x_conf->offloads to rte_eth_[rt]x_queue_setup( ).
If any offloading is enabled in rte_eth_dev_configure( ) by application,
it is enabled on all queues no matter whether it is per-queue or
per-port type and no matter whether it is set or cleared in
[rt]x_conf->offloads to rte_eth_[rt]x_queue_setup( ).
If a per-queue offloading hasn't be enabled in rte_eth_dev_configure( ),
it can be enabled or disabled for individual queue in
ret_eth_[rt]x_queue_setup( ).
A new added offloading is the one which hasn't been enabled in
rte_eth_dev_configure( ) and is reuqested to be enabled in
rte_eth_[rt]x_queue_setup( ), it must be per-queue type,
otherwise return error.
The underlying PMD must be aware that the requested offloadings
to PMD specific queue_setup( ) function only carries those
new added offloadings of per-queue type.

This patch can make above such checking in a common way in rte_ethdev
layer to avoid same checking in underlying PMD.

This patch assumes that all PMDs in 18.05-rc2 have already
converted to offload API defined in 17.11 . It also assumes
that all PMDs can return correct offloading capabilities
in rte_eth_dev_infos_get( ).

In the beginning of [rt]x_queue_setup( ) of underlying PMD,
add offloads = [rt]xconf->offloads |
dev->data->dev_conf.[rt]xmode.offloads; to keep same as offload API
defined in 17.11 to avoid upper application broken due to offload
API change.
PMD can use the info that input [rt]xconf->offloads only carry
the new added per-queue offloads to do some optimization or some
code change on base of this patch.

Signed-off-by: Wei Dai <wei.dai@intel.com>
Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>

[...]


diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index e42d553..fc2b254 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -413,14 +413,16 @@ sfc_rx_queue_setup(struct rte_eth_dev *dev, uint16_t rx_queue_id,
 {
 	struct sfc_adapter *sa = dev->data->dev_private;
 	int rc;
+	uint64_t offloads;
 
 	sfc_log_init(sa, "RxQ=%u nb_rx_desc=%u socket_id=%u",
 		     rx_queue_id, nb_rx_desc, socket_id);
 
 	sfc_adapter_lock(sa);
 
+	offloads = rx_conf->offloads | dev->data->dev_conf.rxmode.offloads;

I'd prefer to see it inside sfc_rx_qinit() function. It would allow to avoid
sfc_rx_qinit() function prototype changes.
[Wei: As rx_conf is a const argument in sfc_rx_queue_setup( ), rx_conf->offloads can't be updated.
  If sfc_rx_qinit( ) function prototype keep unchanged,  the dev->data can be deduced from the
 1st argument sa .  Andrew, if my code works well, can it be kept here,  you can change it later
 in your separate patch, OK ? ]

 	rc = sfc_rx_qinit(sa, rx_queue_id, nb_rx_desc, socket_id,
-			  rx_conf, mb_pool);
+			  rx_conf, mb_pool, offloads);
 	if (rc != 0)
 		goto fail_rx_qinit;
 
@@ -469,13 +471,16 @@ sfc_tx_queue_setup(struct rte_eth_dev *dev, uint16_t tx_queue_id,
 {
 	struct sfc_adapter *sa = dev->data->dev_private;
 	int rc;
+	uint64_t offloads;
 
 	sfc_log_init(sa, "TxQ = %u, nb_tx_desc = %u, socket_id = %u",
 		     tx_queue_id, nb_tx_desc, socket_id);
 
 	sfc_adapter_lock(sa);
 
-	rc = sfc_tx_qinit(sa, tx_queue_id, nb_tx_desc, socket_id, tx_conf);
+	offloads = tx_conf->offloads | dev->data->dev_conf.txmode.offloads;

Same as above.


+	rc = sfc_tx_qinit(sa, tx_queue_id, nb_tx_desc, socket_id,
+			  tx_conf, offloads);
 	if (rc != 0)
 		goto fail_tx_qinit;

[...]

It looks like device level offloads are checked on ethdev layer now.
So, I think check in sfc_rx_check_mode () and sfc_tx_check_mode()
may be removed as well (see offloads_rejected).
I think it will make functions sfc_rx_log_offloads() and
sfc_tx_log_offloads() unused and these functions should be removed.
[Wei: sorry, I miss the sf_[rt]x_check_mode( ). Yes, offloads_rejected in this function checking can be removed.
 But as this patch is a big one, I'd like it keep as it is if it works well. And I'd like you remove such checking or 
do other changes in your own patch]
[Wei: by the way, your mail is HTML format,  I am afraid it is missed from mail archives.]