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

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

[Michael S. Tsirkin]

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?


> ---
>  drivers/vdpa/mlx5/net/mlx5_vnet.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 4abc3a4ee515..3858ba1e8975 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -3154,6 +3154,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>  			return -EINVAL;
>  		}
>  		device_features &= add_config->device_features;
> +	} else {
> +		device_features &= ~BIT_ULL(VIRTIO_NET_F_MRG_RXBUF);
>  	}
>  	if (!(device_features & BIT_ULL(VIRTIO_F_VERSION_1) &&
>  	      device_features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM))) {
> -- 
> 2.38.1

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

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

[Michael S. Tsirkin]

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.

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.

> > 
> > > ---
> > >   drivers/vdpa/mlx5/net/mlx5_vnet.c | 2 ++
> > >   1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > index 4abc3a4ee515..3858ba1e8975 100644
> > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > @@ -3154,6 +3154,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> > >   			return -EINVAL;
> > >   		}
> > >   		device_features &= add_config->device_features;
> > > +	} else {
> > > +		device_features &= ~BIT_ULL(VIRTIO_NET_F_MRG_RXBUF);
> > >   	}
> > >   	if (!(device_features & BIT_ULL(VIRTIO_F_VERSION_1) &&
> > >   	      device_features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM))) {
> > > -- 
> > > 2.38.1

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

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

[Michael S. Tsirkin]

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?

-- 
MST

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

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

[Michael S. Tsirkin]

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

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

[Si-Wei Liu]

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@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.
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@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Michael S. Tsirkin]

On Mon, Mar 13, 2023 at 09:34:50PM +0000, Parav Pandit wrote:
> 
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Monday, March 13, 2023 5:24 PM
> > I mean if this is claiming to fix a performance regression it should have a Fixes:
> > tag with the commit that introduced the regression.
> 
> The fixes tag should be,
> 
> Fixes: deeacf35c922d ("vdpa/mlx5: support device features provisioning")
> 
> Eli,
> Please add.

Hmm okay so to make sure, before this commit device by default
is created without mrg buffers? With the commit it's created
with and what the change does it revert the default
back to what it was?

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

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

[Michael S. Tsirkin]

On Tue, Mar 14, 2023 at 02:07:02AM +0000, Parav Pandit wrote:
> 
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Monday, March 13, 2023 5:45 PM
> 
> > On Mon, Mar 13, 2023 at 09:34:50PM +0000, Parav Pandit wrote:
> > >
> > >
> > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > Sent: Monday, March 13, 2023 5:24 PM I mean if this is claiming to
> > > > fix a performance regression it should have a Fixes:
> > > > tag with the commit that introduced the regression.
> > >
> > > The fixes tag should be,
> > >
> > > Fixes: deeacf35c922d ("vdpa/mlx5: support device features
> > > provisioning")
> > >
> > > Eli,
> > > Please add.
> > 
> > Hmm okay so to make sure, before this commit device by default is created
> > without mrg buffers? With the commit it's created with and what the change
> > does it revert the default back to what it was?
> 
> Yes.

So IIUC currently default comes from device. Which makes sense at some
level, we can say device knows best maybe?
However in this case, we decided to override the default
because changing this improves some things and regresses others.
Which is ok at some level but then how does user know what
are the device defaults?

-- 
MST

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

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

[Si-Wei Liu]

On 3/12/2023 6:28 AM, 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.
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.

-Siwei

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

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

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

[Michael S. Tsirkin]

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 doesn’t supply feature bits, use the device defaults.
> But not to mix both feature bits.

Can't say I understand what this means. What does "both" mean here?

-- 
MST

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

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

[Michael S. Tsirkin]

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

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

[Si-Wei Liu]

On 3/13/2023 8:47 PM, 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.
I think that is the not the current behavior for mlx5_vdpa? As far as I 
can see it would AND user supplied device_features bits with the device 
supported feature bits (find the common part of both feature bits) then 
proceed. I am not sure I follow what is the problem?

> 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.
Noted, we don't have this device defaults exposed to users yet other 
than what's shown in the parent mgmtdev (dev_features). And the current 
and the only safe behavior for mlx5_vdpa is to inherit all device 
supported features from parent mgmtdev, otherwise user can't know in 
advance what default features the device might come up with implicitly 
underneath. Basically it would need to add new CLI interface to achieve 
what you want.

Thanks,
-Siwei

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

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

[Si-Wei Liu]

On 3/14/2023 7:09 PM, Parav Pandit wrote:
>> From: Si-Wei Liu <si-wei.liu@oracle.com>
>> Sent: Tuesday, March 14, 2023 3:29 PM
>>> 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.
>> I think that is the not the current behavior for mlx5_vdpa? As far as I can see it
>> would AND user supplied device_features bits with the device supported
>> feature bits (find the common part of both feature bits) then proceed.
> Right.
> It is not a problem. It is AND operation. My bad.
Thanks for confirming it.
>
> When the user didn’t supply the feature bits, this patch avoids MRG_RXBUF being on as default.
This should be fine, the driver should have the freedom to define its 
own set of default features when user didn't explicitly provision them 
through the device_features attribute. But better to expose a read-only 
filed, e.g. "default_features" to the "vdpa mgmtdev show" command 
output, otherwise users have no way to infer why MRG_RXBUF disappears 
mysteriously by just telling it from the current "dev_features" field. 
By definition, parent mgmtdev can have different default value between 
different devices.
> This is because certain devices support it and certain device don’t.
> So this patch keeps it backward compatible to be always off by default, which the device couldn’t do.
As previously discussed with Jason and I myself also agreed, it's never 
a requirement for driver to keep backward compatibility for the default 
feature bits when device_features is not supplied. The only way forward 
to keep backward compatibility is to perform explicit provisioning 
through device_features. But I think your point is to keep the 
performance promise for packet rate for the default device creation, 
that is another story and it has nothing to do with backward compatibility.


Thanks,
-Siwei
>
>> Noted, we don't have this device defaults exposed to users yet other than
>> what's shown in the parent mgmtdev (dev_features). And the current and the
>> only safe behavior for mlx5_vdpa is to inherit all device supported features
>> from parent mgmtdev, otherwise user can't know in advance what default
>> features the device might come up with implicitly underneath.
> Right. This is fine.
> For mlx5_vdpa MRG_RXBUF as default off is optimal as before that maintains the backward compatibility.

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