Re: [dpdk-dev] [PATCH] net/virtio-user: check negotiated features before set

[Tiwei Bie <tiwei.bie@intel.com>]

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 <eric.zhang@windriver.com>
> > >
> > > 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 <eric.zhang@windriver.com>
> > > Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
> > > ---
> > >  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
> 
>