On 2025/06/03 12:19, Jason Wang wrote:

On Fri, May 30, 2025 at 12:50 PM Akihiko Odaki akihiko.odaki@daynix.com wrote:

...

They are useful to implement VIRTIO_NET_F_RSS and VIRTIO_NET_F_HASH_REPORT.

Signed-off-by: Akihiko Odaki akihiko.odaki@daynix.com Tested-by: Lei Yang leiyang@redhat.com

---

include/linux/virtio_net.h | 188 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 188 insertions(+)

diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h index 02a9f4dc594d..426f33b4b824 100644 --- a/include/linux/virtio_net.h +++ b/include/linux/virtio_net.h @@ -9,6 +9,194 @@ #include <uapi/linux/tcp.h> #include <uapi/linux/virtio_net.h>

+struct virtio_net_hash {

•   u32 value;
  

•   u16 report;
  

+};

+struct virtio_net_toeplitz_state {

•   u32 hash;
  

•   const u32 *key;
  

+};

+#define VIRTIO_NET_SUPPORTED_HASH_TYPES (VIRTIO_NET_RSS_HASH_TYPE_IPv4 | \

•                                    VIRTIO_NET_RSS_HASH_TYPE_TCPv4 | \
  

•                                    VIRTIO_NET_RSS_HASH_TYPE_UDPv4 | \
  

•                                    VIRTIO_NET_RSS_HASH_TYPE_IPv6 | \
  

•                                    VIRTIO_NET_RSS_HASH_TYPE_TCPv6 | \
  

•                                    VIRTIO_NET_RSS_HASH_TYPE_UDPv6)
  

+#define VIRTIO_NET_RSS_MAX_KEY_SIZE 40

+static inline void virtio_net_toeplitz_convert_key(u32 *input, size_t len) +{

•   while (len >= sizeof(*input)) {
  

•           *input = be32_to_cpu((__force __be32)*input);
  

•           input++;
  

•           len -= sizeof(*input);
  

•   }
  

+}

+static inline void virtio_net_toeplitz_calc(struct virtio_net_toeplitz_state *state,

•                                       const __be32 *input, size_t len)
  

+{

•   while (len >= sizeof(*input)) {
  

•           for (u32 map = be32_to_cpu(*input); map; map &= (map - 1)) {
  

•                   u32 i = ffs(map);
  

•                   state->hash ^= state->key[0] << (32 - i) |
  

•                                  (u32)((u64)state->key[1] >> i);
  

•           }
  

•           state->key++;
  

•           input++;
  

•           len -= sizeof(*input);
  

•   }
  

+}

+static inline u8 virtio_net_hash_key_length(u32 types) +{

•   size_t len = 0;
  

•   if (types & VIRTIO_NET_HASH_REPORT_IPv4)
  

•           len = max(len,
  

•                     sizeof(struct flow_dissector_key_ipv4_addrs));
  

•   if (types &
  

•       (VIRTIO_NET_HASH_REPORT_TCPv4 | VIRTIO_NET_HASH_REPORT_UDPv4))
  

•           len = max(len,
  

•                     sizeof(struct flow_dissector_key_ipv4_addrs) +
  

•                     sizeof(struct flow_dissector_key_ports));
  

•   if (types & VIRTIO_NET_HASH_REPORT_IPv6)
  

•           len = max(len,
  

•                     sizeof(struct flow_dissector_key_ipv6_addrs));
  

•   if (types &
  

•       (VIRTIO_NET_HASH_REPORT_TCPv6 | VIRTIO_NET_HASH_REPORT_UDPv6))
  

•           len = max(len,
  

•                     sizeof(struct flow_dissector_key_ipv6_addrs) +
  

•                     sizeof(struct flow_dissector_key_ports));
  

•   return len + sizeof(u32);
  

+}

+static inline u32 virtio_net_hash_report(u32 types,

•                                    const struct flow_keys_basic *keys)
  

+{

•   switch (keys->basic.n_proto) {
  

•   case cpu_to_be16(ETH_P_IP):
  

•           if (!(keys->control.flags & FLOW_DIS_IS_FRAGMENT)) {
  

•                   if (keys->basic.ip_proto == IPPROTO_TCP &&
  

•                       (types & VIRTIO_NET_RSS_HASH_TYPE_TCPv4))
  

•                           return VIRTIO_NET_HASH_REPORT_TCPv4;
  

•                   if (keys->basic.ip_proto == IPPROTO_UDP &&
  

•                       (types & VIRTIO_NET_RSS_HASH_TYPE_UDPv4))
  

•                           return VIRTIO_NET_HASH_REPORT_UDPv4;
  

•           }
  

•           if (types & VIRTIO_NET_RSS_HASH_TYPE_IPv4)
  

•                   return VIRTIO_NET_HASH_REPORT_IPv4;
  

•           return VIRTIO_NET_HASH_REPORT_NONE;
  

•   case cpu_to_be16(ETH_P_IPV6):
  

•           if (!(keys->control.flags & FLOW_DIS_IS_FRAGMENT)) {
  

•                   if (keys->basic.ip_proto == IPPROTO_TCP &&
  

•                       (types & VIRTIO_NET_RSS_HASH_TYPE_TCPv6))
  

•                           return VIRTIO_NET_HASH_REPORT_TCPv6;
  

•                   if (keys->basic.ip_proto == IPPROTO_UDP &&
  

•                       (types & VIRTIO_NET_RSS_HASH_TYPE_UDPv6))
  

•                           return VIRTIO_NET_HASH_REPORT_UDPv6;
  

•           }
  

•           if (types & VIRTIO_NET_RSS_HASH_TYPE_IPv6)
  

•                   return VIRTIO_NET_HASH_REPORT_IPv6;
  

•           return VIRTIO_NET_HASH_REPORT_NONE;
  

•   default:
  

•           return VIRTIO_NET_HASH_REPORT_NONE;
  

•   }
  

+}

+static inline void virtio_net_hash_rss(const struct sk_buff *skb,

•                                  u32 types, const u32 *key,
  

•                                  struct virtio_net_hash *hash)
  

