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

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> 
> <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.

> 
> >> +#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.