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

On Fri, May 14, 2021 at 6:50 AM Willem de Bruijn <willemb at google.com>
wrote:
>
> On Mon, Feb 8, 2021 at 1:56 PM Willem de Bruijn
> <willemdebruijn.kernel at gmail.com> wrote:
> >
> > From: Willem de Bruijn <willemb at 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
>
> > > > 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.
>

On Fri, May 14, 2021 at 3:12 AM Jason Wang <jasowang at redhat.com>
wrote:
>
> On Fri, May 14, 2021 at 6:50 AM Willem de Bruijn <willemb at
google.com> wrote:
> >
> > On Mon, Feb 8, 2021 at 1:56 PM Willem de Bruijn
> > <willemdebruijn.kernel at gmail.com> wrote:
> > >
> > > From: Willem de Bruijn <willemb at 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.
> >
>