+{

•   struct virtio_net_toeplitz_state toeplitz_state = { .key = key };
  

•   struct flow_keys flow;
  

•   struct flow_keys_basic flow_basic;
  

•   u16 report;
  

•   if (!skb_flow_dissect_flow_keys(skb, &flow, 0)) {
  

•           hash->report = VIRTIO_NET_HASH_REPORT_NONE;
  

•           return;
  

•   }
  

•   flow_basic = (struct flow_keys_basic) {
  

•           .control = flow.control,
  

•           .basic = flow.basic
  

•   };
  

•   report = virtio_net_hash_report(types, &flow_basic);
  

•   switch (report) {
  

•   case VIRTIO_NET_HASH_REPORT_IPv4:
  

•           virtio_net_toeplitz_calc(&toeplitz_state,
  

•                                    (__be32 *)&flow.addrs.v4addrs,
  

•                                    sizeof(flow.addrs.v4addrs));
  

•           break;
  

•   case VIRTIO_NET_HASH_REPORT_TCPv4:
  

•           virtio_net_toeplitz_calc(&toeplitz_state,
  

•                                    (__be32 *)&flow.addrs.v4addrs,
  

•                                    sizeof(flow.addrs.v4addrs));
  

•           virtio_net_toeplitz_calc(&toeplitz_state, &flow.ports.ports,
  

•                                    sizeof(flow.ports.ports));
  

•           break;
  

•   case VIRTIO_NET_HASH_REPORT_UDPv4:
  

•           virtio_net_toeplitz_calc(&toeplitz_state,
  

•                                    (__be32 *)&flow.addrs.v4addrs,
  

•                                    sizeof(flow.addrs.v4addrs));
  

•           virtio_net_toeplitz_calc(&toeplitz_state, &flow.ports.ports,
  

•                                    sizeof(flow.ports.ports));
  

•           break;
  

•   case VIRTIO_NET_HASH_REPORT_IPv6:
  

•           virtio_net_toeplitz_calc(&toeplitz_state,
  

•                                    (__be32 *)&flow.addrs.v6addrs,
  

•                                    sizeof(flow.addrs.v6addrs));
  

•           break;
  

•   case VIRTIO_NET_HASH_REPORT_TCPv6:
  

•           virtio_net_toeplitz_calc(&toeplitz_state,
  

•                                    (__be32 *)&flow.addrs.v6addrs,
  

•                                    sizeof(flow.addrs.v6addrs));
  

•           virtio_net_toeplitz_calc(&toeplitz_state, &flow.ports.ports,
  

•                                    sizeof(flow.ports.ports));
  

•           break;
  

•   case VIRTIO_NET_HASH_REPORT_UDPv6:
  

•           virtio_net_toeplitz_calc(&toeplitz_state,
  

•                                    (__be32 *)&flow.addrs.v6addrs,
  

•                                    sizeof(flow.addrs.v6addrs));
  

•           virtio_net_toeplitz_calc(&toeplitz_state, &flow.ports.ports,
  

•                                    sizeof(flow.ports.ports));
  

•           break;
  

•   default:
  

•           hash->report = VIRTIO_NET_HASH_REPORT_NONE;
  

•           return;
  

So I still think we need a comment here to explain why this is not an issue if the device can report HASH_XXX_EX. Or we need to add the support, since this is the code from the driver side, I don't think we need to worry about the device implementation issues.

This is on the device side, and don't report HASH_TYPE_XXX_EX.

For the issue of the number of options, does the spec forbid fallback to VIRTIO_NET_HASH_REPORT_NONE? If not, we can do that.

5.1.6.4.3.4 "IPv6 packets with extension header" says:

If VIRTIO_NET_HASH_TYPE_TCP_EX is set and the packet has a TCPv6 header, the hash is calculated over the following fields:

• Home address from the home address option in the IPv6 destination options header. If the extension header is not present, use the Source IPv6 address.
• IPv6 address that is contained in the Routing-Header-Type-2 from the associated extension header. If the extension header is not present, use the Destination IPv6 address.
• Source TCP port
• Destination TCP port

Therefore, if VIRTIO_NET_HASH_TYPE_TCP_EX is set, the packet has a TCPv6 and an home address option in the IPv6 destination options header is present, the hash is calculated over the home address. If the hash is not calculated over the home address in such a case, the device is contradicting with this section and violating the spec. The same goes for the other HASH_TYPE_XXX_EX types and Routing-Header-Type-2.

Regards, Akihiko Odaki

On 2025/06/04 10:18, Jason Wang wrote:

On Tue, Jun 3, 2025 at 1:31 PM Akihiko Odaki akihiko.odaki@daynix.com wrote:

...

On 2025/06/03 12:19, Jason Wang wrote:

...

On Fri, May 30, 2025 at 12:50 PM Akihiko Odaki akihiko.odaki@daynix.com wrote:

...

They are useful to implement VIRTIO_NET_F_RSS and VIRTIO_NET_F_HASH_REPORT.

Signed-off-by: Akihiko Odaki akihiko.odaki@daynix.com Tested-by: Lei Yang leiyang@redhat.com

---

include/linux/virtio_net.h | 188 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 188 insertions(+)

diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h index 02a9f4dc594d..426f33b4b824 100644 --- a/include/linux/virtio_net.h +++ b/include/linux/virtio_net.h @@ -9,6 +9,194 @@ #include <uapi/linux/tcp.h> #include <uapi/linux/virtio_net.h>

+struct virtio_net_hash {

•   u32 value;
  

•   u16 report;
  

+};

+struct virtio_net_toeplitz_state {

•   u32 hash;
  

•   const u32 *key;
  

+};

+#define VIRTIO_NET_SUPPORTED_HASH_TYPES (VIRTIO_NET_RSS_HASH_TYPE_IPv4 | \

•                                    VIRTIO_NET_RSS_HASH_TYPE_TCPv4 | \
  

•                                    VIRTIO_NET_RSS_HASH_TYPE_UDPv4 | \
  

•                                    VIRTIO_NET_RSS_HASH_TYPE_IPv6 | \
  

•                                    VIRTIO_NET_RSS_HASH_TYPE_TCPv6 | \
  

•                                    VIRTIO_NET_RSS_HASH_TYPE_UDPv6)
  

+#define VIRTIO_NET_RSS_MAX_KEY_SIZE 40

+static inline void virtio_net_toeplitz_convert_key(u32 *input, size_t len) +{

•   while (len >= sizeof(*input)) {
  

•           *input = be32_to_cpu((__force __be32)*input);
  

•           input++;
  

•           len -= sizeof(*input);
  

•   }
  

+}

+static inline void virtio_net_toeplitz_calc(struct virtio_net_toeplitz_state *state,

•                                       const __be32 *input, size_t len)
  

+{

•   while (len >= sizeof(*input)) {
  

•           for (u32 map = be32_to_cpu(*input); map; map &= (map - 1)) {
  

•                   u32 i = ffs(map);
  

•                   state->hash ^= state->key[0] << (32 - i) |
  

•                                  (u32)((u64)state->key[1] >> i);
  

•           }
  

•           state->key++;
  

•           input++;
  

•           len -= sizeof(*input);
  

•   }
  

+}

+static inline u8 virtio_net_hash_key_length(u32 types) +{

•   size_t len = 0;
  

•   if (types & VIRTIO_NET_HASH_REPORT_IPv4)
  

•           len = max(len,
  

•                     sizeof(struct flow_dissector_key_ipv4_addrs));
  

•   if (types &
  

•       (VIRTIO_NET_HASH_REPORT_TCPv4 | VIRTIO_NET_HASH_REPORT_UDPv4))
  

•           len = max(len,
  

•                     sizeof(struct flow_dissector_key_ipv4_addrs) +
  

•                     sizeof(struct flow_dissector_key_ports));
  

•   if (types & VIRTIO_NET_HASH_REPORT_IPv6)
  

•           len = max(len,
  

•                     sizeof(struct flow_dissector_key_ipv6_addrs));
  

•   if (types &
  

•       (VIRTIO_NET_HASH_REPORT_TCPv6 | VIRTIO_NET_HASH_REPORT_UDPv6))
  

•           len = max(len,
  

•                     sizeof(struct flow_dissector_key_ipv6_addrs) +
  

•                     sizeof(struct flow_dissector_key_ports));
  

•   return len + sizeof(u32);
  

+}

+static inline u32 virtio_net_hash_report(u32 types,

•                                    const struct flow_keys_basic *keys)
  

+{

•   switch (keys->basic.n_proto) {
  

•   case cpu_to_be16(ETH_P_IP):
  

•           if (!(keys->control.flags & FLOW_DIS_IS_FRAGMENT)) {
  

•                   if (keys->basic.ip_proto == IPPROTO_TCP &&
  

•                       (types & VIRTIO_NET_RSS_HASH_TYPE_TCPv4))
  

•                           return VIRTIO_NET_HASH_REPORT_TCPv4;
  

•                   if (keys->basic.ip_proto == IPPROTO_UDP &&
  

•                       (types & VIRTIO_NET_RSS_HASH_TYPE_UDPv4))
  

•                           return VIRTIO_NET_HASH_REPORT_UDPv4;
  

•           }
  

•           if (types & VIRTIO_NET_RSS_HASH_TYPE_IPv4)
  

•                   return VIRTIO_NET_HASH_REPORT_IPv4;
  

•           return VIRTIO_NET_HASH_REPORT_NONE;
  

•   case cpu_to_be16(ETH_P_IPV6):
  

•           if (!(keys->control.flags & FLOW_DIS_IS_FRAGMENT)) {
  

•                   if (keys->basic.ip_proto == IPPROTO_TCP &&
  

•                       (types & VIRTIO_NET_RSS_HASH_TYPE_TCPv6))
  

•                           return VIRTIO_NET_HASH_REPORT_TCPv6;
  

•                   if (keys->basic.ip_proto == IPPROTO_UDP &&
  

•                       (types & VIRTIO_NET_RSS_HASH_TYPE_UDPv6))
  

•                           return VIRTIO_NET_HASH_REPORT_UDPv6;
  

•           }
  

•           if (types & VIRTIO_NET_RSS_HASH_TYPE_IPv6)
  

•                   return VIRTIO_NET_HASH_REPORT_IPv6;
  

•           return VIRTIO_NET_HASH_REPORT_NONE;
  

•   default:
  

•           return VIRTIO_NET_HASH_REPORT_NONE;
  

•   }
  

+}

+static inline void virtio_net_hash_rss(const struct sk_buff *skb,

•                                  u32 types, const u32 *key,
  

•                                  struct virtio_net_hash *hash)
  

+{

•   struct virtio_net_toeplitz_state toeplitz_state = { .key = key };
  

•   struct flow_keys flow;
  

•   struct flow_keys_basic flow_basic;
  

•   u16 report;
  

•   if (!skb_flow_dissect_flow_keys(skb, &flow, 0)) {
  

•           hash->report = VIRTIO_NET_HASH_REPORT_NONE;
  

•           return;
  

•   }
  

•   flow_basic = (struct flow_keys_basic) {
  

•           .control = flow.control,
  

•           .basic = flow.basic
  

•   };
  

•   report = virtio_net_hash_report(types, &flow_basic);
  

•   switch (report) {
  

•   case VIRTIO_NET_HASH_REPORT_IPv4:
  

•           virtio_net_toeplitz_calc(&toeplitz_state,
  

•                                    (__be32 *)&flow.addrs.v4addrs,
  

•                                    sizeof(flow.addrs.v4addrs));
  

•           break;
  

•   case VIRTIO_NET_HASH_REPORT_TCPv4:
  

•           virtio_net_toeplitz_calc(&toeplitz_state,
  

•                                    (__be32 *)&flow.addrs.v4addrs,
  

•                                    sizeof(flow.addrs.v4addrs));
  

•           virtio_net_toeplitz_calc(&toeplitz_state, &flow.ports.ports,
  

•                                    sizeof(flow.ports.ports));
  

•           break;
  

•   case VIRTIO_NET_HASH_REPORT_UDPv4:
  

•           virtio_net_toeplitz_calc(&toeplitz_state,
  

•                                    (__be32 *)&flow.addrs.v4addrs,
  

•                                    sizeof(flow.addrs.v4addrs));
  

•           virtio_net_toeplitz_calc(&toeplitz_state, &flow.ports.ports,
  

•                                    sizeof(flow.ports.ports));
  

•           break;
  

•   case VIRTIO_NET_HASH_REPORT_IPv6:
  

•           virtio_net_toeplitz_calc(&toeplitz_state,
  

•                                    (__be32 *)&flow.addrs.v6addrs,
  

•                                    sizeof(flow.addrs.v6addrs));
  

•           break;
  

•   case VIRTIO_NET_HASH_REPORT_TCPv6:
  

•           virtio_net_toeplitz_calc(&toeplitz_state,
  

•                                    (__be32 *)&flow.addrs.v6addrs,
  

•                                    sizeof(flow.addrs.v6addrs));
  

•           virtio_net_toeplitz_calc(&toeplitz_state, &flow.ports.ports,
  

•                                    sizeof(flow.ports.ports));
  

•           break;
  

•   case VIRTIO_NET_HASH_REPORT_UDPv6:
  

•           virtio_net_toeplitz_calc(&toeplitz_state,
  

•                                    (__be32 *)&flow.addrs.v6addrs,
  

•                                    sizeof(flow.addrs.v6addrs));
  

•           virtio_net_toeplitz_calc(&toeplitz_state, &flow.ports.ports,
  

•                                    sizeof(flow.ports.ports));
  

•           break;
  

•   default:
  

•           hash->report = VIRTIO_NET_HASH_REPORT_NONE;
  

•           return;
  

So I still think we need a comment here to explain why this is not an issue if the device can report HASH_XXX_EX. Or we need to add the support, since this is the code from the driver side, I don't think we need to worry about the device implementation issues.

This is on the device side, and don't report HASH_TYPE_XXX_EX.

...

For the issue of the number of options, does the spec forbid fallback to VIRTIO_NET_HASH_REPORT_NONE? If not, we can do that.

5.1.6.4.3.4 "IPv6 packets with extension header" says:

...

If VIRTIO_NET_HASH_TYPE_TCP_EX is set and the packet has a TCPv6 header, the hash is calculated over the following fields:

• Home address from the home address option in the IPv6 destination options header. If the extension header is not present, use the Source IPv6 address.
• IPv6 address that is contained in the Routing-Header-Type-2 from the associated extension header. If the extension header is not present, use the Destination IPv6 address.
• Source TCP port
• Destination TCP port

Therefore, if VIRTIO_NET_HASH_TYPE_TCP_EX is set, the packet has a TCPv6 and an home address option in the IPv6 destination options header is present, the hash is calculated over the home address. If the hash is not calculated over the home address in such a case, the device is contradicting with this section and violating the spec. The same goes for the other HASH_TYPE_XXX_EX types and Routing-Header-Type-2.

Just to make sure we are one the same page. I meant:

1. If the hash is not calculated over the home address (in the case of

IPv6 destination destination), it can still report VIRTIO_NET_RSS_HASH_TYPE_IPv6. This is what you implemented in your series. So the device can simply fallback to e.g TCPv6 if it can't understand all or part of the IPv6 options.

The spec says it can fallback if "the extension header is not present", not if the device can't understand the extension header.

2. the VIRTIO_NET_SUPPORTED_HASH_TYPES is not checked against the

tun_vnet_ioctl_sethash(), so userspace may set VIRTIO_NET_HASH_TYPE_TCP_EX regardless of what has been returned by tun_vnet_ioctl_gethashtypes(). In this case they won't get VIRTIO_NET_HASH_TYPE_TCP_EX.

That's right. It's the responsibility of the userspace to set only the supported hash types.

3. implementing part of the hash types might complicate the migration

or at least we need to describe the expectations of libvirt or other management in this case. For example, do we plan to have a dedicated Qemu command line like:

-device virtio-net-pci,hash_report=on,supported_hash_types=X,Y,Z?

I posted a patch series to implement such a command line for vDPA[1]. The patch series that wires this tuntap feature up[2] reuses the infrastructure so it doesn't bring additional complexity.

[1] https://lore.kernel.org/qemu-devel/20250530-vdpa-v1-0-5af4109b1c19@daynix.co... [2] https://lore.kernel.org/qemu-devel/20250530-hash-v5-0-343d7d7a8200@daynix.co...

Regards, Akihiko Odaki

On 2025/06/05 10:53, Jason Wang wrote:

On Wed, Jun 4, 2025 at 3:20 PM Akihiko Odaki akihiko.odaki@daynix.com wrote:

...

On 2025/06/04 10:18, Jason Wang wrote:

...

On Tue, Jun 3, 2025 at 1:31 PM Akihiko Odaki akihiko.odaki@daynix.com wrote:

...

On 2025/06/03 12:19, Jason Wang wrote:

...

On Fri, May 30, 2025 at 12:50 PM Akihiko Odaki akihiko.odaki@daynix.com wrote:

...

They are useful to implement VIRTIO_NET_F_RSS and VIRTIO_NET_F_HASH_REPORT.

Signed-off-by: Akihiko Odaki akihiko.odaki@daynix.com Tested-by: Lei Yang leiyang@redhat.com

---

include/linux/virtio_net.h | 188 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 188 insertions(+)

diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h index 02a9f4dc594d..426f33b4b824 100644 --- a/include/linux/virtio_net.h +++ b/include/linux/virtio_net.h @@ -9,6 +9,194 @@ #include <uapi/linux/tcp.h> #include <uapi/linux/virtio_net.h>

+struct virtio_net_hash {

•   u32 value;
  

•   u16 report;
  

+};

+struct virtio_net_toeplitz_state {

•   u32 hash;
  

•   const u32 *key;
  

+};

+#define VIRTIO_NET_SUPPORTED_HASH_TYPES (VIRTIO_NET_RSS_HASH_TYPE_IPv4 | \

•                                    VIRTIO_NET_RSS_HASH_TYPE_TCPv4 | \
  

•                                    VIRTIO_NET_RSS_HASH_TYPE_UDPv4 | \
  

•                                    VIRTIO_NET_RSS_HASH_TYPE_IPv6 | \
  

•                                    VIRTIO_NET_RSS_HASH_TYPE_TCPv6 | \
  

•                                    VIRTIO_NET_RSS_HASH_TYPE_UDPv6)
  

+#define VIRTIO_NET_RSS_MAX_KEY_SIZE 40

+static inline void virtio_net_toeplitz_convert_key(u32 *input, size_t len) +{

•   while (len >= sizeof(*input)) {
  

•           *input = be32_to_cpu((__force __be32)*input);
  

•           input++;
  

•           len -= sizeof(*input);
  

•   }
  

+}

+static inline void virtio_net_toeplitz_calc(struct virtio_net_toeplitz_state *state,

•                                       const __be32 *input, size_t len)
  

+{

•   while (len >= sizeof(*input)) {
  

•           for (u32 map = be32_to_cpu(*input); map; map &= (map - 1)) {
  

•                   u32 i = ffs(map);
  

•                   state->hash ^= state->key[0] << (32 - i) |
  

•                                  (u32)((u64)state->key[1] >> i);
  

•           }
  

•           state->key++;
  

•           input++;
  

•           len -= sizeof(*input);
  

•   }
  

+}

+static inline u8 virtio_net_hash_key_length(u32 types) +{

•   size_t len = 0;
  

•   if (types & VIRTIO_NET_HASH_REPORT_IPv4)
  

•           len = max(len,
  

•                     sizeof(struct flow_dissector_key_ipv4_addrs));
  

•   if (types &
  

•       (VIRTIO_NET_HASH_REPORT_TCPv4 | VIRTIO_NET_HASH_REPORT_UDPv4))
  

•           len = max(len,
  

•                     sizeof(struct flow_dissector_key_ipv4_addrs) +
  

•                     sizeof(struct flow_dissector_key_ports));
  

•   if (types & VIRTIO_NET_HASH_REPORT_IPv6)
  

•           len = max(len,
  

•                     sizeof(struct flow_dissector_key_ipv6_addrs));
  

•   if (types &
  

•       (VIRTIO_NET_HASH_REPORT_TCPv6 | VIRTIO_NET_HASH_REPORT_UDPv6))
  

•           len = max(len,
  

•                     sizeof(struct flow_dissector_key_ipv6_addrs) +
  

•                     sizeof(struct flow_dissector_key_ports));
  

•   return len + sizeof(u32);
  

+}

+static inline u32 virtio_net_hash_report(u32 types,

•                                    const struct flow_keys_basic *keys)
  

+{

•   switch (keys->basic.n_proto) {
  

•   case cpu_to_be16(ETH_P_IP):
  

•           if (!(keys->control.flags & FLOW_DIS_IS_FRAGMENT)) {
  

•                   if (keys->basic.ip_proto == IPPROTO_TCP &&
  

•                       (types & VIRTIO_NET_RSS_HASH_TYPE_TCPv4))
  

•                           return VIRTIO_NET_HASH_REPORT_TCPv4;
  

•                   if (keys->basic.ip_proto == IPPROTO_UDP &&
  

•                       (types & VIRTIO_NET_RSS_HASH_TYPE_UDPv4))
  

•                           return VIRTIO_NET_HASH_REPORT_UDPv4;
  

•           }
  

•           if (types & VIRTIO_NET_RSS_HASH_TYPE_IPv4)
  

•                   return VIRTIO_NET_HASH_REPORT_IPv4;
  

•           return VIRTIO_NET_HASH_REPORT_NONE;
  

•   case cpu_to_be16(ETH_P_IPV6):
  

•           if (!(keys->control.flags & FLOW_DIS_IS_FRAGMENT)) {
  

•                   if (keys->basic.ip_proto == IPPROTO_TCP &&
  

•                       (types & VIRTIO_NET_RSS_HASH_TYPE_TCPv6))
  

•                           return VIRTIO_NET_HASH_REPORT_TCPv6;
  

•                   if (keys->basic.ip_proto == IPPROTO_UDP &&
  

•                       (types & VIRTIO_NET_RSS_HASH_TYPE_UDPv6))
  

•                           return VIRTIO_NET_HASH_REPORT_UDPv6;
  

•           }
  

•           if (types & VIRTIO_NET_RSS_HASH_TYPE_IPv6)
  

•                   return VIRTIO_NET_HASH_REPORT_IPv6;
  

•           return VIRTIO_NET_HASH_REPORT_NONE;
  

•   default:
  

•           return VIRTIO_NET_HASH_REPORT_NONE;
  

•   }
  

+}

+static inline void virtio_net_hash_rss(const struct sk_buff *skb,

•                                  u32 types, const u32 *key,
  

•                                  struct virtio_net_hash *hash)
  

+{

•   struct virtio_net_toeplitz_state toeplitz_state = { .key = key };
  

•   struct flow_keys flow;
  

•   struct flow_keys_basic flow_basic;
  

•   u16 report;
  

•   if (!skb_flow_dissect_flow_keys(skb, &flow, 0)) {
  

•           hash->report = VIRTIO_NET_HASH_REPORT_NONE;
  

•           return;
  

•   }
  

•   flow_basic = (struct flow_keys_basic) {
  

•           .control = flow.control,
  

•           .basic = flow.basic
  

•   };
  

•   report = virtio_net_hash_report(types, &flow_basic);
  

•   switch (report) {
  

•   case VIRTIO_NET_HASH_REPORT_IPv4:
  

•           virtio_net_toeplitz_calc(&toeplitz_state,
  

•                                    (__be32 *)&flow.addrs.v4addrs,
  

•                                    sizeof(flow.addrs.v4addrs));
  

•           break;
  

•   case VIRTIO_NET_HASH_REPORT_TCPv4:
  

•           virtio_net_toeplitz_calc(&toeplitz_state,
  

•                                    (__be32 *)&flow.addrs.v4addrs,
  

•                                    sizeof(flow.addrs.v4addrs));
  

•           virtio_net_toeplitz_calc(&toeplitz_state, &flow.ports.ports,
  

•                                    sizeof(flow.ports.ports));
  

•           break;
  

•   case VIRTIO_NET_HASH_REPORT_UDPv4:
  

•           virtio_net_toeplitz_calc(&toeplitz_state,
  

•                                    (__be32 *)&flow.addrs.v4addrs,
  

•                                    sizeof(flow.addrs.v4addrs));
  

•           virtio_net_toeplitz_calc(&toeplitz_state, &flow.ports.ports,
  

•                                    sizeof(flow.ports.ports));
  

•           break;
  

•   case VIRTIO_NET_HASH_REPORT_IPv6:
  

•           virtio_net_toeplitz_calc(&toeplitz_state,
  

•                                    (__be32 *)&flow.addrs.v6addrs,
  

•                                    sizeof(flow.addrs.v6addrs));
  

•           break;
  

•   case VIRTIO_NET_HASH_REPORT_TCPv6:
  

•           virtio_net_toeplitz_calc(&toeplitz_state,
  

•                                    (__be32 *)&flow.addrs.v6addrs,
  

•                                    sizeof(flow.addrs.v6addrs));
  

•           virtio_net_toeplitz_calc(&toeplitz_state, &flow.ports.ports,
  

•                                    sizeof(flow.ports.ports));
  

•           break;
  

•   case VIRTIO_NET_HASH_REPORT_UDPv6:
  

•           virtio_net_toeplitz_calc(&toeplitz_state,
  

•                                    (__be32 *)&flow.addrs.v6addrs,
  

•                                    sizeof(flow.addrs.v6addrs));
  

•           virtio_net_toeplitz_calc(&toeplitz_state, &flow.ports.ports,
  

•                                    sizeof(flow.ports.ports));
  

•           break;
  

•   default:
  

•           hash->report = VIRTIO_NET_HASH_REPORT_NONE;
  

•           return;
  

So I still think we need a comment here to explain why this is not an issue if the device can report HASH_XXX_EX. Or we need to add the support, since this is the code from the driver side, I don't think we need to worry about the device implementation issues.

This is on the device side, and don't report HASH_TYPE_XXX_EX.

...

For the issue of the number of options, does the spec forbid fallback to VIRTIO_NET_HASH_REPORT_NONE? If not, we can do that.

5.1.6.4.3.4 "IPv6 packets with extension header" says:

...

If VIRTIO_NET_HASH_TYPE_TCP_EX is set and the packet has a TCPv6 header, the hash is calculated over the following fields:

• Home address from the home address option in the IPv6 destination options header. If the extension header is not present, use the Source IPv6 address.
• IPv6 address that is contained in the Routing-Header-Type-2 from the associated extension header. If the extension header is not present, use the Destination IPv6 address.
• Source TCP port
• Destination TCP port

Therefore, if VIRTIO_NET_HASH_TYPE_TCP_EX is set, the packet has a TCPv6 and an home address option in the IPv6 destination options header is present, the hash is calculated over the home address. If the hash is not calculated over the home address in such a case, the device is contradicting with this section and violating the spec. The same goes for the other HASH_TYPE_XXX_EX types and Routing-Header-Type-2.

Just to make sure we are one the same page. I meant:

1. If the hash is not calculated over the home address (in the case of

IPv6 destination destination), it can still report VIRTIO_NET_RSS_HASH_TYPE_IPv6. This is what you implemented in your series. So the device can simply fallback to e.g TCPv6 if it can't understand all or part of the IPv6 options.

The spec says it can fallback if "the extension header is not present", not if the device can't understand the extension header.

I don't think so,

1. spec had a condition beforehand:

""" If VIRTIO_NET_HASH_TYPE_TCP_EX is set and the packet has a TCPv6 header, the hash is calculated over the following fields: ... If the extension header is not present ... """

So the device can choose not to set VIRTIO_NET_HASH_TYPE_TCP_EX as spec doesn't say device MUST set VIRTIO_NET_HASH_TYPE_TCP_EX if ...

2. implementation wise, since device has limited resources, we can't

expect the device can parse arbitrary number of ipv6 options

3. if 1) and 2) not the case, we need fix the spec otherwise implement

a spec compliant device is impractical

The statement is preceded by the following:

The device calculates the hash on IPv4 packets according to ’Enabled hash types’ bitmask as follows:

The 'Enabled hash types' bitmask is specified by the device.

I think the spec needs amendment.

I wonder if there are any people interested in the feature though. Looking at virtnet_set_hashflow() in drivers/net/virtio_net.c, the driver of Linux does not let users configure HASH_TYPE_XXX_EX. I suppose Windows supports HASH_TYPE_XXX_EX, but those who care network performance most would use Linux so HASH_TYPE_XXX_EX support without Linux driver's support may not be useful.

...

...

2. the VIRTIO_NET_SUPPORTED_HASH_TYPES is not checked against the

tun_vnet_ioctl_sethash(), so userspace may set VIRTIO_NET_HASH_TYPE_TCP_EX regardless of what has been returned by tun_vnet_ioctl_gethashtypes(). In this case they won't get VIRTIO_NET_HASH_TYPE_TCP_EX.

That's right. It's the responsibility of the userspace to set only the supported hash types.

Well, the kernel should filter out the unsupported one to have a robust uAPI. Otherwise, we give green light to the buggy userspace which will have unexpected results.

My reasoning was that it may be fine for some use cases other than VM (e.g., DPDK); in such a use case, it is fine as long as the UAPI works in the best-effort basis.

For example, suppose a userspace program that processes TCP packets; the program can enable: HASH_TYPE_IPv4, HASH_TYPE_TCPv4, HASH_TYPE_IPv6, and HASH_TYPE_TCPv6. Ideally, the kernel should support all the hash types, but, even if e.g., HASH_TYPE_TCPv6 is not available, it will fall back to HASH_TYPE_IPv6, which still does something good and may be acceptable.

That said, for a use case that involves VM and implements virtio-net (e.g., QEMU), setting an unsupported hash type here is definitely a bug. Catching the bug may outweigh the extra trouble for other use cases.

...

...

3. implementing part of the hash types might complicate the migration

or at least we need to describe the expectations of libvirt or other management in this case. For example, do we plan to have a dedicated Qemu command line like:

-device virtio-net-pci,hash_report=on,supported_hash_types=X,Y,Z?

I posted a patch series to implement such a command line for vDPA[1]. The patch series that wires this tuntap feature up[2] reuses the infrastructure so it doesn't bring additional complexity.

[1] https://lore.kernel.org/qemu-devel/20250530-vdpa-v1-0-5af4109b1c19@daynix.co... [2] https://lore.kernel.org/qemu-devel/20250530-hash-v5-0-343d7d7a8200@daynix.co...

I meant, if we implement a full hash report feature, it means a single hash cmdline option is more than sufficient and so compatibility code can just turn it off when dealing with machine types. This is much more simpler than

1. having both hash as well as supported_hash_features
2. dealing both hash as well as supported_hash_features in compatibility codes
3. libvirt will be happy

For [1], it seems it introduces a per has type option, this seems to be a burden to the management layer as it need to learn new option everytime a new hash type is supported

Even with the command line you proposed (supported_hash_types=X,Y,Z), it is still necessary to know the values the supported_hash_types property accepts (X.Y,Z), so I don't think it makes difference.

The burden to the management layer is already present for features, so it is an existing problem (or its mere extension).

This problem was discussed in the following thread in the past, but no solution is implemented yet, and probably solving it will be difficult. https://lore.kernel.org/qemu-devel/20230731223148.1002258-5-yuri.benditovich...

Regards, Akihiko Odaki

On 2025/06/06 9:48, Jason Wang wrote:

On Thu, Jun 5, 2025 at 3:58 PM Akihiko Odaki akihiko.odaki@daynix.com wrote:

...

On 2025/06/05 10:53, Jason Wang wrote:

...

On Wed, Jun 4, 2025 at 3:20 PM Akihiko Odaki akihiko.odaki@daynix.com wrote:

...

On 2025/06/04 10:18, Jason Wang wrote:

...

On Tue, Jun 3, 2025 at 1:31 PM Akihiko Odaki akihiko.odaki@daynix.com wrote:

...

On 2025/06/03 12:19, Jason Wang wrote: > On Fri, May 30, 2025 at 12:50 PM Akihiko Odaki akihiko.odaki@daynix.com wrote: >> >> They are useful to implement VIRTIO_NET_F_RSS and >> VIRTIO_NET_F_HASH_REPORT. >> >> Signed-off-by: Akihiko Odaki akihiko.odaki@daynix.com >> Tested-by: Lei Yang leiyang@redhat.com >> --- >> include/linux/virtio_net.h | 188 +++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 188 insertions(+) >> >> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h >> index 02a9f4dc594d..426f33b4b824 100644 >> --- a/include/linux/virtio_net.h >> +++ b/include/linux/virtio_net.h >> @@ -9,6 +9,194 @@ >> #include <uapi/linux/tcp.h> >> #include <uapi/linux/virtio_net.h> >> >> +struct virtio_net_hash { >> + u32 value; >> + u16 report; >> +}; >> + >> +struct virtio_net_toeplitz_state { >> + u32 hash; >> + const u32 *key; >> +}; >> + >> +#define VIRTIO_NET_SUPPORTED_HASH_TYPES (VIRTIO_NET_RSS_HASH_TYPE_IPv4 | \ >> + VIRTIO_NET_RSS_HASH_TYPE_TCPv4 | \ >> + VIRTIO_NET_RSS_HASH_TYPE_UDPv4 | \ >> + VIRTIO_NET_RSS_HASH_TYPE_IPv6 | \ >> + VIRTIO_NET_RSS_HASH_TYPE_TCPv6 | \ >> + VIRTIO_NET_RSS_HASH_TYPE_UDPv6) >> + >> +#define VIRTIO_NET_RSS_MAX_KEY_SIZE 40 >> + >> +static inline void virtio_net_toeplitz_convert_key(u32 *input, size_t len) >> +{ >> + while (len >= sizeof(*input)) { >> + *input = be32_to_cpu((__force __be32)*input); >> + input++; >> + len -= sizeof(*input); >> + } >> +} >> + >> +static inline void virtio_net_toeplitz_calc(struct virtio_net_toeplitz_state *state, >> + const __be32 *input, size_t len) >> +{ >> + while (len >= sizeof(*input)) { >> + for (u32 map = be32_to_cpu(*input); map; map &= (map - 1)) { >> + u32 i = ffs(map); >> + >> + state->hash ^= state->key[0] << (32 - i) | >> + (u32)((u64)state->key[1] >> i); >> + } >> + >> + state->key++; >> + input++; >> + len -= sizeof(*input); >> + } >> +} >> + >> +static inline u8 virtio_net_hash_key_length(u32 types) >> +{ >> + size_t len = 0; >> + >> + if (types & VIRTIO_NET_HASH_REPORT_IPv4) >> + len = max(len, >> + sizeof(struct flow_dissector_key_ipv4_addrs)); >> + >> + if (types & >> + (VIRTIO_NET_HASH_REPORT_TCPv4 | VIRTIO_NET_HASH_REPORT_UDPv4)) >> + len = max(len, >> + sizeof(struct flow_dissector_key_ipv4_addrs) + >> + sizeof(struct flow_dissector_key_ports)); >> + >> + if (types & VIRTIO_NET_HASH_REPORT_IPv6) >> + len = max(len, >> + sizeof(struct flow_dissector_key_ipv6_addrs)); >> + >> + if (types & >> + (VIRTIO_NET_HASH_REPORT_TCPv6 | VIRTIO_NET_HASH_REPORT_UDPv6)) >> + len = max(len, >> + sizeof(struct flow_dissector_key_ipv6_addrs) + >> + sizeof(struct flow_dissector_key_ports)); >> + >> + return len + sizeof(u32); >> +} >> + >> +static inline u32 virtio_net_hash_report(u32 types, >> + const struct flow_keys_basic *keys) >> +{ >> + switch (keys->basic.n_proto) { >> + case cpu_to_be16(ETH_P_IP): >> + if (!(keys->control.flags & FLOW_DIS_IS_FRAGMENT)) { >> + if (keys->basic.ip_proto == IPPROTO_TCP && >> + (types & VIRTIO_NET_RSS_HASH_TYPE_TCPv4)) >> + return VIRTIO_NET_HASH_REPORT_TCPv4; >> + >> + if (keys->basic.ip_proto == IPPROTO_UDP && >> + (types & VIRTIO_NET_RSS_HASH_TYPE_UDPv4)) >> + return VIRTIO_NET_HASH_REPORT_UDPv4; >> + } >> + >> + if (types & VIRTIO_NET_RSS_HASH_TYPE_IPv4) >> + return VIRTIO_NET_HASH_REPORT_IPv4; >> + >> + return VIRTIO_NET_HASH_REPORT_NONE; >> + >> + case cpu_to_be16(ETH_P_IPV6): >> + if (!(keys->control.flags & FLOW_DIS_IS_FRAGMENT)) { >> + if (keys->basic.ip_proto == IPPROTO_TCP && >> + (types & VIRTIO_NET_RSS_HASH_TYPE_TCPv6)) >> + return VIRTIO_NET_HASH_REPORT_TCPv6; >> + >> + if (keys->basic.ip_proto == IPPROTO_UDP && >> + (types & VIRTIO_NET_RSS_HASH_TYPE_UDPv6)) >> + return VIRTIO_NET_HASH_REPORT_UDPv6; >> + } >> + >> + if (types & VIRTIO_NET_RSS_HASH_TYPE_IPv6) >> + return VIRTIO_NET_HASH_REPORT_IPv6; >> + >> + return VIRTIO_NET_HASH_REPORT_NONE; >> + >> + default: >> + return VIRTIO_NET_HASH_REPORT_NONE; >> + } >> +} >> + >> +static inline void virtio_net_hash_rss(const struct sk_buff *skb, >> + u32 types, const u32 *key, >> + struct virtio_net_hash *hash) >> +{ >> + struct virtio_net_toeplitz_state toeplitz_state = { .key = key }; >> + struct flow_keys flow; >> + struct flow_keys_basic flow_basic; >> + u16 report; >> + >> + if (!skb_flow_dissect_flow_keys(skb, &flow, 0)) { >> + hash->report = VIRTIO_NET_HASH_REPORT_NONE; >> + return; >> + } >> + >> + flow_basic = (struct flow_keys_basic) { >> + .control = flow.control, >> + .basic = flow.basic >> + }; >> + >> + report = virtio_net_hash_report(types, &flow_basic); >> + >> + switch (report) { >> + case VIRTIO_NET_HASH_REPORT_IPv4: >> + virtio_net_toeplitz_calc(&toeplitz_state, >> + (__be32 *)&flow.addrs.v4addrs, >> + sizeof(flow.addrs.v4addrs)); >> + break; >> + >> + case VIRTIO_NET_HASH_REPORT_TCPv4: >> + virtio_net_toeplitz_calc(&toeplitz_state, >> + (__be32 *)&flow.addrs.v4addrs, >> + sizeof(flow.addrs.v4addrs)); >> + virtio_net_toeplitz_calc(&toeplitz_state, &flow.ports.ports, >> + sizeof(flow.ports.ports)); >> + break; >> + >> + case VIRTIO_NET_HASH_REPORT_UDPv4: >> + virtio_net_toeplitz_calc(&toeplitz_state, >> + (__be32 *)&flow.addrs.v4addrs, >> + sizeof(flow.addrs.v4addrs)); >> + virtio_net_toeplitz_calc(&toeplitz_state, &flow.ports.ports, >> + sizeof(flow.ports.ports)); >> + break; >> + >> + case VIRTIO_NET_HASH_REPORT_IPv6: >> + virtio_net_toeplitz_calc(&toeplitz_state, >> + (__be32 *)&flow.addrs.v6addrs, >> + sizeof(flow.addrs.v6addrs)); >> + break; >> + >> + case VIRTIO_NET_HASH_REPORT_TCPv6: >> + virtio_net_toeplitz_calc(&toeplitz_state, >> + (__be32 *)&flow.addrs.v6addrs, >> + sizeof(flow.addrs.v6addrs)); >> + virtio_net_toeplitz_calc(&toeplitz_state, &flow.ports.ports, >> + sizeof(flow.ports.ports)); >> + break; >> + >> + case VIRTIO_NET_HASH_REPORT_UDPv6: >> + virtio_net_toeplitz_calc(&toeplitz_state, >> + (__be32 *)&flow.addrs.v6addrs, >> + sizeof(flow.addrs.v6addrs)); >> + virtio_net_toeplitz_calc(&toeplitz_state, &flow.ports.ports, >> + sizeof(flow.ports.ports)); >> + break; >> + >> + default: >> + hash->report = VIRTIO_NET_HASH_REPORT_NONE; >> + return; > > So I still think we need a comment here to explain why this is not an > issue if the device can report HASH_XXX_EX. Or we need to add the > support, since this is the code from the driver side, I don't think we > need to worry about the device implementation issues.

This is on the device side, and don't report HASH_TYPE_XXX_EX.

> > For the issue of the number of options, does the spec forbid fallback > to VIRTIO_NET_HASH_REPORT_NONE? If not, we can do that.

5.1.6.4.3.4 "IPv6 packets with extension header" says: > If VIRTIO_NET_HASH_TYPE_TCP_EX is set and the packet has a TCPv6 > header, the hash is calculated over the following fields: > - Home address from the home address option in the IPv6 destination > options header. If the extension header is not present, use the > Source IPv6 address. > - IPv6 address that is contained in the Routing-Header-Type-2 from the > associated extension header. If the extension header is not present, > use the Destination IPv6 address. > - Source TCP port > - Destination TCP port

Therefore, if VIRTIO_NET_HASH_TYPE_TCP_EX is set, the packet has a TCPv6 and an home address option in the IPv6 destination options header is present, the hash is calculated over the home address. If the hash is not calculated over the home address in such a case, the device is contradicting with this section and violating the spec. The same goes for the other HASH_TYPE_XXX_EX types and Routing-Header-Type-2.

Just to make sure we are one the same page. I meant:

1. If the hash is not calculated over the home address (in the case of

IPv6 destination destination), it can still report VIRTIO_NET_RSS_HASH_TYPE_IPv6. This is what you implemented in your series. So the device can simply fallback to e.g TCPv6 if it can't understand all or part of the IPv6 options.

The spec says it can fallback if "the extension header is not present", not if the device can't understand the extension header.

I don't think so,

1. spec had a condition beforehand:

""" If VIRTIO_NET_HASH_TYPE_TCP_EX is set and the packet has a TCPv6 header, the hash is calculated over the following fields: ... If the extension header is not present ... """

So the device can choose not to set VIRTIO_NET_HASH_TYPE_TCP_EX as spec doesn't say device MUST set VIRTIO_NET_HASH_TYPE_TCP_EX if ...

2. implementation wise, since device has limited resources, we can't

expect the device can parse arbitrary number of ipv6 options

3. if 1) and 2) not the case, we need fix the spec otherwise implement

