On 5/2/2024 10:31 PM, Stephen Hemminger wrote:
> The flow RSS support via BPF was not using the key, or
> hash type parameters. Which is good because they were never
> properly setup.
>
> Fix the setup and validate the flow parameters, the BPF
> side gets fixed later.
>
> Signed-off-by: Stephen Hemminger <step...@networkplumber.org>
>
<...>
> @@ -2095,6 +2112,41 @@ static int rss_add_actions(struct rte_flow *flow,
> struct pmd_internals *pmd,
> (error, ENOTSUP, RTE_FLOW_ERROR_TYPE_UNSPECIFIED, NULL,
> "a nonzero RSS encapsulation level is not supported");
>
> + if (rss->queue_num == 0 || rss->queue_num >= TAP_MAX_QUEUES)
> + return rte_flow_error_set(error, EINVAL,
> RTE_FLOW_ERROR_TYPE_UNSPECIFIED, NULL,
> + "invalid number of queues");
> +
> + /* allow RSS key_len 0 in case of NULL (default) RSS key. */
> + if (rss->key_len == 0) {
> + if (rss->key != NULL)
> + return rte_flow_error_set(error, ENOTSUP,
> +
> RTE_FLOW_ERROR_TYPE_ACTION_CONF,
> + &rss->key_len, "RSS hash key
> length 0");
> + key_in = rss_hash_default_key;
> + } else {
> + if (rss->key_len != TAP_RSS_HASH_KEY_SIZE)
> + return rte_flow_error_set(error, EINVAL,
> RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> + NULL, "RSS hash invalid key
> length");
> + if (rss->key == NULL)
> + return rte_flow_error_set(error, EINVAL,
> RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> + NULL, "RSS hash key is NULL");
> + key_in = rss->key;
> + }
>
Does it make sense to add above expectations (like if "key_len == 0"
default key used, or 'key_len' expectations, etc..) to the function comment?
<...>
> @@ -2109,8 +2161,11 @@ static int rss_add_actions(struct rte_flow *flow,
> struct pmd_internals *pmd,
> rss_entry.nb_queues = rss->queue_num;
> for (i = 0; i < rss->queue_num; i++)
> rss_entry.queues[i] = rss->queue[i];
> - rss_entry.hash_fields =
> - (1 << HASH_FIELD_IPV4_L3_L4) | (1 << HASH_FIELD_IPV6_L3_L4);
> +
> + rss_entry.hash_fields = hash_type;
> + rte_convert_rss_key((const uint32_t *)key_in, (uint32_t *)rss_entry.key,
> + TAP_RSS_HASH_KEY_SIZE);
> +
>
Why 'rte_convert_rss_key()' is required? Is this making an assumption on
the CPU architecture?
