Re: [PATCH 0/3] reference implementation of RSS

[Yuri Benditovich <yuri.benditovich@daynix.com>]

On Sun, Mar 8, 2020 at 2:17 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Sun, Mar 08, 2020 at 11:56:38AM +0200, Yuri Benditovich wrote:
> > On Sun, Mar 8, 2020 at 10:06 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Fri, Mar 06, 2020 at 11:50:30AM +0200, Yuri Benditovich wrote:
> > > >
> > > >
> > > > On Fri, Mar 6, 2020 at 11:27 AM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > >
> > > >     On 2020/2/27 上午1:48, Yuri Benditovich wrote:
> > > >     > Support for VIRTIO_NET_F_RSS feature in QEMU for reference
> > > >     > purpose. Implements Toeplitz hash calculation for incoming
> > > >     > packets according to configuration provided by driver.
> > > >     >
> > > >     > This series requires previously submitted and accepted
> > > >     > patch to be applied:
> > > >     > https://lists.gnu.org/archive/html/qemu-devel/2020-01/msg06448.html
> > > >     >
> > > >     > Yuri Benditovich (3):
> > > >     >    virtio-net: introduce RSS RX steering feature
> > > >     >    virtio-net: implement RSS configuration command
> > > >     >    virtio-net: implement RX RSS processing
> > > >     >
> > > >     >   hw/net/trace-events                         |   3 +
> > > >     >   hw/net/virtio-net.c                         | 234
> > > >     +++++++++++++++++++-VIRTIO_NET_F_RSS
> > > >     >   include/hw/virtio/virtio-net.h              |  12 +
> > > >     >   include/standard-headers/linux/virtio_net.h |  37 +++-
> > > >     >   4 files changed, 273 insertions(+), 13 deletions(-)
> > > >     >
> > > >
> > > >     One question before the reviewing.
> > > >
> > > >     Do we need to deal with migration (which I think yes)?
> > > >
> > > >
> > > > I think we don't (yet) as this is a reference implementation and the main goal
> > > > is to provide the functional reference for hardware solution.
> > >
> > >
> > > That's fine, but then we must block migration, and add appropriate code
> > > comments. Just silently losing data isn't a good idea.
> >
> > Note that there is no data lost, just the configuration of RSS is not migrating.
> > So, qemu just will not redirect the data to different queues after migration.
>
> Right. Unlike auto-mq, the spec doesn't say the direction is best effort though,
> so that would be a spec violation.
>
> > I would add the migration prevention in the next series with
> > implementation of hash delivery to prevent different packet sizes in
> > driver and qemu.
>
> And hopefully full migration support will follow.
>
> One other thing to check is vhost - I didn't check
> what happens with this patchset but
> I think at a minimum we need to fail vhost init,
> until the backend implements the feature biit.

RSS feature currently is not indicated in case vhost is on.
The same will be with hash report.

>
>
> > >
> > > > I agree with the general direction that for complete support of RSS and hash
> > > > delivery the best way is to do that in kernel using bpf.
> > > > For that, IMO, the bpf should be a part of the kernel (it uses skb fields) and
> > > > the tap should receive just RSS parameters for it.
> > > > At this stage we definitely will need to add migration support and propagation
> > > > of RSS parameters.
> > > >
> > > >
> > > >
> > > >     Thanks
> > > >
> > > >
> > >
>