a spec compliant device is impractical

The statement is preceded by the following:

...

The device calculates the hash on IPv4 packets according to ’Enabled hash types’ bitmask as follows:

The 'Enabled hash types' bitmask is specified by the device.

I think the spec needs amendment.

Michael, can you help to clarify here?

...

I wonder if there are any people interested in the feature though. Looking at virtnet_set_hashflow() in drivers/net/virtio_net.c, the driver of Linux does not let users configure HASH_TYPE_XXX_EX. I suppose Windows supports HASH_TYPE_XXX_EX, but those who care network performance most would use Linux so HASH_TYPE_XXX_EX support without Linux driver's support may not be useful.

It might be still interesting for example for the hardware virtio vendors to support windows etc.

I don't know if Windows needs them for e.g., device/driver certification so surveying Windows makes sense.

...

...

...

...

2. the VIRTIO_NET_SUPPORTED_HASH_TYPES is not checked against the

tun_vnet_ioctl_sethash(), so userspace may set VIRTIO_NET_HASH_TYPE_TCP_EX regardless of what has been returned by tun_vnet_ioctl_gethashtypes(). In this case they won't get VIRTIO_NET_HASH_TYPE_TCP_EX.

That's right. It's the responsibility of the userspace to set only the supported hash types.

Well, the kernel should filter out the unsupported one to have a robust uAPI. Otherwise, we give green light to the buggy userspace which will have unexpected results.

My reasoning was that it may be fine for some use cases other than VM (e.g., DPDK); in such a use case, it is fine as long as the UAPI works in the best-effort basis.

Best-effort might increase the chance for user visisable changes after migration.

It is a trade-off between catching a migration bug for VMM and making life a bit easier for userspace programs other than VMM.

...

For example, suppose a userspace program that processes TCP packets; the program can enable: HASH_TYPE_IPv4, HASH_TYPE_TCPv4, HASH_TYPE_IPv6, and HASH_TYPE_TCPv6. Ideally, the kernel should support all the hash types, but, even if e.g., HASH_TYPE_TCPv6 is not available,

For "available" did you mean it is not supported by the device?

...

it will fall back to HASH_TYPE_IPv6, which still does something good and may be acceptable.

This fallback is exactly the same as I said above, let VIRTIO_NET_HASH_TYPE_TCP_EX to fallback.

My point is that, the implementation should either:

1. allow fallback so it can claim to support all hash types

or

2. don't allow fallback so it can only support a part of the hash types

If we're doing something in the middle, for example, allow part of the type to fallback.

1) or the middle will make it unsuitable for VM because it violates the virtio spec. 2) makes sense though the trade-off I mentioned should be taken into consideration.

...

That said, for a use case that involves VM and implements virtio-net (e.g., QEMU), setting an unsupported hash type here is definitely a bug. Catching the bug may outweigh the extra trouble for other use cases.

...

...

...

3. implementing part of the hash types might complicate the migration

or at least we need to describe the expectations of libvirt or other management in this case. For example, do we plan to have a dedicated Qemu command line like:

-device virtio-net-pci,hash_report=on,supported_hash_types=X,Y,Z?

I posted a patch series to implement such a command line for vDPA[1]. The patch series that wires this tuntap feature up[2] reuses the infrastructure so it doesn't bring additional complexity.

[1] https://lore.kernel.org/qemu-devel/20250530-vdpa-v1-0-5af4109b1c19@daynix.co... [2] https://lore.kernel.org/qemu-devel/20250530-hash-v5-0-343d7d7a8200@daynix.co...

I meant, if we implement a full hash report feature, it means a single hash cmdline option is more than sufficient and so compatibility code can just turn it off when dealing with machine types. This is much more simpler than

1. having both hash as well as supported_hash_features
2. dealing both hash as well as supported_hash_features in compatibility codes
3. libvirt will be happy

For [1], it seems it introduces a per has type option, this seems to be a burden to the management layer as it need to learn new option everytime a new hash type is supported

Even with the command line you proposed (supported_hash_types=X,Y,Z), it is still necessary to know the values the supported_hash_types property accepts (X.Y,Z), so I don't think it makes difference.

It could be a uint32_t.

The management layer will need to know what bits are accepted even with uint32_t.

...

The burden to the management layer is already present for features, so it is an existing problem (or its mere extension).

Yes, but since this feature is new it's better to try our best to avoid that.

...

This problem was discussed in the following thread in the past, but no solution is implemented yet, and probably solving it will be difficult. https://lore.kernel.org/qemu-devel/20230731223148.1002258-5-yuri.benditovich...

It's a similar issue but not the same, it looks more like a discussion on whether the fallback from vhost-net to qemu works for missing features etc.

Perhaps we may be able to do better since this feature is new as you say and we don't have to worry much about breaking change. I don't have an idea for that yet.

Regards, Akihiko Odaki

On 2025/06/17 12:28, Jason Wang wrote:

On Fri, Jun 6, 2025 at 5:10 PM Akihiko Odaki akihiko.odaki@daynix.com wrote:

...

On 2025/06/06 9:48, Jason Wang wrote:

...

On Thu, Jun 5, 2025 at 3:58 PM Akihiko Odaki akihiko.odaki@daynix.com wrote:

...

On 2025/06/05 10:53, Jason Wang wrote:

...

On Wed, Jun 4, 2025 at 3:20 PM Akihiko Odaki akihiko.odaki@daynix.com wrote:

...

