Re: [Patch v3] vdpa/mlx5: Avoid losing link state updates

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

 



On Sun, Apr 2, 2023 at 10:15 PM Eli Cohen <elic@xxxxxxxxxx> wrote:
>
> Current code ignores link state updates if VIRTIO_NET_F_STATUS was not
> negotiated. However, link state updates could be received before feature
> negotiation was completed , therefore causing link state events to be
> lost, possibly leaving the link state down.
>
> Modify the code so link state notifier is registered only when
> VIRTIO_NET_F_STATUS flips from 0 to 1 and unregister it on driver reset
> or suspend.
>
> Fixes: 033779a708f0 ("vdpa/mlx5: make MTU/STATUS presence conditional on feature bits")
> Signed-off-by: Eli Cohen <elic@xxxxxxxxxx>
> ---
> v2 -> v3
> Only register the link event notifier when VIRTIO_NET_F_STATUS is
> negotiated.
>
>  drivers/vdpa/mlx5/net/mlx5_vnet.c | 200 +++++++++++++++++-------------
>  1 file changed, 112 insertions(+), 88 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 317cef9b7813..9b1432e22540 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -2322,10 +2322,115 @@ static void update_cvq_info(struct mlx5_vdpa_dev *mvdev)
>         }
>  }
>
> +static u8 query_vport_state(struct mlx5_core_dev *mdev, u8 opmod, u16 vport)
> +{
> +       u32 out[MLX5_ST_SZ_DW(query_vport_state_out)] = {};
> +       u32 in[MLX5_ST_SZ_DW(query_vport_state_in)] = {};
> +       int err;
> +
> +       MLX5_SET(query_vport_state_in, in, opcode, MLX5_CMD_OP_QUERY_VPORT_STATE);
> +       MLX5_SET(query_vport_state_in, in, op_mod, opmod);
> +       MLX5_SET(query_vport_state_in, in, vport_number, vport);
> +       if (vport)
> +               MLX5_SET(query_vport_state_in, in, other_vport, 1);
> +
> +       err = mlx5_cmd_exec_inout(mdev, query_vport_state, in, out);
> +       if (err)
> +               return 0;
> +
> +       return MLX5_GET(query_vport_state_out, out, state);
> +}
> +
> +static bool get_link_state(struct mlx5_vdpa_dev *mvdev)
> +{
> +       if (query_vport_state(mvdev->mdev, MLX5_VPORT_STATE_OP_MOD_VNIC_VPORT, 0) ==
> +           VPORT_STATE_UP)
> +               return true;
> +
> +       return false;
> +}
> +
> +static void update_carrier(struct work_struct *work)
> +{
> +       struct mlx5_vdpa_wq_ent *wqent;
> +       struct mlx5_vdpa_dev *mvdev;
> +       struct mlx5_vdpa_net *ndev;
> +
> +       wqent = container_of(work, struct mlx5_vdpa_wq_ent, work);
> +       mvdev = wqent->mvdev;
> +       ndev = to_mlx5_vdpa_ndev(mvdev);
> +       if (get_link_state(mvdev))
> +               ndev->config.status |= cpu_to_mlx5vdpa16(mvdev, VIRTIO_NET_S_LINK_UP);
> +       else
> +               ndev->config.status &= cpu_to_mlx5vdpa16(mvdev, ~VIRTIO_NET_S_LINK_UP);
> +
> +       if (ndev->nb_registered && ndev->config_cb.callback)

It looks to me nb_registered is accessed without synchronization. Or
we don't even need to check that if we do:

unregister();
flush_workqueue();

which has been done in unregister_link_notifier().

> +               ndev->config_cb.callback(ndev->config_cb.private);
> +
> +       kfree(wqent);
> +}
> +
> +static int queue_link_work(struct mlx5_vdpa_net *ndev)
> +{
> +       struct mlx5_vdpa_wq_ent *wqent;
> +
> +       wqent = kzalloc(sizeof(*wqent), GFP_ATOMIC);
> +       if (!wqent)
> +               return -ENOMEM;
> +
> +       wqent->mvdev = &ndev->mvdev;
> +       INIT_WORK(&wqent->work, update_carrier);
> +       queue_work(ndev->mvdev.wq, &wqent->work);
> +       return 0;
> +}
> +
> +static int event_handler(struct notifier_block *nb, unsigned long event, void *param)
> +{
> +       struct mlx5_vdpa_net *ndev = container_of(nb, struct mlx5_vdpa_net, nb);
> +       struct mlx5_eqe *eqe = param;
> +       int ret = NOTIFY_DONE;
> +
> +       if (event == MLX5_EVENT_TYPE_PORT_CHANGE) {
> +               switch (eqe->sub_type) {
> +               case MLX5_PORT_CHANGE_SUBTYPE_DOWN:
> +               case MLX5_PORT_CHANGE_SUBTYPE_ACTIVE:
> +                       if (queue_link_work(ndev))
> +                               return NOTIFY_DONE;
> +
> +                       ret = NOTIFY_OK;
> +                       break;
> +               default:
> +                       return NOTIFY_DONE;
> +               }
> +               return ret;
> +       }
> +       return ret;
> +}
> +
> +static void register_link_notifier(struct mlx5_vdpa_net *ndev)
> +{
> +       ndev->nb.notifier_call = event_handler;
> +       mlx5_notifier_register(ndev->mvdev.mdev, &ndev->nb);
> +       ndev->nb_registered = true;
> +       queue_link_work(ndev);
> +}
> +
> +static void unregister_link_notifier(struct mlx5_vdpa_net *ndev)
> +{
> +       if (!ndev->nb_registered)
> +               return;
> +
> +       ndev->nb_registered = false;
> +       mlx5_notifier_unregister(ndev->mvdev.mdev, &ndev->nb);
> +       if (ndev->mvdev.wq)

Under which case could we hit mvdev.wq = NULL?

(We call unregister_link_notifier() before setting mq to NULL during
device del).

> +               flush_workqueue(ndev->mvdev.wq);
> +}
> +
>  static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
>  {
>         struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
>         struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> +       u64 old;
>         int err;
>
>         print_features(mvdev, features, true);
> @@ -2334,7 +2439,11 @@ static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
>         if (err)
>                 return err;
>
> +       old = ndev->mvdev.actual_features;
>         ndev->mvdev.actual_features = features & ndev->mvdev.mlx_features;
> +       if (~old & ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_STATUS))
> +               register_link_notifier(ndev);
> +

