Re: [PATCH v2 1/2] fib: implement RCU rule reclamation

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

Hi Stephen,

Thanks for the review, I'll address your comments in v3


On 08/10/2024 19:28, Stephen Hemminger wrote:
> On Tue,  8 Oct 2024 17:55:23 +0000
> Vladimir Medvedkin <vladimir.medvedkin@intel.com> wrote:
>> +	if ((tbl8_idx == -ENOSPC) && dp->dq != NULL) {
> Better to either drop the parenthesis here, or put it on both conditions.
>
>> +		/* If there are no tbl8 groups try to reclaim one. */
>> +		if (rte_rcu_qsbr_dq_reclaim(dp->dq, 1,
>> +				NULL, NULL, NULL) == 0)
>> +			tbl8_idx = tbl8_get_idx(dp);
>> +	}
> Could add unlikely() to this expression.
>
> 	/* If there are no tbl8 groups try to reclaim one. */
> 	if (unlikely(tbl8_idx == -ENOSPC && dp->dq &&
> 		     !rte_rcu_qsbr_dq_reclaim(dp->dq, 1, NULL, NULL, NULL)))
> 	    tbl8_idx = tbl8_get_idx(dp);
>
>
>> +static void
>> +__rcu_qsbr_free_resource(void *p, void *data, unsigned int n)
>> +{
>> +	struct dir24_8_tbl *dp = p;
>> +	uint64_t tbl8_idx = *(uint64_t *)data;
>> +	RTE_SET_USED(n);
>> +
>> +	tbl8_cleanup_and_free(dp, tbl8_idx);
>> +}
> My preference (not a requirement) is to use __rte_unused attribute
> instead of RTE_SET_USED
>
>> +	if (dp->v == NULL)
>> +		tbl8_cleanup_and_free(dp, tbl8_idx);
>> +	else if (dp->rcu_mode == RTE_FIB_QSBR_MODE_SYNC) {
>> +		rte_rcu_qsbr_synchronize(dp->v,
>> +			RTE_QSBR_THRID_INVALID);
>> +		tbl8_cleanup_and_free(dp, tbl8_idx);
>> +	} else { /* RTE_FIB_QSBR_MODE_DQ */
>> +		if (rte_rcu_qsbr_dq_enqueue(dp->dq,
>> +				(void *)&tbl8_idx))
> Minor nit: cast to void * is not necessary in C (only in C++).
> And can fit on one line; max line length now for DPDK is 100 characters.
>
> Overall, looks good.
>
> Acked-by: Stephen Hemminger <stephen@networkplumber.org>

-- 
Regards,
Vladimir