diff mbox series

[RFC,v3,2/9] virtio_net: Add functions for hashing

Message ID 20240915-rss-v3-2-c630015db082@daynix.com (mailing list archive)
State Superseded
Headers show
Series tun: Introduce virtio-net hashing feature | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Guessed tree name to be net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 16 this patch: 16
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 2 maintainers not CCed: virtualization@lists.linux.dev eperezma@redhat.com
netdev/build_clang success Errors and warnings before: 16 this patch: 16
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 18 this patch: 18
netdev/checkpatch warning WARNING: line length of 85 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Akihiko Odaki Sept. 15, 2024, 1:17 a.m. UTC 
They are useful to implement VIRTIO_NET_F_RSS and
VIRTIO_NET_F_HASH_REPORT.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 include/linux/virtio_net.h | 198 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 198 insertions(+)

Comments

Gur Stavi Sept. 16, 2024, 7:12 a.m. UTC | #1 
+
+static inline void virtio_net_toeplitz(struct virtio_net_toeplitz_state *state,
+				       const __be32 *input, size_t len)

The function calculates a hash value but its name does not make it
clear. Consider adding a 'calc'.

+{
+	u32 key;
+
+	while (len) {
+		state->key++;
+		key = be32_to_cpu(*state->key);

You perform be32_to_cpu to support both CPU endianities.
If you will follow with an unconditional swab32, you could run the
following loop on a more natural 0 to 31 always referring to bit 0
and avoiding !!(key & bit):

key = swab32(be32_to_cpu(*state->key));
for (i = 0; i < 32; i++, key >>= 1) {
	if (be32_to_cpu(*input) & 1)
		state->hash ^= state->key_buffer;
	state->key_buffer = (state->key_buffer << 1) | (key & 1);
}


+
+		for (u32 bit = BIT(31); bit; bit >>= 1) {
+			if (be32_to_cpu(*input) & bit)
+				state->hash ^= state->key_buffer;
+
+			state->key_buffer =
+				(state->key_buffer << 1) | !!(key & bit);
+		}
+
+		input++;
+		len--;
+	}
+}
+
+static inline u32 virtio_net_hash_report(u32 types,
+					 struct flow_dissector_key_basic key)
+{
+	switch (key.n_proto) {
+	case htons(ETH_P_IP):

Other parts of the code use be_to_cpu and cpu_to_be, Why use legacy
htons here?

+		if (key.ip_proto == IPPROTO_TCP &&
+		    (types & VIRTIO_NET_RSS_HASH_TYPE_TCPv4))
+			return VIRTIO_NET_HASH_REPORT_TCPv4;
+
+		if (key.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 htons(ETH_P_IPV6):
+		if (key.ip_proto == IPPROTO_TCP &&
+		    (types & VIRTIO_NET_RSS_HASH_TYPE_TCPv6))
+			return VIRTIO_NET_HASH_REPORT_TCPv6;
+
+		if (key.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;
+	}
+}
 #endif /* _LINUX_VIRTIO_NET_H */
Gur Stavi Sept. 16, 2024, 8:01 a.m. UTC | #2 
> +
> +static inline void virtio_net_toeplitz(struct virtio_net_toeplitz_state *state,
> +				       const __be32 *input, size_t len)
> 
> The function calculates a hash value but its name does not make it
> clear. Consider adding a 'calc'.
> 
> +{
> +	u32 key;
> +
> +	while (len) {
> +		state->key++;
> +		key = be32_to_cpu(*state->key);
> 
> You perform be32_to_cpu to support both CPU endianities.
> If you will follow with an unconditional swab32, you could run the
> following loop on a more natural 0 to 31 always referring to bit 0
> and avoiding !!(key & bit):
> 
> key = swab32(be32_to_cpu(*state->key));
> for (i = 0; i < 32; i++, key >>= 1) {
> 	if (be32_to_cpu(*input) & 1)
> 		state->hash ^= state->key_buffer;
> 	state->key_buffer = (state->key_buffer << 1) | (key & 1);
> }
> 

Fixing myself, in previous version 'input' was tested against same bit.
Advantage is less clear now, replacing !! with extra shift.
However, since little endian CPUs are more common, the combination of
swab32(be32_to_cpu(x) will actually become a nop.
Similar tactic may be applied to 'input' by assigning it to local
variable. This may produce more efficient version but not necessary
easier to understand.

key = bswap32(be32_to_cpu(*state->key));
for (u32 bit = BIT(31); bit; bit >>= 1, key >>= 1) {
	if (be32_to_cpu(*input) & bit)
		state->hash ^= state->key_buffer;
	state->key_buffer =
		(state->key_buffer << 1) | (key & 1);
}

> 
> +
> +		for (u32 bit = BIT(31); bit; bit >>= 1) {
> +			if (be32_to_cpu(*input) & bit)
> +				state->hash ^= state->key_buffer;
> +
> +			state->key_buffer =
> +				(state->key_buffer << 1) | !!(key & bit);
> +		}
> +
> +		input++;
> +		len--;
> +	}
> +}
> +
Gur Stavi Sept. 16, 2024, 8:46 a.m. UTC | #3 
> +
> +static inline bool virtio_net_hash_rss(const struct sk_buff *skb,
> +				       u32 types, const __be32 *key,
> +				       struct virtio_net_hash *hash)

Based on the guidelines, this function seems imperative rather than
predicate and should return an error-code integer.

https://www.kernel.org/doc/html/latest/process/coding-style.html#function-return-values-and-names

> +{
> +	u16 report;
> +	struct virtio_net_toeplitz_state toeplitz_state = {
> +		.key_buffer = be32_to_cpu(*key),
> +		.key = key
> +	};
> +	struct flow_keys flow;
> +
> +	if (!skb_flow_dissect_flow_keys(skb, &flow, 0))
> +		return false;
> +
> +	report = virtio_net_hash_report(types, flow.basic);
> +
> +	switch (report) {
> +	case VIRTIO_NET_HASH_REPORT_IPv4:
> +		virtio_net_toeplitz(&toeplitz_state,
> +				    (__be32 *)&flow.addrs.v4addrs,
> +				    sizeof(flow.addrs.v4addrs) / 4);
> +		break;
> +
> +	case VIRTIO_NET_HASH_REPORT_TCPv4:
> +		virtio_net_toeplitz(&toeplitz_state,
> +				    (__be32 *)&flow.addrs.v4addrs,
> +				    sizeof(flow.addrs.v4addrs) / 4);
> +		virtio_net_toeplitz(&toeplitz_state, &flow.ports.ports,
> +				    1);
> +		break;
> +
> +	case VIRTIO_NET_HASH_REPORT_UDPv4:
> +		virtio_net_toeplitz(&toeplitz_state,
> +				    (__be32 *)&flow.addrs.v4addrs,
> +				    sizeof(flow.addrs.v4addrs) / 4);
> +		virtio_net_toeplitz(&toeplitz_state, &flow.ports.ports,
> +				    1);
> +		break;
> +
> +	case VIRTIO_NET_HASH_REPORT_IPv6:
> +		virtio_net_toeplitz(&toeplitz_state,
> +				    (__be32 *)&flow.addrs.v6addrs,
> +				    sizeof(flow.addrs.v6addrs) / 4);
> +		break;
> +
> +	case VIRTIO_NET_HASH_REPORT_TCPv6:
> +		virtio_net_toeplitz(&toeplitz_state,
> +				    (__be32 *)&flow.addrs.v6addrs,
> +				    sizeof(flow.addrs.v6addrs) / 4);
> +		virtio_net_toeplitz(&toeplitz_state, &flow.ports.ports,
> +				    1);
> +		break;
> +
> +	case VIRTIO_NET_HASH_REPORT_UDPv6:
> +		virtio_net_toeplitz(&toeplitz_state,
> +				    (__be32 *)&flow.addrs.v6addrs,
> +				    sizeof(flow.addrs.v6addrs) / 4);
> +		virtio_net_toeplitz(&toeplitz_state, &flow.ports.ports,
> +				    1);
> +		break;
> +
> +	default:
> +		return false;
> +	}
> +
> +	hash->value = toeplitz_state.hash;
> +	hash->report = report;
> +
> +	return true;
> +}
> +
Willem de Bruijn Sept. 18, 2024, 12:50 p.m. UTC | #4 
Akihiko Odaki 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>
> ---
>  include/linux/virtio_net.h | 198 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 198 insertions(+)
> 
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index 6c395a2600e8..7ee2e2f2625a 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -9,6 +9,183 @@
>  #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;
> +	u32 key_buffer;
> +	const __be32 *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(struct virtio_net_toeplitz_state *state,
> +				       const __be32 *input, size_t len)
> +{
> +	u32 key;
> +
> +	while (len) {
> +		state->key++;
> +		key = be32_to_cpu(*state->key);
> +
> +		for (u32 bit = BIT(31); bit; bit >>= 1) {
> +			if (be32_to_cpu(*input) & bit)
> +				state->hash ^= state->key_buffer;
> +
> +			state->key_buffer =
> +				(state->key_buffer << 1) | !!(key & bit);
> +		}
> +
> +		input++;
> +		len--;
> +	}
> +}
> +
> +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 4 + len;

Avoid raw constants like this 4. What field does it capture?

Instead of working from shortest to longest and using max, how about
the inverse and return as soon as a match is found.

> +}
> +
> +static inline u32 virtio_net_hash_report(u32 types,
> +					 struct flow_dissector_key_basic key)
> +{
> +	switch (key.n_proto) {
> +	case htons(ETH_P_IP):
> +		if (key.ip_proto == IPPROTO_TCP &&
> +		    (types & VIRTIO_NET_RSS_HASH_TYPE_TCPv4))
> +			return VIRTIO_NET_HASH_REPORT_TCPv4;
> +
> +		if (key.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 htons(ETH_P_IPV6):
> +		if (key.ip_proto == IPPROTO_TCP &&
> +		    (types & VIRTIO_NET_RSS_HASH_TYPE_TCPv6))
> +			return VIRTIO_NET_HASH_REPORT_TCPv6;
> +
> +		if (key.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 bool virtio_net_hash_rss(const struct sk_buff *skb,
> +				       u32 types, const __be32 *key,
> +				       struct virtio_net_hash *hash)
> +{
> +	u16 report;

nit: move below the struct declarations.

> +	struct virtio_net_toeplitz_state toeplitz_state = {
> +		.key_buffer = be32_to_cpu(*key),
> +		.key = key
> +	};
> +	struct flow_keys flow;
> +
> +	if (!skb_flow_dissect_flow_keys(skb, &flow, 0))
> +		return false;
> +
> +	report = virtio_net_hash_report(types, flow.basic);
> +
> +	switch (report) {
> +	case VIRTIO_NET_HASH_REPORT_IPv4:
> +		virtio_net_toeplitz(&toeplitz_state,
> +				    (__be32 *)&flow.addrs.v4addrs,
> +				    sizeof(flow.addrs.v4addrs) / 4);
> +		break;
> +
> +	case VIRTIO_NET_HASH_REPORT_TCPv4:
> +		virtio_net_toeplitz(&toeplitz_state,
> +				    (__be32 *)&flow.addrs.v4addrs,
> +				    sizeof(flow.addrs.v4addrs) / 4);
> +		virtio_net_toeplitz(&toeplitz_state, &flow.ports.ports,
> +				    1);
> +		break;
> +
> +	case VIRTIO_NET_HASH_REPORT_UDPv4:
> +		virtio_net_toeplitz(&toeplitz_state,
> +				    (__be32 *)&flow.addrs.v4addrs,
> +				    sizeof(flow.addrs.v4addrs) / 4);
> +		virtio_net_toeplitz(&toeplitz_state, &flow.ports.ports,
> +				    1);
> +		break;
> +
> +	case VIRTIO_NET_HASH_REPORT_IPv6:
> +		virtio_net_toeplitz(&toeplitz_state,
> +				    (__be32 *)&flow.addrs.v6addrs,
> +				    sizeof(flow.addrs.v6addrs) / 4);
> +		break;
> +
> +	case VIRTIO_NET_HASH_REPORT_TCPv6:
> +		virtio_net_toeplitz(&toeplitz_state,
> +				    (__be32 *)&flow.addrs.v6addrs,
> +				    sizeof(flow.addrs.v6addrs) / 4);
> +		virtio_net_toeplitz(&toeplitz_state, &flow.ports.ports,
> +				    1);
> +		break;
> +
> +	case VIRTIO_NET_HASH_REPORT_UDPv6:
> +		virtio_net_toeplitz(&toeplitz_state,
> +				    (__be32 *)&flow.addrs.v6addrs,
> +				    sizeof(flow.addrs.v6addrs) / 4);
> +		virtio_net_toeplitz(&toeplitz_state, &flow.ports.ports,
> +				    1);
> +		break;
> +
> +	default:
> +		return false;
> +	}
> +
> +	hash->value = toeplitz_state.hash;
> +	hash->report = report;
> +
> +	return true;
> +}
> +
>  static inline bool virtio_net_hdr_match_proto(__be16 protocol, __u8 gso_type)
>  {
>  	switch (gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
> @@ -239,4 +416,25 @@ static inline int virtio_net_hdr_from_skb(const struct sk_buff *skb,
>  	return 0;
>  }
>  
> +static inline int virtio_net_hdr_v1_hash_from_skb(const struct sk_buff *skb,
> +						  struct virtio_net_hdr_v1_hash *hdr,
> +						  bool has_data_valid,
> +						  int vlan_hlen,
> +						  const struct virtio_net_hash *hash)
> +{
> +	int ret;
> +
> +	memset(hdr, 0, sizeof(*hdr));
> +
> +	ret = virtio_net_hdr_from_skb(skb, (struct virtio_net_hdr *)hdr,
> +				      true, has_data_valid, vlan_hlen);
> +	if (!ret) {
> +		hdr->hdr.num_buffers = cpu_to_le16(1);
> +		hdr->hash_value = cpu_to_le32(hash->value);
> +		hdr->hash_report = cpu_to_le16(hash->report);
> +	}
> +
> +	return ret;
> +}
> +

I don't think that this helper is very helpful, as all the information
it sets are first passed in. Just set the hdr fields directy in the
caller. It is easier to follow the control flow.
Akihiko Odaki Sept. 19, 2024, 12:51 p.m. UTC | #5 
On 2024/09/16 10:01, gur.stavi@huawei.com wrote:
>> +
>> +static inline void virtio_net_toeplitz(struct virtio_net_toeplitz_state *state,
>> +				       const __be32 *input, size_t len)
>>
>> The function calculates a hash value but its name does not make it
>> clear. Consider adding a 'calc'.
>>
>> +{
>> +	u32 key;
>> +
>> +	while (len) {
>> +		state->key++;
>> +		key = be32_to_cpu(*state->key);
>>
>> You perform be32_to_cpu to support both CPU endianities.
>> If you will follow with an unconditional swab32, you could run the
>> following loop on a more natural 0 to 31 always referring to bit 0
>> and avoiding !!(key & bit):
>>
>> key = swab32(be32_to_cpu(*state->key));
>> for (i = 0; i < 32; i++, key >>= 1) {
>> 	if (be32_to_cpu(*input) & 1)
>> 		state->hash ^= state->key_buffer;
>> 	state->key_buffer = (state->key_buffer << 1) | (key & 1);
>> }
>>
> 
> Fixing myself, in previous version 'input' was tested against same bit.
> Advantage is less clear now, replacing !! with extra shift.
> However, since little endian CPUs are more common, the combination of
> swab32(be32_to_cpu(x) will actually become a nop.
> Similar tactic may be applied to 'input' by assigning it to local
> variable. This may produce more efficient version but not necessary
> easier to understand.
> 
> key = bswap32(be32_to_cpu(*state->key));
> for (u32 bit = BIT(31); bit; bit >>= 1, key >>= 1) {
> 	if (be32_to_cpu(*input) & bit)
> 		state->hash ^= state->key_buffer;
> 	state->key_buffer =
> 		(state->key_buffer << 1) | (key & 1);
> }

This unfortunately does not work. swab32() works at *byte*-level but we 
need to reverse the order of *bits*. bitrev32() is what we need, but it 
cannot eliminate be32_to_cpu().

Regards,
Akihiko Odaki
Akihiko Odaki Sept. 23, 2024, 6:15 p.m. UTC | #6 
On 2024/09/18 14:50, Willem de Bruijn wrote:
> Akihiko Odaki 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>
>> ---
>>   include/linux/virtio_net.h | 198 +++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 198 insertions(+)
>>
>> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
>> index 6c395a2600e8..7ee2e2f2625a 100644
>> --- a/include/linux/virtio_net.h
>> +++ b/include/linux/virtio_net.h
>> @@ -9,6 +9,183 @@
>>   #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;
>> +	u32 key_buffer;
>> +	const __be32 *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(struct virtio_net_toeplitz_state *state,
>> +				       const __be32 *input, size_t len)
>> +{
>> +	u32 key;
>> +
>> +	while (len) {
>> +		state->key++;
>> +		key = be32_to_cpu(*state->key);
>> +
>> +		for (u32 bit = BIT(31); bit; bit >>= 1) {
>> +			if (be32_to_cpu(*input) & bit)
>> +				state->hash ^= state->key_buffer;
>> +
>> +			state->key_buffer =
>> +				(state->key_buffer << 1) | !!(key & bit);
>> +		}
>> +
>> +		input++;
>> +		len--;
>> +	}
>> +}
>> +
>> +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 4 + len;
> 
> Avoid raw constants like this 4. What field does it capture?

It is: sizeof_field(struct virtio_net_toeplitz_state, key_buffer)
I'll replace it with v4.

> 
> Instead of working from shortest to longest and using max, how about
> the inverse and return as soon as a match is found.

I think it is less error-prone to use max() as it does not require to 
sort the numbers. The compiler should properly optimize it in the way 
you suggested.

> 
>> +}
>> +
>> +static inline u32 virtio_net_hash_report(u32 types,
>> +					 struct flow_dissector_key_basic key)
>> +{
>> +	switch (key.n_proto) {
>> +	case htons(ETH_P_IP):
>> +		if (key.ip_proto == IPPROTO_TCP &&
>> +		    (types & VIRTIO_NET_RSS_HASH_TYPE_TCPv4))
>> +			return VIRTIO_NET_HASH_REPORT_TCPv4;
>> +
>> +		if (key.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 htons(ETH_P_IPV6):
>> +		if (key.ip_proto == IPPROTO_TCP &&
>> +		    (types & VIRTIO_NET_RSS_HASH_TYPE_TCPv6))
>> +			return VIRTIO_NET_HASH_REPORT_TCPv6;
>> +
>> +		if (key.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 bool virtio_net_hash_rss(const struct sk_buff *skb,
>> +				       u32 types, const __be32 *key,
>> +				       struct virtio_net_hash *hash)
>> +{
>> +	u16 report;
> 
> nit: move below the struct declarations.

I'll change accordingly with v4.

> 
>> +	struct virtio_net_toeplitz_state toeplitz_state = {
>> +		.key_buffer = be32_to_cpu(*key),
>> +		.key = key
>> +	};
>> +	struct flow_keys flow;
>> +
>> +	if (!skb_flow_dissect_flow_keys(skb, &flow, 0))
>> +		return false;
>> +
>> +	report = virtio_net_hash_report(types, flow.basic);
>> +
>> +	switch (report) {
>> +	case VIRTIO_NET_HASH_REPORT_IPv4:
>> +		virtio_net_toeplitz(&toeplitz_state,
>> +				    (__be32 *)&flow.addrs.v4addrs,
>> +				    sizeof(flow.addrs.v4addrs) / 4);
>> +		break;
>> +
>> +	case VIRTIO_NET_HASH_REPORT_TCPv4:
>> +		virtio_net_toeplitz(&toeplitz_state,
>> +				    (__be32 *)&flow.addrs.v4addrs,
>> +				    sizeof(flow.addrs.v4addrs) / 4);
>> +		virtio_net_toeplitz(&toeplitz_state, &flow.ports.ports,
>> +				    1);
>> +		break;
>> +
>> +	case VIRTIO_NET_HASH_REPORT_UDPv4:
>> +		virtio_net_toeplitz(&toeplitz_state,
>> +				    (__be32 *)&flow.addrs.v4addrs,
>> +				    sizeof(flow.addrs.v4addrs) / 4);
>> +		virtio_net_toeplitz(&toeplitz_state, &flow.ports.ports,
>> +				    1);
>> +		break;
>> +
>> +	case VIRTIO_NET_HASH_REPORT_IPv6:
>> +		virtio_net_toeplitz(&toeplitz_state,
>> +				    (__be32 *)&flow.addrs.v6addrs,
>> +				    sizeof(flow.addrs.v6addrs) / 4);
>> +		break;
>> +
>> +	case VIRTIO_NET_HASH_REPORT_TCPv6:
>> +		virtio_net_toeplitz(&toeplitz_state,
>> +				    (__be32 *)&flow.addrs.v6addrs,
>> +				    sizeof(flow.addrs.v6addrs) / 4);
>> +		virtio_net_toeplitz(&toeplitz_state, &flow.ports.ports,
>> +				    1);
>> +		break;
>> +
>> +	case VIRTIO_NET_HASH_REPORT_UDPv6:
>> +		virtio_net_toeplitz(&toeplitz_state,
>> +				    (__be32 *)&flow.addrs.v6addrs,
>> +				    sizeof(flow.addrs.v6addrs) / 4);
>> +		virtio_net_toeplitz(&toeplitz_state, &flow.ports.ports,
>> +				    1);
>> +		break;
>> +
>> +	default:
>> +		return false;
>> +	}
>> +
>> +	hash->value = toeplitz_state.hash;
>> +	hash->report = report;
>> +
>> +	return true;
>> +}
>> +
>>   static inline bool virtio_net_hdr_match_proto(__be16 protocol, __u8 gso_type)
>>   {
>>   	switch (gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
>> @@ -239,4 +416,25 @@ static inline int virtio_net_hdr_from_skb(const struct sk_buff *skb,
>>   	return 0;
>>   }
>>   
>> +static inline int virtio_net_hdr_v1_hash_from_skb(const struct sk_buff *skb,
>> +						  struct virtio_net_hdr_v1_hash *hdr,
>> +						  bool has_data_valid,
>> +						  int vlan_hlen,
>> +						  const struct virtio_net_hash *hash)
>> +{
>> +	int ret;
>> +
>> +	memset(hdr, 0, sizeof(*hdr));
>> +
>> +	ret = virtio_net_hdr_from_skb(skb, (struct virtio_net_hdr *)hdr,
>> +				      true, has_data_valid, vlan_hlen);
>> +	if (!ret) {
>> +		hdr->hdr.num_buffers = cpu_to_le16(1);
>> +		hdr->hash_value = cpu_to_le32(hash->value);
>> +		hdr->hash_report = cpu_to_le16(hash->report);
>> +	}
>> +
>> +	return ret;
>> +}
>> +
> 
> I don't think that this helper is very helpful, as all the information
> it sets are first passed in. Just set the hdr fields directy in the
> caller. It is easier to follow the control flow.

I'll remove it in v4.

Regards,
Akihiko Odaki
diff mbox series

Patch

diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index 6c395a2600e8..7ee2e2f2625a 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -9,6 +9,183 @@ 
 #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;
+	u32 key_buffer;
+	const __be32 *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(struct virtio_net_toeplitz_state *state,
+				       const __be32 *input, size_t len)
+{
+	u32 key;
+
+	while (len) {
+		state->key++;
+		key = be32_to_cpu(*state->key);
+
+		for (u32 bit = BIT(31); bit; bit >>= 1) {
+			if (be32_to_cpu(*input) & bit)
+				state->hash ^= state->key_buffer;
+
+			state->key_buffer =
+				(state->key_buffer << 1) | !!(key & bit);
+		}
+
+		input++;
+		len--;
+	}
+}
+
+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 4 + len;
+}
+
+static inline u32 virtio_net_hash_report(u32 types,
+					 struct flow_dissector_key_basic key)
+{
+	switch (key.n_proto) {
+	case htons(ETH_P_IP):
+		if (key.ip_proto == IPPROTO_TCP &&
+		    (types & VIRTIO_NET_RSS_HASH_TYPE_TCPv4))
+			return VIRTIO_NET_HASH_REPORT_TCPv4;
+
+		if (key.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 htons(ETH_P_IPV6):
+		if (key.ip_proto == IPPROTO_TCP &&
+		    (types & VIRTIO_NET_RSS_HASH_TYPE_TCPv6))
+			return VIRTIO_NET_HASH_REPORT_TCPv6;
+
+		if (key.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 bool virtio_net_hash_rss(const struct sk_buff *skb,
+				       u32 types, const __be32 *key,
+				       struct virtio_net_hash *hash)
+{
+	u16 report;
+	struct virtio_net_toeplitz_state toeplitz_state = {
+		.key_buffer = be32_to_cpu(*key),
+		.key = key
+	};
+	struct flow_keys flow;
+
+	if (!skb_flow_dissect_flow_keys(skb, &flow, 0))
+		return false;
+
+	report = virtio_net_hash_report(types, flow.basic);
+
+	switch (report) {
+	case VIRTIO_NET_HASH_REPORT_IPv4:
+		virtio_net_toeplitz(&toeplitz_state,
+				    (__be32 *)&flow.addrs.v4addrs,
+				    sizeof(flow.addrs.v4addrs) / 4);
+		break;
+
+	case VIRTIO_NET_HASH_REPORT_TCPv4:
+		virtio_net_toeplitz(&toeplitz_state,
+				    (__be32 *)&flow.addrs.v4addrs,
+				    sizeof(flow.addrs.v4addrs) / 4);
+		virtio_net_toeplitz(&toeplitz_state, &flow.ports.ports,
+				    1);
+		break;
+
+	case VIRTIO_NET_HASH_REPORT_UDPv4:
+		virtio_net_toeplitz(&toeplitz_state,
+				    (__be32 *)&flow.addrs.v4addrs,
+				    sizeof(flow.addrs.v4addrs) / 4);
+		virtio_net_toeplitz(&toeplitz_state, &flow.ports.ports,
+				    1);
+		break;
+
+	case VIRTIO_NET_HASH_REPORT_IPv6:
+		virtio_net_toeplitz(&toeplitz_state,
+				    (__be32 *)&flow.addrs.v6addrs,
+				    sizeof(flow.addrs.v6addrs) / 4);
+		break;
+
+	case VIRTIO_NET_HASH_REPORT_TCPv6:
+		virtio_net_toeplitz(&toeplitz_state,
+				    (__be32 *)&flow.addrs.v6addrs,
+				    sizeof(flow.addrs.v6addrs) / 4);
+		virtio_net_toeplitz(&toeplitz_state, &flow.ports.ports,
+				    1);
+		break;
+
+	case VIRTIO_NET_HASH_REPORT_UDPv6:
+		virtio_net_toeplitz(&toeplitz_state,
+				    (__be32 *)&flow.addrs.v6addrs,
+				    sizeof(flow.addrs.v6addrs) / 4);
+		virtio_net_toeplitz(&toeplitz_state, &flow.ports.ports,
+				    1);
+		break;
+
+	default:
+		return false;
+	}
+
+	hash->value = toeplitz_state.hash;
+	hash->report = report;
+
+	return true;
+}
+
 static inline bool virtio_net_hdr_match_proto(__be16 protocol, __u8 gso_type)
 {
 	switch (gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
@@ -239,4 +416,25 @@  static inline int virtio_net_hdr_from_skb(const struct sk_buff *skb,
 	return 0;
 }
 
+static inline int virtio_net_hdr_v1_hash_from_skb(const struct sk_buff *skb,
+						  struct virtio_net_hdr_v1_hash *hdr,
+						  bool has_data_valid,
+						  int vlan_hlen,
+						  const struct virtio_net_hash *hash)
+{
+	int ret;
+
+	memset(hdr, 0, sizeof(*hdr));
+
+	ret = virtio_net_hdr_from_skb(skb, (struct virtio_net_hdr *)hdr,
+				      true, has_data_valid, vlan_hlen);
+	if (!ret) {
+		hdr->hdr.num_buffers = cpu_to_le16(1);
+		hdr->hash_value = cpu_to_le32(hash->value);
+		hdr->hash_report = cpu_to_le16(hash->report);
+	}
+
+	return ret;
+}
+
 #endif /* _LINUX_VIRTIO_NET_H */