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

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

Thanks to Shuler and Ferruh for your feedback and guidance.

PMD at least has these 2 options with this patch:
a). If PMD doesn't want to make much more changes, it still can do "[rt]x_conf->offloads |= dev->data->dev_conf.rxmode.offloads;"
   in the beginning of its specific queue_setup( ) and just remove offload checking (although the checking always pass now) and all
   others keep same. In this way, PMDs still comply with the offload APIs defined in 17.11.
b). PMD also can use the info that only new added queue-level offloads in the input argument [rt]x_conf->offloads to make some
   optimization or other code changes. It may be more efficient than a).

As Ferruh said, only this patch and without relevant change in PMD will cause application broken,
I will submit v8 patch which will include this patch for ethdev and code changes in PMDs with above option a and document update.
I'd like include all these changes in only one patch to avoid application failure if some patches are not applied and some are applied.
PMD maintainers call go on with option b) 

Shuler's suggestion to simplify the new added offloads in queue_setup( ) is better.
I will adopt it in my v8 patch.

> -----Original Message-----
> From: Shahaf Shuler [mailto:shahafs@mellanox.com]
> Sent: Sunday, May 6, 2018 3:00 AM
> To: Dai, Wei <wei.dai@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v7] ethdev: check Rx/Tx offloads
> 
> Hi Ferruh, Dai,
> > Subject: [dpdk-dev] [PATCH v7] ethdev: check Rx/Tx offloads
> >
> > 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( ).
> > From application, a pure per-port offloading can't be enabled on any
> > queue if it hasn't been enabled in rte_eth_dev_configure( ).
> > 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( ).
> > The underlying PMD must be aware that the requested offloadings to PMD
> > specific queue_setup( ) function only carries those offloadings only
> > enabled for the queue but not enabled in rte_eth_dev_configure( ) and
> > they are certain 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.
> >
> > Signed-off-by: Wei Dai <wei.dai@intel.com>
> > Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> >
> > ---
> > v7:
> > Give the maximum freedom for upper application, only minimal checking
> > is performed in ethdev layer.
> > Only requested specific pure per-queue offloadings are input to
> > underlying PMD.
> >
> > v6:
> > No need enable an offload in queue_setup( ) if it has already been
> > enabled in dev_configure( )
> >
> > v5:
> > keep offload settings sent to PMD same as those from application
> >
> > v4:
> > fix a wrong description in git log message.
> >
> > v3:
> > rework according to dicision of offloading API in community
> >
> > v2:
> > add offloads checking in rte_eth_dev_configure( ).
> > check if a requested offloading is supported.
> > ---
> >  lib/librte_ethdev/rte_ethdev.c | 150
> > +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 150 insertions(+)
> >
> > diff --git a/lib/librte_ethdev/rte_ethdev.c
> > b/lib/librte_ethdev/rte_ethdev.c index e560524..0ad05eb 100644
> > --- a/lib/librte_ethdev/rte_ethdev.c
> > +++ b/lib/librte_ethdev/rte_ethdev.c
> > @@ -1139,6 +1139,28 @@ rte_eth_dev_configure(uint16_t port_id,
> > uint16_t nb_rx_q, uint16_t nb_tx_q,
> >  							ETHER_MAX_LEN;
> >  	}
> >
> > +	/* Any requested offloading must be within its device capabilities */
> > +	if ((local_conf.rxmode.offloads & dev_info.rx_offload_capa) !=
> > +	     local_conf.rxmode.offloads) {
> > +		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d requested Rx
> > offloads "
> > +				    "0x%" PRIx64 " doesn't match Rx offloads "
> > +				    "capabilities 0x%" PRIx64 "\n",
> > +				    port_id,
> > +				    local_conf.rxmode.offloads,
> > +				    dev_info.rx_offload_capa);
> > +		return -EINVAL;
> 
> While I am OK with such behavior, we should be more careful not to get into
> the same issue as in [1].
> There are PMD which don't report the capabilities correctly however do
> expect to have the offload configured.
> 
> All I am saying it is worth a check and cautious decision if it is right to include
> this one w/o prior application notice and at such late RC of the release.
> 
> > +	}
> > +	if ((local_conf.txmode.offloads & dev_info.tx_offload_capa) !=
> > +	     local_conf.txmode.offloads) {
> > +		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d requested Tx
> > offloads "
> > +				    "0x%" PRIx64 " doesn't match Tx offloads "
> > +				    "capabilities 0x%" PRIx64 "\n",
> > +				    port_id,
> > +				    local_conf.txmode.offloads,
> > +				    dev_info.tx_offload_capa);
> > +		return -EINVAL;
> > +	}
> > +
> >  	/* Check that device supports requested rss hash functions. */
> >  	if ((dev_info.flow_type_rss_offloads |
> >  	     dev_conf->rx_adv_conf.rss_conf.rss_hf) != @@ -1414,6 +1436,8
> @@
> > rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
> >  	struct rte_eth_dev_info dev_info;
> >  	struct rte_eth_rxconf local_conf;
> >  	void **rxq;
> > +	uint64_t pure_port_offload_capa;
> > +	uint64_t only_enabled_for_queue;
> >
> >  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
> >
> > @@ -1504,6 +1528,68 @@ rte_eth_rx_queue_setup(uint16_t port_id,
> > uint16_t rx_queue_id,
> >  						    &local_conf.offloads);
> >  	}
> >
> > +	/*
> > +	 * The requested offloadings by application for this queue
> > +	 * can be per-queue type or per-port type. and
> > +	 * they must be within the device offloading capabilities.
> > +	 */
> > +	if ((local_conf.offloads & dev_info.rx_offload_capa) !=
> > +	     local_conf.offloads) {
> > +		RTE_PMD_DEBUG_TRACE("Ethdev port_id=%d
> > rx_queue_id=%d "
> > +				    "Requested offload 0x%" PRIx64 "doesn't "
> > +				    "match per-queue capability 0x%" PRIx64
> > +				    " in %s\n",
> > +				    port_id,
> > +				    rx_queue_id,
> > +				    local_conf.offloads,
> > +				    dev_info.rx_queue_offload_capa,
> > +				    __func__);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/*
> > +	 * A pure per-port offloading can't be enabled for any queue
> > +	 * if it hasn't been enabled in rte_eth_dev_configure( ).
> > +	 *
> > +	 * Following pure_port_offload_capa is the capabilities which
> > +	 * can't be enabled on some queue while disabled on other queue.
> > +	 * pure_port_offload_capa must be enabled or disabled on all
> > +	 * queues at same time.
> > +	 *
> > +	 * Following only_enabled_for_queue is the offloadings which
> > +	 * are enabled for this queue but hasn't been enabled in
> > +	 * rte_eth_dev_configure( ).
> > +	 */
> > +	pure_port_offload_capa = dev_info.rx_offload_capa ^
> > +				 dev_info.rx_queue_offload_capa;
> > +	only_enabled_for_queue = (local_conf.offloads ^
> > +		dev->data->dev_conf.rxmode.offloads) &
> > local_conf.offloads;
> 
> It looks like above logic could be a lot simpler.
> 
> How about:
> local_conf.offloads &= ~dev->data->dev_conf.rxmode.offloads; // keep
> only the added offloads on top of the port ones if ((local_conf.offloads &
> dev_info.rx_queue_offload_capa) !=
>     local_conf.offloads) { //check if added offloads are part of the queue
> offload capa
> 	ERROR...
> 
> 
> > +	if (only_enabled_for_queue & pure_port_offload_capa) {
> > +		RTE_PMD_DEBUG_TRACE("Ethdev port_id=%d
> > rx_queue_id=%d, only "
> > +				    "enabled offload 0x%" PRIx64 "for this "
> > +				    "queue haven't been enabled in "
> > +				    "dev_configure( ), they are within "
> > +				    "pure per-port capabilities 0x%" PRIx64
> 
> Need to re-work this error message. The user doesn't know what are "pure
> per-port capabilities"
> 
> > +				    " in %s\n",
> > +				    port_id,
> > +				    rx_queue_id,
> > +				    only_enabled_for_queue,
> > +				    pure_port_offload_capa,
> > +				    __func__);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/*
> > +	 * If an offloading has already been enabled in
> > +	 * rte_eth_dev_configure(), it has been enabled on all queues,
> > +	 * so there is no need to enable it in this queue again.
> > +	 * The local_conf.offloads input to underlying PMD only carries
> > +	 * those offloadings which are only enabled on this queue and
> > +	 * not enabled on all queues.
> > +	 * The underlying PMD must be aware of this point.
> > +	 */
> > +	local_conf.offloads &= ~dev->data->dev_conf.rxmode.offloads;
> > +
> >  	ret = (*dev->dev_ops->rx_queue_setup)(dev, rx_queue_id, nb_rx_desc,
> >  					      socket_id, &local_conf, mp);
> >  	if (!ret) {
> > @@ -1549,6 +1635,8 @@ rte_eth_tx_queue_setup(uint16_t port_id,
> > uint16_t tx_queue_id,
> >  	struct rte_eth_dev_info dev_info;
> >  	struct rte_eth_txconf local_conf;
> >  	void **txq;
> > +	uint64_t pure_port_offload_capa;
> > +	uint64_t only_enabled_for_queue;
> >
> >  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
> >
> > @@ -1612,6 +1700,68 @@ rte_eth_tx_queue_setup(uint16_t port_id,
> > uint16_t tx_queue_id,
> >  					  &local_conf.offloads);
> >  	}
> >
> > +	/*
> > +	 * The requested offloadings by application for this queue
> > +	 * can be per-queue type or per-port type. and
> > +	 * they must be within the device offloading capabilities.
> > +	 */
> > +	if ((local_conf.offloads & dev_info.tx_offload_capa) !=
> > +	     local_conf.offloads) {
> > +		RTE_PMD_DEBUG_TRACE("Ethdev port_id=%d
> > tx_queue_id=%d "
> > +				    "Requested offload 0x%" PRIx64 "doesn't "
> > +				    "match per-queue capability 0x%" PRIx64
> > +				    " in %s\n",
> > +				    port_id,
> > +				    tx_queue_id,
> > +				    local_conf.offloads,
> > +				    dev_info.tx_queue_offload_capa,
> > +				    __func__);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/*
> > +	 * A pure per-port offloading can't be enabled for any queue
> > +	 * if it hasn't been enabled in rte_eth_dev_configure( ).
> > +	 *
> > +	 * Following pure_port_offload_capa is the capabilities which
> > +	 * can't be enabled on some queue while disabled on other queue.
> > +	 * pure_port_offload_capa must be enabled or disabled on all
> > +	 * queues at same time.
> > +	 *
> > +	 * Following only_enabled_for_queue is the offloadings which
> > +	 * are enabled for this queue but hasn't been enabled in
> > +	 * rte_eth_dev_configure( ).
> > +	 */
> > +	pure_port_offload_capa = dev_info.tx_offload_capa ^
> > +				 dev_info.tx_queue_offload_capa;
> > +	only_enabled_for_queue = (local_conf.offloads ^
> > +		dev->data->dev_conf.txmode.offloads) &
> > local_conf.offloads;
> 
> Same comments as in the Rx part.
> 
> > +	if (only_enabled_for_queue & pure_port_offload_capa) {
> > +		RTE_PMD_DEBUG_TRACE("Ethdev port_id=%d
> > tx_queue_id=%d, only "
> > +				    "enabled offload 0x%" PRIx64 "for this "
> > +				    "queue haven't been enabled in "
> > +				    "dev_configure( ), they are within "
> > +				    "pure per-port capabilities 0x%" PRIx64
> > +				    " in %s\n",
> > +				    port_id,
> > +				    tx_queue_id,
> > +				    only_enabled_for_queue,
> > +				    pure_port_offload_capa,
> > +				    __func__);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/*
> > +	 * If an offloading has already been enabled in
> > +	 * rte_eth_dev_configure(), it has been enabled on all queues,
> > +	 * so there is no need to enable it in this queue again.
> > +	 * The local_conf.offloads input to underlying PMD only carries
> > +	 * those offloadings which are only enabled on this queue and
> > +	 * not enabled on all queues.
> > +	 * The underlying PMD must be aware of this point.
> > +	 */
> > +	local_conf.offloads &= ~dev->data->dev_conf.txmode.offloads;
> > +
> >  	return eth_err(port_id, (*dev->dev_ops->tx_queue_setup)(dev,
> >  		       tx_queue_id, nb_tx_desc, socket_id, &local_conf));  }
> > --
> > 2.7.5
> 
> 
> As for Ferruh's comment
> >
> > PMDs needs to be updated for:
> > 1- Remove existing offload verify checks
> > 2- Update offload configure logic based on new values
> >
> > (1) can be part of this patch. But PMD maintainers should send update
> > for (2) if a change required.
> >
> >cc'ed Shahaf, specially for (2) one.
> 
> I think PMD maintainers can help with that. If it will be integrated enough
> time before the release Mellanox PMDs can be converted by us.
> 
> 
> 
> 
> [1]
> http://dpdk.org/dev/patchwork/patch/38645/
>