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 <ferruh.yi...@intel.com> > >>>>>>>>>> --- > >>>>>>>>>> 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 > >>> > >> >