diff mbox series

[v3,3/3] ring:add ring walk routine

Message ID 20181216172721.91042-3-keith.wiles@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series [v2,1/3] dfs:add FUSE based filesystem for DPDK |

Checks

Context Check Description
ci/checkpatch warning coding style issues

Commit Message

Wiles, Keith Dec. 16, 2018, 5:27 p.m. UTC 
  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(+)

Comments

Olivier Matz Dec. 27, 2018, 10:02 a.m. UTC | #1 
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.


> 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
Wiles, Keith Dec. 27, 2018, 2:47 p.m. UTC | #2 
> 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?

> 
> 
>> 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
Wiles, Keith Dec. 27, 2018, 2:52 p.m. UTC | #3 
> 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
Olivier Matz Dec. 27, 2018, 3:17 p.m. UTC | #4 
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.

So ok, let's keep your version.

Thanks,
Olivier
Wiles, Keith Dec. 27, 2018, 3:40 p.m. UTC | #5 
> 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
diff mbox series

Patch

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);
+}
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.
+ */
+void __rte_experimental
+rte_ring_walk(void (*func)(struct rte_ring *r, void *arg), void *arg);
+
 /**
  * Search a ring from its name
  *
diff --git a/lib/librte_ring/rte_ring_version.map b/lib/librte_ring/rte_ring_version.map
index d935efd0d..17c05c1b2 100644
--- a/lib/librte_ring/rte_ring_version.map
+++ b/lib/librte_ring/rte_ring_version.map
@@ -17,3 +17,10 @@  DPDK_2.2 {
 	rte_ring_free;
 
 } DPDK_2.0;
+
+EXPERIMENTAL {
+	global:
+
+	rte_ring_walk;
+};
+