Re: [RFC PATCH v2 4/5] virtio-net: Added eBPF RSS to virtio-net.

[Jason Wang <jasowang@redhat.com>]

On 2020/12/2 下午3:16, Yuri Benditovich wrote:
>
>
> On Wed, Dec 2, 2020 at 6:06 AM Jason Wang <jasowang@redhat.com 
> <mailto:jasowang@redhat.com>> wrote:
>
>
>     On 2020/12/1 下午3:40, Yuri Benditovich wrote:
>     >
>     >
>     > On Tue, Nov 24, 2020 at 10:49 AM Jason Wang <jasowang@redhat.com
>     <mailto:jasowang@redhat.com>
>     > <mailto:jasowang@redhat.com <mailto:jasowang@redhat.com>>> wrote:
>     >
>     >
>     >     On 2020/11/19 下午7:13, Andrew Melnychenko wrote:
>     >     > From: Andrew <andrew@daynix.com <mailto:andrew@daynix.com>
>     <mailto:andrew@daynix.com <mailto:andrew@daynix.com>>>
>     >     >
>     >     > When RSS is enabled the device tries to load the eBPF program
>     >     > to select RX virtqueue in the TUN. If eBPF can be loaded
>     >     > the RSS will function also with vhost (works with kernel
>     5.8 and
>     >     later).
>     >     > Software RSS is used as a fallback with vhost=off when eBPF
>     >     can't be loaded
>     >     > or when hash population requested by the guest.
>     >     >
>     >     > Signed-off-by: Yuri Benditovich
>     <yuri.benditovich@daynix.com <mailto:yuri.benditovich@daynix.com>
>     >     <mailto:yuri.benditovich@daynix.com
>     <mailto:yuri.benditovich@daynix.com>>>
>     >     > Signed-off-by: Andrew Melnychenko <andrew@daynix.com
>     <mailto:andrew@daynix.com>
>     >     <mailto:andrew@daynix.com <mailto:andrew@daynix.com>>>
>     >     > ---
>     >     >   hw/net/vhost_net.c             |   2 +
>     >     >   hw/net/virtio-net.c            | 120
>     >     +++++++++++++++++++++++++++++++--
>     >     >   include/hw/virtio/virtio-net.h |   4 ++
>     >     >   net/vhost-vdpa.c               |   2 +
>     >     >   4 files changed, 124 insertions(+), 4 deletions(-)
>     >     >
>     >     > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>     >     > index 24d555e764..16124f99c3 100644
>     >     > --- a/hw/net/vhost_net.c
>     >     > +++ b/hw/net/vhost_net.c
>     >     > @@ -71,6 +71,8 @@ static const int user_feature_bits[] = {
>     >     >       VIRTIO_NET_F_MTU,
>     >     >       VIRTIO_F_IOMMU_PLATFORM,
>     >     >       VIRTIO_F_RING_PACKED,
>     >     > +    VIRTIO_NET_F_RSS,
>     >     > +    VIRTIO_NET_F_HASH_REPORT,
>     >     >
>     >     >       /* This bit implies RARP isn't sent by QEMU out of
>     band */
>     >     >       VIRTIO_NET_F_GUEST_ANNOUNCE,
>     >     > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>     >     > index 277289d56e..afcc3032ec 100644
>     >     > --- a/hw/net/virtio-net.c
>     >     > +++ b/hw/net/virtio-net.c
>     >     > @@ -698,6 +698,19 @@ static void
>     virtio_net_set_queues(VirtIONet *n)
>     >     >
>     >     >   static void virtio_net_set_multiqueue(VirtIONet *n, int
>     >     multiqueue);
>     >     >
>     >     > +static uint64_t fix_ebpf_vhost_features(uint64_t features)
>     >     > +{
>     >     > +    /* If vhost=on & CONFIG_EBPF doesn't set - disable RSS
>     >     feature */
>     >     > +    uint64_t ret = features;
>     >     > +#ifndef CONFIG_EBPF
>     >     > +    virtio_clear_feature(&ret, VIRTIO_NET_F_RSS);
>     >     > +#endif
>     >     > +    /* for now, there is no solution for populating the hash
>     >     from eBPF */
>     >     > +    virtio_clear_feature(&ret, VIRTIO_NET_F_HASH_REPORT);
>     >
>     >
>     >     I think there's still some misunderstanding here.
>     >
>     >     When "rss" is enabled via command line, qemu can't not turn
>     it off
>     >     silently, otherwise it may break migration. Instead, qemu should
>     >     disable
>     >     vhost-net if eBPF can't be loaded.
>     >
>     >     When "hash_report" is enabled via command line, qemu should
>     disable
>     >     vhost-net unconditionally.
>     >
>     >
>     > I agree in general with this requirement and I'm preparing an
>     > implementation of such fallback.
>     >
>     > The problem is that qemu already uses the mechanism of turning off
>     > host features
>     > silently if they are not supported by the current vhost in kernel:
>     >
>     https://github.com/qemu/qemu/blob/b0f8c22d6d4d07f3bd2307bcc62e1660ef965472/hw/virtio/vhost.c#L1526
>     >
>     > Can you please comment on it and let me know how it should be
>     modified
>     > in future?
>     > I've planned to use it in next work (implementing hash report in
>     kernel)
>
>
>     This looks like a bug that needs to be solved. Otherwise we break
>     migration from rss=on, vhost=off to rss=on,vhost=on.
>
> I think I need to fill the gap in my understanding of migration's 
> prerequisites.
> According to 
> https://github.com/qemu/qemu/blob/b0f8c22d6d4d07f3bd2307bcc62e1660ef965472/docs/devel/migration.rst
> "... QEMU has to be launched with the same arguments the two times 
> ..." and we test the migration during development
> according to this statement.


Yes, that's the overall requirement. And it shows the issue of disabling 
feature silently. If we had src whose vhost support feature A and dst 
vhost doesn't support. Even if we launch the QEMU with the same 
arguments, it can still fail.

So actually two issues:

1) whether or not to disable features silently

2) whether or not to support migration between vhost=on to vhost=off

For 1), I think we'd better don't do that, and if we can do 1), it would 
be possible to achieve 2).


> What are the real requirements and prerequisites of the migration?


For virtio, I think we allow some kind of extra flexibility. We try to 
make migration work between vhost=off and vhost=on. What we need is just 
to keep everything that visible to guest after migration the same as 
before migration.  Obviously, device features is one of the thing.

Thanks



>
>     I think you can keep the current code as is and I will try to seek
>     a way
>     to solve the issue.
>
>     Thanks
>