From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by dpdk.org (Postfix) with ESMTP id 652888D36 for ; Wed, 18 Apr 2018 13:35:32 +0200 (CEST) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 Apr 2018 04:35:31 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,465,1517904000"; d="scan'208";a="42835716" Received: from debian.sh.intel.com (HELO debian) ([10.67.104.164]) by FMSMGA003.fm.intel.com with ESMTP; 18 Apr 2018 04:35:27 -0700 Date: Wed, 18 Apr 2018 19:36:15 +0800 From: Tiwei Bie To: Ferruh Yigit Cc: "Tan, Jianfeng" , Qi Zhang , Xiao Wang , John McNamara , Marko Kovacevic , Beilei Xing , Wenzhuo Lu , Rasesh Mody , Harish Patil , Shahed Shaikh , Tetsuya Mukawa , Yuanhan Liu , Maxime Coquelin , Marcin Wojtas , Michal Krawczyk , Guy Tzalik , Evgeny Schemeilin , Konstantin Ananyev , Adrien Mazarguil , Nelio Laranjeiro , Yongseok Koh , dev@dpdk.org Message-ID: <20180418113614.lythwhhxmgucouvh@debian> References: <20180313180534.232296-1-ferruh.yigit@intel.com> <20180410154102.uv7og3ff4y5ylc3m@debian> <20180414105546.25vqtpcfkexwlkhp@debian> <20180417045427.6guh7x7yqpyfdnwj@debian> <3517e7a5-dec6-fca1-b24f-8fdc2691216c@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH] drivers/net: update link 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: , X-List-Received-Date: Wed, 18 Apr 2018 11:35:33 -0000 On Wed, Apr 18, 2018 at 11:42:56AM +0100, Ferruh Yigit wrote: > On 4/18/2018 7:49 AM, Tan, Jianfeng wrote: > > Hi Ferruh, > > > > > > On 4/17/2018 7:26 PM, Ferruh Yigit wrote: > >> On 4/17/2018 5:54 AM, Tiwei Bie wrote: > >>> On Mon, Apr 16, 2018 at 05:10:24PM +0100, Ferruh Yigit wrote: > >>>> On 4/14/2018 11:55 AM, Tiwei Bie wrote: > >>>>> On Fri, Apr 13, 2018 at 10:53:55PM +0100, Ferruh Yigit wrote: > >>>>>> On 4/10/2018 4:41 PM, Tiwei Bie wrote: > >>>>>>> On Tue, Mar 13, 2018 at 06:05:34PM +0000, Ferruh Yigit wrote: > >>>>>>>> Update link status related feature document items and minor updates in > >>>>>>>> some link status related functions. > >>>>>>>> > >>>>>>>> Signed-off-by: Ferruh Yigit > >>>>>>>> --- > >>>>>>>> doc/guides/nics/features/fm10k.ini | 2 ++ > >>>>>>>> doc/guides/nics/features/fm10k_vf.ini | 2 ++ > >>>>>>>> doc/guides/nics/features/i40e_vf.ini | 1 + > >>>>>>>> doc/guides/nics/features/igb_vf.ini | 1 + > >>>>>>>> doc/guides/nics/features/qede.ini | 1 - > >>>>>>>> doc/guides/nics/features/qede_vf.ini | 1 - > >>>>>>>> doc/guides/nics/features/vhost.ini | 2 -- > >>>>>>>> doc/guides/nics/features/virtio_vec.ini | 1 + > >>>>>>>> drivers/net/e1000/em_ethdev.c | 2 +- > >>>>>>>> drivers/net/ena/ena_ethdev.c | 2 +- > >>>>>>>> drivers/net/fm10k/fm10k_ethdev.c | 6 ++---- > >>>>>>>> drivers/net/i40e/i40e_ethdev_vf.c | 2 +- > >>>>>>>> drivers/net/ixgbe/ixgbe_ethdev.c | 2 +- > >>>>>>>> drivers/net/mlx4/mlx4_ethdev.c | 2 +- > >>>>>>>> drivers/net/mlx5/mlx5_ethdev.c | 2 +- > >>>>>>>> 15 files changed, 15 insertions(+), 14 deletions(-) > >>>>>>> [...] > >>>>>>>> diff --git a/doc/guides/nics/features/vhost.ini b/doc/guides/nics/features/vhost.ini > >>>>>>>> index dffd1f493..31302745a 100644 > >>>>>>>> --- a/doc/guides/nics/features/vhost.ini > >>>>>>>> +++ b/doc/guides/nics/features/vhost.ini > >>>>>>>> @@ -4,8 +4,6 @@ > >>>>>>>> ; Refer to default.ini for the full list of available PMD features. > >>>>>>>> ; > >>>>>>>> [Features] > >>>>>>>> -Link status = Y > >>>>>>>> -Link status event = Y > >>>>>>> I think vhost PMD supports above features. > >>>>>> I am not able to find where it is supported. > >>>>>> > >>>>>> Some virtual PMDs report fixed link, with empty link_update() dev_ops, and they > >>>>>> are not reported as supporting Link status, as far as I can see vhost also one > >>>>>> of them. > >>>>>> > >>>>>> And for Link status event, PMD needs to support LSC interrupts and should > >>>>>> register interrupt handler for it, which I can't find for vhost. > >>>>>> > >>>>>> I will send next version without updating above one, please point me where these > >>>>>> support added if I missed them. > >>>>> In drivers/net/vhost/rte_eth_vhost.c you could find below functions: > >>>>> > >>>>> static int > >>>>> new_device(int vid) > >>>>> { > >>>>> ...... > >>>>> > >>>>> eth_dev->data->dev_link.link_status = ETH_LINK_UP; > >>>>> > >>>>> ...... > >>>>> > >>>>> _rte_eth_dev_callback_process(eth_dev, RTE_ETH_EVENT_INTR_LSC, NULL); > >>>>> > >>>>> ...... > >>>>> } > >>>>> > >>>>> static void > >>>>> destroy_device(int vid) > >>>>> { > >>>>> ...... > >>>>> > >>>>> eth_dev->data->dev_link.link_status = ETH_LINK_DOWN; > >>>>> > >>>>> ...... > >>>>> > >>>>> _rte_eth_dev_callback_process(eth_dev, RTE_ETH_EVENT_INTR_LSC, NULL); > >>>>> > >>>>> ...... > >>>>> } > >>>>> > >>>>> They are the callbacks for vhost library. > >>>>> > >>>>> When a frontend (e.g. QEMU) is connected to this vhost backend > >>>>> and the frontend virtio device becomes ready, new_device() will > >>>>> be called by the vhost library, and the link status will be > >>>>> updated to UP. > >>>>> > >>>>> And when e.g. the connection is closed, destroy_device() will be > >>>>> called by the vhost library, and the link status will be updated > >>>>> to DOWN. > >>>> > >>>> Got it. This behavior is similar for virtual PMDs. Provide static link > >>>> information and update link as UP during start and update it as DOWN during stop. > >>> No, the link status isn't updated during vhost PMD start > >>> and stop. When the vhost PMD has been started, the link > >>> status still may be DOWN. The link status becomes UP only > >>> when the QEMU (it's another virtual machine process which > >>> has a virtio device) connects to this vhost PMD via a UNIX > >>> socket and the virtio driver in the virtual machine has > >>> setup the virtio device of the virtual machine. > >>> > >>> So if vhost PMD reports the link status as DOWN, it means > >>> there is no QEMU (virtual machine) connects to it or the > >>> virtio device in the virtual machine hasn't been setup. > >>> (PS. The frontend can also be virtio-user PMD besides QEMU) > >> I believe announcing link feature reporting on virtual pmds still in gray area, > >> but because of qemu involvement in vhost case, I will keep link feature but will > >> drop link event. > > > > AFAIK, link status means we can get link status through APIs like > > rte_eth_link_get(); while link status event means applications can > > register link status events, and those events get called if link status > > is changed. > > > > If I understand it correctly, for vhost, we can keep both link status > > and link status event for vhost. > > > > Could you specify the reason why we remove link status event? > > Hi Jianfeng, > > I think problem is the definition of the features are not clear, that is why I > started a doc to document them (doc/guides/nics/features.rst) > > "Link status", I think we agree on this one. PMD should provide up-to-date, > valid link data on rte_eth_dev_data->dev_link. So that this link information can > be get by rte_eth_link_get(), rte_eth_link_get_nowait() ethdev APIs. > > rte_eth_link_get() uses dev_ops->link_update() to get the latest link > information, for virtual PMDs, including vhost, this function does nothing, > because there is no actual physical link. That is why I was not sure advertising > link status feature for vhost, and other virtual PMDs doesn't report this > feature. After Tiwei's comment that link status shows that qemu connected to > vhost, added this feature back to vhost PMD. > > > "Link status event", can be > a- PMD calls application callback in link change > b- PMD registers interrupt handler for link status change interrupts > > Based on how other PMDs report this feature, I believe it is (b), and I have > documented that way. And vhost "Link status event" feature removed based on this. > > There are some set of config options and flags to control the LSC interrupt, > that also effects rte_eth_link_get() and rte_eth_link_get_nowait() APIs, I > believe that is the main concern here. > > As commented above, I don't understand why calling user register callback for > link change event is something on PMD decision, it should be default behavior > for PMD. Do you mean you don't understand why vhost PMD calls _rte_eth_dev_callback_process(eth_dev, RTE_ETH_EVENT_INTR_LSC, NULL) in new_device() and destroy_device()? While other physical NIC drivers call this in their interrupt handlers which are registered via rte_intr_callback_register(). The new_device() and destroy_device() in vhost PMD can be treated as something like the interrupt handlers in physical NIC drivers. They are the callbacks which are registered via rte_vhost_driver_callback_register() and will be called by vhost library when vhost events happen (and they should be translated to link status change events when we try to wrap it as a net PMD). Thanks > What is the point of leaving this into PMD and think this as a feature > of PMD? > > > Overall, practical reason of this table is to inform developer/user about PMD > features, which is indeed device + driver features, and help her to in > development or on setting expectations. We can always discuss what helps more to > developer/user and update the features table. > > Thanks, > ferruh > > > > > Thanks, > > Jianfeng > > > >> > >> Will send a new patch to reflect this. > >> > >> Thanks, > >> ferruh > > >