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

On Fri, May 14, 2021 at 3:12 AM Jason Wang [off-list ref] wrote:

On Fri, May 14, 2021 at 6:50 AM Willem de Bruijn [off-list ref] wrote:

quoted

On Mon, Feb 8, 2021 at 1:56 PM Willem de Bruijn
[off-list ref] wrote:

quoted

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.

quoted

quoted

quoted

quoted

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.