From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id 653D02BAF for ; Thu, 16 Aug 2018 07:38:51 +0200 (CEST) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 15 Aug 2018 22:38:50 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.53,246,1531810800"; d="scan'208";a="225026637" Received: from debian.sh.intel.com (HELO debian) ([10.67.104.194]) by orsmga004.jf.intel.com with ESMTP; 15 Aug 2018 22:38:48 -0700 Date: Thu, 16 Aug 2018 13:38:06 +0800 From: Tiwei Bie To: "Legacy, Allain" Cc: "maxime.coquelin@redhat.com" , "zhihong.wang@intel.com" , "dev@dpdk.org" , "Peters, Matt" , "Zhang, Qing Long (Eric)" Message-ID: <20180816053806.GB8252@debian> References: <20180809183455.32442-1-allain.legacy@windriver.com> <20180815034001.GA21177@debian.sh.intel.com> <70A7408C6E1BFB41B192A929744D8523BAB91B23@ALA-MBD.corp.ad.wrs.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <70A7408C6E1BFB41B192A929744D8523BAB91B23@ALA-MBD.corp.ad.wrs.com> User-Agent: Mutt/1.10.1 (2018-07-13) Subject: Re: [dpdk-dev] [PATCH] net/virtio-user: check negotiated features before set 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: Thu, 16 Aug 2018 05:38:52 -0000 On Wed, Aug 15, 2018 at 12:28:05PM +0000, Legacy, Allain wrote: > > -----Original Message----- > > From: Tiwei Bie [mailto:tiwei.bie@intel.com] > > Sent: Tuesday, August 14, 2018 11:40 PM > > To: Legacy, Allain > > Cc: maxime.coquelin@redhat.com; zhihong.wang@intel.com; > > dev@dpdk.org; Peters, Matt; Zhang, Qing Long (Eric) > > Subject: Re: [PATCH] net/virtio-user: check negotiated features before set > > > > On Thu, Aug 09, 2018 at 01:34:55PM -0500, Allain Legacy wrote: > > > From: eric zhang > > > > > > This patch checks negotiated features to see if necessary to offload > > > before set the tap device offload capabilities. It also checks if > > > kernel support the TUNSETOFFLOAD operation. > > > > > > Signed-off-by: eric zhang > > > Signed-off-by: Allain Legacy > > > --- > > > drivers/net/virtio/virtio_user/vhost_kernel.c | 2 +- > > > drivers/net/virtio/virtio_user/vhost_kernel_tap.c | 56 > > > +++++++++++++++++------ > > > drivers/net/virtio/virtio_user/vhost_kernel_tap.h | 2 +- > > > 3 files changed, 44 insertions(+), 16 deletions(-) > > > > <...> > > > > I'm not quite sure whether it's a good idea to return failure when failed to set > > offloads to tap. And QEMU also doesn't do this: > > > > https://github.com/qemu/qemu/blob/6ad90805383e/net/tap-linux.c#L261 > > Regardless of what has already been done in qemu my preference is that if a requested feature cannot be set then a failure should be propagated up to the application. Otherwise, the application is left to debug a silent failure which, depending on what that is, may be difficult. It's still logged as an error, isn't it? Currently, it's just related to the guest offloads. It seems that failed to set offloads to tap just means that users of tap won't be able to leverage the offload capabilities provided by guest (i.e. virtio-user) as tap won't announce them. And it's not really necessary to fail the virtio-user's device start in this case. Thoughts? > But, as the maintainer, that is your call so if you want it changed to a silent failure I will do that. Just so that I am clear you want both failures to be ignored? That is, if a requested offload is not supported we should silently ignore the -ENOTSUP, and also if the ioctl fails for some other reason we should ignore that too? > > > > > > We should also check whether offloads available when handling > > VHOST_GET_FEATURES. > > That will have to be a different patch as it is outside of the scope of what was being fixed by this one. For this issue we were fixing the fact that offload features were being enabled even when they were not requested by the application. It's better to fix it in the same patch set if the error handling is changed. Thanks > > > Allain > >