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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 3/13/2023 2:23 PM, Michael S. Tsirkin wrote:
On Mon, Mar 13, 2023 at 09:14:38PM +0000, Parav Pandit wrote:

From: Michael S. Tsirkin <mst@xxxxxxxxxx>
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@xxxxxxxxxx>
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@xxxxxxxxxx>
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.
I guess Eli and Parav may refer to the case where the hardware/firmware is going to offer VIRTIO_NET_F_MRG_RXBUF feature which has sort of performance impact to certain type of workload, while they want to keep the performance promise for the default vdap dev creation without having to mask the corresponding feature bit by explicitly specifying device_features. I don't think Fixes: tag is applicable here, right?

-Siwei




_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization



[Index of Archives]     [KVM Development]     [Libvirt Development]     [Libvirt Users]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux