From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [67.231.154.164]) by dpdk.org (Postfix) with ESMTP id B4DD55F13 for ; Wed, 21 Mar 2018 12:19:40 +0100 (CET) X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (webmail.solarflare.com [12.187.104.26]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1-us4.ppe-hosted.com (Proofpoint Essentials ESMTP Server) with ESMTPS id 82B9B280064; Wed, 21 Mar 2018 11:19:38 +0000 (UTC) Received: from [192.168.38.17] (84.52.114.114) by ocex03.SolarFlarecom.com (10.20.40.36) with Microsoft SMTP Server (TLS) id 15.0.1044.25; Wed, 21 Mar 2018 04:19:33 -0700 To: Shahaf Shuler , Ferruh Yigit , John McNamara , Marko Kovacevic CC: "dev@dpdk.org" , Thomas Monjalon , "Patil@dpdk.org" , Harish , Ivan Malov References: <44e451f86e4582815767cf75b4e0f01f5cc60b5f.1507104596.git.shahafs@mellanox.com> <20180316155138.125423-1-ferruh.yigit@intel.com> <5efda914-7017-9095-2546-ae6e4c627295@solarflare.com> <4a4330be-a8c4-599d-d8a7-3703e5af285c@intel.com> <49e3a7c8-cda2-c129-70a4-6a166fa1b466@solarflare.com> From: Andrew Rybchenko Message-ID: <4b3cdf50-165e-e3c6-6fb0-d354e0d0dc91@solarflare.com> Date: Wed, 21 Mar 2018 14:19:30 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: Content-Language: en-GB X-Originating-IP: [84.52.114.114] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ocex03.SolarFlarecom.com (10.20.40.36) X-MDID: 1521631179-fmQTLwzAA1RZ Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH] doc: update new ethdev offload API description X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 21 Mar 2018 11:19:41 -0000 On 03/21/2018 02:10 PM, Shahaf Shuler wrote: > Wednesday, March 21, 2018 1:09 PM, Andrew Rybchenko >> On 03/21/2018 01:54 PM, Ferruh Yigit wrote: >>> On 3/21/2018 9:47 AM, Andrew Rybchenko wrote: >>>> On 03/16/2018 06:51 PM, Ferruh Yigit wrote: >>>>> Don't mandate API to pass port offload configuration during queue >>>>> setup, this is unnecessary for devices that support only port level >> offloads. >>>>> Fixes: 81ac560dc1b4 ("doc: add details on ethdev offloads API") >>>>> Cc: shahafs@mellanox.com >>>>> >>>>> Signed-off-by: Ferruh Yigit >>>>> --- >>>>> Cc: Patil, Harish >>>>> >>>>> Btw, this expectation from API should be clear from source code and >>>>> API documentation (doxygen comments in header file) instead of >>>>> documentation. Am I missing something or we are doing something >>>>> wrong here? >>>>> --- >>>>> doc/guides/prog_guide/poll_mode_drv.rst | 4 +--- >>>>> 1 file changed, 1 insertion(+), 3 deletions(-) >>>>> >>>>> diff --git a/doc/guides/prog_guide/poll_mode_drv.rst >>>>> b/doc/guides/prog_guide/poll_mode_drv.rst >>>>> index e5d01874e..3247f309f 100644 >>>>> --- a/doc/guides/prog_guide/poll_mode_drv.rst >>>>> +++ b/doc/guides/prog_guide/poll_mode_drv.rst >>>>> @@ -303,9 +303,7 @@ Supported offloads can be either per-port or per- >> queue. >>>>> Offloads are enabled using the existing ``DEV_TX_OFFLOAD_*`` or >> ``DEV_RX_OFFLOAD_*`` flags. >>>>> Per-port offload configuration is set using ``rte_eth_dev_configure``. >>>>> Per-queue offload configuration is set using >> ``rte_eth_rx_queue_setup`` and ``rte_eth_tx_queue_setup``. >>>>> -To enable per-port offload, the offload should be set on both device >> configuration and queue setup. >>>>> -In case of a mixed configuration the queue setup shall return with an >> error. >>>>> -To enable per-queue offload, the offload can be set only on the queue >> setup. >>>>> +Per-port offloads should be set on the port configuration. Queue >> offloads should be set on the queue configuration. >>>>> Offloads which are not enabled are disabled by default. >>>>> >>>>> For an application to use the Tx offloads API it should set the >> ``ETH_TXQ_FLAGS_IGNORE`` flag in the ``txq_flags`` field located in >> ``rte_eth_txconf`` struct. >>>> net/sfc has code which double-checks old behaviour. So, it is not >>>> just documentation update. We can provide patches if the behaviour >>>> change is accepted. >>> Not definitely just doc update, PMDs needs to be modified. This patch >>> is just to agree on the behavior. >>> >>>> IMHO, it should be allowed to specify queue offloads on port level. >>>> It should simply enable these offloads on all queues. Also it will >>>> match dev_info [rt]x_offload_capa which include both port and queue >>>> offloads. >>>> >>>> Yes, we lose possibility to enable on port level, but disable on >>>> queue level by suggested changes, but I think it is OK - if you don't >>>> need it for all queues, just control separately on queue level. >>> What I understand was queue offload can only enable more, but it seems >>> it can both enable or disable. >>> >>> My concern was, even PMD reports no [rt]x_offload_capa at all, API >>> forces application to send at least port offloads during queue setup. >> I guess you mean [rt]x_queue_offload_capa above. >> >>> As long as application only allowed to send queue offloads within the >>> boundaries of the "queue offload capabilities", I am OK. >> If so, queue offloads should not be included in [rt]x_offload_capa. >> But I'm afraid it is too restrictive for apps. >> >>> This will work fine for devices that support queue level offloads to >>> enable - disable queue specific offloads on top of port offloads. Will this >> make sense? >> >> IMHO, disable on queue level is not required for enabled on port level. >> If app always wants some offloads, just check [rt]x_offload_capa and enable >> on port level (regardless if it is actually per-port or per-queue). >> If app wants to some offload per queue, check [rt]x_queue_offload_capa, >> do not enable on port level and control on queue level. > +1. > > And I think Ferruh this is the suggestion by this patch, isn't it? Not exactly. We should add statement to allow to enable queue offloads on port level (to enable on all queues).