On 2025/06/04 10:18, Jason Wang wrote: > On Tue, Jun 3, 2025 at 1:31 PM Akihiko Odaki akihiko.odaki@daynix.com wrote: >> >> On 2025/06/03 12:19, Jason Wang wrote: >>> On Fri, May 30, 2025 at 12:50 PM Akihiko Odaki akihiko.odaki@daynix.com wrote: >>>> >>>> They are useful to implement VIRTIO_NET_F_RSS and >>>> VIRTIO_NET_F_HASH_REPORT. >>>> >>>> Signed-off-by: Akihiko Odaki akihiko.odaki@daynix.com >>>> Tested-by: Lei Yang leiyang@redhat.com >>>> --- >>>> include/linux/virtio_net.h | 188 +++++++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 188 insertions(+) >>>> >>>> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h >>>> index 02a9f4dc594d..426f33b4b824 100644 >>>> --- a/include/linux/virtio_net.h >>>> +++ b/include/linux/virtio_net.h >>>> @@ -9,6 +9,194 @@ >>>> #include <uapi/linux/tcp.h> >>>> #include <uapi/linux/virtio_net.h> >>>> >>>> +struct virtio_net_hash { >>>> + u32 value; >>>> + u16 report; >>>> +}; >>>> + >>>> +struct virtio_net_toeplitz_state { >>>> + u32 hash; >>>> + const u32 *key; >>>> +}; >>>> + >>>> +#define VIRTIO_NET_SUPPORTED_HASH_TYPES (VIRTIO_NET_RSS_HASH_TYPE_IPv4 | \ >>>> + VIRTIO_NET_RSS_HASH_TYPE_TCPv4 | \ >>>> + VIRTIO_NET_RSS_HASH_TYPE_UDPv4 | \ >>>> + VIRTIO_NET_RSS_HASH_TYPE_IPv6 | \ >>>> + VIRTIO_NET_RSS_HASH_TYPE_TCPv6 | \ >>>> + VIRTIO_NET_RSS_HASH_TYPE_UDPv6) >>>> + >>>> +#define VIRTIO_NET_RSS_MAX_KEY_SIZE 40 >>>> + >>>> +static inline void virtio_net_toeplitz_convert_key(u32 *input, size_t len) >>>> +{ >>>> + while (len >= sizeof(*input)) { >>>> + *input = be32_to_cpu((__force __be32)*input); >>>> + input++; >>>> + len -= sizeof(*input); >>>> + } >>>> +} >>>> + >>>> +static inline void virtio_net_toeplitz_calc(struct virtio_net_toeplitz_state *state, >>>> + const __be32 *input, size_t len) >>>> +{ >>>> + while (len >= sizeof(*input)) { >>>> + for (u32 map = be32_to_cpu(*input); map; map &= (map - 1)) { >>>> + u32 i = ffs(map); >>>> + >>>> + state->hash ^= state->key[0] << (32 - i) | >>>> + (u32)((u64)state->key[1] >> i); >>>> + } >>>> + >>>> + state->key++; >>>> + input++; >>>> + len -= sizeof(*input); >>>> + } >>>> +} >>>> + >>>> +static inline u8 virtio_net_hash_key_length(u32 types) >>>> +{ >>>> + size_t len = 0; >>>> + >>>> + if (types & VIRTIO_NET_HASH_REPORT_IPv4) >>>> + len = max(len, >>>> + sizeof(struct flow_dissector_key_ipv4_addrs)); >>>> + >>>> + if (types & >>>> + (VIRTIO_NET_HASH_REPORT_TCPv4 | VIRTIO_NET_HASH_REPORT_UDPv4)) >>>> + len = max(len, >>>> + sizeof(struct flow_dissector_key_ipv4_addrs) + >>>> + sizeof(struct flow_dissector_key_ports)); >>>> + >>>> + if (types & VIRTIO_NET_HASH_REPORT_IPv6) >>>> + len = max(len, >>>> + sizeof(struct flow_dissector_key_ipv6_addrs)); >>>> + >>>> + if (types & >>>> + (VIRTIO_NET_HASH_REPORT_TCPv6 | VIRTIO_NET_HASH_REPORT_UDPv6)) >>>> + len = max(len, >>>> + sizeof(struct flow_dissector_key_ipv6_addrs) + >>>> + sizeof(struct flow_dissector_key_ports)); >>>> + >>>> + return len + sizeof(u32); >>>> +} >>>> + >>>> +static inline u32 virtio_net_hash_report(u32 types, >>>> + const struct flow_keys_basic *keys) >>>> +{ >>>> + switch (keys->basic.n_proto) { >>>> + case cpu_to_be16(ETH_P_IP): >>>> + if (!(keys->control.flags & FLOW_DIS_IS_FRAGMENT)) { >>>> + if (keys->basic.ip_proto == IPPROTO_TCP && >>>> + (types & VIRTIO_NET_RSS_HASH_TYPE_TCPv4)) >>>> + return VIRTIO_NET_HASH_REPORT_TCPv4; >>>> + >>>> + if (keys->basic.ip_proto == IPPROTO_UDP && >>>> + (types & VIRTIO_NET_RSS_HASH_TYPE_UDPv4)) >>>> + return VIRTIO_NET_HASH_REPORT_UDPv4; >>>> + } >>>> + >>>> + if (types & VIRTIO_NET_RSS_HASH_TYPE_IPv4) >>>> + return VIRTIO_NET_HASH_REPORT_IPv4; >>>> + >>>> + return VIRTIO_NET_HASH_REPORT_NONE; >>>> + >>>> + case cpu_to_be16(ETH_P_IPV6): >>>> + if (!(keys->control.flags & FLOW_DIS_IS_FRAGMENT)) { >>>> + if (keys->basic.ip_proto == IPPROTO_TCP && >>>> + (types & VIRTIO_NET_RSS_HASH_TYPE_TCPv6)) >>>> + return VIRTIO_NET_HASH_REPORT_TCPv6; >>>> + >>>> + if (keys->basic.ip_proto == IPPROTO_UDP && >>>> + (types & VIRTIO_NET_RSS_HASH_TYPE_UDPv6)) >>>> + return VIRTIO_NET_HASH_REPORT_UDPv6; >>>> + } >>>> + >>>> + if (types & VIRTIO_NET_RSS_HASH_TYPE_IPv6) >>>> + return VIRTIO_NET_HASH_REPORT_IPv6; >>>> + >>>> + return VIRTIO_NET_HASH_REPORT_NONE; >>>> + >>>> + default: >>>> + return VIRTIO_NET_HASH_REPORT_NONE; >>>> + } >>>> +} >>>> + >>>> +static inline void virtio_net_hash_rss(const struct sk_buff *skb, >>>> + u32 types, const u32 *key, >>>> + struct virtio_net_hash *hash) >>>> +{ >>>> + struct virtio_net_toeplitz_state toeplitz_state = { .key = key }; >>>> + struct flow_keys flow; >>>> + struct flow_keys_basic flow_basic; >>>> + u16 report; >>>> + >>>> + if (!skb_flow_dissect_flow_keys(skb, &flow, 0)) { >>>> + hash->report = VIRTIO_NET_HASH_REPORT_NONE; >>>> + return; >>>> + } >>>> + >>>> + flow_basic = (struct flow_keys_basic) { >>>> + .control = flow.control, >>>> + .basic = flow.basic >>>> + }; >>>> + >>>> + report = virtio_net_hash_report(types, &flow_basic); >>>> + >>>> + switch (report) { >>>> + case VIRTIO_NET_HASH_REPORT_IPv4: >>>> + virtio_net_toeplitz_calc(&toeplitz_state, >>>> + (__be32 *)&flow.addrs.v4addrs, >>>> + sizeof(flow.addrs.v4addrs)); >>>> + break; >>>> + >>>> + case VIRTIO_NET_HASH_REPORT_TCPv4: >>>> + virtio_net_toeplitz_calc(&toeplitz_state, >>>> + (__be32 *)&flow.addrs.v4addrs, >>>> + sizeof(flow.addrs.v4addrs)); >>>> + virtio_net_toeplitz_calc(&toeplitz_state, &flow.ports.ports, >>>> + sizeof(flow.ports.ports)); >>>> + break; >>>> + >>>> + case VIRTIO_NET_HASH_REPORT_UDPv4: >>>> + virtio_net_toeplitz_calc(&toeplitz_state, >>>> + (__be32 *)&flow.addrs.v4addrs, >>>> + sizeof(flow.addrs.v4addrs)); >>>> + virtio_net_toeplitz_calc(&toeplitz_state, &flow.ports.ports, >>>> + sizeof(flow.ports.ports)); >>>> + break; >>>> + >>>> + case VIRTIO_NET_HASH_REPORT_IPv6: >>>> + virtio_net_toeplitz_calc(&toeplitz_state, >>>> + (__be32 *)&flow.addrs.v6addrs, >>>> + sizeof(flow.addrs.v6addrs)); >>>> + break; >>>> + >>>> + case VIRTIO_NET_HASH_REPORT_TCPv6: >>>> + virtio_net_toeplitz_calc(&toeplitz_state, >>>> + (__be32 *)&flow.addrs.v6addrs, >>>> + sizeof(flow.addrs.v6addrs)); >>>> + virtio_net_toeplitz_calc(&toeplitz_state, &flow.ports.ports, >>>> + sizeof(flow.ports.ports)); >>>> + break; >>>> + >>>> + case VIRTIO_NET_HASH_REPORT_UDPv6: >>>> + virtio_net_toeplitz_calc(&toeplitz_state, >>>> + (__be32 *)&flow.addrs.v6addrs, >>>> + sizeof(flow.addrs.v6addrs)); >>>> + virtio_net_toeplitz_calc(&toeplitz_state, &flow.ports.ports, >>>> + sizeof(flow.ports.ports)); >>>> + break; >>>> + >>>> + default: >>>> + hash->report = VIRTIO_NET_HASH_REPORT_NONE; >>>> + return; >>> >>> So I still think we need a comment here to explain why this is not an >>> issue if the device can report HASH_XXX_EX. Or we need to add the >>> support, since this is the code from the driver side, I don't think we >>> need to worry about the device implementation issues. >> >> This is on the device side, and don't report HASH_TYPE_XXX_EX. >> >>> >>> For the issue of the number of options, does the spec forbid fallback >>> to VIRTIO_NET_HASH_REPORT_NONE? If not, we can do that. >> >> 5.1.6.4.3.4 "IPv6 packets with extension header" says: >> > If VIRTIO_NET_HASH_TYPE_TCP_EX is set and the packet has a TCPv6 >> > header, the hash is calculated over the following fields: >> > - Home address from the home address option in the IPv6 destination >> > options header. If the extension header is not present, use the >> > Source IPv6 address. >> > - IPv6 address that is contained in the Routing-Header-Type-2 from the >> > associated extension header. If the extension header is not present, >> > use the Destination IPv6 address. >> > - Source TCP port >> > - Destination TCP port >> >> Therefore, if VIRTIO_NET_HASH_TYPE_TCP_EX is set, the packet has a TCPv6 >> and an home address option in the IPv6 destination options header is >> present, the hash is calculated over the home address. If the hash is >> not calculated over the home address in such a case, the device is >> contradicting with this section and violating the spec. The same goes >> for the other HASH_TYPE_XXX_EX types and Routing-Header-Type-2. > > Just to make sure we are one the same page. I meant: > > 1) If the hash is not calculated over the home address (in the case of > IPv6 destination destination), it can still report > VIRTIO_NET_RSS_HASH_TYPE_IPv6. This is what you implemented in your > series. So the device can simply fallback to e.g TCPv6 if it can't > understand all or part of the IPv6 options.

The spec says it can fallback if "the extension header is not present", not if the device can't understand the extension header.

I don't think so,

1. spec had a condition beforehand:

""" If VIRTIO_NET_HASH_TYPE_TCP_EX is set and the packet has a TCPv6 header, the hash is calculated over the following fields: ... If the extension header is not present ... """

So the device can choose not to set VIRTIO_NET_HASH_TYPE_TCP_EX as spec doesn't say device MUST set VIRTIO_NET_HASH_TYPE_TCP_EX if ...

2. implementation wise, since device has limited resources, we can't

expect the device can parse arbitrary number of ipv6 options

3. if 1) and 2) not the case, we need fix the spec otherwise implement

a spec compliant device is impractical

The statement is preceded by the following:

...

The device calculates the hash on IPv4 packets according to ’Enabled hash types’ bitmask as follows:

The 'Enabled hash types' bitmask is specified by the device.

I think the spec needs amendment.

Michael, can you help to clarify here?

...

I wonder if there are any people interested in the feature though. Looking at virtnet_set_hashflow() in drivers/net/virtio_net.c, the driver of Linux does not let users configure HASH_TYPE_XXX_EX. I suppose Windows supports HASH_TYPE_XXX_EX, but those who care network performance most would use Linux so HASH_TYPE_XXX_EX support without Linux driver's support may not be useful.

It might be still interesting for example for the hardware virtio vendors to support windows etc.

I don't know if Windows needs them for e.g., device/driver certification so surveying Windows makes sense.

Yuri, can you help to clarify this?

...

...

...

...

...

> 2) the VIRTIO_NET_SUPPORTED_HASH_TYPES is not checked against the > tun_vnet_ioctl_sethash(), so userspace may set > VIRTIO_NET_HASH_TYPE_TCP_EX regardless of what has been returned by > tun_vnet_ioctl_gethashtypes(). In this case they won't get > VIRTIO_NET_HASH_TYPE_TCP_EX.

That's right. It's the responsibility of the userspace to set only the supported hash types.

Well, the kernel should filter out the unsupported one to have a robust uAPI. Otherwise, we give green light to the buggy userspace which will have unexpected results.

My reasoning was that it may be fine for some use cases other than VM (e.g., DPDK); in such a use case, it is fine as long as the UAPI works in the best-effort basis.

Best-effort might increase the chance for user visisable changes after migration.

It is a trade-off between catching a migration bug for VMM and making life a bit easier for userspace programs other than VMM.

My understanding is to avoid breaking the migration compatibility as much as possible as fixing that would be complicated or even impossible.

That makes a point.

...

...

...

For example, suppose a userspace program that processes TCP packets; the program can enable: HASH_TYPE_IPv4, HASH_TYPE_TCPv4, HASH_TYPE_IPv6, and HASH_TYPE_TCPv6. Ideally, the kernel should support all the hash types, but, even if e.g., HASH_TYPE_TCPv6 is not available,

For "available" did you mean it is not supported by the device?

...

it will fall back to HASH_TYPE_IPv6, which still does something good and may be acceptable.

This fallback is exactly the same as I said above, let VIRTIO_NET_HASH_TYPE_TCP_EX to fallback.

My point is that, the implementation should either:

1. allow fallback so it can claim to support all hash types

or

2. don't allow fallback so it can only support a part of the hash types

If we're doing something in the middle, for example, allow part of the type to fallback.

1. or the middle will make it unsuitable for VM because it violates the

virtio spec. 2) makes sense though the trade-off I mentioned should be taken into consideration.

...

...

That said, for a use case that involves VM and implements virtio-net (e.g., QEMU), setting an unsupported hash type here is definitely a bug. Catching the bug may outweigh the extra trouble for other use cases.

...

...

> 3) implementing part of the hash types might complicate the migration > or at least we need to describe the expectations of libvirt or other > management in this case. For example, do we plan to have a dedicated > Qemu command line like: > > -device virtio-net-pci,hash_report=on,supported_hash_types=X,Y,Z?

I posted a patch series to implement such a command line for vDPA[1]. The patch series that wires this tuntap feature up[2] reuses the infrastructure so it doesn't bring additional complexity.

[1] https://lore.kernel.org/qemu-devel/20250530-vdpa-v1-0-5af4109b1c19@daynix.co... [2] https://lore.kernel.org/qemu-devel/20250530-hash-v5-0-343d7d7a8200@daynix.co...

I meant, if we implement a full hash report feature, it means a single hash cmdline option is more than sufficient and so compatibility code can just turn it off when dealing with machine types. This is much more simpler than

1. having both hash as well as supported_hash_features
2. dealing both hash as well as supported_hash_features in compatibility codes
3. libvirt will be happy

For [1], it seems it introduces a per has type option, this seems to be a burden to the management layer as it need to learn new option everytime a new hash type is supported

Even with the command line you proposed (supported_hash_types=X,Y,Z), it is still necessary to know the values the supported_hash_types property accepts (X.Y,Z), so I don't think it makes difference.

It could be a uint32_t.

The management layer will need to know what bits are accepted even with uint32_t.

Ease the management, basically it would be used by debugging or machine type only.

Users need to be able to specify hash types for the reasons same with virtio features (via domain XML for libvirt). In particular:

1) To ensure migration compatibility in case that hosts have different sets of supported hash types. This kind of situation can happen when vDPA devices have different capabilities or when the spec gains new hash types and a new version of kernel/tuntap starts to support it.

2) For debugging as you mentioned.

It makes sense to let users specify hash types with mnemonics instead of cryptic bitmasks. 2) is not an end-user requirement so it may be tolerable to expose bitmasks, but it is less so for 1). The management layer needs to have a list of hash types to support mnemonics whether QOM properties are uint32_t or not.

We can have an interface different from the one for features without worrying about backward compatibility, but this aspect of hash types is exactly same with features and does not motivate to do so.

Regards, Akihiko Odaki

...

...

...

The burden to the management layer is already present for features, so it is an existing problem (or its mere extension).

Yes, but since this feature is new it's better to try our best to avoid that.

...

This problem was discussed in the following thread in the past, but no solution is implemented yet, and probably solving it will be difficult. https://lore.kernel.org/qemu-devel/20230731223148.1002258-5-yuri.benditovich...

It's a similar issue but not the same, it looks more like a discussion on whether the fallback from vhost-net to qemu works for missing features etc.

Perhaps we may be able to do better since this feature is new as you say and we don't have to worry much about breaking change. I don't have an idea for that yet.

Right.

...

Regards, Akihiko Odaki

Thanks

On Fri, May 30, 2025 at 12:50 PM Akihiko Odaki akihiko.odaki@daynix.com wrote:

This clarifies a steering eBPF program takes precedence over the other steering algorithms.

Let's give an example on the use case for this.

Signed-off-by: Akihiko Odaki akihiko.odaki@daynix.com

Documentation/networking/tuntap.rst | 7 +++++++ drivers/net/tun.c | 28 +++++++++++++++++----------- include/uapi/linux/if_tun.h | 9 +++++++++ 3 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/Documentation/networking/tuntap.rst b/Documentation/networking/tuntap.rst index 4d7087f727be..86b4ae8caa8a 100644 --- a/Documentation/networking/tuntap.rst +++ b/Documentation/networking/tuntap.rst @@ -206,6 +206,13 @@ enable is true we enable it, otherwise we disable it:: return ioctl(fd, TUNSETQUEUE, (void *)&ifr); }

+3.4 Reference +-------------

+``linux/if_tun.h`` defines the interface described below:

+.. kernel-doc:: include/uapi/linux/if_tun.h

Universal TUN/TAP device driver Frequently Asked Question

diff --git a/drivers/net/tun.c b/drivers/net/tun.c index d8f4d3e996a7..9133ab9ed3f5 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -476,21 +476,29 @@ static u16 tun_automq_select_queue(struct tun_struct *tun, struct sk_buff *skb) return txq; }