I still think it's better to move this check during set_status.
Otherwise, we may have a small window where the link update event is
sent before DRIVER_OK.

Thanks

>         if (ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ))
>                 ndev->rqt_size = mlx5vdpa16_to_cpu(mvdev, ndev->config.max_virtqueue_pairs);
>         else
> @@ -2629,6 +2738,7 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
>         mlx5_vdpa_info(mvdev, "performing device reset\n");
>
>         down_write(&ndev->reslock);
> +       unregister_link_notifier(ndev);
>         teardown_driver(ndev);
>         clear_vqs_ready(ndev);
>         mlx5_vdpa_destroy_mr(&ndev->mvdev);
> @@ -2883,9 +2993,7 @@ static int mlx5_vdpa_suspend(struct vdpa_device *vdev)
>         mlx5_vdpa_info(mvdev, "suspending device\n");
>
>         down_write(&ndev->reslock);
> -       ndev->nb_registered = false;
> -       mlx5_notifier_unregister(mvdev->mdev, &ndev->nb);
> -       flush_workqueue(ndev->mvdev.wq);
> +       unregister_link_notifier(ndev);
>         for (i = 0; i < ndev->cur_num_vqs; i++) {
>                 mvq = &ndev->vqs[i];
>                 suspend_vq(ndev, mvq);
> @@ -3022,84 +3130,6 @@ struct mlx5_vdpa_mgmtdev {
>         struct mlx5_vdpa_net *ndev;
>  };
>
> -static u8 query_vport_state(struct mlx5_core_dev *mdev, u8 opmod, u16 vport)
> -{
> -       u32 out[MLX5_ST_SZ_DW(query_vport_state_out)] = {};
> -       u32 in[MLX5_ST_SZ_DW(query_vport_state_in)] = {};
> -       int err;
> -
> -       MLX5_SET(query_vport_state_in, in, opcode, MLX5_CMD_OP_QUERY_VPORT_STATE);
> -       MLX5_SET(query_vport_state_in, in, op_mod, opmod);
> -       MLX5_SET(query_vport_state_in, in, vport_number, vport);
> -       if (vport)
> -               MLX5_SET(query_vport_state_in, in, other_vport, 1);
> -
> -       err = mlx5_cmd_exec_inout(mdev, query_vport_state, in, out);
> -       if (err)
> -               return 0;
> -
> -       return MLX5_GET(query_vport_state_out, out, state);
> -}
> -
> -static bool get_link_state(struct mlx5_vdpa_dev *mvdev)
> -{
> -       if (query_vport_state(mvdev->mdev, MLX5_VPORT_STATE_OP_MOD_VNIC_VPORT, 0) ==
> -           VPORT_STATE_UP)
> -               return true;
> -
> -       return false;
> -}
> -
> -static void update_carrier(struct work_struct *work)
> -{
> -       struct mlx5_vdpa_wq_ent *wqent;
> -       struct mlx5_vdpa_dev *mvdev;
> -       struct mlx5_vdpa_net *ndev;
> -
> -       wqent = container_of(work, struct mlx5_vdpa_wq_ent, work);
> -       mvdev = wqent->mvdev;
> -       ndev = to_mlx5_vdpa_ndev(mvdev);
> -       if (get_link_state(mvdev))
> -               ndev->config.status |= cpu_to_mlx5vdpa16(mvdev, VIRTIO_NET_S_LINK_UP);
> -       else
> -               ndev->config.status &= cpu_to_mlx5vdpa16(mvdev, ~VIRTIO_NET_S_LINK_UP);
> -
> -       if (ndev->nb_registered && ndev->config_cb.callback)
> -               ndev->config_cb.callback(ndev->config_cb.private);
> -
> -       kfree(wqent);
> -}
> -
> -static int event_handler(struct notifier_block *nb, unsigned long event, void *param)
> -{
> -       struct mlx5_vdpa_net *ndev = container_of(nb, struct mlx5_vdpa_net, nb);
> -       struct mlx5_eqe *eqe = param;
> -       int ret = NOTIFY_DONE;
> -       struct mlx5_vdpa_wq_ent *wqent;
> -
> -       if (event == MLX5_EVENT_TYPE_PORT_CHANGE) {
> -               if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_STATUS)))
> -                       return NOTIFY_DONE;
> -               switch (eqe->sub_type) {
> -               case MLX5_PORT_CHANGE_SUBTYPE_DOWN:
> -               case MLX5_PORT_CHANGE_SUBTYPE_ACTIVE:
> -                       wqent = kzalloc(sizeof(*wqent), GFP_ATOMIC);
> -                       if (!wqent)
> -                               return NOTIFY_DONE;
> -
> -                       wqent->mvdev = &ndev->mvdev;
> -                       INIT_WORK(&wqent->work, update_carrier);
> -                       queue_work(ndev->mvdev.wq, &wqent->work);
> -                       ret = NOTIFY_OK;
> -                       break;
> -               default:
> -                       return NOTIFY_DONE;
> -               }
> -               return ret;
> -       }
> -       return ret;
> -}
> -
>  static int config_func_mtu(struct mlx5_core_dev *mdev, u16 mtu)
>  {
>         int inlen = MLX5_ST_SZ_BYTES(modify_nic_vport_context_in);
> @@ -3282,9 +3312,6 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>                 goto err_res2;
>         }
>
> -       ndev->nb.notifier_call = event_handler;
> -       mlx5_notifier_register(mdev, &ndev->nb);
> -       ndev->nb_registered = true;
>         mvdev->vdev.mdev = &mgtdev->mgtdev;
>         err = _vdpa_register_device(&mvdev->vdev, max_vqs + 1);
>         if (err)
> @@ -3318,10 +3345,7 @@ static void mlx5_vdpa_dev_del(struct vdpa_mgmt_dev *v_mdev, struct vdpa_device *
>
>         mlx5_vdpa_remove_debugfs(ndev->debugfs);
>         ndev->debugfs = NULL;
> -       if (ndev->nb_registered) {
> -               ndev->nb_registered = false;
> -               mlx5_notifier_unregister(mvdev->mdev, &ndev->nb);
> -       }
> +       unregister_link_notifier(ndev);
>         wq = mvdev->wq;
>         mvdev->wq = NULL;
>         destroy_workqueue(wq);
> --
> 2.38.1
>

_______________________________________________
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