Re: [PATCH v2 02/24] net/nfp: add the structures and functions for flow offload

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 10/19/2022 3:50 AM, Chaoyong He wrote:
>> On 10/10/2022 7:08 AM, Chaoyong He wrote:
>>> Add the structures and functions to process mask table, flow table,
>>> and flow stats id, which are used in the rte_flow offload logics.
>>>
>>> Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
>>
>> <...>
>>
>>> +__rte_unused static int
>>> +nfp_flow_table_delete(struct nfp_flow_priv *priv,
>>> +             struct rte_flow *nfp_flow) {
>>> +     int ret;
>>> +     char *hash_data;
>>> +     uint32_t hash_key;
>>> +
>>> +     hash_data = (char *)(nfp_flow->payload.unmasked_data);
>>> +     hash_key = rte_jhash(hash_data, nfp_flow->length,
>>> + priv->hash_seed);
>>
>> It can be an option to store hash information in 'struct rte_flow', instead of
>> keep calculating it.
>>
> 
> Okay, I will revise like this in the next version patch, thanks.
> 
>> <...>
>>
>>> +     /* flow stats */
>>> +     rte_spinlock_init(&priv->stats_lock);
>>> +     stats_size = (ctx_count & NFP_FL_STAT_ID_STAT) |
>>> +                     ((ctx_split - 1) & NFP_FL_STAT_ID_MU_NUM);
>>> +     PMD_INIT_LOG(INFO, "ctx_count:%0lx, ctx_split:%0lx, stats_size:%0lx
>> ",
>>> +                     ctx_count, ctx_split, stats_size);
>>> +     priv->stats = rte_zmalloc("nfp_flow_stats",
>>> +                     stats_size * sizeof(struct nfp_fl_stats), 0);
>>
>> Is this allocates 'stats' for max possible flow rule count? Isn't this a too big
>> number to allocate in advance?
> 
> For now, the 'stats_size' for our card is 500,000, and sizeof(structure nfp_fl_stats) is 16,
> so the size of memory need alloc here is about 8MB. Maybe it's not a very big memory?
> 

agree, 8MB doesn't look much.

> And we also calculate an index use these two macros(in function nfp_stats_id_alloc()),
> and then send it to firmware, and the firmware will send the stats with this index to PMD.
> If we alloc a small memory at start and then enlarge it at some point, we can't make sure
> the index we calculated is unique anymore, and the stats will confusion, which is not good.
> 

ack