From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by dpdk.org (Postfix) with ESMTP id 0AFBE1B4F9 for ; Tue, 26 Jun 2018 11:38:26 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 57DCD40201A2; Tue, 26 Jun 2018 09:38:25 +0000 (UTC) Received: from [10.36.112.48] (ovpn-112-48.ams2.redhat.com [10.36.112.48]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 35F4B111AF0B; Tue, 26 Jun 2018 09:38:20 +0000 (UTC) To: Tiwei Bie , "Stojaczyk, DariuszX" Cc: Dariusz Stojaczyk , "dev@dpdk.org" , Tetsuya Mukawa , Stefan Hajnoczi , Thomas Monjalon , "yliu@fridaylinux.org" , "Harris, James R" , "Kulasek, TomaszX" , "Wodkowski, PawelX" References: <1526648465-62579-1-git-send-email-dariuszx.stojaczyk@intel.com> <20180607151227.23660-1-darek.stojaczyk@gmail.com> <20180625110146.GA18211@debian> <20180626082226.GA15665@debian> <20180626091428.GA20198@debian> From: Maxime Coquelin Message-ID: <82add50b-bbfc-b3e6-e530-a0130bf90b3c@redhat.com> Date: Tue, 26 Jun 2018 11:38:18 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: <20180626091428.GA20198@debian> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.78 on 10.11.54.3 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Tue, 26 Jun 2018 09:38:25 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Tue, 26 Jun 2018 09:38:25 +0000 (UTC) for IP:'10.11.54.3' DOMAIN:'int-mx03.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'maxime.coquelin@redhat.com' RCPT:'' Subject: Re: [dpdk-dev] [RFC v3 0/7] vhost2: new librte_vhost2 proposal 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: Tue, 26 Jun 2018 09:38:26 -0000 On 06/26/2018 11:14 AM, Tiwei Bie wrote: > On Tue, Jun 26, 2018 at 04:47:33PM +0800, Stojaczyk, DariuszX wrote: >>> -----Original Message----- >>> From: Bie, Tiwei >>> Sent: Tuesday, June 26, 2018 10:22 AM >>> To: Stojaczyk, DariuszX >>> Cc: Dariusz Stojaczyk ; dev@dpdk.org; Maxime >>> Coquelin ; Tetsuya Mukawa >>> ; Stefan Hajnoczi ; Thomas >>> Monjalon ; yliu@fridaylinux.org; Harris, James R >>> ; Kulasek, TomaszX ; >>> Wodkowski, PawelX >>> Subject: Re: [dpdk-dev] [RFC v3 0/7] vhost2: new librte_vhost2 proposal >>> >>> On Mon, Jun 25, 2018 at 08:17:08PM +0800, Stojaczyk, DariuszX wrote: >>>>> -----Original Message----- >>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tiwei Bie >>>>> Sent: Monday, June 25, 2018 1:02 PM >>>>> >>>>> >>>>> Hi Dariusz, >>>>> >>>> >>>> Hi Tiwei, >>>> >>>>> Thank you for putting efforts in making the DPDK >>>>> vhost more generic! >>>>> >>>>> From my understanding, your proposal is that: >>>>> >>>>> 1) Introduce rte_vhost2 to provide the APIs which >>>>> allow users to implement vhost backends like >>>>> SCSI, net, crypto, .. >>>>> >>>> >>>> That's right. >>>> >>>>> 2) Refactor the existing rte_vhost to use rte_vhost2. >>>>> The rte_vhost will still provide below existing >>>>> sets of APIs: >>>>> 1. The APIs which allow users to implement >>>>> external vhost backends (these APIs were >>>>> designed for SPDK previously) >>>>> 2. The APIs provided by the net backend >>>>> 3. The APIs provided by the crypto backend >>>>> And above APIs in rte_vhost won't be changed. >>>> >>>> That's correct. Rte_vhost would register its own rte_vhost2_tgt_ops >>> underneath and will call existing vhost_device_ops for e.g. starting the device >>> once all queues are started. >>> >>> Currently I have below concerns and questions: >>> >>> - The rte_vhost's problem is still there. Even though >>> rte_vhost2 is introduced, the net and crypto backends >>> in rte_vhost won't benefit from the new callbacks. >>> >>> The existing rte_vhost in DPDK not only provides the >>> APIs for DPDK applications to implement the external >>> backends. But also provides high performance net and >>> crypto backends implementation (maybe more in the >>> future). So it's important that besides the DPDK >>> applications which implement their external backends, >>> the DPDK applications which use the builtin backends >>> will also benefit from the new callbacks. >>> >>> So we should have a clear plan on how will the legacy >>> callbacks in rte_vhost be dealt with in the next step. >>> >>> Besides, the new library's name is a bit misleading. >>> It makes the existing rte_vhost library sound like an >>> obsolete library. But actually the existing rte_vhost >>> isn't an obsolete library. It will still provide the >>> net and crypto backends. So if we want to introduce >>> this new library, we should give it a better name. >>> >>> - It's possible to solve rte_vhost's problem you met >>> by refactoring the existing vhost library directly >>> instead of re-implementing a new vhost library from >>> scratch and keeping the old one's problem as is. >>> >>> In this way, it will solve the problem you met and >>> also solve the problem for rte_vhost. Why not go >>> this way? Something like: >>> >>> Below is the existing callbacks set in rte_vhost.h: >>> >>> /** >>> * Device and vring operations. >>> */ >>> struct vhost_device_ops { >>> ...... >>> }; >>> >>> It's a legacy implementation, and doesn't really >>> follow the DPDK API design (e.g. no rte_ prefix). >>> We can design and implement a new message handling >>> and a new set of callbacks for rte_vhost to solve >>> the problem you met without changing the old one. >>> Something like: >>> >>> struct rte_vhost_device_ops { >>> ...... >>> } >>> >>> int >>> vhost_user_msg_handler(struct vhost_dev *vdev, struct vhost_user_msg >>> *msg) >>> { >>> ...... >>> >>> if (!vdev->is_using_new_device_ops) { >>> // Call the existing message handler >>> return vhost_user_msg_handler_legacy(vdev, msg); >>> } >>> >>> // Implement the new logic here >>> ...... >>> } >>> >>> A vhost application is allowed to register only struct >>> rte_vhost_device_ops or struct vhost_device_ops (which >>> should be deprecated in the future). The two ops cannot >>> be registered at the same time. >>> >>> The existing applications could use the old ops. And >>> if an application registers struct rte_vhost_device_ops, >>> the new callbacks and message handler will be used. >> >> Please notice that some features like vIOMMU are not even a part of the public rte_vhost API. Only vhost-net benefits from vIOMMU right now. Separating vhost-net from a generic vhost library (rte_vhost2) would avoid making such design mistakes in future. What's the point of having a single rte_vhost library, if some vhost-user features are only implemented for vhost-net. > > These APIs can be safely added at any time. > And we can also ask developers to add public > APIs if it matters when adding new features > in the future. I don't think it's a big > problem. +1, I don't think it is a problem. It is better to have it internal only at the beginning than having to break the API. Thanks, Maxime > Best regards, > Tiwei Bie > >> >>> >>> Best regards, >>> Tiwei Bie >>> >>> >>>> Regards, >>>> D. >>>> >>>>> >>>>> Is my above understanding correct? Thanks! >>>>> >>>>> Best regards, >>>>> Tiwei Bie >>>>>