Re: [PATCH 1/2] vdpa/mlx5: Make VIRTIO_NET_F_MRG_RXBUF off by default

From: "Michael S. Tsirkin" <mst@redhat.com>
To: Parav Pandit <parav@mellanox.com>
Cc: "perezma@redhat.com" <perezma@redhat.com>,
	"virtualization@lists.linux-foundation.org"
	<virtualization@lists.linux-foundation.org>,
	Eugenio Perez Martin <eperezma@redhat.com>,
	Eli Cohen <elic@nvidia.com>
Subject: Re: [PATCH 1/2] vdpa/mlx5: Make VIRTIO_NET_F_MRG_RXBUF off by default
Date: Tue, 14 Mar 2023 00:33:06 -0400	[thread overview]
Message-ID: <20230314002939-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <PH0PR12MB54816DE6E7A0594D38019305DCBE9@PH0PR12MB5481.namprd12.prod.outlook.com>

On Tue, Mar 14, 2023 at 03:47:43AM +0000, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Monday, March 13, 2023 11:38 PM
> > 
> > On Tue, Mar 14, 2023 at 02:05:50AM +0000, Parav Pandit wrote:
> > >
> > > > From: Si-Wei Liu <si-wei.liu@oracle.com>
> > > > Sent: Monday, March 13, 2023 6:19 PM
> > >
> > > > Actually there's no such burden or requirement to maintain backward
> > > > compatibility for the default 'vdpa dev add' behavior if dedicated
> > > > device_features is not specified. Historically the default vdpa
> > > > creation on mlx5 ever got changed from single queue to 8 queue pairs
> > > > when VIRTIO_NET_F_MQ feature was first introduced to mlx5_vdpa, then
> > > > the default switched back to 1 data queue pair again when max_vqp
> > attribute was added to the vdpa tool.
> > > > Essentially, every addition of new feature to mlx5_vdpa, e.g.
> > > > CTRL_VQ, CTRL_VLAN, and et al, effectively changed the default "vdpa
> > > > dev add" behavior not just only once: the backward compatibility
> > > > guarantee is simply just not there and ever.
> > > This requires that every change in the device attributes will change the
> > behavior for vdpa dev add.
> > > The OR operation of the user supplied feature bits with device defaults
> > feature bit doesn’t look good to me.
> > > It brings uncertain behavior.
> > >
> > > The right behavior should be, if user supplied the feature bits, it should
> > supply all desired bits.
> > 
> > I think u mean all that device also supports.
> >
> If user supplied feature bits and device doesn’t support some of the feature bits -> add command fails.
> If user supplied feature bits, than use only the feature bits. Do not OR user supplied feature bits with device supported feature bits.
> For example,
> User asked to set,
> F_CONFIG_MTU
> F_CONFIG_MAC
> F_PACKED
> In that device should only use above 3 features. (because user is the master).
> 
> It should not OR user supplied feature bits with device defaults feature bits.
>  
> > > If user doesn’t supply feature bits, use the device defaults.
> 
> If user doesn’t supply any feature fits, that means user wants to run with some device defaults.
> In that case its fine to run with device defaults.

But this is not what this patch does?

> > > But not to mix both feature bits.
> > 
> > Can't say I understand what this means. What does "both" mean here?
> 
> Mix Both means, to perform OR operation on user supplied feature bits with device defaults.

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization