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

[Olivier Matz]

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 at intel.com> wrote:
> > 
> > 
> > 
> >> On Dec 27, 2018, at 4:02 AM, Olivier Matz <olivier.matz at 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 at 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.

So ok, let's keep your version.

Thanks,
Olivier