Re: [PATCH RFC v2 0/4] virtio-net: add tx-hash, rx-tstamp, tx-tstamp and tx-time

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

On Fri, May 14, 2021 at 3:12 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Fri, May 14, 2021 at 6:50 AM Willem de Bruijn <willemb@google.com> wrote:
> >
> > On Mon, Feb 8, 2021 at 1:56 PM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > From: Willem de Bruijn <willemb@google.com>
> > >
> > > RFCv2 for four new features to the virtio network device:
> > >
> > > 1. pass tx flow state to host, for routing + telemetry
> > > 2. pass rx tstamp to guest, for better RTT estimation
> > > 3. pass tx tstamp to guest, idem
> > > 3. pass tx delivery time to host, for accurate pacing
> > >
> > > All would introduce an extension to the virtio spec.
> > > Concurrently with code review I will write ballots to
> > > https://www.oasis-open.org/committees/ballots.php?wg_abbrev=virtio
> > >
> > > These changes are to the driver side. Evaluation additionally requires
> > > achanges to qemu and at least one back-end. I implemented preliminary
> > > support in Linux vhost-net. Both patches available through github at
> > >
> > > https://github.com/wdebruij/linux/tree/virtio-net-txhash-2
> > > https://github.com/wdebruij/qemu/tree/virtio-net-txhash-2
> > >
> > > Changes RFC -> RFCv2
> > >   - add transmit timestamp patch
> > >   - see individual patches for other changes
> > >
> > > Willem de Bruijn (4):
> > >   virtio-net: support transmit hash report
> > >   virtio-net: support receive timestamp
> > >   virtio-net: support transmit timestamp
> > >   virtio-net: support future packet transmit time
> >
> > Seeing Yuri's patchset adding new features reminded me that I did not
> > follow-up on this patch series on the list.
> >
> > The patches themselves are mostly in good shape. The last tx tstamp
> > issue can be resolved.
> >
> > But the device implementation I target only supports legacy mode.
> > Below conversation that we had in one of the patches makes clear that
> > supporting this in legacy is not feasible. Nor is upgrading that
> > device in the short term. Until there is a device implementation that
> > implements these offloads, these features are a dead letter. Not moving
> > forward for now.
> >
> > Somewhat related: is there a plan for when we run out of 64 feature bits?
>
> A quick thought: we need add (or reserve) a new feature bit to
> indicate that we need more bits, and have transport specific
> implementation of those extra bits negotiation. E.g for PCI, we can
> introduce new fields in the capability.

Thanks Jason. Yes, that makes sense to me.

The difference from 32 to 64 bit between virtio_pci_legacy.c and
virtio_pci_modern.c is a good example:

  static u64 vp_get_features(struct virtio_device *vdev)
  {
        struct virtio_pci_device *vp_dev = to_vp_device(vdev);

        /* When someone needs more than 32 feature bits, we'll need to
         * steal a bit to indicate that the rest are somewhere else. */
        return ioread32(vp_dev->ioaddr + VIRTIO_PCI_HOST_FEATURES);
 }

  u64 vp_modern_get_features(struct virtio_pci_modern_device *mdev)
  {
        struct virtio_pci_common_cfg __iomem *cfg = mdev->common;

        u64 features;

        vp_iowrite32(0, &cfg->device_feature_select);
        features = vp_ioread32(&cfg->device_feature);
        vp_iowrite32(1, &cfg->device_feature_select);
        features |= ((u64)vp_ioread32(&cfg->device_feature) << 32);

        return features;
  }

device_feature_select is a 32-bit field, of which only values 0 and 1
are defined so far, per the virtio 1.1 spec:

"
device_feature_select
The driver uses this to select which feature bits device_feature
shows. Value 0x0 selects Feature Bits 0 to 31, 0x1 selects Feature
Bits 32 to 63, etc.
"

That leaves plenty of room for expansion, at least for pci devices.

>
> >
> > > > > Actually, would it be possible to make new features available on
> > > > > legacy devices? There is nothing in the features bits precluding it.
> > > >
> > > > I think it won't be possible: you are using feature bit 55,
> > > > legacy devices have up to 32 feature bits. And of course the
> > > > header looks a bit differently for legacy, you would have to add special
> > > > code to handle that when mergeable buffers are off.
> > >
> > > I think I can make the latter work. I did start without a dependency
> > > on the v1 header initially.
> > >
> > > Feature bit array length I had not considered. Good point. Need to
> > > think about that. It would be very appealing if in particular the
> > > tx-hash feature could work in legacy mode.
> >
>