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

[Thomas Monjalon]

26/04/2018 09:59, Zhang, Qi Z:
> 
> > -----Original Message-----
> > From: Yigit, Ferruh
> > Sent: Thursday, April 26, 2018 1:05 AM
> > To: Dai, Wei <wei.dai at intel.com>; thomas at monjalon.net; Zhang, Qi Z
> > <qi.z.zhang at intel.com>
> > Cc: dev at dpdk.org
> > Subject: Re: [PATCH v4] ethdev: check Rx/Tx offloads
> > 
> > On 4/25/2018 12:50 PM, Wei Dai wrote:
> > > This patch check if a requested offloading is supported in the device
> > > capability.
> > > Any offloading is disabled by default if it is not set in
> > > rte_eth_dev_configure( ) and rte_eth_[rt]x_queue_setup().
> > > A per port offloading can only be enabled in rte_eth_dev_configure().
> > > If a per port offloading is sent to rte_eth_[rt]x_queue_setup( ),
> > > return error.
> > > Only per queue offloading can be sent to rte_eth_[rt]x_queue_setup( ).
> > > A per queue offloading is enabled only if it is enabled in
> > > rte_eth_dev_configure( ) OR if it is enabled in
> > > rte_eth_[rt]x_queue_setup( ).
> > > If a per queue offloading is enabled in rte_eth_dev_configure(), it
> > > can't be disabled in rte_eth_[rt]x_queue_setup( ).
> > > If a per queue offloading is disabled in rte_eth_dev_configure( ), it
> > > can be enabled or disabled( ) in rte_eth_[rt]x_queue_setup( ).
> > >
> > > This patch can make such checking in a common way in rte_ethdev layer
> > > to avoid same checking in underlying PMD.
> > 
> > Hi Wei,
> > 
> > For clarification, there is existing API for rc1, and there is a suggested update
> > in API for rc2. I guess this patch is for suggested update in rc2?
> > 
> > > Signed-off-by: Wei Dai <wei.dai at intel.com>
> > >
> > > ---
> > > 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_ether/rte_ethdev.c | 76
> > > +++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 76 insertions(+)
> > >
> > > diff --git a/lib/librte_ether/rte_ethdev.c
> > > b/lib/librte_ether/rte_ethdev.c index f0f53d4..70a7904 100644
> > > --- a/lib/librte_ether/rte_ethdev.c
> > > +++ b/lib/librte_ether/rte_ethdev.c
> > > @@ -1196,6 +1196,28 @@ rte_eth_dev_configure(uint16_t port_id,
> > uint16_t nb_rx_q, uint16_t nb_tx_q,
> > >  							ETHER_MAX_LEN;
> > >  	}
> > >
> > > +	/* Any requested offload must be within its device capability */
> > > +	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 "
> > > +				    "capability 0x%" PRIx64 "\n",
> > > +				    port_id,
> > > +				    local_conf.rxmode.offloads,
> > > +				    dev_info.rx_offload_capa);
> > > +		return -EINVAL;
> > > +	}
> > > +	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 "
> > > +				    "capability 0x%" PRIx64 "\n",
> > > +				    port_id,
> > > +				    local_conf.txmode.offloads,
> > > +				    dev_info.tx_offload_capa);
> > > +		return -EINVAL;
> > > +	}
> > +1 having these checks here.
> > 
> > > +
> > >  	/*
> > >  	 * Setup new number of RX/TX queues and reconfigure device.
> > >  	 */
> > > @@ -1547,6 +1569,33 @@ rte_eth_rx_queue_setup(uint16_t port_id,
> > uint16_t rx_queue_id,
> > >  						    &local_conf.offloads);
> > >  	}
> > >
> > > +	/*
> > > +	 * Only per-queue offload can be enabled from application.
> > > +	 * If any pure per-port offload is sent to this function, return -EINVAL
> > > +	 */
> > > +	if ((local_conf.offloads & dev_info.rx_queue_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 rte_eth_rx_queue_setup( )\n",
> > > +				    port_id,
> > > +				    rx_queue_id,
> > > +				    local_conf.offloads,
> > > +				    dev_info.rx_queue_offload_capa);
> > > +		return -EINVAL;
> > > +	}
> > 
> > There is a change here. If requested offload is already enabled in port level,
> > instead of returning error, ignore it.
> > So this removes the restriction for apps that "only an offload from queue
> > capabilities can be send for queue_setup() functions". This is not
> > requirement for application as it has been before, but this is allowed for app
> > now.
> > 
> > If app tried to enable a port offload in queue level that is not already enabled,
> > it should still return error.
> > 
> > > +
> > > +	/*
> > > +	 * If a per-queue offload is enabled in rte_eth_dev_configure( ),
> > > +	 * it is also enabled on all queues and can't be disabled here.
> > > +	 * If it is diabled in rte_eth_dev_configure( ), it can be enabled
> > > +	 * or disabled here.
> > > +	 * If a per-port offload is enabled in rte_eth_dev_configure( ),
> > > +	 * it is also enabled for all queues here.
> > > +	 */
> > > +	local_conf.offloads |= dev->data->dev_conf.rxmode.offloads;
> > 
> > I didn't get this one, why add rxmode.offloads into queue offloads?
> > 
> > Based on above change Thomas has an suggestion [1]:
> > 
> > "
> > In the case of offload already enabled at port level and repeated in queue
> > setup, ethdev can avoid passing it to the PMD queue setup function.
> > "
> > 
> > So almost reverse of what you are doing, strip rxmode.offloads from
> > local_conf.offloads for PMDs. What do you think?
> 
> Should we do like below
> 	local_conf.offloads |= dev->data->dev_conf.rxmode.offloads;
> 	local_conf.offloads &= dev_info.rx_queue_offload_capa
> 
> I thinks it's better to only strip port offloads. But keep all queue offload,
>  since this is exact we going to configure the queue and during device start, it can simply iterate on each bit on local_conf.offloads to
> turn on queue offload and don't need to worry about rxmode.offloads.

No
The offloads which are already enabled at port level does not need to be
enabled again at queue level.
But the PMD can decide to not configure the offload at port level for real,
and configure the port offloads in every queue setups.
It is an implementation choice, and can be different per-offload.
So it is simpler to filter such request for queue setups.
This way, we will be sure that all offloads, requested in queue setup PMD
function, must be setup for the queue.
The PMD implementation will need to setup all the requested offloads
in queue setup, plus the port offloads which were deferred to all queues.

Hope it's clear.