From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 32D317CF8 for ; Wed, 18 Apr 2018 14:08:13 +0200 (CEST) X-Amp-Result: UNSCANNABLE X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 Apr 2018 05:08:10 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,465,1517904000"; d="scan'208";a="221393351" Received: from debian.sh.intel.com (HELO debian) ([10.67.104.164]) by fmsmga006.fm.intel.com with ESMTP; 18 Apr 2018 05:08:06 -0700 Date: Wed, 18 Apr 2018 20:08:54 +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: <20180418120854.vqbsc7akqhthvjg7@debian> References: <20180410154102.uv7og3ff4y5ylc3m@debian> <20180414105546.25vqtpcfkexwlkhp@debian> <20180417045427.6guh7x7yqpyfdnwj@debian> <3517e7a5-dec6-fca1-b24f-8fdc2691216c@intel.com> <20180418113614.lythwhhxmgucouvh@debian> <8a3f8cf4-4c07-28e4-076e-ef6aa436b175@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <8a3f8cf4-4c07-28e4-076e-ef6aa436b175@intel.com> 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 12:08:16 -0000 On Wed, Apr 18, 2018 at 12:44:54PM +0100, Ferruh Yigit wrote: > On 4/18/2018 12:36 PM, Tiwei Bie wrote: > > 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(). > > No, I understand this part. > > My question is why not all PMDs call callback_process() when link changed as a > requirement? Why PMD is giving decision to call or not call callback_process() ? I got your point now. Maybe because in the library level, it just knows an interrupt happened but doesn't know whether it's a LSC interrupt or not (sometimes it needs to check some vendor specific registers). So the simplest way is just to ask each PMD to call callback_process() directly. But unfortunately, some PMDs didn't do it as we expected.. (I didn't look into other PMDs, so I'm not sure about the details.) Thanks > > > > > 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 > >>> > >> >