DPDK patches and discussions
 help / color / mirror / Atom feed
From: Ferruh Yigit <ferruh.yigit@intel.com>
To: Andrew Rybchenko <arybchenko@solarflare.com>,
	Wei Dai <wei.dai@intel.com>,
	thomas@monjalon.net, shahafs@mellanox.com, qi.z.zhang@intel.com
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v8] ethdev: check Rx/Tx offloads
Date: Wed, 9 May 2018 15:11:11 +0100	[thread overview]
Message-ID: <ad104324-11b6-257f-2c8c-375ab4118777@intel.com> (raw)
In-Reply-To: <839659ef-9ac8-5a67-37fb-b84be9e46dca@solarflare.com>

On 5/8/2018 6:51 PM, Andrew Rybchenko wrote:
> On 05/08/2018 01:10 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/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>> index e560524..523a07b 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;
>> +	}
>> +	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) !=
>> @@ -1504,6 +1526,39 @@ rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
>>   						    &local_conf.offloads);
>>   	}
>>   
>> +	/*
>> +	 * 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;
> 
> In fact it has a problem with MULTI_SEG.
> It is a new offload which was introduced to substitute NOMULTISEG TxQ flag.
> If PMD reports the offload on device level, but application does not use
> the new interface and request the offload on queue level using txq_flags
> (absent NOMULTISEG flag). As the result below check fails.
> Sounds like it requires dedicated code to handle it. Anything else?

Old applications don't set Tx offloads on configure() at all. But they set them
in tx_queue_setup().
For the PMDs that support only port offloads old applications will fail in
tx_queue_setup(), if I don't miss anything.

I suggest keeping the log but remove the error return until all applications
switch to new offloading API which should be on next release. What do you think?


btw, the log macro "RTE_PMD_DEBUG_TRACE" is in ERR log type but it controlled by
CONFIG_RTE_LIBRTE_ETHDEV_DEBUG config option which is disabled by default. So by
default we don't get the error log but the error only. I suggest switching to
new dynamic logging ethdev_log()


> 
>> +
>> +	/*
>> +	 * New added offloadings for this queue are those not enabled in
>> +	 * rte_eth_dev_configure( ) and they must be per-queue type.
>> +	 * A pure per-port offloading can't be enabled on a queue while
>> +	 * disabled on another queue. A pure per-port offloading can't
>> +	 * be enabled for any queue as new added one if it hasn't been
>> +	 * enabled in rte_eth_dev_configure( ).
>> +	 */
>> +	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, new "
>> +				    "added offloads 0x" PRIx64 " must be "
>> +				    "within pre-queue offload capabilities 0x"
>> +				    PRIx64 " in %s\n",
>> +				    port_id,
>> +				    rx_queue_id,
>> +				    local_conf.offloads,
>> +				    dev_info.rx_queue_offload_capa,
>> +				    __func__);
>> +		return -EINVAL;
>> +	}
>> +
>>   	ret = (*dev->dev_ops->rx_queue_setup)(dev, rx_queue_id, nb_rx_desc,
>>   					      socket_id, &local_conf, mp);
>>   	if (!ret) {
> 
> [...]
> 

  parent reply	other threads:[~2018-05-09 14:11 UTC|newest]

Thread overview: 60+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-02-01 13:53 [dpdk-dev] [PATCH] ethdev: check consistency of per port offloads Wei Dai
2018-03-28  8:57 ` [dpdk-dev] [PATCH v2] ethdev: check Rx/Tx offloads Wei Dai
2018-04-13 17:31   ` Ferruh Yigit
2018-04-15 10:37     ` Thomas Monjalon
2018-04-16  3:06       ` Dai, Wei
2018-04-25 11:26   ` [dpdk-dev] [PATCH] " Wei Dai
2018-04-25 11:31   ` [dpdk-dev] [PATCH v3] " Wei Dai
2018-04-25 11:49     ` Wei Dai
2018-04-25 11:50   ` [dpdk-dev] [PATCH v4] " Wei Dai
2018-04-25 17:04     ` Ferruh Yigit
2018-04-26  7:59       ` Zhang, Qi Z
2018-04-26  8:18         ` Thomas Monjalon
2018-04-26  8:51           ` Zhang, Qi Z
2018-04-26 14:45             ` Dai, Wei
2018-04-26 14:37     ` [dpdk-dev] [PATCH v5] " Wei Dai
2018-04-26 15:50       ` Ferruh Yigit
2018-04-26 15:56         ` Thomas Monjalon
2018-04-26 15:59           ` Ferruh Yigit
2018-04-26 16:11         ` Ferruh Yigit
2018-05-03  1:30       ` [dpdk-dev] [PATCH v6] " Wei Dai
2018-05-04 11:12         ` Ferruh Yigit
2018-05-04 14:02         ` [dpdk-dev] [PATCH v7] " Wei Dai
2018-05-04 14:42           ` Ferruh Yigit
2018-05-04 14:45             ` Ferruh Yigit
2018-05-05 18:59           ` Shahaf Shuler
2018-05-07  7:15             ` Dai, Wei
2018-05-08 10:58             ` Ferruh Yigit
2018-05-08 10:05           ` [dpdk-dev] [PATCH v8] " Wei Dai
2018-05-08 10:41             ` Andrew Rybchenko
2018-05-08 11:02               ` Ferruh Yigit
2018-05-08 11:22                 ` Andrew Rybchenko
2018-05-08 11:37             ` Andrew Rybchenko
2018-05-08 12:34               ` Dai, Wei
2018-05-08 12:12             ` Ferruh Yigit
2018-05-09 12:45               ` Dai, Wei
2018-05-10  0:49             ` [dpdk-dev] [PATCH v9] ethdev: new Rx/Tx offloads API Wei Dai
2018-05-10  0:56               ` [dpdk-dev] [PATCH v10] " Wei Dai
2018-05-10  1:28                 ` Ferruh Yigit
2018-05-10  2:35                 ` Thomas Monjalon
2018-05-10 11:27                   ` Dai, Wei
2018-05-10  9:25                 ` Andrew Rybchenko
2018-05-10 19:47                   ` Ferruh Yigit
2018-05-10 11:30                 ` [dpdk-dev] [PATCH v11] " Wei Dai
2018-05-10 11:56                   ` [dpdk-dev] [PATCH v12] " Wei Dai
2018-05-10 21:39                     ` Thomas Monjalon
2018-05-14  8:37                       ` Thomas Monjalon
2018-05-14 11:19                         ` Dai, Wei
2018-05-10 21:48                     ` Ferruh Yigit
2018-05-14 12:00                     ` [dpdk-dev] [PATCH v13] " Wei Dai
2018-05-14 12:54                       ` Thomas Monjalon
2018-05-14 13:26                         ` Dai, Wei
2018-05-14 13:20                       ` [dpdk-dev] [PATCH v14] " Wei Dai
2018-05-14 14:11                         ` Thomas Monjalon
2018-05-14 14:46                           ` Ferruh Yigit
2018-05-10 21:08                 ` [dpdk-dev] [PATCH v10] " Ferruh Yigit
2018-05-08 10:10           ` [dpdk-dev] [PATCH v8] ethdev: check Rx/Tx offloads Wei Dai
2018-05-08 17:51             ` Andrew Rybchenko
2018-05-09  2:10               ` Dai, Wei
2018-05-09 14:11               ` Ferruh Yigit [this message]
2018-05-09 22:40                 ` 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=ad104324-11b6-257f-2c8c-375ab4118777@intel.com \
    --to=ferruh.yigit@intel.com \
    --cc=arybchenko@solarflare.com \
    --cc=dev@dpdk.org \
    --cc=qi.z.zhang@intel.com \
    --cc=shahafs@mellanox.com \
    --cc=thomas@monjalon.net \
    --cc=wei.dai@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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).