Re: [Patch v1 3/3] vdpa/mlx5: Add multiqueue support

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Aug 16, 2021 at 07:56:59PM +0300, Eli Cohen wrote:
> On Mon, Aug 16, 2021 at 01:58:06PM +0800, Jason Wang wrote:
> > 
> > 在 2021/8/16 下午1:47, Eli Cohen 写道:
> > > On Mon, Aug 16, 2021 at 12:16:14PM +0800, Jason Wang wrote:
> > > > 在 2021/8/12 下午5:50, Eli Cohen 写道:
> > > > > On Thu, Aug 12, 2021 at 03:04:35PM +0800, Jason Wang wrote:
> > > > > > 在 2021/8/12 下午3:01, Eli Cohen 写道:
> > > > > > > On Thu, Aug 12, 2021 at 02:47:06PM +0800, Jason Wang wrote:
> > > > > > > > On Thu, Aug 12, 2021 at 12:55 PM Eli Cohen <elic@nvidia.com> wrote:
> > > > > > > > > On Thu, Aug 12, 2021 at 11:19:19AM +0800, Jason Wang wrote:
> > > > > > > > > > 在 2021/8/11 下午7:04, Eli Cohen 写道:
> > > > > > > > > > > On Wed, Aug 11, 2021 at 04:37:44PM +0800, Jason Wang wrote:
> > > > > > > > > > > > 在 2021/8/11 下午3:53, Eli Cohen 写道:
> > > > > > > > > > > > > > One thing need to solve for mq is that the:
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > +static u16 ctrl_vq_idx(struct  mlx5_vdpa_dev *mvdev)
> > > > > > > > > > > > > > > +{
> > > > > > > > > > > > > > > +     return 2 *  mlx5_vdpa_max_qps(mvdev->max_vqs);
> > > > > > > > > > > > > > > +}
> > > > > > > > > > > > > > We should handle the case when MQ is supported by the device but not the
> > > > > > > > > > > > > > driver.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > E.g in the case when we have 2 queue pairs:
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > When MQ is enabled, cvq is queue 4
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > When MQ is not enabled, cvq is queue 2
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > There's some issue with this. I get callbacks targeting specific
> > > > > > > > > > > > > virtqueues before features negotiation has been completed.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Specifically, I get set_vq_cb() calls. At this point I must know the
> > > > > > > > > > > > > control vq index.
> > > > > > > > > > > > So I think we need do both:
> > > > > > > > > > > > 
> > > > > > > > > > > > 1) At one hand, it's a bug for the userspace to use vq_index before feature
> > > > > > > > > > > > is negotiated
> > > > > > > > > > > > 
> > > > > > > > > > > > (looks like a bug in my cvq series that will call SET_VRING_CALL before
> > > > > > > > > > > > feature is negotiate, which I will look).
> > > > > > > > > > > > 
> > > > > > > > > > > > 2) At the other hand, the driver should be able to deal with that
> > > > > > > > > > > > 
> > > > > > > > > > > All I can do is drop callbacks for VQs before features negotation has
> > > > > > > > > > > been completed.
> > > > > > > > > > Or just leave queue index 0, 1 work.
> > > > > > > > > > 
> > > > > > > > > > Since it is not expected to be changed.
> > > > > > > > > > 
> > > > > > > > > Right, will do.
> > > > > > > > > 
> > > > > > > > > > > > > I think the CVQ index must not depend on the negotiated features but
> > > > > > > > > > > > > rather depend of the value the device driver provides in the call to
> > > > > > > > > > > > > _vdpa_register_device().
> > > > > > > > > > > > At the virtio level, it's too late to change that and it breaks the backward
> > > > > > > > > > > > compatibility.
> > > > > > > > > > > > 
> > > > > > > > > > > > But at the vDPA level, the under layer device can map virtio cvq to any of
> > > > > > > > > > > > it's virtqueue.
> > > > > > > > > > > > 
> > > > > > > > > > > > E.g map cvq (index 2) to mlx5 cvq (the last).
> > > > > > > > > > > > 
> > > > > > > > > > > I am not following you here. I still don't know what index is cvq.
> > > > > > > > > > Right, we still need to wait for the feature being negotiated in order to
> > > > > > > > > > proceed.
> > > > > > > > > > 
> > > > > > > > > So to summarise, before feature negotiation complete, I accept calls
> > > > > > > > > referring to VQs only for indices 0 and 1.
> > > > > > > > > After feature negotiation complete I know CVQ index and will accept
> > > > > > > > > indices 0 to cvq index.
> > > > > > > > I don't get this "accept indices 0 to cvq index".
> > > > > > > What I meant to say is that there are several callbacks that refer to
> > > > > > > specific virtqueues, e.g. set_vq_address(), set_vq_num() etc. They all
> > > > > > > accept virtqueue index as an argument.
> > > > > > > 
> > > > > > > What we want to do is verify wheather the index provided is valid or
> > > > > > > not. If it is not valid, either return error (if the callback can return
> > > > > > > a value) or just avoid processing it. If the index is valid then we
> > > > > > > process it normally.
> > > > > > > 
> > > > > > > Now we need to decide which index is valid or not. We need something
> > > > > > > like this to identifiy valid indexes range:
> > > > > > > 
> > > > > > > CVQ clear: 0 and 1
> > > > > > > CVQ set, MQ clear: 0, 1 and 2 (for CVQ).
> > > > > > > CVQ set, MQ set: 0..nvq where nvq is whatever provided to
> > > > > > > _vdpa_register_device()
> > > > > > Yes.
> > > > > > 
> > > > > Unfortunately it does not work.
> > > > > set_vq_cb() for all the multiqueues is called beofre feature
> > > > > negotiation. If I apply the above logic, I will lose these settings.
> > > > > 
> > > > > I can make an exception for set_vq_cb(), save callbacks and restore
> > > > > them afterwards. This looks too convoluted and maybe we should seek
> > > > > another solution.
> > > > 
> > > > I agree.
> > > > 
> > > > 
> > > > > Let me know what you think.
> > > > 
> > > > Rethink about this issue. It looks to the only issue we face is the
> > > > set_vq_cb().
> > > > 
> > > > With the assumption that the userspace can use the index correctly (even
> > > > before set_features). I wonder the following works.
> > > > 
> > > > Instead of checking whether the index is cvq in set_vq_cb() how about:
> > > > 
> > > > 1) decouple event_cb out of mlx5_vdpa_virtqueue and mlx5_congro_vq
> > > > 2) have a dedicated event_cb array in mlx5_vdpa_net
> > > > 3) then we can do
> > > > 
> > > > ndev->event_cbs[index] = *cb;
> > > > 
> > > So actually you're suggesting to save all the callabck configurations in
> > > an array and evaluate cvq index after feature negotiation has been
> > > completed.
> > 
> > 
> > Yes.
> > 
> > 
> > > 
> > > I think that could work. I will code this and update.
> > 
> 
> It works fine when I am working with your version of qemu with support
> for multi queue.
> 
> The problem is that it is broken on qemu v6.0.0. If I register my vdpa
> device with more than 2 data virtqueues, qemu won't even create a
> netdevice in the VM.
> 
> I am not sure how to handle this. Is there some kind of indication I can
> get as to the version of qemu so I can avoid using multiqueue for
> versions I know are problematic?

No versions ;) This is what feature bits are for ...

> > 
> > Thanks.
> > 
> > 
> > > 
> > > > in mlx5_vdpa_set_vq_cb()
> > > > 
> > > > 4) in the mlx5_cvq_kick_handler(), we know the feature is negotiated and we
> > > > can use the correct index there.
> > > > 
> > > > In the mean time, I will look at Qemu code to see if we can guarantee that
> > > > set_features is called before set_vq_callback. (At first glance, it's not
> > > > trivial but let's see).
> > > > 
> > > > Thanks
> > > > 
> > > > 
> > > > > > > And while writing this, I think this logic does not belog in mlx5_vdpa
> > > > > > > but probably in vdpa.c
> > > > > > The problem is that vdpa should be unaware of a specific device type. E.g
> > > > > > the above indices may work only for virtio-net but not other.
> > > > > > 
> > > > > > Thanks
> > > > > > 
> > > > > > 
> > > > > > > 	
> > > > > > > > Thanks
> > > > > > > > 
> > > > > > > > > > Thanks
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > > > Thanks
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > 

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