From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id A598C7E23 for ; Fri, 18 Dec 2015 11:03:50 +0100 (CET) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga103.jf.intel.com with ESMTP; 18 Dec 2015 02:03:49 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,445,1444719600"; d="scan'208";a="874150167" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga002.jf.intel.com with ESMTP; 18 Dec 2015 02:03:50 -0800 Received: from fmsmsx102.amr.corp.intel.com (10.18.124.200) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.248.2; Fri, 18 Dec 2015 02:03:49 -0800 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by FMSMSX102.amr.corp.intel.com (10.18.124.200) with Microsoft SMTP Server (TLS) id 14.3.248.2; Fri, 18 Dec 2015 02:03:49 -0800 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.190]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.92]) with mapi id 14.03.0248.002; Fri, 18 Dec 2015 18:03:47 +0800 From: "Xie, Huawei" To: Yuanhan Liu , Tetsuya Mukawa Thread-Topic: [dpdk-dev] [PATCH v5 1/3] vhost: Add callback and private data for vhost PMD Thread-Index: AdE5e2JAxcoHKS5mgkKyQyvFXqyLmQ== Date: Fri, 18 Dec 2015 10:03:46 +0000 Message-ID: References: <1447392031-24970-3-git-send-email-mukawa@igel.co.jp> <1448355603-21275-1-git-send-email-mukawa@igel.co.jp> <1448355603-21275-2-git-send-email-mukawa@igel.co.jp> <20151217114223.GC29571@yliu-dev.sh.intel.com> <56737A5E.1030803@igel.co.jp> <20151218041536.GI29571@yliu-dev.sh.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" , "ann.zhuangyanying@huawei.com" Subject: Re: [dpdk-dev] [PATCH v5 1/3] vhost: Add callback and private data for vhost PMD X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 18 Dec 2015 10:03:51 -0000 On 12/18/2015 12:15 PM, Yuanhan Liu wrote:=0A= > On Fri, Dec 18, 2015 at 12:15:42PM +0900, Tetsuya Mukawa wrote:=0A= >> On 2015/12/17 20:42, Yuanhan Liu wrote:=0A= >>> On Tue, Nov 24, 2015 at 06:00:01PM +0900, Tetsuya Mukawa wrote:=0A= >>>> The vhost PMD will be a wrapper of vhost library, but some of vhost=0A= >>>> library APIs cannot be mapped to ethdev library APIs.=0A= >>>> Becasue of this, in some cases, we still need to use vhost library API= s=0A= >>>> for a port created by the vhost PMD.=0A= >>>>=0A= >>>> Currently, when virtio device is created and destroyed, vhost library= =0A= >>>> will call one of callback handlers. The vhost PMD need to use this=0A= >>>> pair of callback handlers to know which virtio devices are connected= =0A= >>>> actually.=0A= >>>> Because we can register only one pair of callbacks to vhost library, i= f=0A= >>>> the PMD use it, DPDK applications cannot have a way to know the events= .=0A= >>>>=0A= >>>> This may break legacy DPDK applications that uses vhost library. To pr= event=0A= >>>> it, this patch adds one more pair of callbacks to vhost library especi= ally=0A= >>>> for the vhost PMD.=0A= >>>> With the patch, legacy applications can use the vhost PMD even if they= need=0A= >>>> additional specific handling for virtio device creation and destructio= n.=0A= >>>>=0A= >>>> For example, legacy application can call=0A= >>>> rte_vhost_enable_guest_notification() in callbacks to change setting.= =0A= >>> TBH, I never liked it since the beginning. Introducing two callbacks=0A= >>> for one event is a bit messy, and therefore error prone.=0A= >> I agree with you.=0A= >>=0A= >>> I have been thinking this occasionally last few weeks, and have came=0A= >>> up something that we may introduce another layer callback based on=0A= >>> the vhost pmd itself, by a new API:=0A= >>>=0A= >>> rte_eth_vhost_register_callback().=0A= >>>=0A= >>> And we then call those new callback inside the vhost pmd new_device()= =0A= >>> and vhost pmd destroy_device() implementations.=0A= >>>=0A= >>> And we could have same callbacks like vhost have, but I'm thinking=0A= >>> that new_device() and destroy_device() doesn't sound like a good name= =0A= >>> to a PMD driver. Maybe a name like "link_state_changed" is better?=0A= >>>=0A= >>> What do you think of that?=0A= >> Yes, "link_state_changed" will be good.=0A= >>=0A= >> BTW, I thought it was ok that an DPDK app that used vhost PMD called=0A= >> vhost library APIs directly.=0A= >> But probably you may feel strangeness about it. Is this correct?=0A= > Unluckily, that's true :)=0A= >=0A= >> If so, how about implementing legacy status interrupt mechanism to vhost= =0A= >> PMD?=0A= >> For example, an DPDK app can register callback handler like=0A= >> "examples/link_status_interrupt".=0A= >>=0A= >> Also, if the app doesn't call vhost library APIs directly,=0A= >> rte_eth_vhost_portid2vdev() will be needless, because the app doesn't=0A= >> need to handle virtio device structure anymore.=0A= >>=0A= >>>=0A= >>> On the other hand, I'm still thinking is that really necessary to let= =0A= >>> the application be able to call vhost functions like rte_vhost_enable_g= uest_notification()=0A= >>> with the vhost PMD driver?=0A= >> Basic concept of my patch is that vhost PMD will provides the features= =0A= >> that vhost library provides.=0A= > I don't think that's necessary. Let's just treat it as a normal pmd=0A= > driver, having nothing to do with vhost library.=0A= >=0A= >> How about removing rte_vhost_enable_guest_notification() from "vhost=0A= >> library"?=0A= >> (I also not sure what are use cases)=0A= >> If we can do this, vhost PMD also doesn't need to take care of it.=0A= >> Or if rte_vhost_enable_guest_notification() will be removed in the=0A= >> future, vhost PMD is able to ignore it.=0A= > You could either call it in vhost-pmd (which you already have done that),= =0A= > or ignore it in vhost-pmd, but dont' remove it from vhost library.=0A= >=0A= >> Please let me correct up my thinking about your questions.=0A= >> - Change concept of patch not to call vhost library APIs directly.=0A= >> These should be wrapped by ethdev APIs.=0A= >> - Remove rte_eth_vhost_portid2vdev(), because of above concept changing= .=0A= >> - Implement legacy status changed interrupt to vhost PMD instead of=0A= >> using own callback mechanism.=0A= >> - Check if we can remove rte_vhost_enable_guest_notification() from=0A= >> vhost library.=0A= > So, how about making it __fare__ simple as the first step, to get merged= =0A= > easily, that we don't assume the applications will call any vhost library= =0A= > functions any more, so that we don't need the callback, and we don't need= =0A= > the rte_eth_vhost_portid2vdev(), either. Again, just let it be a fare=0A= > normal (nothing special) pmd driver. (UNLESS, there is a real must, whic= h=0A= > I don't see so far).=0A= >=0A= > Tetsuya, what do you think of that then?=0A= >=0A= >> Hi Xie,=0A= >>=0A= >> Do you know the use cases of rte_vhost_enable_guest_notification()?=0A= If vhost runs in loop mode, it doesn't need to be notified. You have=0A= wrapped vhost as the PMD, which is nice for OVS integration. If we=0A= require that all PMDs could be polled by select/poll, then we could use=0A= this API for vhost PMD, and wait on the kickfd. For physical nics, we=0A= could wait on the fd for user space interrupt.=0A= > Setting the arg to 0 avoids the guest kicking the virtqueue, which=0A= > is good for performance, and we should keep it.=0A= >=0A= > --yliu=0A= >=0A= =0A=