From: Eugenio Perez Martin <eperezma@redhat.com>
To: Dragos Tatulea <dtatulea@nvidia.com>
Cc: "virtualization@lists.linux-foundation.org"
<virtualization@lists.linux-foundation.org>,
"jasowang@redhat.com" <jasowang@redhat.com>,
"mst@redhat.com" <mst@redhat.com>,
"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
Parav Pandit <parav@nvidia.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"xuanzhuo@linux.alibaba.com" <xuanzhuo@linux.alibaba.com>,
"si-wei.liu@oracle.com" <si-wei.liu@oracle.com>
Subject: Re: [PATCH vhost v4 12/16] vdpa/mlx5: Improve mr update flow
Date: Fri, 20 Oct 2023 18:01:09 +0200 [thread overview]
Message-ID: <CAJaqyWfuVV57syaUWkh7-QJ_rKHCPpKbHLv7LdN6j_KeGez2VQ@mail.gmail.com> (raw)
In-Reply-To: <a164fb3975cf9f574314fffba0f2e67bec7e2851.camel@nvidia.com>
On Wed, Oct 18, 2023 at 7:21 PM Dragos Tatulea <dtatulea@nvidia.com> wrote:
>
> On Wed, 2023-10-18 at 20:14 +0300, Dragos Tatulea wrote:
> > The current flow for updating an mr works directly on mvdev->mr which
> > makes it cumbersome to handle multiple new mr structs.
> >
> > This patch makes the flow more straightforward by having
> > mlx5_vdpa_create_mr return a new mr which will update the old mr (if
> > any). The old mr will be deleted and unlinked from mvdev. For the case
> > when the iotlb is empty (not NULL), the old mr will be cleared.
> >
> > This change paves the way for adding mrs for different ASIDs.
> >
> > The initialized bool is no longer needed as mr is now a pointer in the
> > mlx5_vdpa_dev struct which will be NULL when not initialized.
> >
> > Acked-by: Eugenio Pérez <eperezma@redhat.com>
> > Acked-by: Jason Wang <jasowang@redhat.com>
> > Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
> > ---
> > drivers/vdpa/mlx5/core/mlx5_vdpa.h | 14 +++--
> > drivers/vdpa/mlx5/core/mr.c | 87 ++++++++++++++++--------------
> > drivers/vdpa/mlx5/net/mlx5_vnet.c | 53 +++++++++---------
> > 3 files changed, 82 insertions(+), 72 deletions(-)
> >
> > diff --git a/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> > b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> > index 9c6ac42c21e1..bbe4335106bd 100644
> > --- a/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> > +++ b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> > @@ -31,8 +31,6 @@ struct mlx5_vdpa_mr {
> > struct list_head head;
> > unsigned long num_directs;
> > unsigned long num_klms;
> > - /* state of dvq mr */
> > - bool initialized;
> >
> > bool user_mr;
> > };
> > @@ -91,7 +89,7 @@ struct mlx5_vdpa_dev {
> > u16 max_idx;
> > u32 generation;
> >
> > - struct mlx5_vdpa_mr mr;
> > + struct mlx5_vdpa_mr *mr;
> > /* serialize mr access */
> > struct mutex mr_mtx;
> > struct mlx5_control_vq cvq;
> > @@ -114,14 +112,14 @@ void mlx5_vdpa_free_resources(struct mlx5_vdpa_dev
> > *mvdev);
> > int mlx5_vdpa_create_mkey(struct mlx5_vdpa_dev *mvdev, u32 *mkey, u32 *in,
> > int inlen);
> > int mlx5_vdpa_destroy_mkey(struct mlx5_vdpa_dev *mvdev, u32 mkey);
> > -int mlx5_vdpa_handle_set_map(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
> > *iotlb,
> > - bool *change_map, unsigned int asid);
> > -int mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev,
> > - struct mlx5_vdpa_mr *mr,
> > - struct vhost_iotlb *iotlb);
> > +struct mlx5_vdpa_mr *mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev,
> > + struct vhost_iotlb *iotlb);
> > void mlx5_vdpa_destroy_mr_resources(struct mlx5_vdpa_dev *mvdev);
> > void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev,
> > struct mlx5_vdpa_mr *mr);
> > +void mlx5_vdpa_update_mr(struct mlx5_vdpa_dev *mvdev,
> > + struct mlx5_vdpa_mr *mr,
> > + unsigned int asid);
> > int mlx5_vdpa_update_cvq_iotlb(struct mlx5_vdpa_dev *mvdev,
> > struct vhost_iotlb *iotlb,
> > unsigned int asid);
> > diff --git a/drivers/vdpa/mlx5/core/mr.c b/drivers/vdpa/mlx5/core/mr.c
> > index abd6a6fb122f..00eff5a07152 100644
> > --- a/drivers/vdpa/mlx5/core/mr.c
> > +++ b/drivers/vdpa/mlx5/core/mr.c
> > @@ -495,30 +495,51 @@ static void destroy_user_mr(struct mlx5_vdpa_dev *mvdev,
> > struct mlx5_vdpa_mr *mr
> >
> > static void _mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev, struct
> > mlx5_vdpa_mr *mr)
> > {
> > - if (!mr->initialized)
> > - return;
> > -
> > if (mr->user_mr)
> > destroy_user_mr(mvdev, mr);
> > else
> > destroy_dma_mr(mvdev, mr);
> > -
> > - mr->initialized = false;
> > }
> >
> > void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev,
> > struct mlx5_vdpa_mr *mr)
> > {
> > + if (!mr)
> > + return;
> > +
> > mutex_lock(&mvdev->mr_mtx);
> >
> > _mlx5_vdpa_destroy_mr(mvdev, mr);
> >
> > + if (mvdev->mr == mr)
> > + mvdev->mr = NULL;
> > +
> > + mutex_unlock(&mvdev->mr_mtx);
> > +
> > + kfree(mr);
> > +}
> > +
> > +void mlx5_vdpa_update_mr(struct mlx5_vdpa_dev *mvdev,
> > + struct mlx5_vdpa_mr *new_mr,
> > + unsigned int asid)
> > +{
> > + struct mlx5_vdpa_mr *old_mr = mvdev->mr;
> > +
> > + mutex_lock(&mvdev->mr_mtx);
> > +
> > + mvdev->mr = new_mr;
> > + if (old_mr) {
> > + _mlx5_vdpa_destroy_mr(mvdev, old_mr);
> > + kfree(old_mr);
> > + }
> > +
> > mutex_unlock(&mvdev->mr_mtx);
> > +
> > }
> >
> > void mlx5_vdpa_destroy_mr_resources(struct mlx5_vdpa_dev *mvdev)
> > {
> > - mlx5_vdpa_destroy_mr(mvdev, &mvdev->mr);
> > + mlx5_vdpa_destroy_mr(mvdev, mvdev->mr);
> > prune_iotlb(mvdev);
> > }
> >
> > @@ -528,52 +549,36 @@ static int _mlx5_vdpa_create_mr(struct mlx5_vdpa_dev
> > *mvdev,
> > {
> > int err;
> >
> > - if (mr->initialized)
> > - return 0;
> > -
> > if (iotlb)
> > err = create_user_mr(mvdev, mr, iotlb);
> > else
> > err = create_dma_mr(mvdev, mr);
> >
> > - if (err)
> > - return err;
> > -
> > - mr->initialized = true;
> > -
> > - return 0;
> > + return err;
> > }
> >
> > -int mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev,
> > - struct mlx5_vdpa_mr *mr,
> > - struct vhost_iotlb *iotlb)
> > +struct mlx5_vdpa_mr *mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev,
> > + struct vhost_iotlb *iotlb)
> > {
> > + struct mlx5_vdpa_mr *mr;
> > int err;
> >
> > + mr = kzalloc(sizeof(*mr), GFP_KERNEL);
> > + if (!mr)
> > + return ERR_PTR(-ENOMEM);
> > +
> > mutex_lock(&mvdev->mr_mtx);
> > err = _mlx5_vdpa_create_mr(mvdev, mr, iotlb);
> > mutex_unlock(&mvdev->mr_mtx);
> >
> > - return err;
> > -}
> > -
> > -int mlx5_vdpa_handle_set_map(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
> > *iotlb,
> > - bool *change_map, unsigned int asid)
> > -{
> > - struct mlx5_vdpa_mr *mr = &mvdev->mr;
> > - int err = 0;
> > + if (err)
> > + goto out_err;
> >
> > - *change_map = false;
> > - mutex_lock(&mvdev->mr_mtx);
> > - if (mr->initialized) {
> > - mlx5_vdpa_info(mvdev, "memory map update\n");
> > - *change_map = true;
> > - }
> > - if (!*change_map)
> > - err = _mlx5_vdpa_create_mr(mvdev, mr, iotlb);
> > - mutex_unlock(&mvdev->mr_mtx);
> > + return mr;
> >
> > - return err;
> > +out_err:
> > + kfree(mr);
> > + return ERR_PTR(err);
> > }
> >
> > int mlx5_vdpa_update_cvq_iotlb(struct mlx5_vdpa_dev *mvdev,
> > @@ -597,11 +602,13 @@ int mlx5_vdpa_update_cvq_iotlb(struct mlx5_vdpa_dev
> > *mvdev,
> >
> > int mlx5_vdpa_create_dma_mr(struct mlx5_vdpa_dev *mvdev)
> > {
> > - int err;
> > + struct mlx5_vdpa_mr *mr;
> >
> > - err = mlx5_vdpa_create_mr(mvdev, &mvdev->mr, NULL);
> > - if (err)
> > - return err;
> > + mr = mlx5_vdpa_create_mr(mvdev, NULL);
> > + if (IS_ERR(mr))
> > + return PTR_ERR(mr);
> > +
> > + mlx5_vdpa_update_mr(mvdev, mr, 0);
> >
> > return mlx5_vdpa_update_cvq_iotlb(mvdev, NULL, 0);
> > }
> > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > index 256fdd80c321..7b878995b6aa 100644
> > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > @@ -873,7 +873,7 @@ static int create_virtqueue(struct mlx5_vdpa_net *ndev,
> > struct mlx5_vdpa_virtque
> > MLX5_SET64(virtio_q, vq_ctx, desc_addr, mvq->desc_addr);
> > MLX5_SET64(virtio_q, vq_ctx, used_addr, mvq->device_addr);
> > MLX5_SET64(virtio_q, vq_ctx, available_addr, mvq->driver_addr);
> > - MLX5_SET(virtio_q, vq_ctx, virtio_q_mkey, ndev->mvdev.mr.mkey);
> > + MLX5_SET(virtio_q, vq_ctx, virtio_q_mkey, ndev->mvdev.mr->mkey);
> > MLX5_SET(virtio_q, vq_ctx, umem_1_id, mvq->umem1.id);
> > MLX5_SET(virtio_q, vq_ctx, umem_1_size, mvq->umem1.size);
> > MLX5_SET(virtio_q, vq_ctx, umem_2_id, mvq->umem2.id);
> > @@ -2633,7 +2633,7 @@ static void restore_channels_info(struct mlx5_vdpa_net
> > *ndev)
> > }
> >
> > static int mlx5_vdpa_change_map(struct mlx5_vdpa_dev *mvdev,
> > - struct vhost_iotlb *iotlb, unsigned int asid)
> > + struct mlx5_vdpa_mr *new_mr, unsigned int
> > asid)
> > {
> > struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > int err;
> > @@ -2641,27 +2641,18 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_dev
> > *mvdev,
> > suspend_vqs(ndev);
> > err = save_channels_info(ndev);
> > if (err)
> > - goto err_mr;
> > + return err;
> >
> > teardown_driver(ndev);
> > - mlx5_vdpa_destroy_mr(mvdev, &mvdev->mr);
> > - err = mlx5_vdpa_create_mr(mvdev, &mvdev->mr, iotlb);
> > - if (err)
> > - goto err_mr;
> > +
> > + mlx5_vdpa_update_mr(mvdev, new_mr, asid);
> >
> > if (!(mvdev->status & VIRTIO_CONFIG_S_DRIVER_OK) || mvdev->suspended)
> > - goto err_mr;
> > + return 0;
> >
> > restore_channels_info(ndev);
> > err = setup_driver(mvdev);
> > - if (err)
> > - goto err_setup;
> > -
> > - return 0;
> >
> > -err_setup:
> > - mlx5_vdpa_destroy_mr(mvdev, &mvdev->mr);
> > -err_mr:
> > return err;
> > }
> >
> > @@ -2875,26 +2866,40 @@ static u32 mlx5_vdpa_get_generation(struct vdpa_device
> > *vdev)
> > static int set_map_data(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
> > *iotlb,
> > unsigned int asid)
> > {
> > - bool change_map;
> > + struct mlx5_vdpa_mr *new_mr;
> > int err;
> >
> > if (mvdev->group2asid[MLX5_VDPA_DATAVQ_GROUP] != asid)
> > goto end;
> >
> > - err = mlx5_vdpa_handle_set_map(mvdev, iotlb, &change_map, asid);
> > - if (err) {
> > - mlx5_vdpa_warn(mvdev, "set map failed(%d)\n", err);
> > - return err;
> > + if (vhost_iotlb_itree_first(iotlb, 0, U64_MAX)) {
> > + new_mr = mlx5_vdpa_create_mr(mvdev, iotlb);
> > + if (IS_ERR(new_mr)) {
> > + err = PTR_ERR(new_mr);
> > + mlx5_vdpa_warn(mvdev, "create map failed(%d)\n", err);
> > + return err;
> > + }
> > + } else {
> > + /* Empty iotlbs don't have an mr but will clear the previous
> > mr. */
> > + new_mr = NULL;
> > }
> Hi Jason and/or Eugenio, could you have a quick look at this part of the patch
> that changed please?
>
> Thanks,
> Dragos
> >
> > - if (change_map) {
> > - err = mlx5_vdpa_change_map(mvdev, iotlb, asid);
> > - if (err)
> > - return err;
> > + if (!mvdev->mr) {
> > + mlx5_vdpa_update_mr(mvdev, new_mr, asid);
> > + } else {
> > + err = mlx5_vdpa_change_map(mvdev, new_mr, asid);
> > + if (err) {
> > + mlx5_vdpa_warn(mvdev, "change map failed(%d)\n", err);
> > + goto out_err;
> > + }
> > }
> >
> > end:
> > return mlx5_vdpa_update_cvq_iotlb(mvdev, iotlb, asid);
> > +
> > +out_err:
> > + mlx5_vdpa_destroy_mr(mvdev, new_mr);
Is it possible to reach this mlx5_vdpa_destroy_mr call with new_mr ==
NULL? Like:
* iotlb does not have any entries
* mdev already has a mr
* mlx5_vdpa_change_map fails
If I'm not wrong, mlx5_vdpa_destroy_mr may dereference new_mr through
_mlx5_vdpa_destroy_mr -> vhost_iotlb_free(mr->iotlb).
Am I missing something?
Thanks!
> > + return err;
> > }
> >
> > static int mlx5_vdpa_set_map(struct vdpa_device *vdev, unsigned int asid,
>
next prev parent reply other threads:[~2023-10-20 16:02 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-18 17:14 [PATCH vhost v4 00/16] vdpa: Add support for vq descriptor mappings Dragos Tatulea
2023-10-18 17:14 ` [PATCH vhost v4 01/16] vdpa/mlx5: Expose descriptor group mkey hw capability Dragos Tatulea
2023-10-18 17:14 ` [PATCH vhost v4 02/16] vdpa: introduce dedicated descriptor group for virtqueue Dragos Tatulea
2023-10-18 17:14 ` [PATCH vhost v4 03/16] vhost-vdpa: introduce descriptor group backend feature Dragos Tatulea
2023-10-18 17:14 ` [PATCH vhost v4 04/16] vhost-vdpa: uAPI to get dedicated descriptor group id Dragos Tatulea
2023-10-18 17:14 ` [PATCH vhost v4 05/16] vdpa/mlx5: Create helper function for dma mappings Dragos Tatulea
2023-10-18 17:14 ` [PATCH vhost v4 06/16] vdpa/mlx5: Decouple cvq iotlb handling from hw mapping code Dragos Tatulea
2023-10-18 17:14 ` [PATCH vhost v4 07/16] vdpa/mlx5: Take cvq iotlb lock during refresh Dragos Tatulea
2023-10-18 17:14 ` [PATCH vhost v4 08/16] vdpa/mlx5: Collapse "dvq" mr add/delete functions Dragos Tatulea
2023-10-18 17:14 ` [PATCH vhost v4 09/16] vdpa/mlx5: Rename mr destroy functions Dragos Tatulea
2023-10-18 17:14 ` [PATCH vhost v4 10/16] vdpa/mlx5: Allow creation/deletion of any given mr struct Dragos Tatulea
2023-10-18 17:14 ` [PATCH vhost v4 11/16] vdpa/mlx5: Move mr mutex out of " Dragos Tatulea
2023-10-18 17:14 ` [PATCH vhost v4 12/16] vdpa/mlx5: Improve mr update flow Dragos Tatulea
2023-10-18 17:21 ` Dragos Tatulea
2023-10-20 16:01 ` Eugenio Perez Martin [this message]
2023-10-23 8:07 ` Dragos Tatulea
2023-10-24 14:50 ` Eugenio Perez Martin
2023-10-18 17:14 ` [PATCH vhost v4 13/16] vdpa/mlx5: Introduce mr for vq descriptor Dragos Tatulea
2023-10-18 17:14 ` [PATCH vhost v4 14/16] vdpa/mlx5: Enable hw support for vq descriptor mapping Dragos Tatulea
2023-10-18 17:14 ` [PATCH vhost v4 15/16] vdpa/mlx5: Make iotlb helper functions more generic Dragos Tatulea
2023-10-18 17:14 ` [PATCH vhost v4 16/16] vdpa/mlx5: Update cvq iotlb mapping on ASID change Dragos Tatulea
2023-10-18 17:24 ` [PATCH vhost v4 00/16] vdpa: Add support for vq descriptor mappings Michael S. Tsirkin
2023-10-19 23:47 ` Si-Wei Liu
2023-10-24 6:45 ` Lei Yang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAJaqyWfuVV57syaUWkh7-QJ_rKHCPpKbHLv7LdN6j_KeGez2VQ@mail.gmail.com \
--to=eperezma@redhat.com \
--cc=dtatulea@nvidia.com \
--cc=jasowang@redhat.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mst@redhat.com \
--cc=parav@nvidia.com \
--cc=si-wei.liu@oracle.com \
--cc=virtualization@lists.linux-foundation.org \
--cc=xuanzhuo@linux.alibaba.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).