[v2,2/4] ethdev: add siblings iterators

Message ID 20190220221051.7928-3-thomas@monjalon.net (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers
Series ethdev iterators for multi-ports device |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Thomas Monjalon Feb. 20, 2019, 10:10 p.m. UTC
  If multiple ports share the same hardware device (rte_device),
they are siblings and can be found thanks to the new functions
and loop macros.
One iterator takes a port id as reference,
while the other one directly refers to the parent device.

The ownership is not checked because siblings may have
different owners.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 lib/librte_ethdev/rte_ethdev.c           | 20 +++++++++++
 lib/librte_ethdev/rte_ethdev.h           | 46 ++++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev_version.map |  2 ++
 3 files changed, 68 insertions(+)
  

Comments

Andrew Rybchenko Feb. 24, 2019, 5:22 p.m. UTC | #1
On 2/21/19 1:10 AM, Thomas Monjalon wrote:
> If multiple ports share the same hardware device (rte_device),
> they are siblings and can be found thanks to the new functions
> and loop macros.
> One iterator takes a port id as reference,
> while the other one directly refers to the parent device.
>
> The ownership is not checked because siblings may have
> different owners.
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

LGTM

Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
  
Gaëtan Rivet Feb. 27, 2019, 10:07 a.m. UTC | #2
Hi Thomas,

On Wed, Feb 20, 2019 at 11:10:49PM +0100, Thomas Monjalon wrote:
> If multiple ports share the same hardware device (rte_device),
> they are siblings and can be found thanks to the new functions
> and loop macros.
> One iterator takes a port id as reference,
> while the other one directly refers to the parent device.
> 
> The ownership is not checked because siblings may have
> different owners.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  lib/librte_ethdev/rte_ethdev.c           | 20 +++++++++++
>  lib/librte_ethdev/rte_ethdev.h           | 46 ++++++++++++++++++++++++
>  lib/librte_ethdev/rte_ethdev_version.map |  2 ++
>  3 files changed, 68 insertions(+)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index b3b2fb1dba..42154787f8 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -340,6 +340,26 @@ rte_eth_find_next(uint16_t port_id)
>  	return port_id;
>  }
>  
> +uint16_t __rte_experimental
> +rte_eth_find_next_of(uint16_t port_id, const struct rte_device *parent)
> +{
> +	while (port_id < RTE_MAX_ETHPORTS &&
> +			rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED &&
> +			rte_eth_devices[port_id].device != parent)
> +		port_id++;

Why not call rte_eth_find_next directly from this function, and
add your specific test on top of it?

Something like:

    	while (port_id < RTE_MAX_ETHPORTS &&
    	       rte_eth_devices[port_id].device != parent)
    		port_id = rte_eth_find_next(port_id + 1);

this way you won't have to rewrite the test on the device state. Having the
logic expressed in several places would make reworking the device states more
complicated than necessary if it were to happen (just as you did when switching
the test from !(ATTACHED || REMOVED) to (UNUSED).
  
Thomas Monjalon Feb. 27, 2019, 10:51 a.m. UTC | #3
27/02/2019 11:07, Gaëtan Rivet:
> Hi Thomas,
> 
> On Wed, Feb 20, 2019 at 11:10:49PM +0100, Thomas Monjalon wrote:
> > If multiple ports share the same hardware device (rte_device),
> > they are siblings and can be found thanks to the new functions
> > and loop macros.
> > One iterator takes a port id as reference,
> > while the other one directly refers to the parent device.
> > 
> > The ownership is not checked because siblings may have
> > different owners.
> > 
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > ---
> >  lib/librte_ethdev/rte_ethdev.c           | 20 +++++++++++
> >  lib/librte_ethdev/rte_ethdev.h           | 46 ++++++++++++++++++++++++
> >  lib/librte_ethdev/rte_ethdev_version.map |  2 ++
> >  3 files changed, 68 insertions(+)
> > 
> > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> > index b3b2fb1dba..42154787f8 100644
> > --- a/lib/librte_ethdev/rte_ethdev.c
> > +++ b/lib/librte_ethdev/rte_ethdev.c
> > @@ -340,6 +340,26 @@ rte_eth_find_next(uint16_t port_id)
> >  	return port_id;
> >  }
> >  
> > +uint16_t __rte_experimental
> > +rte_eth_find_next_of(uint16_t port_id, const struct rte_device *parent)
> > +{
> > +	while (port_id < RTE_MAX_ETHPORTS &&
> > +			rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED &&
> > +			rte_eth_devices[port_id].device != parent)
> > +		port_id++;
> 
> Why not call rte_eth_find_next directly from this function, and
> add your specific test on top of it?
> 
> Something like:
> 
>     	while (port_id < RTE_MAX_ETHPORTS &&
>     	       rte_eth_devices[port_id].device != parent)
>     		port_id = rte_eth_find_next(port_id + 1);
> 
> this way you won't have to rewrite the test on the device state. Having the
> logic expressed in several places would make reworking the device states more
> complicated than necessary if it were to happen (just as you did when switching
> the test from !(ATTACHED || REMOVED) to (UNUSED).

About the intent, you are right.
About the solution, it seems buggy. We can try to find another way
of coding this loop by using rte_eth_find_next()
and adding the parent condition.
  
Ferruh Yigit March 19, 2019, 3:47 p.m. UTC | #4
On 2/20/2019 10:10 PM, Thomas Monjalon wrote:
> If multiple ports share the same hardware device (rte_device),
> they are siblings and can be found thanks to the new functions
> and loop macros.
> One iterator takes a port id as reference,
> while the other one directly refers to the parent device.
> 
> The ownership is not checked because siblings may have
> different owners.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  lib/librte_ethdev/rte_ethdev.c           | 20 +++++++++++
>  lib/librte_ethdev/rte_ethdev.h           | 46 ++++++++++++++++++++++++
>  lib/librte_ethdev/rte_ethdev_version.map |  2 ++
>  3 files changed, 68 insertions(+)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index b3b2fb1dba..42154787f8 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -340,6 +340,26 @@ rte_eth_find_next(uint16_t port_id)
>  	return port_id;
>  }
>  
> +uint16_t __rte_experimental

Do we need _rte_experimental on function definitions? I guess only in .h file,
function declaration is enough.

> +rte_eth_find_next_of(uint16_t port_id, const struct rte_device *parent)

Out of curiosity, what '_of' refers to?

> +{
> +	while (port_id < RTE_MAX_ETHPORTS &&
> +			rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED &&
> +			rte_eth_devices[port_id].device != parent)
> +		port_id++;

Is this logic correct, or am I missing something.
When port status is ATTACHED, check will return false and exit from loop without
checking if the 'device' is same.
+1 to Gaetan's suggestion to use 'rte_eth_find_next()', which moves status
concern to that function.

> +
> +	if (port_id >= RTE_MAX_ETHPORTS)
> +		return RTE_MAX_ETHPORTS;
> +
> +	return port_id;
> +}
> +
> +uint16_t __rte_experimental
> +rte_eth_find_next_sibling(uint16_t port_id, uint16_t ref)

I think better to say 'ref_port_id' to clarify what we expect here is a port_id

> +{
> +	return rte_eth_find_next_of(port_id, rte_eth_devices[ref].device);

This is a public API, shouldn't we check if 'ref' if valid port_id value, before
accessing the '.device' field?

> +}
> +
>  static void
>  rte_eth_dev_shared_data_prepare(void)
>  {
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index a3c864a134..a7c5c36277 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1383,6 +1383,52 @@ uint16_t rte_eth_find_next(uint16_t port_id);
>  #define RTE_ETH_FOREACH_DEV(p) \
>  	RTE_ETH_FOREACH_DEV_OWNED_BY(p, RTE_ETH_DEV_NO_OWNER)
>  
> +/**
> + * Iterates over ethdev ports of a specified device.
> + *
> + * @param port_id_start
> + *   The id of the next possible valid port.
> + * @param parent
> + *   The generic device behind the ports to iterate.
> + * @return
> + *   Next port id of the device, RTE_MAX_ETHPORTS if there is none.

Can return 'port_id_start', right? Should it be documented as it is done in
below 'next_sibling' one?

> + */
> +__rte_experimental
> +uint16_t rte_eth_find_next_of(uint16_t port_id_start,
> +		const struct rte_device *parent);
> +
> +/**
> + * Macro to iterate over all ethdev ports sharing the same rte_device
> + * as the specified port.

'specified port'? No port specified, a device pointer is specified.

> + * Note: the specified port is part of the loop iterations.
> + */

Does it make sense to clarify what 'p' is and what 'parent' is as we do in
function doxygen comments? Since these are macros, harder to grasp the types, I
think better to describe more in macro documentation.

> +#define RTE_ETH_FOREACH_DEV_OF(p, parent) \
> +	for (p = rte_eth_find_next_of(0, parent); \
> +		p < RTE_MAX_ETHPORTS; \
> +		p = rte_eth_find_next_of(p + 1, parent))
> +
> +/**
> + * Iterates over sibling ethdev ports (i.e. sharing the same rte_device).
> + *
> + * @param port_id_start
> + *   The id of the next possible valid sibling port.
> + * @param ref
> + *   The id of a reference port to compare rte_device with.
> + * @return
> + *   Next sibling port id (or ref itself), RTE_MAX_ETHPORTS if there is none.
> + */
> +__rte_experimental
> +uint16_t rte_eth_find_next_sibling(uint16_t port_id_start, uint16_t ref);
> +
> +/**
> + * Macro to iterate over all ethdev ports sharing the same rte_device
> + * as the specified port.
> + * Note: the specified port is part of the loop iterations.
> + */
> +#define RTE_ETH_FOREACH_DEV_SIBLING(p, ref) \
> +	for (p = rte_eth_find_next_sibling(0, ref); \
> +		p < RTE_MAX_ETHPORTS; \
> +		p = rte_eth_find_next_sibling(p + 1, ref))
>  
>  /**
>   * @warning
> diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
> index 92ac3de250..b37a4167d7 100644
> --- a/lib/librte_ethdev/rte_ethdev_version.map
> +++ b/lib/librte_ethdev/rte_ethdev_version.map
> @@ -245,6 +245,8 @@ EXPERIMENTAL {
>  	rte_eth_dev_owner_set;
>  	rte_eth_dev_owner_unset;
>  	rte_eth_dev_rx_intr_ctl_q_get_fd;
> +	rte_eth_find_next_of;
> +	rte_eth_find_next_sibling;
>  	rte_eth_switch_domain_alloc;
>  	rte_eth_switch_domain_free;
>  	rte_flow_conv;
>
  
Thomas Monjalon March 19, 2019, 5:34 p.m. UTC | #5
19/03/2019 16:47, Ferruh Yigit:
> On 2/20/2019 10:10 PM, Thomas Monjalon wrote:
> > If multiple ports share the same hardware device (rte_device),
> > they are siblings and can be found thanks to the new functions
> > and loop macros.
> > One iterator takes a port id as reference,
> > while the other one directly refers to the parent device.
> > 
> > The ownership is not checked because siblings may have
> > different owners.
> > 
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > ---
> >  lib/librte_ethdev/rte_ethdev.c           | 20 +++++++++++
> >  lib/librte_ethdev/rte_ethdev.h           | 46 ++++++++++++++++++++++++
> >  lib/librte_ethdev/rte_ethdev_version.map |  2 ++
> >  3 files changed, 68 insertions(+)
> > 
> > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> > index b3b2fb1dba..42154787f8 100644
> > --- a/lib/librte_ethdev/rte_ethdev.c
> > +++ b/lib/librte_ethdev/rte_ethdev.c
> > @@ -340,6 +340,26 @@ rte_eth_find_next(uint16_t port_id)
> >  	return port_id;
> >  }
> >  
> > +uint16_t __rte_experimental
> 
> Do we need _rte_experimental on function definitions? I guess only in .h file,
> function declaration is enough.

Yes we need them both in .h and .c files.

> > +rte_eth_find_next_of(uint16_t port_id, const struct rte_device *parent)
> 
> Out of curiosity, what '_of' refers to?

It means "next port of parent".
Is there a better word than "of"?

> > +{
> > +	while (port_id < RTE_MAX_ETHPORTS &&
> > +			rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED &&
> > +			rte_eth_devices[port_id].device != parent)
> > +		port_id++;
> 
> Is this logic correct, or am I missing something.
> When port status is ATTACHED, check will return false and exit from loop without
> checking if the 'device' is same.

Oops, I think you are right.

> +1 to Gaetan's suggestion to use 'rte_eth_find_next()', which moves status
> concern to that function.

OK, will change.

> > +
> > +	if (port_id >= RTE_MAX_ETHPORTS)
> > +		return RTE_MAX_ETHPORTS;
> > +
> > +	return port_id;
> > +}
> > +
> > +uint16_t __rte_experimental
> > +rte_eth_find_next_sibling(uint16_t port_id, uint16_t ref)
> 
> I think better to say 'ref_port_id' to clarify what we expect here is a port_id

OK

> > +{
> > +	return rte_eth_find_next_of(port_id, rte_eth_devices[ref].device);
> 
> This is a public API, shouldn't we check if 'ref' if valid port_id value, before
> accessing the '.device' field?

I'm not a fan of extra checks, but yes we may add one.

> > --- a/lib/librte_ethdev/rte_ethdev.h
> > +++ b/lib/librte_ethdev/rte_ethdev.h
> > +/**
> > + * Iterates over ethdev ports of a specified device.
> > + *
> > + * @param port_id_start
> > + *   The id of the next possible valid port.
> > + * @param parent
> > + *   The generic device behind the ports to iterate.
> > + * @return
> > + *   Next port id of the device, RTE_MAX_ETHPORTS if there is none.
> 
> Can return 'port_id_start', right? Should it be documented as it is done in
> below 'next_sibling' one?

Yes, OK.

> > + */
> > +__rte_experimental
> > +uint16_t rte_eth_find_next_of(uint16_t port_id_start,
> > +		const struct rte_device *parent);
> > +
> > +/**
> > + * Macro to iterate over all ethdev ports sharing the same rte_device
> > + * as the specified port.
> 
> 'specified port'? No port specified, a device pointer is specified.

Right, looks like a wrong a copy/paste.

> > + * Note: the specified port is part of the loop iterations.
> > + */
> 
> Does it make sense to clarify what 'p' is and what 'parent' is as we do in
> function doxygen comments? Since these are macros, harder to grasp the types, I
> think better to describe more in macro documentation.

Absolutely, yes. I thought I did it.

> > +#define RTE_ETH_FOREACH_DEV_OF(p, parent) \
> > +	for (p = rte_eth_find_next_of(0, parent); \
> > +		p < RTE_MAX_ETHPORTS; \
> > +		p = rte_eth_find_next_of(p + 1, parent))
> > +
> > +/**
> > + * Iterates over sibling ethdev ports (i.e. sharing the same rte_device).
> > + *
> > + * @param port_id_start
> > + *   The id of the next possible valid sibling port.
> > + * @param ref
> > + *   The id of a reference port to compare rte_device with.
> > + * @return
> > + *   Next sibling port id (or ref itself), RTE_MAX_ETHPORTS if there is none.
> > + */
> > +__rte_experimental
> > +uint16_t rte_eth_find_next_sibling(uint16_t port_id_start, uint16_t ref);
> > +
> > +/**
> > + * Macro to iterate over all ethdev ports sharing the same rte_device
> > + * as the specified port.
> > + * Note: the specified port is part of the loop iterations.
> > + */
> > +#define RTE_ETH_FOREACH_DEV_SIBLING(p, ref) \
> > +	for (p = rte_eth_find_next_sibling(0, ref); \
> > +		p < RTE_MAX_ETHPORTS; \
> > +		p = rte_eth_find_next_sibling(p + 1, ref))

Thanks for the complete review Ferruh.
  
Ferruh Yigit March 19, 2019, 6:04 p.m. UTC | #6
On 3/19/2019 5:34 PM, Thomas Monjalon wrote:
>>> +uint16_t __rte_experimental
>> Do we need _rte_experimental on function definitions? I guess only in .h file,
>> function declaration is enough.
> Yes we need them both in .h and .c files.
> 

Why we need them in .c file?
I think the compiler is interested in ones in .h file, because of the
experimental checks.
  
Thomas Monjalon April 1, 2019, 1:59 a.m. UTC | #7
27/02/2019 11:51, Thomas Monjalon:
> 27/02/2019 11:07, Gaëtan Rivet:
> > On Wed, Feb 20, 2019 at 11:10:49PM +0100, Thomas Monjalon wrote:
> > > +uint16_t __rte_experimental
> > > +rte_eth_find_next_of(uint16_t port_id, const struct rte_device *parent)
> > > +{
> > > +	while (port_id < RTE_MAX_ETHPORTS &&
> > > +			rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED &&
> > > +			rte_eth_devices[port_id].device != parent)
> > > +		port_id++;
> > 
> > Why not call rte_eth_find_next directly from this function, and
> > add your specific test on top of it?
> > 
> > Something like:
> > 
> >     	while (port_id < RTE_MAX_ETHPORTS &&
> >     	       rte_eth_devices[port_id].device != parent)
> >     		port_id = rte_eth_find_next(port_id + 1);
> > 
> > this way you won't have to rewrite the test on the device state. Having the
> > logic expressed in several places would make reworking the device states more
> > complicated than necessary if it were to happen (just as you did when switching
> > the test from !(ATTACHED || REMOVED) to (UNUSED).
> 
> About the intent, you are right.
> About the solution, it seems buggy. We can try to find another way
> of coding this loop by using rte_eth_find_next()
> and adding the parent condition.

Your proposal is correct if adding a first call before the loop:
	port_id = rte_eth_find_next(port_id);
  
Thomas Monjalon April 1, 2019, 2:16 a.m. UTC | #8
19/03/2019 19:04, Ferruh Yigit:
> On 3/19/2019 5:34 PM, Thomas Monjalon wrote:
> >>> +uint16_t __rte_experimental
> >> 
> >> Do we need _rte_experimental on function definitions? I guess only in .h file,
> >> function declaration is enough.
> > 
> > Yes we need them both in .h and .c files.
> 
> Why we need them in .c file?
> I think the compiler is interested in ones in .h file, because of the
> experimental checks.

We need the tag in .c file because a check is done in the ELF object
by buildtools/check-experimental-syms.sh

David tried a replacement of this script to run on header files,
but it looks a bit slow:
	https://patches.dpdk.org/patch/49118/
  
David Marchand April 1, 2019, 6:46 a.m. UTC | #9
On Mon, Apr 1, 2019 at 4:16 AM Thomas Monjalon <thomas@monjalon.net> wrote:

> 19/03/2019 19:04, Ferruh Yigit:
> > On 3/19/2019 5:34 PM, Thomas Monjalon wrote:
> > >>> +uint16_t __rte_experimental
> > >>
> > >> Do we need _rte_experimental on function definitions? I guess only in
> .h file,
> > >> function declaration is enough.
> > >
> > > Yes we need them both in .h and .c files.
> >
> > Why we need them in .c file?
> > I think the compiler is interested in ones in .h file, because of the
> > experimental checks.
>
> We need the tag in .c file because a check is done in the ELF object
> by buildtools/check-experimental-syms.sh
>

?
The attribute should be inherited from the declaration in the header.
If you have a case where it does not work, I'd like to look at it.



> David tried a replacement of this script to run on header files,
> but it looks a bit slow:
>         https://patches.dpdk.org/patch/49118/


Never got any review/comment, will do a quick update in this thread.
  
Thomas Monjalon April 1, 2019, 8:09 a.m. UTC | #10
01/04/2019 08:46, David Marchand:
> On Mon, Apr 1, 2019 at 4:16 AM Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> > 19/03/2019 19:04, Ferruh Yigit:
> > > On 3/19/2019 5:34 PM, Thomas Monjalon wrote:
> > > >>> +uint16_t __rte_experimental
> > > >>
> > > >> Do we need _rte_experimental on function definitions? I guess only in
> > .h file,
> > > >> function declaration is enough.
> > > >
> > > > Yes we need them both in .h and .c files.
> > >
> > > Why we need them in .c file?
> > > I think the compiler is interested in ones in .h file, because of the
> > > experimental checks.
> >
> > We need the tag in .c file because a check is done in the ELF object
> > by buildtools/check-experimental-syms.sh
> >
> 
> ?
> The attribute should be inherited from the declaration in the header.
> If you have a case where it does not work, I'd like to look at it.

I don't know such case, it was just a belief.

If we can confirm it works well with tag in headers only,
I suggest we remove all of them at once.
For this patch, I prefer being on the safe side for now.
  
Ferruh Yigit April 2, 2019, 11:35 p.m. UTC | #11
On 4/1/2019 9:09 AM, Thomas Monjalon wrote:
> 01/04/2019 08:46, David Marchand:
>> On Mon, Apr 1, 2019 at 4:16 AM Thomas Monjalon <thomas@monjalon.net> wrote:
>>
>>> 19/03/2019 19:04, Ferruh Yigit:
>>>> On 3/19/2019 5:34 PM, Thomas Monjalon wrote:
>>>>>>> +uint16_t __rte_experimental
>>>>>>
>>>>>> Do we need _rte_experimental on function definitions? I guess only in
>>> .h file,
>>>>>> function declaration is enough.
>>>>>
>>>>> Yes we need them both in .h and .c files.
>>>>
>>>> Why we need them in .c file?
>>>> I think the compiler is interested in ones in .h file, because of the
>>>> experimental checks.
>>>
>>> We need the tag in .c file because a check is done in the ELF object
>>> by buildtools/check-experimental-syms.sh
>>>
>>
>> ?
>> The attribute should be inherited from the declaration in the header.
>> If you have a case where it does not work, I'd like to look at it.
> 
> I don't know such case, it was just a belief.

Putting the __rte_experimental tag into header files should be sufficient.

- buildtools/check-experimental-syms.sh
checks if symbols in .map file are marked with __rte_experimental, both putting
the tag to the symbol in .c file or .h file is working.

- tag should be in header file so that application using it can get the warning.

Briefly, __rte_experimental needs to be in .h file, it is optional to have it in
.c, I am for keeping it only in .h file function declaration.

> 
> If we can confirm it works well with tag in headers only,
> I suggest we remove all of them at once.
> For this patch, I prefer being on the safe side for now.
> 
>
  
Thomas Monjalon April 2, 2019, 11:37 p.m. UTC | #12
03/04/2019 01:35, Ferruh Yigit:
> On 4/1/2019 9:09 AM, Thomas Monjalon wrote:
> > 01/04/2019 08:46, David Marchand:
> >> On Mon, Apr 1, 2019 at 4:16 AM Thomas Monjalon <thomas@monjalon.net> wrote:
> >>
> >>> 19/03/2019 19:04, Ferruh Yigit:
> >>>> On 3/19/2019 5:34 PM, Thomas Monjalon wrote:
> >>>>>>> +uint16_t __rte_experimental
> >>>>>>
> >>>>>> Do we need _rte_experimental on function definitions? I guess only in
> >>> .h file,
> >>>>>> function declaration is enough.
> >>>>>
> >>>>> Yes we need them both in .h and .c files.
> >>>>
> >>>> Why we need them in .c file?
> >>>> I think the compiler is interested in ones in .h file, because of the
> >>>> experimental checks.
> >>>
> >>> We need the tag in .c file because a check is done in the ELF object
> >>> by buildtools/check-experimental-syms.sh
> >>>
> >>
> >> ?
> >> The attribute should be inherited from the declaration in the header.
> >> If you have a case where it does not work, I'd like to look at it.
> > 
> > I don't know such case, it was just a belief.
> 
> Putting the __rte_experimental tag into header files should be sufficient.
> 
> - buildtools/check-experimental-syms.sh
> checks if symbols in .map file are marked with __rte_experimental, both putting
> the tag to the symbol in .c file or .h file is working.
> 
> - tag should be in header file so that application using it can get the warning.
> 
> Briefly, __rte_experimental needs to be in .h file, it is optional to have it in
> .c, I am for keeping it only in .h file function declaration.

I agree
As I said below, we can remove all experimental tags from .c files
in a separate patch.

> > If we can confirm it works well with tag in headers only,
> > I suggest we remove all of them at once.
> > For this patch, I prefer being on the safe side for now.
  

Patch

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index b3b2fb1dba..42154787f8 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -340,6 +340,26 @@  rte_eth_find_next(uint16_t port_id)
 	return port_id;
 }
 
+uint16_t __rte_experimental
+rte_eth_find_next_of(uint16_t port_id, const struct rte_device *parent)
+{
+	while (port_id < RTE_MAX_ETHPORTS &&
+			rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED &&
+			rte_eth_devices[port_id].device != parent)
+		port_id++;
+
+	if (port_id >= RTE_MAX_ETHPORTS)
+		return RTE_MAX_ETHPORTS;
+
+	return port_id;
+}
+
+uint16_t __rte_experimental
+rte_eth_find_next_sibling(uint16_t port_id, uint16_t ref)
+{
+	return rte_eth_find_next_of(port_id, rte_eth_devices[ref].device);
+}
+
 static void
 rte_eth_dev_shared_data_prepare(void)
 {
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index a3c864a134..a7c5c36277 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1383,6 +1383,52 @@  uint16_t rte_eth_find_next(uint16_t port_id);
 #define RTE_ETH_FOREACH_DEV(p) \
 	RTE_ETH_FOREACH_DEV_OWNED_BY(p, RTE_ETH_DEV_NO_OWNER)
 
+/**
+ * Iterates over ethdev ports of a specified device.
+ *
+ * @param port_id_start
+ *   The id of the next possible valid port.
+ * @param parent
+ *   The generic device behind the ports to iterate.
+ * @return
+ *   Next port id of the device, RTE_MAX_ETHPORTS if there is none.
+ */
+__rte_experimental
+uint16_t rte_eth_find_next_of(uint16_t port_id_start,
+		const struct rte_device *parent);
+
+/**
+ * Macro to iterate over all ethdev ports sharing the same rte_device
+ * as the specified port.
+ * Note: the specified port is part of the loop iterations.
+ */
+#define RTE_ETH_FOREACH_DEV_OF(p, parent) \
+	for (p = rte_eth_find_next_of(0, parent); \
+		p < RTE_MAX_ETHPORTS; \
+		p = rte_eth_find_next_of(p + 1, parent))
+
+/**
+ * Iterates over sibling ethdev ports (i.e. sharing the same rte_device).
+ *
+ * @param port_id_start
+ *   The id of the next possible valid sibling port.
+ * @param ref
+ *   The id of a reference port to compare rte_device with.
+ * @return
+ *   Next sibling port id (or ref itself), RTE_MAX_ETHPORTS if there is none.
+ */
+__rte_experimental
+uint16_t rte_eth_find_next_sibling(uint16_t port_id_start, uint16_t ref);
+
+/**
+ * Macro to iterate over all ethdev ports sharing the same rte_device
+ * as the specified port.
+ * Note: the specified port is part of the loop iterations.
+ */
+#define RTE_ETH_FOREACH_DEV_SIBLING(p, ref) \
+	for (p = rte_eth_find_next_sibling(0, ref); \
+		p < RTE_MAX_ETHPORTS; \
+		p = rte_eth_find_next_sibling(p + 1, ref))
 
 /**
  * @warning
diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
index 92ac3de250..b37a4167d7 100644
--- a/lib/librte_ethdev/rte_ethdev_version.map
+++ b/lib/librte_ethdev/rte_ethdev_version.map
@@ -245,6 +245,8 @@  EXPERIMENTAL {
 	rte_eth_dev_owner_set;
 	rte_eth_dev_owner_unset;
 	rte_eth_dev_rx_intr_ctl_q_get_fd;
+	rte_eth_find_next_of;
+	rte_eth_find_next_sibling;
 	rte_eth_switch_domain_alloc;
 	rte_eth_switch_domain_free;
 	rte_flow_conv;