-static u16 tun_ebpf_select_queue(struct tun_struct *tun, struct sk_buff *skb) +static bool tun_ebpf_select_queue(struct tun_struct *tun, struct sk_buff *skb,

•                             u16 *ret)
  

{ struct tun_prog *prog; u32 numqueues;

•   u16 ret = 0;
  

•   u32 prog_ret;
  

•   prog = rcu_dereference(tun->steering_prog);
  

•   if (!prog)
  

•           return false;
  
    numqueues = READ_ONCE(tun->numqueues);
  

•   if (!numqueues)
  

•           return 0;
  

•   if (!numqueues) {
  

•           *ret = 0;
  

•           return true;
  

•   }
  
  

•   prog = rcu_dereference(tun->steering_prog);
  

•   if (prog)
  

•           ret = bpf_prog_run_clear_cb(prog->prog, skb);
  

•   prog_ret = bpf_prog_run_clear_cb(prog->prog, skb);
  

•   if (prog_ret == TUN_STEERINGEBPF_FALLBACK)
  

•           return false;
  

This seems to break the uAPI. So I think we need a new ioctl to enable the behaviour

•   return ret % numqueues;
  

•   *ret = (u16)prog_ret % numqueues;
  

•   return true;
  

}

static u16 tun_select_queue(struct net_device *dev, struct sk_buff *skb, @@ -500,9 +508,7 @@ static u16 tun_select_queue(struct net_device *dev, struct sk_buff *skb, u16 ret;

    rcu_read_lock();

•   if (rcu_dereference(tun->steering_prog))
  

•           ret = tun_ebpf_select_queue(tun, skb);
  

•   else
  

•   if (!tun_ebpf_select_queue(tun, skb, &ret))
            ret = tun_automq_select_queue(tun, skb);
    rcu_read_unlock();
  
  

diff --git a/include/uapi/linux/if_tun.h b/include/uapi/linux/if_tun.h index 287cdc81c939..980de74724fc 100644 --- a/include/uapi/linux/if_tun.h +++ b/include/uapi/linux/if_tun.h @@ -115,4 +115,13 @@ struct tun_filter { __u8 addr[][ETH_ALEN]; };

+/**

• • define TUN_STEERINGEBPF_FALLBACK - A steering eBPF return value to fall back
• • A steering eBPF program may return this value to fall back to the steering
• • algorithm that should have been used if the program was not set. This allows
• • selectively overriding the steering decision.
• */

+#define TUN_STEERINGEBPF_FALLBACK -1

Not a native speaker, consider it works more like XDP_PASS, would it be better to use "TUN_STERRING_PASS"?

Thanks

#endif /* _UAPI__IF_TUN_H */

-- 2.49.0

On Wed, Jun 4, 2025 at 3:24 PM Akihiko Odaki akihiko.odaki@daynix.com wrote:

On 2025/06/04 10:27, Jason Wang wrote:

...

On Fri, May 30, 2025 at 12:50 PM Akihiko Odaki akihiko.odaki@daynix.com wrote:

...

This clarifies a steering eBPF program takes precedence over the other steering algorithms.

Let's give an example on the use case for this.

...

Signed-off-by: Akihiko Odaki akihiko.odaki@daynix.com

Documentation/networking/tuntap.rst | 7 +++++++ drivers/net/tun.c | 28 +++++++++++++++++----------- include/uapi/linux/if_tun.h | 9 +++++++++ 3 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/Documentation/networking/tuntap.rst b/Documentation/networking/tuntap.rst index 4d7087f727be..86b4ae8caa8a 100644 --- a/Documentation/networking/tuntap.rst +++ b/Documentation/networking/tuntap.rst @@ -206,6 +206,13 @@ enable is true we enable it, otherwise we disable it:: return ioctl(fd, TUNSETQUEUE, (void *)&ifr); }

+3.4 Reference +-------------

+``linux/if_tun.h`` defines the interface described below:

+.. kernel-doc:: include/uapi/linux/if_tun.h

• Universal TUN/TAP device driver Frequently Asked Question

diff --git a/drivers/net/tun.c b/drivers/net/tun.c index d8f4d3e996a7..9133ab9ed3f5 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -476,21 +476,29 @@ static u16 tun_automq_select_queue(struct tun_struct *tun, struct sk_buff *skb) return txq; }

-static u16 tun_ebpf_select_queue(struct tun_struct *tun, struct sk_buff *skb) +static bool tun_ebpf_select_queue(struct tun_struct *tun, struct sk_buff *skb,

•                             u16 *ret)
  

  { struct tun_prog *prog; u32 numqueues;

•   u16 ret = 0;
  

•   u32 prog_ret;
  

•   prog = rcu_dereference(tun->steering_prog);
  

•   if (!prog)
  

•           return false;
  
     numqueues = READ_ONCE(tun->numqueues);
  

•   if (!numqueues)
  

•           return 0;
  

•   if (!numqueues) {
  

•           *ret = 0;
  

•           return true;
  

•   }
  
  

•   prog = rcu_dereference(tun->steering_prog);
  

•   if (prog)
  

•           ret = bpf_prog_run_clear_cb(prog->prog, skb);
  

•   prog_ret = bpf_prog_run_clear_cb(prog->prog, skb);
  

•   if (prog_ret == TUN_STEERINGEBPF_FALLBACK)
  

•           return false;
  

This seems to break the uAPI. So I think we need a new ioctl to enable the behaviour

I assumed it is fine to repurpose one of the 32-bit integer values since 32-bit integer is too big to specify the queue number, but it may not be fine. I don't have a concrete use case either.

Perhaps it is safer to note that TUNSETSTEERINGEBPF takes precedence over TUNSETVNETRSS to allow such an extension in the future (but without implementing one now).

Yes, that's my original point. Let's start from something that is simpler and safer.

Thanks

On Fri, May 30, 2025 at 12:50 PM Akihiko Odaki akihiko.odaki@daynix.com wrote:

Add common code required for the features being added to TUN and TAP. They will be enabled for each of them in following patches.

Added Features

Hash reporting

Allow the guest to reuse the hash value to make receive steering consistent between the host and guest, and to save hash computation.

Receive Side Scaling (RSS)

RSS is a receive steering algorithm that can be negotiated to use with virtio_net. Conventionally the hash calculation was done by the VMM. However, computing the hash after the queue was chosen defeats the purpose of RSS.

Another approach is to use eBPF steering program. This approach has another downside: it cannot report the calculated hash due to the restrictive nature of eBPF steering program.

Introduce the code to perform RSS to the kernel in order to overcome thse challenges. An alternative solution is to extend the eBPF steering program so that it will be able to report to the userspace, but I didn't opt for it because extending the current mechanism of eBPF steering program as is because it relies on legacy context rewriting, and introducing kfunc-based eBPF will result in non-UAPI dependency while the other relevant virtualization APIs such as KVM and vhost_net are UAPIs.

Added ioctls

They are designed to make extensibility and VM migration compatible. This change only adds the implementation and does not expose them to the userspace.

TUNGETVNETHASHTYPES

This ioctl tells supported hash types. It is useful to check if a VM can be migrated to the current host.

TUNSETVNETREPORTINGAUTOMQ, TUNSETVNETREPORTINGRSS, and TUNSETVNETRSS

These ioctls configures a steering algorithm and, if needed, hash reporting.

Signed-off-by: Akihiko Odaki akihiko.odaki@daynix.com Tested-by: Lei Yang leiyang@redhat.com

---

drivers/net/tap.c | 10 ++- drivers/net/tun.c | 12 +++- drivers/net/tun_vnet.h | 165 +++++++++++++++++++++++++++++++++++++++++--- include/uapi/linux/if_tun.h | 71 +++++++++++++++++++ 4 files changed, 244 insertions(+), 14 deletions(-)

diff --git a/drivers/net/tap.c b/drivers/net/tap.c index d4ece538f1b2..25c60ff2d3f2 100644 --- a/drivers/net/tap.c +++ b/drivers/net/tap.c @@ -179,6 +179,11 @@ static void tap_put_queue(struct tap_queue *q) sock_put(&q->sk); }

+static const struct virtio_net_hash *tap_find_hash(const struct sk_buff *skb) +{

•   return NULL;
  

+}

/*

• Select a queue based on the rxq of the device on which this packet
• arrived. If the incoming device is not mq, calculate a flow hash

@@ -711,11 +716,12 @@ static ssize_t tap_put_user(struct tap_queue *q, int total;

    if (q->flags & IFF_VNET_HDR) {

•           struct virtio_net_hdr vnet_hdr;
  

•           struct virtio_net_hdr_v1_hash vnet_hdr;
  
            vnet_hdr_len = READ_ONCE(q->vnet_hdr_sz);
  
  

•           ret = tun_vnet_hdr_from_skb(q->flags, NULL, skb, &vnet_hdr);
  

•           ret = tun_vnet_hdr_from_skb(vnet_hdr_len, q->flags, NULL, skb,
  

•                                       tap_find_hash, &vnet_hdr);
            if (ret)
                    return ret;
  
  

diff --git a/drivers/net/tun.c b/drivers/net/tun.c index 9133ab9ed3f5..03d47799e9bd 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -451,6 +451,11 @@ static inline void tun_flow_save_rps_rxhash(struct tun_flow_entry *e, u32 hash) e->rps_rxhash = hash; }

+static const struct virtio_net_hash *tun_find_hash(const struct sk_buff *skb) +{

•   return NULL;
  

+}

/* We try to identify a flow through its rxhash. The reason that

• we do not check rxq no. is because some cards(e.g 82599), chooses
• the rxq based on the txq where the last packet of the flow comes. As

@@ -1993,7 +1998,7 @@ static ssize_t tun_put_user_xdp(struct tun_struct *tun, ssize_t ret;

    if (tun->flags & IFF_VNET_HDR) {

•           struct virtio_net_hdr gso = { 0 };
  

•           struct virtio_net_hdr_v1_hash gso = { 0 };
  
            vnet_hdr_sz = READ_ONCE(tun->vnet_hdr_sz);
            ret = tun_vnet_hdr_put(vnet_hdr_sz, iter, &gso);
  

@@ -2046,9 +2051,10 @@ static ssize_t tun_put_user(struct tun_struct *tun, }

    if (vnet_hdr_sz) {

•           struct virtio_net_hdr gso;
  

•           struct virtio_net_hdr_v1_hash gso;
  
  

•           ret = tun_vnet_hdr_from_skb(tun->flags, tun->dev, skb, &gso);
  

•           ret = tun_vnet_hdr_from_skb(vnet_hdr_sz, tun->flags, tun->dev,
  

•                                       skb, tun_find_hash, &gso);
            if (ret)
                    return ret;
  
  

diff --git a/drivers/net/tun_vnet.h b/drivers/net/tun_vnet.h index 58b9ac7a5fc4..45d0533efc8d 100644 --- a/drivers/net/tun_vnet.h +++ b/drivers/net/tun_vnet.h @@ -6,6 +6,17 @@ #define TUN_VNET_LE 0x80000000 #define TUN_VNET_BE 0x40000000

+typedef struct virtio_net_hash *(*tun_vnet_hash_add)(struct sk_buff *); +typedef const struct virtio_net_hash *(*tun_vnet_hash_find)(const struct sk_buff *);

+struct tun_vnet_hash {

•   bool report;
  

•   bool rss;
  

•   struct tun_vnet_rss common;
  

•   u32 rss_key[VIRTIO_NET_RSS_MAX_KEY_SIZE];
  

•   u16 rss_indirection_table[];
  

+};

static inline bool tun_vnet_legacy_is_little_endian(unsigned int flags) { bool be = IS_ENABLED(CONFIG_TUN_VNET_CROSS_LE) && @@ -107,6 +118,128 @@ static inline long tun_vnet_ioctl(int *vnet_hdr_sz, unsigned int *flags, } }

+static inline long tun_vnet_ioctl_gethashtypes(u32 __user *argp) +{

•   return put_user(VIRTIO_NET_SUPPORTED_HASH_TYPES, argp) ? -EFAULT : 0;
  

+}

+static inline long tun_vnet_ioctl_sethash(struct tun_vnet_hash __rcu **hashp,

•                                     unsigned int cmd,
  

•                                     void __user *argp)
  

+{

•   struct tun_vnet_rss common;
  

•   struct tun_vnet_hash *hash;
  

•   size_t indirection_table_size;
  

•   size_t key_size;
  

•   size_t size;
  

•   switch (cmd) {
  

•   case TUNSETVNETREPORTINGAUTOMQ:
  

•           if (get_user(common.hash_types, (u32 __user *)argp))
  

•                   return -EFAULT;
  

•           if (common.hash_types) {
  

•                   hash = kzalloc(sizeof(*hash), GFP_KERNEL);
  

•                   if (!hash)
  

•                           return -ENOMEM;
  

•                   hash->report = true;
  

•                   hash->common.hash_types = common.hash_types;
  

•           } else {
  

•                   hash = NULL;
  

•           }
  

•           break;
  

•   case TUNSETVNETREPORTINGRSS:
  

•   case TUNSETVNETRSS:
  

So the above three shows unnecessary design redundancy as well as a burden for the future extension. Why not simply have

1) TUNSETVNET_RSS 2) TUNSETVNET_HASH_REPORT ?

Which maps to

#define VIRTIO_NET_CTRL_MQ_RSS_CONFIG 1 (for configurable receive steering) #define VIRTIO_NET_CTRL_MQ_HASH_CONFIG 2 (for configurable hash calculation)

It would be always easier to start with what spec had or at least we need to explain why we choose a different design here or in the changelog to ease our life.

One day we would have tun_select_queue_algorithm_x() we don't have to duplicate the ioctls once again here like TUNSETVNETREPORTINGXYZ

•           if (copy_from_user(&common, argp, sizeof(common)))
  

•                   return -EFAULT;
  

•           argp = (struct tun_vnet_rss __user *)argp + 1;
  

•           indirection_table_size = ((size_t)common.indirection_table_mask + 1) * 2;
  

•           key_size = virtio_net_hash_key_length(common.hash_types);
  

•           size = struct_size(hash, rss_indirection_table,
  

•                              (size_t)common.indirection_table_mask + 1);
  

•           hash = kmalloc(size, GFP_KERNEL);
  

•           if (!hash)
  

•                   return -ENOMEM;
  

•           if (copy_from_user(hash->rss_indirection_table,
  

•                              argp, indirection_table_size)) {
  

•                   kfree(hash);
  

•                   return -EFAULT;
  

•           }
  

•           argp = (u16 __user *)argp + common.indirection_table_mask + 1;
  

•           if (copy_from_user(hash->rss_key, argp, key_size)) {
  

•                   kfree(hash);
  

•                   return -EFAULT;
  

•           }
  

•           virtio_net_toeplitz_convert_key(hash->rss_key, key_size);
  

•           hash->report = cmd == TUNSETVNETREPORTINGRSS;
  

At least, if this is the only difference why not simply code this into the ioctl itself other than having a very similar command?

•           hash->rss = true;
  

•           hash->common = common;
  

•           break;
  

•   default:
  

•           return -EINVAL;
  

•   }
  

•   kfree_rcu_mightsleep(rcu_replace_pointer_rtnl(*hashp, hash));
  

•   return 0;
  

+}

+static inline void tun_vnet_hash_report(const struct tun_vnet_hash *hash,

•                                   struct sk_buff *skb,
  

•                                   const struct flow_keys_basic *keys,
  

•                                   u32 value,
  

•                                   tun_vnet_hash_add vnet_hash_add)
  

+{

•   struct virtio_net_hash *report;
  

•   if (!hash || !hash->report)
  

•           return;
  

•   report = vnet_hash_add(skb);
  

•   if (!report)
  

•           return;
  

•   *report = (struct virtio_net_hash) {
  

•           .report = virtio_net_hash_report(hash->common.hash_types, keys),
  

•           .value = value
  

•   };
  

+}

+static inline u16 tun_vnet_rss_select_queue(u32 numqueues,

•                                       const struct tun_vnet_hash *hash,
  

•                                       struct sk_buff *skb,
  

•                                       tun_vnet_hash_add vnet_hash_add)
  

+{

•   struct virtio_net_hash *report;
  

•   struct virtio_net_hash ret;
  

•   u16 index;
  

•   if (!numqueues)
  

•           return 0;
  

•   virtio_net_hash_rss(skb, hash->common.hash_types, hash->rss_key, &ret);
  

•   if (!ret.report)
  

•           return hash->common.unclassified_queue % numqueues;
  

•   if (hash->report) {
  

•           report = vnet_hash_add(skb);
  

•           if (report)
  

•                   *report = ret;
  

•   }
  

•   index = ret.value & hash->common.indirection_table_mask;
  

•   return hash->rss_indirection_table[index] % numqueues;
  

+}

static inline int tun_vnet_hdr_get(int sz, unsigned int flags, struct iov_iter *from, struct virtio_net_hdr *hdr) @@ -135,15 +268,17 @@ static inline int tun_vnet_hdr_get(int sz, unsigned int flags, }

static inline int tun_vnet_hdr_put(int sz, struct iov_iter *iter,

•                              const struct virtio_net_hdr *hdr)
  

•                              const struct virtio_net_hdr_v1_hash *hdr)
  

{

•   int content_sz = MIN(sizeof(*hdr), sz);
  

•   if (unlikely(iov_iter_count(iter) < sz))
            return -EINVAL;
  
  

•   if (unlikely(copy_to_iter(hdr, sizeof(*hdr), iter) != sizeof(*hdr)))
  

•   if (unlikely(copy_to_iter(hdr, content_sz, iter) != content_sz))
            return -EFAULT;
  
  

•   if (iov_iter_zero(sz - sizeof(*hdr), iter) != sz - sizeof(*hdr))
  

•   if (iov_iter_zero(sz - content_sz, iter) != sz - content_sz)
            return -EFAULT;
  
    return 0;
  

@@ -155,26 +290,38 @@ static inline int tun_vnet_hdr_to_skb(unsigned int flags, struct sk_buff *skb, return virtio_net_hdr_to_skb(skb, hdr, tun_vnet_is_little_endian(flags)); }

-static inline int tun_vnet_hdr_from_skb(unsigned int flags, +static inline int tun_vnet_hdr_from_skb(int sz, unsigned int flags, const struct net_device *dev, const struct sk_buff *skb,

•                                   struct virtio_net_hdr *hdr)
  

•                                   tun_vnet_hash_find vnet_hash_find,
  

•                                   struct virtio_net_hdr_v1_hash *hdr)
  

{ int vlan_hlen = skb_vlan_tag_present(skb) ? VLAN_HLEN : 0;

•   const struct virtio_net_hash *report = sz < sizeof(struct virtio_net_hdr_v1_hash) ?
  

•                                          NULL : vnet_hash_find(skb);
  

•   *hdr = (struct virtio_net_hdr_v1_hash) {
  

•           .hash_report = VIRTIO_NET_HASH_REPORT_NONE
  

•   };
  

•   if (report) {
  

•           hdr->hash_value = cpu_to_le32(report->value);
  

•           hdr->hash_report = cpu_to_le16(report->report);
  

•   }
  
  

•   if (virtio_net_hdr_from_skb(skb, hdr,
  

•   if (virtio_net_hdr_from_skb(skb, (struct virtio_net_hdr *)hdr,
                                tun_vnet_is_little_endian(flags), true,
                                vlan_hlen)) {
            struct skb_shared_info *sinfo = skb_shinfo(skb);
  
            if (net_ratelimit()) {
                    netdev_err(dev, "unexpected GSO type: 0x%x, gso_size %d, hdr_len %d\n",
  

•                              sinfo->gso_type, tun_vnet16_to_cpu(flags, hdr->gso_size),
  

•                              tun_vnet16_to_cpu(flags, hdr->hdr_len));
  

•                              sinfo->gso_type, tun_vnet16_to_cpu(flags, hdr->hdr.gso_size),
  

•                              tun_vnet16_to_cpu(flags, hdr->hdr.hdr_len));
                    print_hex_dump(KERN_ERR, "tun: ",
                                   DUMP_PREFIX_NONE,
                                   16, 1, skb->head,
  

•                                  min(tun_vnet16_to_cpu(flags, hdr->hdr_len), 64), true);
  

•                                  min(tun_vnet16_to_cpu(flags, hdr->hdr.hdr_len), 64), true);
            }
            WARN_ON_ONCE(1);
            return -EINVAL;
  

diff --git a/include/uapi/linux/if_tun.h b/include/uapi/linux/if_tun.h index 980de74724fc..fe4b984d3bbb 100644 --- a/include/uapi/linux/if_tun.h +++ b/include/uapi/linux/if_tun.h @@ -62,6 +62,62 @@ #define TUNSETCARRIER _IOW('T', 226, int) #define TUNGETDEVNETNS _IO('T', 227)

+/**

• • define TUNGETVNETHASHTYPES - ioctl to get supported virtio_net hashing types
• • The argument is a pointer to __u32 which will store the supported virtio_net
• • hashing types.
• */

