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

[Michael S. Tsirkin]

On Sun, Mar 08, 2020 at 02:44:59PM +0200, Yuri Benditovich wrote:
> On Sun, Mar 8, 2020 at 2:17 PM Michael S. Tsirkin <address@hidden> 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 <address@hidden> 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 <address@hidden> 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.

IIRC with vhost features not listed are assumed to be
implemented by qemu and not to need backend support.

Maybe we should change that to make things more robust
in the future ... Jason, Marc-André am I right? what do you think?



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