From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ferruh Yigit Subject: Re: [PATCH] drivers/net: update link status Date: Wed, 18 Apr 2018 13:17:40 +0100 Message-ID: <1054638a-4397-bc63-521f-26c327bb28e7@intel.com> 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> <20180418120854.vqbsc7akqhthvjg7@debian> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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 To: Tiwei Bie Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 5E0255F2B for ; Wed, 18 Apr 2018 14:17:49 +0200 (CEST) In-Reply-To: <20180418120854.vqbsc7akqhthvjg7@debian> Content-Language: en-US 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 4/18/2018 1:08 PM, Tiwei Bie wrote: > 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. Yep. > 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.) Yes, some PMDs doesn't call callback_process() even they support link status and link status interrupts. This may be because this is not documented clearly. Best option can be handling this in ethdev somehow. > > 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 >>>>> >>>> >>