Linux Virtualization - Mar 2023 - [PATCH 1/2] vdpa/mlx5: Make VIRTIO_NET_F_MRG_RXBUF off by default

On Mon, Mar 13, 2023 at 09:14:38PM +0000, Parav Pandit
wrote:
> 
> 
> > From: Michael S. Tsirkin <mst at 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 at 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 at
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

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 at 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 at 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 at
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.
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