From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 05797A31F3 for ; Fri, 18 Oct 2019 13:57:15 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 1DBBC1D447; Fri, 18 Oct 2019 13:57:15 +0200 (CEST) Received: from dispatchb-us1.ppe-hosted.com (dispatchb-us1.ppe-hosted.com [148.163.129.53]) by dpdk.org (Postfix) with ESMTP id 776661D426 for ; Fri, 18 Oct 2019 13:57:13 +0200 (CEST) X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by mx1-us4.ppe-hosted.com (PPE Hosted ESMTP Server) with ESMTPS id 88E924C007D; Fri, 18 Oct 2019 11:57:11 +0000 (UTC) Received: from [192.168.38.17] (91.220.146.112) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Fri, 18 Oct 2019 12:57:05 +0100 To: Ferruh Yigit , Bruce Richardson CC: Ciara Power , , , Thomas Monjalon References: <20191016154606.39218-1-ciara.power@intel.com> <5b6af628-7101-484b-01db-16272025105f@solarflare.com> <04b0cc0a-633f-212e-22ab-147b7f3e6a1a@intel.com> <20191017134341.GA912@bricha3-MOBL.ger.corp.intel.com> <983d5c2f-e287-ccc2-5d7d-0b56e9b2da3a@intel.com> <20191018101357.GA919@bricha3-MOBL.ger.corp.intel.com> From: Andrew Rybchenko Message-ID: Date: Fri, 18 Oct 2019 14:57:01 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB X-Originating-IP: [91.220.146.112] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ukex01.SolarFlarecom.com (10.17.10.4) X-TM-AS-Product-Ver: SMEX-12.5.0.1300-8.5.1010-24984.003 X-TM-AS-Result: No-14.640500-8.000000-10 X-TMASE-MatchedRID: 8HTFlOrbAtHmLzc6AOD8DfHkpkyUphL91mc05rOhnpLNWDA/tkxh/1Tm giBTbJevOj3xN8utVCedA5Lw7x/MKRTynB8ACpSldhnFihmbnwXLRD51bz5RZPeawpJ3WDjF2jN t44OdemViveUebyLf6LblYix0V8kYIg67HHizFeGie2FBq5CMBrvGYJkNeu61kOBHeeXGAHrv6F j2Xtj47Acm4twMjJx/NJzOzmWbO4QGIaHm10uKh0rM69p7lDSspYXNuN0M8fXkMnUVL5d0E7aGt +9v8VND0fYfjeKjgllSfaDNVJL5Shy/DZ8tz1CLQuFiD+xrWCwP6OWzRxLAk4pc3JtqeiRP2YYj bHXT/UmCXoRNGj51FxRePlympFUe2gl/2oj6CXRl2ityh8f8afTWYbN2f76BWoQBC/aPk1uCP1L TUo0L+LQ/6v1d5HILwzvsWxCP1hZR9D0WPDCIuLrbxxduc6FPqUdpDBnLMO07FE26mju9Ox0Grn AdMXjaNhw+wGY62+KzPYUrKL7w8Fvmaxq5YzoOZ93oz43dfXFBmlBF/IJ0fMnZhDdFIXZEXlTd2 Oire2OoyN3SROZ4xTkrNNZddYC1/9qKnyIzkmjm96eHJyFxjeRETaH2dwRc1LDQolhzEXHQDjkg BdHEzS2nrX9l2SNYhEUOngRAxNW433sKec1LAcGNvKPnBgOaWNbBpQ++I1nmWHHSYEnI8YkuS2z ciXo9bXRjJgWvj8uDL8knCR0/YxspN4FbPOcmPEoSfSIsZrvyCvICuK46cpK3xNqgdxsiInnIpb w93WbnrD72+Cvg6f7PwsR+s48n0kFhcmalTb2eAiCmPx4NwFkMvWAuahr8+gD2vYtOFhgqtq5d3 cxkNQwWxr7XDKH8/h/aokix2WttaHOLo+nr9kzMnhnoRoa61uV5iG6xR2j1IsMAEmST4A== X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--14.640500-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-24984.003 X-MDID: 1571399832-kNZhhT0hf-qx Subject: Re: [dpdk-dev] [RFC] net/null: add empty promiscuous mode functions 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 10/18/19 2:38 PM, Ferruh Yigit wrote: > On 10/18/2019 11:13 AM, Bruce Richardson wrote: >> On Thu, Oct 17, 2019 at 04:33:59PM +0100, Ferruh Yigit wrote: >>> On 10/17/2019 2:43 PM, Bruce Richardson wrote: >>>> On Thu, Oct 17, 2019 at 12:05:56PM +0100, Ferruh Yigit wrote: >>>>> On 10/17/2019 11:51 AM, Andrew Rybchenko wrote: >>>>>> On 10/17/19 1:47 PM, Ferruh Yigit wrote: >>>>>>> On 10/17/2019 11:37 AM, Andrew Rybchenko wrote: >>>>>>>> On 10/16/19 9:07 PM, Ferruh Yigit wrote: >>>>>>>>> On 10/16/2019 4:46 PM, Ciara Power wrote: >>>>>>>>>> Adding promiscuous functions prevents sample applications failing when run >>>>>>>>>> on this virtual PMD. The sample applications call promiscuous functions, >>>>>>>>>> and fail if this function call returns an error, which occurs when the >>>>>>>>>> virtual PMD does not support the promiscuous function being called. >>>>>>>>>> >>>>>>>>>> This change will be implemented for all virtual PMDs that currently do not >>>>>>>>>> have existing promiscuous functions. Multicast functions will also be >>>>>>>>>> added for virtual PMDs to prevent sample application breakages here also. >>>>>>>>> +Andrew >>>>>>>>> >>>>>>>>> With the some ethdev APIs returning error code, some sample applications stop >>>>>>>>> working with virtual interfaces, >>>>>>>>> >>>>>>>>> We can, >>>>>>>>> 1- update sample applications to ignore the errors >>>>>>>>> 2- Add dummy dev_ops support to (almost all) virtual PMDs (what this RFC suggests) >>>>>>>>> >>>>>>>>> (1) puts us back to before the ethdev APIs updated status, and this may be wrong >>>>>>>>> for the physical devices case, so I am for this RFC. >>>>>>>>> >>>>>>>>> Only perhaps we can have some common empty function and keep assigning that one >>>>>>>>> to reduce the dummy code, what do you think? >>>>>>>> I don't like the idea to have common empty/dummy functions. >>>>>>>> If virtual PMD behaves in accordance with enabled promiscuous mode, >>>>>>>> it should initialize it properly on init: >>>>>>>>      eth_dev->data->promiscuous = 1; >>>>>>>> If so, if application requires promiscuous mode, attempt to enable will >>>>>>>> do nothing. If application requires non-promiscuous mode, disable will >>>>>>>> fail and it is good. >>>>>>> It is technically correct that we can't disable promiscuous mode in virtual PMDs >>>>>>> but I think mainly we don't really care so it returning error may make the >>>>>>> applications fail/exit unnecessarily with virtual PMDs. >>>>>> If I test virtual PMD promiscuous mode, I would prefer enable/disable >>>>>> callback to say me truth. >>>>>> >>>>>> If application really does not care, it should be in the application code. >>>>> Application can't change this because they may be caring return result for the >>>>> physical devices. >>>>> >>>>> Up until this release these missing dev_ops in virtual PMDs were silently >>>>> ignored, now APIs are more strict on this (which is good) but to get close the >>>>> previous behavior for virtual PMDs we need to relax on these features (like >>>>> saying success on promiscuous disable although it didn't). >>>>> >>>> The other variable here is how often an app is going to request promiscuous >>>> disabling? Given that most ports generally come up in that state anyway, >>>> and one needs to request enabling it, surely the disable case is relatively >>>> rare? In that case I'd tend to agree with having disabling it returning >>>> error for vpmds. >>>> >>> Yes disabling most probably rare, but still it will generate an error and >>> application is failing because of ring PMD promiscuous disable doesn't look >>> right to me. >> Well, if an app needs promiscuous mode disabled then having it fail is the >> right thing to do. If the app doesn't care about promiscuous mode failing, >> why is it checking the return value at all? >> >>> Perhaps application should differentiate between -ENOTSUP error and operation >>> fail error, but that looks to me adding unnecessary complexity to the app. >>> >> Again, does the app care or not? It's probably still better to return >> correct info to the app in all cases, and then let the app decide how best >> to handle it. > My thinking is, application "cares" about the ethdev API return values, but to > check/test quickly with null/ring PMD perhaps not really care that much abut > promisc/allmuticast support of these PMDs and let's relax these support of > virtual PMDs to make life easy. > > But eventually the main target is to fix sample applications to run with virtual > PMDs which has been broken in this release. > Both approach works, > a) Implement dummy dev_ops to virtual PMDs to report success > b) Update ethdev APIs to not call dev_ops if the requested configuration is > already satisfied and change virtual PMDs to report promisc & allmulticast > enabled by default. (disable still will have same issue) > > Is the consensus option (b)? Yes. > btw, the problem exists in high level for the offload support, if the > application is requesting a specific offload support it fails to run with the > virtual PMDs since virtual PMDs doesn't support any offloads. Indeed I have same > suggestion for this case too, relax the virtual PMD by claiming it supports all > offloads. Because at least for me when I use those virtual PMDs I don't really > would like to test offloads or the procmisc/allmulticast features ... It looks like I simply don't understand virtual PMDs usacase. >>> With a common function shared by all PMDs for both promisc and allmuticast will >>> add a little code and an easier solution.