On Wed, 23 Oct 2024 19:00:01 +0200
Serhii Iliushyk <sil-...@napatech.com> wrote:
> +
> + rte_memcpy(&tmp_rss_conf.rss_key, rss_conf->rss_key,
> rss_conf->rss_key_len);
Avoid use of rte_memcpy(), it has less checking that memcpy().
The only place it can make sense is in the hot data path where compiler is more
conservative about alignment.
> + rte_memcpy(&ndev->rss_conf, &tmp_rss_conf, sizeof(struct
> nt_eth_rss_conf));
>
Use structure assignment instead of memcpy() to keep type checking.
> + if (rss_conf->rss_key != NULL) {
> + int key_len = rss_conf->rss_key_len < MAX_RSS_KEY_LEN ?
> rss_conf->rss_key_len
> + : MAX_RSS_KEY_LEN;
Use RTE_MIN() and the key_len variable should not be signed.
> + memset(rss_conf->rss_key, 0, rss_conf->rss_key_len);
> + rte_memcpy(rss_conf->rss_key, &ndev->rss_conf.rss_key, key_len);
> +static int eth_dev_rss_hash_update(struct rte_eth_dev *eth_dev, struct
> rte_eth_rss_conf *rss_conf)
> +{
> + const struct flow_filter_ops *flow_filter_ops = get_flow_filter_ops();
> +
> + if (flow_filter_ops == NULL) {
> + NT_LOG_DBGX(ERR, NTNIC, "flow_filter module uninitialized");
> + return -1;
> + }
> +
> + struct pmd_internals *internals = (struct pmd_internals
> *)eth_dev->data->dev_private;
Since dev_private is void *, no need for the cast here (in C code).
> +
> + struct flow_nic_dev *ndev = internals->flw_dev->ndev;
> + struct nt_eth_rss_conf tmp_rss_conf = { 0 };
> + const int hsh_idx = 0; /* hsh index 0 means the default receipt in HSH
> module */
> + int res = 0;
> +
> + if (rss_conf->rss_key != NULL) {
> + if (rss_conf->rss_key_len > MAX_RSS_KEY_LEN) {
> + NT_LOG(ERR, NTNIC,
> + "ERROR: - RSS hash key length %u exceeds
> maximum value %u",
> + rss_conf->rss_key_len, MAX_RSS_KEY_LEN);
> + return -1;
> + }
> +
> + rte_memcpy(&tmp_rss_conf.rss_key, rss_conf->rss_key,
> rss_conf->rss_key_len);
> + }
> +
> + tmp_rss_conf.algorithm = rss_conf->algorithm;
> +
> + tmp_rss_conf.rss_hf = rss_conf->rss_hf;
> + res = flow_filter_ops->flow_nic_set_hasher_fields(ndev, hsh_idx,
> tmp_rss_conf);
In general, this code is good about moving declarations next to first use.
But here res is initialized to 0 but then set again from hasher_fields.
Why not just move the declaration there.
