Hi Maxime,
> -----Original Message-----
> From: Maxime Coquelin <maxime.coque...@redhat.com>
> Sent: Wednesday, January 26, 2022 10:03 PM
> To: Wang, YuanX <yuanx.w...@intel.com>; Xia, Chenbo
> <chenbo....@intel.com>
> Cc: dev@dpdk.org; Hu, Jiayu <jiayu...@intel.com>; Ding, Xuan
> <xuan.d...@intel.com>; Ma, WenwuX <wenwux...@intel.com>; Ling,
> WeiX <weix.l...@intel.com>
> Subject: Re: [PATCH] vhost: fix data-plane access to released vq
>
> Hi Yuan,
>
> On 12/3/21 17:34, Yuan Wang wrote:
> > From: yuan wang <yuanx.w...@intel.com>
> >
> > When numa reallocation occurs, numa_realoc() on the control plane will
> > free the old vq. If rte_vhost_dequeue_burst() on the data plane get
> > the vq just before release, then it will access the released vq. We
> > need to put the
> > vq->access_lock into struct virtio_net to ensure that it
> > can prevents this situation.
>
>
> This patch is a fix, so the Fixes tag would be needed.
>
> But are you really facing this issue, or this is just based on code review?
This issue is run-time checked with AddressSanitizer which can be turned on by:
meson configure -Db_sanitize=address <build_dir>
>
> Currently NUMA reallocation is called whenever
> translate_ring_addresses() is called.
>
> translate_ring_addresses() is primarly called at device initialization, before
> the .new_device() callback is called. At that stage, there is no risk to
> performa NUMA reallocation as the application is not expected to use APIs
> requiring vq->access_lock acquisition.
>
> But I agree there are possibilities that numa_realloc() gets called while
> device
> is in running state. But even if that happened, I don't think it is possible
> that
> numa_realloc() ends-up reallocating the virtqueue on a different NUMA
> node (the vring should not have moved from a physical memory standpoint).
> And if even it happened, we should be safe because we ensure the VQ was
> not ready (so not usable by the
> application) before proceeding with reallocation:
Here is a scenario where VQ ready has not been set:
1. run the testpmd and then start the data plane process.
2. run the front-end.
3. new_device() gets called when the first two queues are ready, even if the
later queues are not.
4. when processing messages from the later queues, it may go to numa_realloc(),
the ready flag has not been set and therefore can be reallocated.
If all the queues are ready before call new_deivce(), this issue does not occur.
I think maybe it is another solution.
Thanks,
Yuan
>
> static struct virtio_net*
> numa_realloc(struct virtio_net *dev, int index) {
> int node, dev_node;
> struct virtio_net *old_dev;
> struct vhost_virtqueue *vq;
> struct batch_copy_elem *bce;
> struct guest_page *gp;
> struct rte_vhost_memory *mem;
> size_t mem_size;
> int ret;
>
> old_dev = dev;
> vq = dev->virtqueue[index];
>
> /*
> * If VQ is ready, it is too late to reallocate, it certainly already
> * happened anyway on VHOST_USER_SET_VRING_ADRR.
> */
> if (vq->ready)
> return dev;
>
> So, if this is fixing a real issue, I would need more details on the issue in
> order
> to understand why vq->ready was not set when it should have been.
>
> On a side note, while trying to understand how you could face an issue, I
> noticed that translate_ring_addresses() may be called by
> vhost_user_iotlb_msg(). In that case, vq->access_lock is not held as this is
> the handler for VHOST_USER_IOTLB_MSG. We may want to protect
> translate_ring_addresses() calls with locking the VQ locks. I will post a fix
> for
> it.
>
> > Signed-off-by: Yuan Wang <yuanx.w...@intel.com>
> > ---
> > lib/vhost/vhost.c | 26 +++++++++++++-------------
> > lib/vhost/vhost.h | 4 +---
> > lib/vhost/vhost_user.c | 4 ++--
> > lib/vhost/virtio_net.c | 16 ++++++++--------
> > 4 files changed, 24 insertions(+), 26 deletions(-)
> >
>
> ...
>
> > diff --git a/lib/vhost/vhost.h b/lib/vhost/vhost.h index
> > 7085e0885c..f85ce4fda5 100644
> > --- a/lib/vhost/vhost.h
> > +++ b/lib/vhost/vhost.h
> > @@ -185,9 +185,6 @@ struct vhost_virtqueue {
> > bool access_ok;
> > bool ready;
> >
> > - rte_spinlock_t access_lock;
> > -
> > -
> > union {
> > struct vring_used_elem *shadow_used_split;
> > struct vring_used_elem_packed *shadow_used_packed;
> @@ -384,6
> > +381,7 @@ struct virtio_net {
> > int extbuf;
> > int linearbuf;
> > struct vhost_virtqueue *virtqueue[VHOST_MAX_QUEUE_PAIRS * 2];
> > + rte_spinlock_t vq_access_lock[VHOST_MAX_QUEUE_PAIRS
> * 2];
>
> The problem here is that you'll be introducing false sharing, so I expect
> performance to no more scale with the number of queues.
>
> It also consumes unnecessary memory.
>
> > struct inflight_mem_info *inflight_info;
> > #define IF_NAME_SZ (PATH_MAX > IFNAMSIZ ? PATH_MAX : IFNAMSIZ)
> > char ifname[IF_NAME_SZ];
>
> Thanks,
> Maxime
