From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id 5F49B11DE for ; Fri, 17 Mar 2017 06:52:16 +0100 (CET) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga104.jf.intel.com with ESMTP; 16 Mar 2017 22:52:16 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,175,1486454400"; d="scan'208";a="237379299" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.67.162]) by fmsmga004.fm.intel.com with ESMTP; 16 Mar 2017 22:52:15 -0700 Date: Fri, 17 Mar 2017 13:50:29 +0800 From: Yuanhan Liu To: Maxime Coquelin Cc: dev@dpdk.org, Harris James R , Liu Changpeng Message-ID: <20170317055029.GF18844@yliu-dev.sh.intel.com> References: <1488534682-3494-1-git-send-email-yuanhan.liu@linux.intel.com> <1488534682-3494-2-git-send-email-yuanhan.liu@linux.intel.com> <337dbdcd-0913-dd1f-5cba-15844a7380b1@redhat.com> <20170316070807.GO18844@yliu-dev.sh.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH 01/17] vhost: introduce driver features related APIs 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: Fri, 17 Mar 2017 05:52:17 -0000 On Thu, Mar 16, 2017 at 10:18:21AM +0100, Maxime Coquelin wrote: > > > On 03/16/2017 08:08 AM, Yuanhan Liu wrote: > >On Tue, Mar 14, 2017 at 10:53:23AM +0100, Maxime Coquelin wrote: > >> > >> > >>On 03/14/2017 10:46 AM, Maxime Coquelin wrote: > >>> > >>> > >>>On 03/03/2017 10:51 AM, Yuanhan Liu wrote: > >>>>Introduce few APIs to set/get/enable/disable driver features. > >>>> > >>>>Signed-off-by: Yuanhan Liu > >>>>--- > >>>>lib/librte_vhost/rte_vhost_version.map | 10 ++++ > >>>>lib/librte_vhost/rte_virtio_net.h | 9 ++++ > >>>>lib/librte_vhost/socket.c | 90 > >>>>++++++++++++++++++++++++++++++++++ > >>>>3 files changed, 109 insertions(+) > >>> > >>>Nice! > >>>Reviewed-by: Maxime Coquelin > >>> > >>>I wonder whether we could protect from setting/enabling/disabling > >>>features once negotiation is done? > > > >Those APIs won't be able to change the negotiated features. They are > >just some interfaces before the vhost-user connection is established. > > Right. > I meant it could return an error is the connection is already > established. Else, the caller might think the feature has been > successfully enabled/disabled, whereas it is not the case. > But this is maybe over-engineering to handle this case. > > >Ideally, we could/should get rid of the enabling/disabling functions: > >let the vhost-user driver to handle the features directly (say, for > >vhost-user pmd, we could use vdev options to disable/enable few specific > >features). Once that is done, use the rte_vhost_driver_set_features() > >set the features once. > > Ok, but what about vhost lib users and TSO (I'm thinking of OVS). Yes, that's why I kept them :) --yliu > > >The reason I introduced the enable/disable_features() is to keep the > >compatability with the builtin vhost-user net driver (virtio_net.c). > >If there is a plan to move it into vhost-pmd, they won't be needed. > >And I don't think that will happen soon. > > I agree with you that would be ideal, but unlikely to happen soon. > > >>Oh, I forgot one comment on this patch. > >>Since these new functions are part to the API, shouldn't them be > >>documented? > > > >Yes, indeed, it's also noted in my cover letter. > > Oops, I missed that! > > Thanks, > Maxime > > --yliu > >