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 2BFF7A2EFC for ; Mon, 16 Sep 2019 17:46:04 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id E71A11C030; Mon, 16 Sep 2019 17:46:02 +0200 (CEST) Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [148.163.129.52]) by dpdk.org (Postfix) with ESMTP id 5D7D11C012 for ; Mon, 16 Sep 2019 17:46:01 +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-us2.ppe-hosted.com (PPE Hosted ESMTP Server) with ESMTPS id 6D0FBA40087; Mon, 16 Sep 2019 15:45:57 +0000 (UTC) Received: from [192.168.1.192] (188.242.181.57) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Mon, 16 Sep 2019 16:45:15 +0100 To: Ferruh Yigit , "John W. Linville" , Xiaolong Ye , Qi Zhang , Igor Russkikh , "Pavel Belous" , Allain Legacy , Matt Peters , "Ravi Kumar" , Rasesh Mody , Shahed Shaikh , Ajit Khaparde , "Somnath Kotur" , Chas Williams , "Rahul Lakkireddy" , Hemant Agrawal , Sachin Saxena , Wenzhuo Lu , Gagandeep Singh , John Daley , Hyong Youb Kim , Gaetan Rivet , Xiao Wang , Ziyang Xuan , Xiaoyun Wang , Guoyang Zhou , Beilei Xing , Jingjing Wu , Qiming Yang , Rosen Xu , Konstantin Ananyev , Shijith Thotton , Srisivasubramanian Srinivasan , Matan Azrad , Shahaf Shuler , Yongseok Koh , Viacheslav Ovsiienko , "Zyta Szpak" , Liron Himi , Tomasz Duszynski , Stephen Hemminger , "K. Y. Srinivasan" , Haiyang Zhang , Rastislav Cernay , Jan Remes , Alejandro Lucero , Jerin Jacob , Nithin Dabilpuram , "Kiran Kumar K" , Keith Wiles , Maciej Czekaj , Maxime Coquelin , Tiwei Bie , Zhihong Wang , Yong Wang , Thomas Monjalon CC: References: <1567699852-31693-1-git-send-email-arybchenko@solarflare.com> <1568030331-16526-1-git-send-email-arybchenko@solarflare.com> <1568030331-16526-5-git-send-email-arybchenko@solarflare.com> <5093c27d-4251-e997-f4bb-d1f56a60443a@intel.com> <0aec5016-2130-26f9-dfd6-f65222514ced@solarflare.com> <1c8c01b2-b244-5cf9-8687-9ee897c73f7c@intel.com> <60e4ec91-7f2c-890a-b8d6-237f09b17c11@solarflare.com> <1696df72-998b-d189-aa19-70a16766f3ab@intel.com> From: Andrew Rybchenko Message-ID: <4cce6033-0347-04f6-3655-f36fbf9d32e8@solarflare.com> Date: Mon, 16 Sep 2019 18:45:09 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: <1696df72-998b-d189-aa19-70a16766f3ab@intel.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-Originating-IP: [188.242.181.57] 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-24914.003 X-TM-AS-Result: No-5.858600-8.000000-10 X-TMASE-MatchedRID: oHOSwQSJZWjA46G+uSzVzSLVdThWsHxY69aS+7/zbj+qvcIF1TcLYM7/ vP2uWVRBK/S70kzmfv7rL7s+y7z25R1YpEPWJiyzqJSK+HSPY+/pVMb1xnESMrZk7gsuflVKhWD 4TtDcfzQe27BljbjQO7q5QJeil1hr9+5WkBOeNwd2GcWKGZufBam9/6ObPjnDEdZrmoJ0iw1576 my5IxjuqhPDwZeB+AQRPk8NBFRp6qRwaU8+7ToHvKR06Kw3DzKwwD0mzFpRrcjRiu1AuxJTIDAz EDOc9caIlkvvprHirnAZETqpeNEgelVmb365UrbJNzc11O35noTTY+vgKQ1I3c925yOJXmFRyqM LIj/WA2Do3byUXkIS5THVCmi8Md66OKEpitFnobFW296Y1uTJwD4keG7QhHmbPfGfHxv+pG5UpW x7YiH+deNpaH9JVR3ZqHTQk8AKbEYB2fOueQzjzl/1fD/GopdWQy9YC5qGvzcyHXbbFd0K3g3WO FFVAUtJ0RPnyOnrZIDJfI2Uf7etN+nBBPAE4HhkrcBk9Ad5HCyLmnCCOr/qMNHEc/+gdmV0qsTN TfgxIXo2qD6ZiiipUw62d3uRSHpd1d6M1SpUGLwHX5+Q8jjw1wuriZ3P6dErIJZJbQfMXRqaM5L mpUkwzr8RUm73lTc X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--5.858600-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-24914.003 X-MDID: 1568648760-syRjuSxwcNHs Subject: Re: [dpdk-dev] [PATCH v2 04/13] ethdev: change promiscuous callbacks to return status 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 9/16/19 4:22 PM, Ferruh Yigit wrote: > On 9/13/2019 8:57 PM, Andrew Rybchenko wrote: >> On 9/13/19 7:34 PM, Ferruh Yigit wrote: >>> On 9/13/2019 5:05 PM, Andrew Rybchenko wrote: >>>> On 9/13/19 6:39 PM, Ferruh Yigit wrote: >>>>> On 9/9/2019 12:58 PM, Andrew Rybchenko wrote: >>>>>> Enabling/disabling of promiscuous mode is not always successful and >>>>>> it should be taken into account to be able to handle it properly. >>>>>> >>>>>> When correct return status is unclear from driver code, -EAGAIN is used. >>>>>> >>>>>> Signed-off-by: Andrew Rybchenko >>>>> <...> >>>>> >>>>>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c >>>>>> index f85458c3cd..41612ce838 100644 >>>>>> --- a/drivers/net/tap/rte_eth_tap.c >>>>>> +++ b/drivers/net/tap/rte_eth_tap.c >>>>>> @@ -1100,28 +1100,60 @@ tap_link_update(struct rte_eth_dev *dev, int wait_to_complete __rte_unused) >>>>>> return 0; >>>>>> } >>>>>> >>>>>> -static void >>>>>> +static int >>>>>> tap_promisc_enable(struct rte_eth_dev *dev) >>>>>> { >>>>>> struct pmd_internals *pmd = dev->data->dev_private; >>>>>> struct ifreq ifr = { .ifr_flags = IFF_PROMISC }; >>>>>> + int ret; >>>>>> >>>>>> - dev->data->promiscuous = 1; >>>>>> - tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 1, LOCAL_AND_REMOTE); >>>>>> - if (pmd->remote_if_index && !pmd->flow_isolate) >>>>>> - tap_flow_implicit_create(pmd, TAP_REMOTE_PROMISC); >>>>>> + ret = tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 1, LOCAL_AND_REMOTE); >>>>>> + if (ret != 0) >>>>>> + return ret; >>>>>> + >>>>>> + if (pmd->remote_if_index && !pmd->flow_isolate) { >>>>>> + dev->data->promiscuous = 1; >>>>> I think PMD shouldn't be setting this variable, it is already set by the API. >>>>> I quickly checked if an internal function requires this but it looks like it has >>>>> been set by mistake, I think we can remove this. >>>> It is set after callback in the case of enable. >>> I see, but do we need it enabled earlier? >> Not sure that I understand the question, but tap_ioctl() does not use it. >> So, it is safe to move just before tap_flow_implicit_create(). > I think 'dev->data->promiscuous' shouldn't be set be PMD dev_ops, and API > already does it. Is there a specific reason in tap to set this in dev_ops? If > not can we remove setting 'dev->data->promiscuous' from dev_ops? The problem is the following: right now enable function sets data->promiscuous after driver callback execution, some drivers definitely use the variable internally and require it to be set before some driver operations. That's why these drivers set it on entry. However, it does not mean that no drivers rely on the fact that data->promiscuous value is set to 1 after callback. Most likely these drivers are buggy since it is false in the case of configuration restore on startup, but anyway it is not that simply. Yes, I would prefer to avoid setting data->promiscuous from driver code in enable/disable callbacks, but it should be a separate patch which change it. Right now: net/bnxt sets data->promiscuous=1 on configure if Rx mode is VMDQ_DCB net/mlx4 sets data->promiscuous always on enable/disable entry net/octeontx sets data->promiscuous before exit from callback and I think it can be safely removed since API functions definitely do it before or after callback. net/softnic sets data->promiscuous=1 on driver register net/tap sets data->promiscuous=1 in enable/disable hooks before tap_flow_implicit_create() net/nfb sets data->promiscuous in driver init net/octeontx2 sets data->promiscuous in the middle of processing, but I guess before it is really used >>>>>> + ret = tap_flow_implicit_create(pmd, TAP_REMOTE_PROMISC); >>>>>> + if (ret != 0) { >>>>>> + /* Rollback promisc flag */ >>>>>> + tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 0, LOCAL_AND_REMOTE); >>>>>> + /* >>>>>> + * rte_eth_dev_promiscuous_enable() rollback >>>>>> + * dev->data->promiscuous in the case of failure. >>>>>> + */ >>>>>> + return ret; >>>>>> + } >>>>>> + } >>>>>> + >>>>>> + return 0; >>>>>> } >>>>>> >>>>>> -static void >>>>>> +static int >>>>>> tap_promisc_disable(struct rte_eth_dev *dev) >>>>>> { >>>>>> struct pmd_internals *pmd = dev->data->dev_private; >>>>>> struct ifreq ifr = { .ifr_flags = IFF_PROMISC }; >>>>>> + int ret; >>>>>> >>>>>> - dev->data->promiscuous = 0; >>>>>> - tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 0, LOCAL_AND_REMOTE); >>>>>> - if (pmd->remote_if_index && !pmd->flow_isolate) >>>>>> - tap_flow_implicit_destroy(pmd, TAP_REMOTE_PROMISC); >>>>>> + ret = tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 0, LOCAL_AND_REMOTE); >>>>>> + if (ret != 0) >>>>>> + return ret; >>>>>> + >>>>>> + if (pmd->remote_if_index && !pmd->flow_isolate) { >>>>>> + dev->data->promiscuous = 0; >>>>> Ditto >>>>> >>>>>> + ret = tap_flow_implicit_destroy(pmd, TAP_REMOTE_PROMISC); >>>>>> + if (ret != 0) { >>>>>> + /* Rollback promisc flag */ >>>>>> + tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 1, LOCAL_AND_REMOTE); >>>>>> + /* >>>>>> + * rte_eth_dev_promiscuous_disable() rollback >>>>>> + * dev->data->promiscuous in the case of failure. >>>>>> + */ >>>>>> + return ret; >>>>>> + } >>>>>> + } >>>>>> + >>>>>> + return 0; >>>>>> }