+#define TUNGETVNETHASHTYPES _IOR('T', 228, __u32)

+/**

• • define TUNSETVNETREPORTINGAUTOMQ - ioctl to enable automq with hash reporting
• • Disable RSS and enable automatic receive steering with hash reporting.
• • The argument is a pointer to __u32 that contains a bitmask of hash types
• • allowed to be reported.
• • This ioctl results in %EBADFD if the underlying device is deleted. It affects
• • all queues attached to the same device.
• • This ioctl currently has no effect on XDP packets and packets with
• • queue_mapping set by TC.
• */

+#define TUNSETVNETREPORTINGAUTOMQ _IOR('T', 229, __u32)

+/**

• • define TUNSETVNETREPORTINGRSS - ioctl to enable RSS with hash reporting
• • Disable automatic receive steering and enable RSS with hash reporting.

This is unnecessary, e.g one day will have select_queue_xyz(), we don't want to say "Disable automatic receive steering and xyz ..."

• • This ioctl results in %EBADFD if the underlying device is deleted. It affects
• • all queues attached to the same device.
• • This ioctl currently has no effect on XDP packets and packets with
• • queue_mapping set by TC.
• */

+#define TUNSETVNETREPORTINGRSS _IOR('T', 230, struct tun_vnet_rss)

+/**

• • define TUNSETVNETRSS - ioctl to enable RSS without hash reporting
• • Disable automatic receive steering and enable RSS without hash reporting.

Same here.

• • The argument is a pointer to the compound of the following in order:
• • 1. &struct tun_vnet_rss
• • 3. Indirection table
• • 4. Key
• • This ioctl results in %EBADFD if the underlying device is deleted. It affects
• • all queues attached to the same device.
• • This ioctl currently has no effect on XDP packets and packets with
• • queue_mapping set by TC.
• */

+#define TUNSETVNETRSS _IOR('T', 231, struct tun_vnet_rss)

/* TUNSETIFF ifr flags */ #define IFF_TUN 0x0001 #define IFF_TAP 0x0002 @@ -124,4 +180,19 @@ struct tun_filter { */ #define TUN_STEERINGEBPF_FALLBACK -1

+/**

• • struct tun_vnet_rss - virtio_net RSS configuration
• • @hash_types:

• •         Bitmask of allowed hash types
    

• • @indirection_table_mask:

• •         Bitmask to be applied to the indirection table index
    

• • @unclassified_queue:

• •         The index of the queue to place unclassified packets in
    

• */

+struct tun_vnet_rss {

•   __u32 hash_types;
  

•   __u16 indirection_table_mask;
  

•   __u16 unclassified_queue;
  

+};

#endif /* _UAPI__IF_TUN_H */

-- 2.49.0

Thanks

On Wed, Jun 4, 2025 at 4:42 PM Akihiko Odaki akihiko.odaki@daynix.com wrote:

On 2025/06/04 10:53, Jason Wang wrote:

...

On Fri, May 30, 2025 at 12:50 PM Akihiko Odaki akihiko.odaki@daynix.com wrote:

...

Add common code required for the features being added to TUN and TAP. They will be enabled for each of them in following patches.

Added Features

Hash reporting

Allow the guest to reuse the hash value to make receive steering consistent between the host and guest, and to save hash computation.

Receive Side Scaling (RSS)

RSS is a receive steering algorithm that can be negotiated to use with virtio_net. Conventionally the hash calculation was done by the VMM. However, computing the hash after the queue was chosen defeats the purpose of RSS.

Another approach is to use eBPF steering program. This approach has another downside: it cannot report the calculated hash due to the restrictive nature of eBPF steering program.

Introduce the code to perform RSS to the kernel in order to overcome thse challenges. An alternative solution is to extend the eBPF steering program so that it will be able to report to the userspace, but I didn't opt for it because extending the current mechanism of eBPF steering program as is because it relies on legacy context rewriting, and introducing kfunc-based eBPF will result in non-UAPI dependency while the other relevant virtualization APIs such as KVM and vhost_net are UAPIs.

Added ioctls

They are designed to make extensibility and VM migration compatible. This change only adds the implementation and does not expose them to the userspace.

TUNGETVNETHASHTYPES

This ioctl tells supported hash types. It is useful to check if a VM can be migrated to the current host.

TUNSETVNETREPORTINGAUTOMQ, TUNSETVNETREPORTINGRSS, and TUNSETVNETRSS

These ioctls configures a steering algorithm and, if needed, hash reporting.

Signed-off-by: Akihiko Odaki akihiko.odaki@daynix.com Tested-by: Lei Yang leiyang@redhat.com

---

drivers/net/tap.c | 10 ++- drivers/net/tun.c | 12 +++- drivers/net/tun_vnet.h | 165 +++++++++++++++++++++++++++++++++++++++++--- include/uapi/linux/if_tun.h | 71 +++++++++++++++++++ 4 files changed, 244 insertions(+), 14 deletions(-)

diff --git a/drivers/net/tap.c b/drivers/net/tap.c index d4ece538f1b2..25c60ff2d3f2 100644 --- a/drivers/net/tap.c +++ b/drivers/net/tap.c @@ -179,6 +179,11 @@ static void tap_put_queue(struct tap_queue *q) sock_put(&q->sk); }

+static const struct virtio_net_hash *tap_find_hash(const struct sk_buff *skb) +{

•   return NULL;
  

+}

