Re: [dpdk-dev] [PATCH v2 2/3] hash: add predictable RSS implementation

["Medvedkin, Vladimir" <vladimir.medvedkin@intel.com>]

Hi Konstantin,

On 12/04/2021 12:47, Ananyev, Konstantin wrote:
>>
>> <snip>
>>
>>>> +#defineRETA_SZ_MIN2U
>>>> +#defineRETA_SZ_MAX16U
>>>
>>> Should these RETA_SZ defines be in public header?
>>> So user can know what are allowed values?
>>>
>>
>> I don't think this is necessary, because the user chooses it not
>> arbitrary, but depending on the NIC.
> 
> Sure thing, but it would be goo for the user to know what are the SW
> Limitations on it without digging through .c.
> 

I see, I'll move it to .h

>>
>>>> +#define RETA_SZ_IN_RANGE(reta_sz)((reta_sz >= RETA_SZ_MIN) && \
>>
>> <snip>
>>
>>>> +uint32_t i;
>>>
>>> Empty line is  missing.
>>>
>>
>> Thanks
>>
>>>> +if ((name == NULL) || (key_len == 0) || !RETA_SZ_IN_RANGE(reta_sz)) {
>>>> +rte_errno = EINVAL;
>>>> +return NULL;
>>>> +}
>>
>> <snip>
>>
>>>> +static inline void
>>>> +set_bit(uint8_t *ptr, uint32_t bit, uint32_t pos)
>>>> +{
>>>> +uint32_t byte_idx = pos >> 3;
>>>
>>> Just as a nit to be consistent with the line below:
>>> pos / CHAR_BIT;
>>>
>>
>> Fixed
>>
>>>> +uint32_t bit_idx = (CHAR_BIT - 1) - (pos & (CHAR_BIT - 1));
>>>> +uint8_t tmp;
>>
>> <snip>
>>
>>>> +ent = rte_zmalloc(NULL, sizeof(struct rte_thash_subtuple_helper) +
>>>> +sizeof(uint32_t) * (1 << ctx->reta_sz_log), 0);
>>>
>>> Helper can be used by data-path code (via rte_thash_get_compliment()) right?
>>> Then might be better to align it at cache-line.
>>>
>>
>> Agree, I'll fix it
>>
>>>> +if (ent == NULL)
>>>> +return -ENOMEM;
>>
>> <snip>
>>
>>>>    uint32_t
>>>> -rte_thash_get_compliment(struct rte_thash_subtuple_helper *h __rte_unused,
>>>> -uint32_t hash __rte_unused, uint32_t desired_hash __rte_unused)
>>>> +rte_thash_get_compliment(struct rte_thash_subtuple_helper *h,
>>>> +uint32_t hash, uint32_t desired_hash)
>>>>    {
>>>> -return 0;
>>>> +return h->compl_table[(hash ^ desired_hash) & h->lsb_msk];
>>>>    }
>>>
>>> Would it make sense to add another-one for multi values:
>>> rte_thash_get_compliment(uint32_t hash, const uint32_t desired_hashes[], uint32_t adj_hash[], uint32_t num);
>>> So user can get adjustment values for multiple queues at once?
>>>
>>
>> At the moment I can't find scenarios why do we need to have a bulk
>> version for this function
> 
> My thought was about case when number of configured
> HW queues is less than reta_size.
> Let say reta_size==4, but user configured only 3 queues and reta={0,1,2,0}.
> In that case for queue 0, both 0 and 3 values would suit.
> 

Ah, yes, I remember out discussion about this. I'll think about this and 
add next releases.
I should be kind of

int
rte_thash_get_compliment_reta(struct rte_thash_subtuple_helper *helper, 
uint32_t hash, const uint16_t queue_id, struct rte_eth_rss_reta_entry64 
*reta, int reta_sz);

where reta is completed by rte_eth_dev_rss_reta_query()


-- 
Regards,
Vladimir