From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 29C966D45 for ; Thu, 15 Mar 2018 15:34:31 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 15 Mar 2018 07:34:30 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,311,1517904000"; d="scan'208";a="211604503" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.237.221.62]) ([10.237.221.62]) by fmsmga006.fm.intel.com with ESMTP; 15 Mar 2018 07:34:29 -0700 To: Shahaf Shuler , Mordechay Haimovsky , "pascal.mazon@6wind.com" Cc: "dev@dpdk.org" References: <1515601248-39458-2-git-send-email-motih@mellanox.com> <1516197874-133169-1-git-send-email-motih@mellanox.com> <1516197874-133169-2-git-send-email-motih@mellanox.com> <95d434f5-438a-19a7-1227-18c1230201c0@intel.com> <8973efd1-ec77-2e51-0516-634ab878bb1c@intel.com> <54a887f7-5d2c-4200-de87-1a96a68df0cd@intel.com> <44688765-996e-4a76-005c-9d2d42fe29da@intel.com> <457bcfb5-b2bc-8616-1cb4-c9b6fdeb0e57@intel.com> From: Ferruh Yigit Message-ID: <271afafa-a3ab-fcb1-6d9a-da3cfd68afdf@intel.com> Date: Thu, 15 Mar 2018 14:34:28 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH V5 2/2] net/tap: use new Rx offloads API 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: Thu, 15 Mar 2018 14:34:31 -0000 On 3/15/2018 6:16 AM, Shahaf Shuler wrote: > Thursday, March 15, 2018 12:41 AM, Ferruh Yigit: >> On 3/14/2018 5:49 AM, Shahaf Shuler wrote: >>> Tuesday, March 13, 2018 1:57 PM, Ferruh Yigit: >>>>> >>>>> Again - the application should follow the API which currently >>>>> dictates how >>>> to set port offload. It is not depends on the rx_queue_offloads >> capabilities. >>>>> For example, PMD which don't support queue offloads can still have >>>> verification for the API that each port offload is set also on the >>>> queue offloads. >>>> >>>> I am not agree with this part, why to dictate application to set >>>> queue offloads if it already knows device doesn't support queue specific >> offloads? >>> >>> I agree we can make a small change in the API to not force the application >> to set the port offloads in the queue configuration. It makes sense. >>> The change will be: >>> "port offloads should be set on the port configuration. Queue offloads >> should be set on the queue configuration" >> >> I am OK to this one, this is more reasonable for devices that support only port >> level offloads. >> >> This looks like same as option #2 mentioned in the previous mails. >> >>> >>>> >>>> In some of the existing PMD patches, to switch to new offloading API, >>>> PMD sets [rt]x_queue_offload_capa as same as [rt]x_offload_capa, >>> >>> Well this is just wrong. Unless those PMDs support all the offloads in a >> queue level. >>> >>> The logic is "every queue offload can be counted as port offload", because >> such offload can be set on each and every queue. >>> The other way around is not correct, port offload cannot be counted as >> queue offload. >>> >>> So if such PMDs has offloads which are supported only on the port level >> they cannot be declared as queue offloads. >> >> Thanks for confirming, it would be great if you can help on the PMD new >> offload API patch reviews, to catch these kind of issues. > > Sure, have me Cc in the patches so It can pass through my mailbox filters. > >> >>> >>> >>>> in that case >>>> application can't know if queue specific offloads are supported or >>>> not and application may try to set queue offloads, this forces PMD to >> verify them. >>>> >>>> You confirmed [rt]x_queue_offload_capa is the way for application to >>>> know if device supports queue specific offloads or not. If these >>>> values always set to [rt]x_offload_capa, application losts this capability. >>>> >>>> Instead: >>>> - PMD that doesn't support queue specific offloads should set >>>> [rt]x_queue_offload_capa to 0 >>>> - When [rt]x_queue_offload_capa is 0, application should be free to >>>> set queue offloads whatever it wants >>> >>> I don't agree, when queue_offload_capa is 0 the expected behavior from >> application is not to set any offload (if we do the change in the API that you >> are pushing to). >>> PMDs can verify it or not, but if capability is not set the application should >> not set the offload. This is how the API should be defined. >> >> OK for this one. >> >>> >>>> - When [rt]x_queue_offload_capa is 0, PMD should be free to verify >>>> queue offloads but most probably shouldn't verify them since we don't >>>> know what application will send. >>>> >>>> - When [rt]x_queue_offload_capa is != 0, applications should set >>>> queue offloads at least "[rt]x_queue_offload = [rt]x_offload" >>> >>> If we do the change you are pushing it is not needed. >>> Application will set the port offload in the port configuration, and the >> queue offload in the queue configuration. >>> No need to make special treatment based on the offloads_capa. >> >> Right. >> >>> >>>> - When [rt]x_queue_offload_capa is != 0, PMD should verify the queue >>>> offloads >>>> >> >> >> Back to initial question J, is tap supports queue level offloads? >> If not it shouldn't be reporting or checking queue offloads. >> >> >> Although it will be changed after above suggested change in API, I think >> check in existing tap queue_setup, also same in mlx5, is wrong. >> >> tap_rxq_are_offloads_valid(struct rte_eth_dev *dev, uint64_t offloads) { >> >> uint64_t port_offloads = dev->data->dev_conf.rxmode.offloads; >> uint64_t queue_supp_offloads = tap_rx_offload_get_queue_capa(); >> uint64_t port_supp_offloads = tap_rx_offload_get_port_capa(); >> >> >> <...> >> if ((port_offloads ^ offloads) & port_supp_offloads) >> return false; >> return true; >> >> } >> >> >> take the example: >> port_supp_offloads = 11111 >> port_offloads = 111 >> queue_supp_offloads = 1111 >> offloads = 1111 >> >> (port_offloads ^ offloads) & port_supp_offloads = 1000 Which will return >> false. >> >> This only works if "port_offloads == offloads" which is practically only >> supporting port level offloads. > > For mlx5, the port_supp_offloads is internal function which returns **only** the pure port offloads (the port offloads in dev_info are rx_offload_get_queue_capa() | rx_offload_get_port_capa()) > That is, offload cannot be in both port and queue offload. So the scenario above is not feasible. Right, so only tap is broken J Also, can you please verify following with mlx5: port_supp_offloads = 10000 port_offloads = 111 queue_supp_offloads = 1111 offloads = 110 Since "offloads" is missing one of the "port_offloads" it should return error but it doesn't. (111 ^ 110) & 10000 = 0 It can be helpful to comment these lines about the intention, otherwise hard to understand what exactly checked from bitwise ops.