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: Mon, 13 Mar 2023 17:23:51 -0400	[thread overview]
Message-ID: <20230313172309-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <PH0PR12MB548140734E1A0A8A2FD2CE30DCB99@PH0PR12MB5481.namprd12.prod.outlook.com>

On Mon, Mar 13, 2023 at 09:14:38PM +0000, Parav Pandit wrote:
> 
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Sunday, March 12, 2023 12:24 PM
> > 
> > On Sun, Mar 12, 2023 at 01:28:06PM +0000, Parav Pandit wrote:
> > >
> > >
> > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > Sent: Sunday, March 12, 2023 6:25 AM
> > > >
> > > > On Sun, Mar 12, 2023 at 11:10:25AM +0200, Eli Cohen wrote:
> > > > >
> > > > > On 12/03/2023 10:58, Michael S. Tsirkin wrote:
> > > > > > On Sun, Mar 12, 2023 at 10:39:19AM +0200, Eli Cohen wrote:
> > > > > > > One can still enable it when creating the vdpa device using
> > > > > > > vdpa tool by providing features that include it.
> > > > > > >
> > > > > > > For example:
> > > > > > > $ vdpa dev add name vdpa0 mgmtdev pci/0000:86:00.2
> > > > > > > device_features 0x300cb982b
> > > > > > >
> > > > > > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > > > > What's the reason to turn it off by default? It's generally a
> > > > > > performance win isn't it?
> > > > > It has negative impact on packet rate so we want to keep it off by default.
> > > >
> > > The performance characteristics is very workload specific.
> > > It is less of interest given the primary reason is backward compatibility, more
> > below.
> > >
> > > > Interesting.  I feel this would benefit from a bit more analysis.
> > > > Packet rate with dpdk? With linux? Is there a chance this will
> > > > regress some workloads?
> > > > VIRTIO_NET_F_MRG_RXBUF was designed to save memory, which is good
> > > > for small tcp buffers.
> > >
> > > Eli,
> > > Please update the commit message.
> > > This change is to avoid regression in existing systems.
> > > The device previously didn't report MRG_RXBUF cap and it was not in use.
> > > Lately, certain devices are reporting this feature bit and it is breaking the
> > backward compatibility.
> > > So the driver keeps it disabled by default.
> > > User should enable it when user prefers to.
> > 
> > OK. And which commit changes that?
> vdpa dev add command [1] has the ability to set the desired features.
> The commit log of this patch has an example too.
> 
> [1] https://elixir.bootlin.com/linux/v6.3-rc2/C/ident/vdpa_nl_cmd_dev_add_set_doit

I mean if this is claiming to fix a performance regression it should have
a Fixes: tag with the commit that introduced the regression.

-- 
MST

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