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

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

> 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. 
> 
>> 
>> 
>>> diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h
>>> index af5444a9f..b9391a655 100644
>>> --- a/lib/librte_ring/rte_ring.h
>>> +++ b/lib/librte_ring/rte_ring.h
>>> @@ -769,6 +769,20 @@ rte_ring_get_capacity(const struct rte_ring *r)
>>> */
>>> void rte_ring_list_dump(FILE *f);
>>> 
>>> +/**
>>> + * Walk the list of ring entries and call the function provided
>>> + *
>>> + * @param func
>>> + *   The function to call for each ring entry using the following prototype
>>> + *     void (*func)(struct rte_ring *r, void *arg)
>>> + * @param arg
>>> + *   argument for the call to function
>>> + * @return
>>> + *   None.
>>> + */
>> 
>> I don't think we need to duplicate the prototype in the comment. Please
>> add the dots at the end of the sentences, and remove @return.
>> 
>> 
>> Thanks,
>> Olivier
> 
> Regards,
> Keith

Regards,
Keith