[dpdk-dev,v4] ethdev: check Rx/Tx offloads
Checks
Commit Message
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.
Signed-off-by: Wei Dai <wei.dai@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(+)
Comments
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@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?
[1]
https://dpdk.org/ml/archives/dev/2018-April/098956.html
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Thursday, April 26, 2018 1:05 AM
> To: Dai, Wei <wei.dai@intel.com>; thomas@monjalon.net; Zhang, Qi Z
> <qi.z.zhang@intel.com>
> Cc: dev@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@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.
Regards
Qi.
>
>
> [1]
> https://dpdk.org/ml/archives/dev/2018-April/098956.html
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@intel.com>; thomas@monjalon.net; Zhang, Qi Z
> > <qi.z.zhang@intel.com>
> > Cc: dev@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@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.
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, April 26, 2018 4:19 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Dai, Wei <wei.dai@intel.com>;
> dev@dpdk.org
> Subject: Re: [PATCH v4] ethdev: check Rx/Tx offloads
>
> 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@intel.com>; thomas@monjalon.net; Zhang, Qi Z
> > > <qi.z.zhang@intel.com>
> > > Cc: dev@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@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.
OK, got your point, that make sense.
> 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.
>
>
Thanks to Thomas, Ferruh and Zhang Qi for your feedback.
I will rework v5 patch to follow your guidance.
> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Thursday, April 26, 2018 4:51 PM
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Dai, Wei <wei.dai@intel.com>;
> dev@dpdk.org
> Subject: RE: [PATCH v4] ethdev: check Rx/Tx offloads
>
>
>
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Sent: Thursday, April 26, 2018 4:19 PM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Dai, Wei
> > <wei.dai@intel.com>; dev@dpdk.org
> > Subject: Re: [PATCH v4] ethdev: check Rx/Tx offloads
> >
> > 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@intel.com>; thomas@monjalon.net; Zhang, Qi Z
> > > > <qi.z.zhang@intel.com>
> > > > Cc: dev@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@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.
>
> OK, got your point, that make sense.
>
> > 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.
> >
> >
@@ -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;
+ }
+
/*
* 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;
+ }
+
+ /*
+ * 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;
+
ret = (*dev->dev_ops->rx_queue_setup)(dev, rx_queue_id, nb_rx_desc,
socket_id, &local_conf, mp);
if (!ret) {
@@ -1681,6 +1730,33 @@ rte_eth_tx_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
&local_conf.offloads);
}
+ /*
+ * Only per-queue offload can be enabled from applcation.
+ * If any pure per-port offload is sent to this function, return -EINVAL
+ */
+ if ((local_conf.offloads & dev_info.tx_queue_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 rte_eth_tx_queue_setup( )\n",
+ port_id,
+ tx_queue_id,
+ local_conf.offloads,
+ dev_info.tx_queue_offload_capa);
+ return -EINVAL;
+ }
+
+ /*
+ * 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.txmode.offloads;
+
return eth_err(port_id, (*dev->dev_ops->tx_queue_setup)(dev,
tx_queue_id, nb_tx_desc, socket_id, &local_conf));
}