Re: [dpdk-dev] [PATCH v3 3/3] ring:add ring walk routine

["Wiles, Keith" <keith.wiles@intel.com>]

> On Dec 27, 2018, at 9:17 AM, Olivier Matz <olivier.matz@6wind.com> wrote:
> 
> Hi,
> 
> On Thu, Dec 27, 2018 at 02:52:51PM +0000, Wiles, Keith wrote:
>> 
>> 
>>> On Dec 27, 2018, at 8:47 AM, Wiles, Keith <keith.wiles@intel.com> wrote:
>>> 
>>> 
>>> 
>>>> On Dec 27, 2018, at 4:02 AM, Olivier Matz <olivier.matz@6wind.com> wrote:
>>>> 
>>>> Hi,
>>>> 
>>>> On Sun, Dec 16, 2018 at 11:27:21AM -0600, Keith Wiles wrote:
>>>>> Add a ring walk routine for debugging and DFS.
>>>>> 
>>>>> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
>>>>> ---
>>>>> V3
>>>>> Fix checkpatch warnings adding a commit message.
>>>>> Must be using a different checkpatch then on my Ubuntu 18.04 system 
>>>>> V2
>>>>> Fix checkpatch warnings.
>>>>> 
>>>>> lib/librte_ring/rte_ring.c           | 20 ++++++++++++++++++++
>>>>> lib/librte_ring/rte_ring.h           | 14 ++++++++++++++
>>>>> lib/librte_ring/rte_ring_version.map |  7 +++++++
>>>>> 3 files changed, 41 insertions(+)
>>>>> 
>>>>> diff --git a/lib/librte_ring/rte_ring.c b/lib/librte_ring/rte_ring.c
>>>>> index d215acecc..fb5819e4b 100644
>>>>> --- a/lib/librte_ring/rte_ring.c
>>>>> +++ b/lib/librte_ring/rte_ring.c
>>>>> @@ -280,3 +280,23 @@ rte_ring_lookup(const char *name)
>>>>> 
>>>>> 	return r;
>>>>> }
>>>>> +
>>>>> +void
>>>>> +rte_ring_walk(void (*func)(struct rte_ring *r, void *arg), void *arg)
>>>>> +{
>>>>> +	const struct rte_tailq_entry *te;
>>>>> +	struct rte_ring_list *ring_list;
>>>>> +
>>>>> +	if (!func)
>>>>> +		return;
>>>>> +
>>>>> +	ring_list = RTE_TAILQ_CAST(rte_ring_tailq.head, rte_ring_list);
>>>>> +
>>>>> +	rte_rwlock_read_lock(RTE_EAL_TAILQ_RWLOCK);
>>>>> +
>>>>> +	TAILQ_FOREACH(te, ring_list, next) {
>>>>> +		func((struct rte_ring *) te->data, arg);
>>>>> +	}
>>>>> +
>>>>> +	rte_rwlock_read_unlock(RTE_EAL_TAILQ_RWLOCK);
>>>>> +}
>>>> 
>>>> In mempool, a FOREACH_SAFE() macro is using starting from this commit:
>>>> cae54ac47ced ("mempool: fix unsafe removal from list by callback")
>>>> 
>>>> Maybe the same should be done for the ring.
>>> 
>>> I am not removing or modifying the ring tailq list here and I have the lock already, why do I need to use _SAFE macro?
>> 
>> OK, I do see a possible case. If the function freed the node, but it would mean it would have to grab the lock and walk the list to free the te value and unlink it from the list. The function does not get the ’te’ pointer only the data. To free it they would have to grab the lock. 
> 
> Unlinking the element from the list looks indeed laborious with only
> te->data, and I agree there is probably no use case for that. Looking at
> commit cae54ac47ced, it seems there was one, but I cannot find any usage
> of this feature in git history.

I could change the function to return > 0 value to have the entry removed and the ’te’ freed leaving the data value up to the called function to free or do whatever it needs. return of 0 means just step to the next entry and < 0 means stop. What do you think of that change? 
> 
> So ok, let's keep your version.
> 
> Thanks,
> Olivier

Regards,
Keith