* Re: [PATCH] net/mlx5_vdpa: Offer VIRTIO_NET_F_MTU when setting MTU
[not found] <20211124170949.51725-1-elic@nvidia.com>
@ 2021-11-24 23:47 ` Si-Wei Liu
2021-11-25 4:37 ` Jason Wang
2021-11-25 5:29 ` Parav Pandit via Virtualization
2 siblings, 0 replies; 5+ messages in thread
From: Si-Wei Liu @ 2021-11-24 23:47 UTC (permalink / raw)
To: Eli Cohen, mst, jasowang, virtualization; +Cc: lvivier, eperezma
On 11/24/2021 9:09 AM, Eli Cohen wrote:
> Make sure to offer VIRTIO_NET_F_MTU since we configure the MTU based on
> what was queried from the device.
>
> This allows the virtio driver to allocate large enough buffers based on
> the reported MTU.
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
Reviewed-by: Si-Wei Liu <si-wei.liu@oracle.com>
> ---
> drivers/vdpa/mlx5/net/mlx5_vnet.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 465e832f2ad1..ed7a63e48335 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -1956,6 +1956,7 @@ static u64 mlx5_vdpa_get_features(struct vdpa_device *vdev)
> ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR);
> ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MQ);
> ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_STATUS);
> + ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MTU);
>
> print_features(mvdev, ndev->mvdev.mlx_features, false);
> return ndev->mvdev.mlx_features;
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net/mlx5_vdpa: Offer VIRTIO_NET_F_MTU when setting MTU
[not found] <20211124170949.51725-1-elic@nvidia.com>
2021-11-24 23:47 ` [PATCH] net/mlx5_vdpa: Offer VIRTIO_NET_F_MTU when setting MTU Si-Wei Liu
@ 2021-11-25 4:37 ` Jason Wang
2021-11-25 5:29 ` Parav Pandit via Virtualization
2 siblings, 0 replies; 5+ messages in thread
From: Jason Wang @ 2021-11-25 4:37 UTC (permalink / raw)
To: Eli Cohen; +Cc: Laurent Vivier, mst, virtualization, eperezma, si-wei.liu
On Thu, Nov 25, 2021 at 1:10 AM Eli Cohen <elic@nvidia.com> wrote:
>
> Make sure to offer VIRTIO_NET_F_MTU since we configure the MTU based on
> what was queried from the device.
>
> This allows the virtio driver to allocate large enough buffers based on
> the reported MTU.
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
Acked-by: Jason Wang <jasowang@redhat.com>
> ---
> drivers/vdpa/mlx5/net/mlx5_vnet.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 465e832f2ad1..ed7a63e48335 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -1956,6 +1956,7 @@ static u64 mlx5_vdpa_get_features(struct vdpa_device *vdev)
> ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR);
> ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MQ);
> ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_STATUS);
> + ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MTU);
>
> print_features(mvdev, ndev->mvdev.mlx_features, false);
> return ndev->mvdev.mlx_features;
> --
> 2.33.1
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] net/mlx5_vdpa: Offer VIRTIO_NET_F_MTU when setting MTU
[not found] <20211124170949.51725-1-elic@nvidia.com>
2021-11-24 23:47 ` [PATCH] net/mlx5_vdpa: Offer VIRTIO_NET_F_MTU when setting MTU Si-Wei Liu
2021-11-25 4:37 ` Jason Wang
@ 2021-11-25 5:29 ` Parav Pandit via Virtualization
[not found] ` <20211125080227.GE214101@mtl-vdi-166.wap.labs.mlnx>
2 siblings, 1 reply; 5+ messages in thread
From: Parav Pandit via Virtualization @ 2021-11-25 5:29 UTC (permalink / raw)
To: Eli Cohen, mst, jasowang, virtualization; +Cc: lvivier, eperezma, si-wei.liu
> From: Eli Cohen <elic@nvidia.com>
> Sent: Wednesday, November 24, 2021 10:40 PM
>
> Make sure to offer VIRTIO_NET_F_MTU since we configure the MTU based on
> what was queried from the device.
>
> This allows the virtio driver to allocate large enough buffers based on the
> reported MTU.
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> ---
> drivers/vdpa/mlx5/net/mlx5_vnet.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 465e832f2ad1..ed7a63e48335 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -1956,6 +1956,7 @@ static u64 mlx5_vdpa_get_features(struct
> vdpa_device *vdev)
> ndev->mvdev.mlx_features |=
> BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR);
> ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MQ);
> ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_STATUS);
> + ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MTU);
It is better to set this feature bit along with the writing the RO config.mtu area, adjacent to query_mtu() call.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] net/mlx5_vdpa: Offer VIRTIO_NET_F_MTU when setting MTU
[not found] ` <20211125080227.GE214101@mtl-vdi-166.wap.labs.mlnx>
@ 2021-11-25 18:27 ` Parav Pandit via Virtualization
2021-11-25 20:42 ` Michael S. Tsirkin
0 siblings, 1 reply; 5+ messages in thread
From: Parav Pandit via Virtualization @ 2021-11-25 18:27 UTC (permalink / raw)
To: Eli Cohen; +Cc: lvivier, mst, virtualization, eperezma, si-wei.liu
> From: Eli Cohen <elic@nvidia.com>
> Sent: Thursday, November 25, 2021 1:32 PM
>
> On Thu, Nov 25, 2021 at 07:29:18AM +0200, Parav Pandit wrote:
> >
> >
> > > From: Eli Cohen <elic@nvidia.com>
> > > Sent: Wednesday, November 24, 2021 10:40 PM
> > >
> > > Make sure to offer VIRTIO_NET_F_MTU since we configure the MTU based
> > > on what was queried from the device.
> > >
> > > This allows the virtio driver to allocate large enough buffers based
> > > on the reported MTU.
> > >
> > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > ---
> > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > index 465e832f2ad1..ed7a63e48335 100644
> > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > @@ -1956,6 +1956,7 @@ static u64 mlx5_vdpa_get_features(struct
> > > vdpa_device *vdev)
> > > ndev->mvdev.mlx_features |=
> > > BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR);
> > > ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MQ);
> > > ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_STATUS);
> > > + ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MTU);
> > It is better to set this feature bit along with the writing the RO config.mtu
> area, adjacent to query_mtu() call.
>
> Why is that so important? We always query the mtu and if the query fails, we
> fail the initialization so it does not look critical.
It is not so important. But it is more appropriate to set read only field and its associated feature bit at one place, which never changes.
There is no need to perform OR operation for those many feature bits on every get_features() callback when they are immutable.
So I wanted to move setting other 5 feature bits assignment at one place in device initialization time similar to how you have done VALID_FEATURES_MASK.
Something like below.
But again, its minor.
If you happen to revise the series (I think you should for the supporting dumpit in future), please consider one time assignment like below.
diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index 63813fbb5f62..21802b25b0f5 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -1890,11 +1890,6 @@ static u64 mlx5_vdpa_get_features(struct vdpa_device *vdev)
ndev->mvdev.mlx_features |= mlx_to_vritio_features(dev_features);
if (MLX5_CAP_DEV_VDPA_EMULATION(mvdev->mdev, virtio_version_1_0))
ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_F_VERSION_1);
- ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_F_ACCESS_PLATFORM);
- ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_CTRL_VQ);
- ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR);
- ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MQ);
- ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_STATUS);
print_features(mvdev, ndev->mvdev.mlx_features, false);
return ndev->mvdev.mlx_features;
@@ -2522,6 +2517,14 @@ static int event_handler(struct notifier_block *nb, unsigned long event, void *p
return ret;
}
+#define DEFAULT_FEATURES \
+ BIT_ULL(VIRTIO_F_ACCESS_PLATFORM) | \
+ BIT_ULL(VIRTIO_NET_F_CTRL_VQ) | \
+ BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR) \
+ BIT_ULL(VIRTIO_NET_F_MQ) \
+ BIT_ULL(VIRTIO_NET_F_STATUS) \
+ BIT_ULL(VIRTIO_NET_F_MTU)
+
static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
const struct vdpa_dev_set_config *add_config)
{
@@ -2565,6 +2568,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
goto err_mtu;
ndev->config.mtu = cpu_to_mlx5vdpa16(mvdev, mtu);
+ ndev->mvdev.mlx_features = DEFAULT_FEATURES;
if (get_link_state(mvdev))
ndev->config.status |= cpu_to_mlx5vdpa16(mvdev, VIRTIO_NET_S_LINK_UP);
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] net/mlx5_vdpa: Offer VIRTIO_NET_F_MTU when setting MTU
2021-11-25 18:27 ` Parav Pandit via Virtualization
@ 2021-11-25 20:42 ` Michael S. Tsirkin
0 siblings, 0 replies; 5+ messages in thread
From: Michael S. Tsirkin @ 2021-11-25 20:42 UTC (permalink / raw)
To: Parav Pandit; +Cc: lvivier, virtualization, eperezma, si-wei.liu, Eli Cohen
On Thu, Nov 25, 2021 at 06:27:15PM +0000, Parav Pandit wrote:
>
>
> > From: Eli Cohen <elic@nvidia.com>
> > Sent: Thursday, November 25, 2021 1:32 PM
> >
> > On Thu, Nov 25, 2021 at 07:29:18AM +0200, Parav Pandit wrote:
> > >
> > >
> > > > From: Eli Cohen <elic@nvidia.com>
> > > > Sent: Wednesday, November 24, 2021 10:40 PM
> > > >
> > > > Make sure to offer VIRTIO_NET_F_MTU since we configure the MTU based
> > > > on what was queried from the device.
> > > >
> > > > This allows the virtio driver to allocate large enough buffers based
> > > > on the reported MTU.
> > > >
> > > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > > ---
> > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 1 +
> > > > 1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > index 465e832f2ad1..ed7a63e48335 100644
> > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > @@ -1956,6 +1956,7 @@ static u64 mlx5_vdpa_get_features(struct
> > > > vdpa_device *vdev)
> > > > ndev->mvdev.mlx_features |=
> > > > BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR);
> > > > ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MQ);
> > > > ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_STATUS);
> > > > + ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MTU);
> > > It is better to set this feature bit along with the writing the RO config.mtu
> > area, adjacent to query_mtu() call.
> >
> > Why is that so important? We always query the mtu and if the query fails, we
> > fail the initialization so it does not look critical.
> It is not so important. But it is more appropriate to set read only field and its associated feature bit at one place, which never changes.
> There is no need to perform OR operation for those many feature bits on every get_features() callback when they are immutable.
>
> So I wanted to move setting other 5 feature bits assignment at one place in device initialization time similar to how you have done VALID_FEATURES_MASK.
> Something like below.
> But again, its minor.
> If you happen to revise the series (I think you should for the supporting dumpit in future), please consider one time assignment like below.
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 63813fbb5f62..21802b25b0f5 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -1890,11 +1890,6 @@ static u64 mlx5_vdpa_get_features(struct vdpa_device *vdev)
> ndev->mvdev.mlx_features |= mlx_to_vritio_features(dev_features);
> if (MLX5_CAP_DEV_VDPA_EMULATION(mvdev->mdev, virtio_version_1_0))
> ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_F_VERSION_1);
> - ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_F_ACCESS_PLATFORM);
> - ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_CTRL_VQ);
> - ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR);
> - ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MQ);
> - ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_STATUS);
>
> print_features(mvdev, ndev->mvdev.mlx_features, false);
> return ndev->mvdev.mlx_features;
> @@ -2522,6 +2517,14 @@ static int event_handler(struct notifier_block *nb, unsigned long event, void *p
> return ret;
> }
>
> +#define DEFAULT_FEATURES \
> + BIT_ULL(VIRTIO_F_ACCESS_PLATFORM) | \
> + BIT_ULL(VIRTIO_NET_F_CTRL_VQ) | \
> + BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR) \
> + BIT_ULL(VIRTIO_NET_F_MQ) \
> + BIT_ULL(VIRTIO_NET_F_STATUS) \
> + BIT_ULL(VIRTIO_NET_F_MTU)
> +
I would just open-code it, don't see what good does the macro do.
A single assignment is a bit clearer I think I agree,
there's not much of a difference.
> static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> const struct vdpa_dev_set_config *add_config)
> {
> @@ -2565,6 +2568,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> goto err_mtu;
>
> ndev->config.mtu = cpu_to_mlx5vdpa16(mvdev, mtu);
> + ndev->mvdev.mlx_features = DEFAULT_FEATURES;
>
> if (get_link_state(mvdev))
> ndev->config.status |= cpu_to_mlx5vdpa16(mvdev, VIRTIO_NET_S_LINK_UP);
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-11-25 20:42 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20211124170949.51725-1-elic@nvidia.com>
2021-11-24 23:47 ` [PATCH] net/mlx5_vdpa: Offer VIRTIO_NET_F_MTU when setting MTU Si-Wei Liu
2021-11-25 4:37 ` Jason Wang
2021-11-25 5:29 ` Parav Pandit via Virtualization
[not found] ` <20211125080227.GE214101@mtl-vdi-166.wap.labs.mlnx>
2021-11-25 18:27 ` Parav Pandit via Virtualization
2021-11-25 20:42 ` Michael S. Tsirkin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.