Re: [PATCH RFC v2 2/4] virtio-net: support receive timestamp

[Jason Wang <jasowang@redhat.com>]

On 2021/2/9 下午9:53, Willem de Bruijn wrote:
> On Mon, Feb 8, 2021 at 11:13 PM Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2021/2/9 上午2:55, Willem de Bruijn wrote:
>>> From: Willem de Bruijn <willemb@google.com>
>>>
>>> Add optional PTP hardware rx timestamp offload for virtio-net.
>>>
>>> Accurate RTT measurement requires timestamps close to the wire.
>>> Introduce virtio feature VIRTIO_NET_F_RX_TSTAMP. If negotiated, the
>>> virtio-net header is expanded with room for a timestamp.
>>>
>>> A device may pass receive timestamps for all or some packets. Flag
>>> VIRTIO_NET_HDR_F_TSTAMP signals whether a timestamp is recorded.
>>>
>>> A driver that supports hardware timestamping must also support
>>> ioctl SIOCSHWTSTAMP. Implement that, as well as information getters
>>> ioctl SIOCGHWTSTAMP and ethtool get_ts_info (`ethtool -T $DEV`).
>>>
>>> The timestamp straddles (virtual) hardware domains. Like PTP, use
>>> international atomic time (CLOCK_TAI) as global clock base. The driver
>>> must sync with the device, e.g., through kvm-clock.
>>>
>>> Tested:
>>>     guest: ./timestamping eth0 \
>>>             SOF_TIMESTAMPING_RAW_HARDWARE \
>>>             SOF_TIMESTAMPING_RX_HARDWARE
>>>     host: nc -4 -u 192.168.1.1 319
>>>
>>> Changes RFC -> RFCv2
>>>     - rename virtio_net_hdr_v12 to virtio_net_hdr_hash_ts
>>>     - add ethtool .get_ts_info to query capabilities
>>>     - add ioctl SIOC[GS]HWTSTAMP to configure feature
>>>     - add vi->enable_rx_tstamp to store configuration
>>>     - convert virtioXX_to_cpu to leXX_to_cpu
>>>     - convert reserved to __u32
>>>
>>> Signed-off-by: Willem de Bruijn <willemb@google.com>
>>>    static const struct net_device_ops virtnet_netdev = {
>>>        .ndo_open            = virtnet_open,
>>>        .ndo_stop            = virtnet_close,
>>> @@ -2573,6 +2676,7 @@ static const struct net_device_ops virtnet_netdev = {
>>>        .ndo_features_check     = passthru_features_check,
>>>        .ndo_get_phys_port_name = virtnet_get_phys_port_name,
>>>        .ndo_set_features       = virtnet_set_features,
>>> +     .ndo_do_ioctl           = virtnet_ioctl,
>>>    };
>>>
>>>    static void virtnet_config_changed_work(struct work_struct *work)
>>> @@ -3069,6 +3173,11 @@ static int virtnet_probe(struct virtio_device *vdev)
>>>                vi->hdr_len = sizeof(struct virtio_net_hdr_v1_hash);
>>>        }
>>>
>>> +     if (virtio_has_feature(vdev, VIRTIO_NET_F_RX_TSTAMP)) {
>>> +             vi->has_rx_tstamp = true;
>>> +             vi->hdr_len = sizeof(struct virtio_net_hdr_hash_ts);
>>
>> Does this mean even if the device doesn't pass timestamp, the header
>> still contains the timestamp fields.
> Yes. As implemented, the size of the header is constant across
> packets. If both sides negotiate the feature, then all headers reserve
> space, whether or not the specific packet has a timestamp.
>
> So far headers are fixed size. I suppose we could investigate variable
> size headers. This goes back to our discussion in the previous
> patchset, that we can always add a packed-header feature later, if the
> number of optional features reaches a size that makes the complexity
> worthwhile.


Right, so for timstamp it's probably OK but we probably need to do as 
you said here if we want to add more in the header. Let's see how 
Michael think about this.


>
>>> +     }
>>> +
>>>        if (virtio_has_feature(vdev, VIRTIO_F_ANY_LAYOUT) ||
>>>            virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
>>>                vi->any_header_sg = true;
>>> @@ -3260,7 +3369,7 @@ static struct virtio_device_id id_table[] = {
>>>        VIRTIO_NET_F_CTRL_MAC_ADDR, \
>>>        VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
>>>        VIRTIO_NET_F_SPEED_DUPLEX, VIRTIO_NET_F_STANDBY, \
>>> -     VIRTIO_NET_F_TX_HASH
>>> +     VIRTIO_NET_F_TX_HASH, VIRTIO_NET_F_RX_TSTAMP
>>>
>>>    static unsigned int features[] = {
>>>        VIRTNET_FEATURES,
>>> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
>>> index 273d43c35f59..a5c84410cf92 100644
>>> --- a/include/uapi/linux/virtio_net.h
>>> +++ b/include/uapi/linux/virtio_net.h
>>> @@ -57,6 +57,7 @@
>>>                                         * Steering */
>>>    #define VIRTIO_NET_F_CTRL_MAC_ADDR 23       /* Set MAC address */
>>>
>>> +#define VIRTIO_NET_F_RX_TSTAMP         55    /* Device sends TAI receive time */
>>>    #define VIRTIO_NET_F_TX_HASH          56    /* Driver sends hash report */
>>>    #define VIRTIO_NET_F_HASH_REPORT  57        /* Supports hash report */
>>>    #define VIRTIO_NET_F_RSS      60    /* Supports RSS RX steering */
>>> @@ -126,6 +127,7 @@ struct virtio_net_hdr_v1 {
>>>    #define VIRTIO_NET_HDR_F_NEEDS_CSUM 1       /* Use csum_start, csum_offset */
>>>    #define VIRTIO_NET_HDR_F_DATA_VALID 2       /* Csum is valid */
>>>    #define VIRTIO_NET_HDR_F_RSC_INFO   4       /* rsc info in csum_ fields */
>>> +#define VIRTIO_NET_HDR_F_TSTAMP              8       /* timestamp is recorded */
>>>        __u8 flags;
>>>    #define VIRTIO_NET_HDR_GSO_NONE             0       /* Not a GSO frame */
>>>    #define VIRTIO_NET_HDR_GSO_TCPV4    1       /* GSO frame, IPv4 TCP (TSO) */
>>> @@ -181,6 +183,17 @@ struct virtio_net_hdr_v1_hash {
>>>        };
>>>    };
>>>
>>> +struct virtio_net_hdr_hash_ts {
>>> +     struct virtio_net_hdr_v1 hdr;
>>> +     struct {
>>> +             __le32 value;
>>> +             __le16 report;
>>> +             __le16 flow_state;
>>> +     } hash;
>>
>> Any reason for not embedding structure virtio_net_hdr_v1_hash?
> Just that it becomes an onion of struct inside structs. I can change
> if you prefer.


Yes please (unless Michael has other opinion).


>
>> Thanks
> As always, thanks for reviewing, Jason.
>

You're welcome :)

Thanks