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

(off-list ancestor, not in this archive)

On Tue, Aug 17, 2021 at 1:26 PM Eli Cohen [off-list ref] wrote:
On Tue, Aug 17, 2021 at 12:19:55PM +0800, Jason Wang wrote:
在 2021/8/17 下午12:03, Parav Pandit 写道:
From: Jason Wang <redacted>
Sent: Tuesday, August 17, 2021 9:26 AM

On Tue, Aug 17, 2021 at 3:37 AM Michael S. Tsirkin [off-list ref]
wrote:
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
[off-list ref] 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.
Qemu should hide MQ feature in this case but looks like it doesn't.

Will have a look.

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 ...
Yes.

So does it work if "mq=off" is specified in the command line?
We should not add driver module parameter.

Note that, it's not a module parameter but a qemu command line to disable mq
feature.

We anyway need number of VQs to be driven by the number of vcpus used by the VM.
So why not specify this when creating a vdpa device?

Yes, I think it should work as well.

So management need either:

1) disable multiqueue via "mq=off"

or

2) using netlink API to create a single queue pair device

for the qemu <=6.1.
Which management entity are you referring to here?
The one that launches Qemu. (E.g libvirt or other).

Thanks

Thanks

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help