• /*
  • Select a queue based on the rxq of the device on which this packet
  • arrived. If the incoming device is not mq, calculate a flow hash

@@ -711,11 +716,12 @@ static ssize_t tap_put_user(struct tap_queue *q, int total;

     if (q->flags & IFF_VNET_HDR) {

•           struct virtio_net_hdr vnet_hdr;
  

•           struct virtio_net_hdr_v1_hash vnet_hdr;
  
             vnet_hdr_len = READ_ONCE(q->vnet_hdr_sz);
  
  

•           ret = tun_vnet_hdr_from_skb(q->flags, NULL, skb, &vnet_hdr);
  

•           ret = tun_vnet_hdr_from_skb(vnet_hdr_len, q->flags, NULL, skb,
  

•                                       tap_find_hash, &vnet_hdr);
             if (ret)
                     return ret;
  
  

diff --git a/drivers/net/tun.c b/drivers/net/tun.c index 9133ab9ed3f5..03d47799e9bd 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -451,6 +451,11 @@ static inline void tun_flow_save_rps_rxhash(struct tun_flow_entry *e, u32 hash) e->rps_rxhash = hash; }

+static const struct virtio_net_hash *tun_find_hash(const struct sk_buff *skb) +{

•   return NULL;
  

+}

• /* We try to identify a flow through its rxhash. The reason that
  • we do not check rxq no. is because some cards(e.g 82599), chooses
  • the rxq based on the txq where the last packet of the flow comes. As

@@ -1993,7 +1998,7 @@ static ssize_t tun_put_user_xdp(struct tun_struct *tun, ssize_t ret;

     if (tun->flags & IFF_VNET_HDR) {

•           struct virtio_net_hdr gso = { 0 };
  

•           struct virtio_net_hdr_v1_hash gso = { 0 };
  
             vnet_hdr_sz = READ_ONCE(tun->vnet_hdr_sz);
             ret = tun_vnet_hdr_put(vnet_hdr_sz, iter, &gso);
  

@@ -2046,9 +2051,10 @@ static ssize_t tun_put_user(struct tun_struct *tun, }

     if (vnet_hdr_sz) {

•           struct virtio_net_hdr gso;
  

•           struct virtio_net_hdr_v1_hash gso;
  
  

•           ret = tun_vnet_hdr_from_skb(tun->flags, tun->dev, skb, &gso);
  

•           ret = tun_vnet_hdr_from_skb(vnet_hdr_sz, tun->flags, tun->dev,
  

•                                       skb, tun_find_hash, &gso);
             if (ret)
                     return ret;
  
  

diff --git a/drivers/net/tun_vnet.h b/drivers/net/tun_vnet.h index 58b9ac7a5fc4..45d0533efc8d 100644 --- a/drivers/net/tun_vnet.h +++ b/drivers/net/tun_vnet.h @@ -6,6 +6,17 @@ #define TUN_VNET_LE 0x80000000 #define TUN_VNET_BE 0x40000000

+typedef struct virtio_net_hash *(*tun_vnet_hash_add)(struct sk_buff *); +typedef const struct virtio_net_hash *(*tun_vnet_hash_find)(const struct sk_buff *);

+struct tun_vnet_hash {

•   bool report;
  

•   bool rss;
  

•   struct tun_vnet_rss common;
  

•   u32 rss_key[VIRTIO_NET_RSS_MAX_KEY_SIZE];
  

•   u16 rss_indirection_table[];
  

+};

• static inline bool tun_vnet_legacy_is_little_endian(unsigned int flags) { bool be = IS_ENABLED(CONFIG_TUN_VNET_CROSS_LE) &&

@@ -107,6 +118,128 @@ static inline long tun_vnet_ioctl(int *vnet_hdr_sz, unsigned int *flags, } }

+static inline long tun_vnet_ioctl_gethashtypes(u32 __user *argp) +{

•   return put_user(VIRTIO_NET_SUPPORTED_HASH_TYPES, argp) ? -EFAULT : 0;
  

+}

+static inline long tun_vnet_ioctl_sethash(struct tun_vnet_hash __rcu **hashp,

•                                     unsigned int cmd,
  

•                                     void __user *argp)
  

+{

•   struct tun_vnet_rss common;
  

•   struct tun_vnet_hash *hash;
  

•   size_t indirection_table_size;
  

•   size_t key_size;
  

•   size_t size;
  

•   switch (cmd) {
  

•   case TUNSETVNETREPORTINGAUTOMQ:
  

•           if (get_user(common.hash_types, (u32 __user *)argp))
  

•                   return -EFAULT;
  

•           if (common.hash_types) {
  

•                   hash = kzalloc(sizeof(*hash), GFP_KERNEL);
  

•                   if (!hash)
  

•                           return -ENOMEM;
  

•                   hash->report = true;
  

•                   hash->common.hash_types = common.hash_types;
  

•           } else {
  

•                   hash = NULL;
  

•           }
  

•           break;
  

•   case TUNSETVNETREPORTINGRSS:
  

•   case TUNSETVNETRSS:
  

So the above three shows unnecessary design redundancy as well as a burden for the future extension. Why not simply have

1. TUNSETVNET_RSS
2. TUNSETVNET_HASH_REPORT

?

Which maps to

#define VIRTIO_NET_CTRL_MQ_RSS_CONFIG 1 (for configurable receive steering) #define VIRTIO_NET_CTRL_MQ_HASH_CONFIG 2 (for configurable hash calculation)

It would be always easier to start with what spec had or at least we need to explain why we choose a different design here or in the changelog to ease our life.

TUNSETVNETREPORTINGAUTOMQ maps to VIRTIO_NET_CTRL_MQ_HASH_CONFIG.

It's not:

VIRTIO_NET_CTRL_MQ_HASH_CONFIG uses:

struct virtio_net_hash_config { le32 hash_types; le16 reserved[4]; u8 hash_key_length; u8 hash_key_data[hash_key_length]; };

but TUNSETVNETREPORTINGAUTOMQ only accepts hash_types without others:

TUNSETVNETREPORTINGRSS and TUNSETVNETRSS map to VIRTIO_NET_CTRL_MQ_RSS_CONFIG.

I think we've already had a discussion about this.

Reusing virtio-net uAPI is much better instead of having a tun specific one considering tun may need to support more virtio commands in the future. Or maybe it's the time to introduce a transport for the virtio control virtqueue uAPI in tuntap to avoid inventing new uAPI endlessly.

What's more I see:

struct tun_vnet_rss { __u32 hash_types; __u16 indirection_table_mask; __u16 unclassified_queue; };

struct tun_vnet_hash { bool report; bool rss; struct tun_vnet_rss common; u32 rss_key[VIRTIO_NET_RSS_MAX_KEY_SIZE]; u16 rss_indirection_table[]; };

As I pointed out in the past, let's just decouple the rss from hash, everything would be much simpler, or you need to explain why you couple this somewhere.

For example:

1) why is the tun_vnet_hash not part of the uAPI but tun_vnet_rss, or how could userspace know what kind of format it would use for TUNSETVNETREPORTINGRSS? 2) what's the advantages of embedding rss specific stuff into hash report structure 3) what's the advantages of not using virtio-net uAPI

More issues:

1) max_tx_vq is ignored, so do you expect the userspace to intercept this and switch to using TUNSETQUEUE? This seems like a burden as TUN can simply accept it and do the attaching/detaching by itself 2) the rx depends on the indirection table, so userspace need to intercept the indirection and change the rx queue numbers accordingly 3) RSS allows a async TX/RX queue, this is not supported by TUN now, no matter if we decide to let userspace to intercept max_tx_vq or not, we need to implement it first

Things would be much more simpler, if kernel can do 1) and 2).

We have two ioctls here because VIRTIO_NET_CTRL_MQ_RSS_CONFIG behaves differently depending on whether VIRTIO_NET_F_HASH_REPORT is negotiated or not;

It wouldn't be a problem if you do 1:1 mapping between virtio commands and TUN uAPI, otherwise it should have a bug somewhere.

it also enables hash reporting if the feature is negotiated.

Again, starting from virtio-net command is easier, a strong justification is needed to explain why we choose another for tun/tap.

...

One day we would have tun_select_queue_algorithm_x() we don't have to duplicate the ioctls once again here like TUNSETVNETREPORTINGXYZ

5.1.6.5.6.4 Hash calculation says:

...

If VIRTIO_NET_F_HASH_REPORT was negotiated and the device uses automatic receive steering, the device MUST support a command to configure hash calculation parameters.

The driver provides parameters for hash calculation as follows:

class VIRTIO_NET_CTRL_MQ, command VIRTIO_NET_CTRL_MQ_HASH_CONFIG.

VIRTIO_NET_CTRL_MQ_HASH_CONFIG is for automatic receive steering and not for RSS (or other steering algorithms if the spec gets any in the future).

I'm not sure but the spec needs some tweaking. For example, I don't expect there would be a dedicated hash config command for flow filters in the future. I think the reason why spec says like this is that virtio-net only supports automatic receive steering.

Note that macvtap doesn't implement automatic receive steering.

...

...

•           if (copy_from_user(&common, argp, sizeof(common)))
  

•                   return -EFAULT;
  

•           argp = (struct tun_vnet_rss __user *)argp + 1;
  

•           indirection_table_size = ((size_t)common.indirection_table_mask + 1) * 2;
  

•           key_size = virtio_net_hash_key_length(common.hash_types);
  

•           size = struct_size(hash, rss_indirection_table,
  

•                              (size_t)common.indirection_table_mask + 1);
  

•           hash = kmalloc(size, GFP_KERNEL);
  

•           if (!hash)
  

•                   return -ENOMEM;
  

•           if (copy_from_user(hash->rss_indirection_table,
  

•                              argp, indirection_table_size)) {
  

•                   kfree(hash);
  

•                   return -EFAULT;
  

•           }
  

•           argp = (u16 __user *)argp + common.indirection_table_mask + 1;
  

•           if (copy_from_user(hash->rss_key, argp, key_size)) {
  

•                   kfree(hash);
  

•                   return -EFAULT;
  

•           }
  

•           virtio_net_toeplitz_convert_key(hash->rss_key, key_size);
  

•           hash->report = cmd == TUNSETVNETREPORTINGRSS;
  

At least, if this is the only difference why not simply code this into the ioctl itself other than having a very similar command?

It is what the previous version did. Either is fine I guess; the only practical difference would be the size of the configuration struct is smaller with this approach.

...

...

•           hash->rss = true;
  

•           hash->common = common;
  

•           break;
  

•   default:
  

•           return -EINVAL;
  

•   }
  

•   kfree_rcu_mightsleep(rcu_replace_pointer_rtnl(*hashp, hash));
  

•   return 0;
  

+}

+static inline void tun_vnet_hash_report(const struct tun_vnet_hash *hash,

•                                   struct sk_buff *skb,
  

•                                   const struct flow_keys_basic *keys,
  

•                                   u32 value,
  

•                                   tun_vnet_hash_add vnet_hash_add)
  

+{

•   struct virtio_net_hash *report;
  

•   if (!hash || !hash->report)
  

•           return;
  

•   report = vnet_hash_add(skb);
  

•   if (!report)
  

•           return;
  

•   *report = (struct virtio_net_hash) {
  

•           .report = virtio_net_hash_report(hash->common.hash_types, keys),
  

•           .value = value
  

•   };
  

+}

+static inline u16 tun_vnet_rss_select_queue(u32 numqueues,

•                                       const struct tun_vnet_hash *hash,
  

•                                       struct sk_buff *skb,
  

•                                       tun_vnet_hash_add vnet_hash_add)
  

+{

•   struct virtio_net_hash *report;
  

•   struct virtio_net_hash ret;
  

•   u16 index;
  

•   if (!numqueues)
  

•           return 0;
  

•   virtio_net_hash_rss(skb, hash->common.hash_types, hash->rss_key, &ret);
  

•   if (!ret.report)
  

•           return hash->common.unclassified_queue % numqueues;
  

•   if (hash->report) {
  

•           report = vnet_hash_add(skb);
  

•           if (report)
  

•                   *report = ret;
  

•   }
  

•   index = ret.value & hash->common.indirection_table_mask;
  

•   return hash->rss_indirection_table[index] % numqueues;
  

+}

• static inline int tun_vnet_hdr_get(int sz, unsigned int flags, struct iov_iter *from, struct virtio_net_hdr *hdr)

@@ -135,15 +268,17 @@ static inline int tun_vnet_hdr_get(int sz, unsigned int flags, }

static inline int tun_vnet_hdr_put(int sz, struct iov_iter *iter,

•                              const struct virtio_net_hdr *hdr)
  

•                              const struct virtio_net_hdr_v1_hash *hdr)
  

  {

•   int content_sz = MIN(sizeof(*hdr), sz);
  

•    if (unlikely(iov_iter_count(iter) < sz))
             return -EINVAL;
  
  

•   if (unlikely(copy_to_iter(hdr, sizeof(*hdr), iter) != sizeof(*hdr)))
  

•   if (unlikely(copy_to_iter(hdr, content_sz, iter) != content_sz))
             return -EFAULT;
  
  

•   if (iov_iter_zero(sz - sizeof(*hdr), iter) != sz - sizeof(*hdr))
  

•   if (iov_iter_zero(sz - content_sz, iter) != sz - content_sz)
             return -EFAULT;
  
     return 0;
  

@@ -155,26 +290,38 @@ static inline int tun_vnet_hdr_to_skb(unsigned int flags, struct sk_buff *skb, return virtio_net_hdr_to_skb(skb, hdr, tun_vnet_is_little_endian(flags)); }

-static inline int tun_vnet_hdr_from_skb(unsigned int flags, +static inline int tun_vnet_hdr_from_skb(int sz, unsigned int flags, const struct net_device *dev, const struct sk_buff *skb,

•                                   struct virtio_net_hdr *hdr)
  

•                                   tun_vnet_hash_find vnet_hash_find,
  

•                                   struct virtio_net_hdr_v1_hash *hdr)
  

  { int vlan_hlen = skb_vlan_tag_present(skb) ? VLAN_HLEN : 0;

•   const struct virtio_net_hash *report = sz < sizeof(struct virtio_net_hdr_v1_hash) ?
  

•                                          NULL : vnet_hash_find(skb);
  

•   *hdr = (struct virtio_net_hdr_v1_hash) {
  

•           .hash_report = VIRTIO_NET_HASH_REPORT_NONE
  

•   };
  

•   if (report) {
  

•           hdr->hash_value = cpu_to_le32(report->value);
  

•           hdr->hash_report = cpu_to_le16(report->report);
  

•   }
  
  

•   if (virtio_net_hdr_from_skb(skb, hdr,
  

•   if (virtio_net_hdr_from_skb(skb, (struct virtio_net_hdr *)hdr,
                                 tun_vnet_is_little_endian(flags), true,
                                 vlan_hlen)) {
             struct skb_shared_info *sinfo = skb_shinfo(skb);
  
             if (net_ratelimit()) {
                     netdev_err(dev, "unexpected GSO type: 0x%x, gso_size %d, hdr_len %d\n",
  

•                              sinfo->gso_type, tun_vnet16_to_cpu(flags, hdr->gso_size),
  

•                              tun_vnet16_to_cpu(flags, hdr->hdr_len));
  

•                              sinfo->gso_type, tun_vnet16_to_cpu(flags, hdr->hdr.gso_size),
  

•                              tun_vnet16_to_cpu(flags, hdr->hdr.hdr_len));
                     print_hex_dump(KERN_ERR, "tun: ",
                                    DUMP_PREFIX_NONE,
                                    16, 1, skb->head,
  

•                                  min(tun_vnet16_to_cpu(flags, hdr->hdr_len), 64), true);
  

•                                  min(tun_vnet16_to_cpu(flags, hdr->hdr.hdr_len), 64), true);
             }
             WARN_ON_ONCE(1);
             return -EINVAL;
  

diff --git a/include/uapi/linux/if_tun.h b/include/uapi/linux/if_tun.h index 980de74724fc..fe4b984d3bbb 100644 --- a/include/uapi/linux/if_tun.h +++ b/include/uapi/linux/if_tun.h @@ -62,6 +62,62 @@ #define TUNSETCARRIER _IOW('T', 226, int) #define TUNGETDEVNETNS _IO('T', 227)

+/**

• • define TUNGETVNETHASHTYPES - ioctl to get supported virtio_net hashing types
• • The argument is a pointer to __u32 which will store the supported virtio_net
• • hashing types.
• */

+#define TUNGETVNETHASHTYPES _IOR('T', 228, __u32)

+/**

• • define TUNSETVNETREPORTINGAUTOMQ - ioctl to enable automq with hash reporting
• • Disable RSS and enable automatic receive steering with hash reporting.
• • The argument is a pointer to __u32 that contains a bitmask of hash types
• • allowed to be reported.
• • This ioctl results in %EBADFD if the underlying device is deleted. It affects
• • all queues attached to the same device.
• • This ioctl currently has no effect on XDP packets and packets with
• • queue_mapping set by TC.
• */

+#define TUNSETVNETREPORTINGAUTOMQ _IOR('T', 229, __u32)

+/**

• • define TUNSETVNETREPORTINGRSS - ioctl to enable RSS with hash reporting
• • Disable automatic receive steering and enable RSS with hash reporting.

This is unnecessary, e.g one day will have select_queue_xyz(), we don't want to say "Disable automatic receive steering and xyz ..."

It is still something better to be documented as its behavior is somewhat complicated.

This is a hint of uAPI design issue.

Concretely, this ioctl disables automatic receive steering but doesn't disable steering by eBPF, which is implied by TUN_STEERINGEBPF_FALLBACK.

It would be simpler:

1) not having TUN_STEERINGEBPF_FALLBACK 2) the steering algorithm depends on the last uAPI call

Perhaps it may be rephrased to tell it disables "the other receive steering mechanism except eBPF".

Regards, Akihiko Odaki

...

Thanks