diff mbox series

[v1,05/13] ethdev: add private generic device iterator

Message ID e42237129b4191c629a711cf6081b1045bc83986.1535633784.git.gaetan.rivet@6wind.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series Implement new devargs framework |

Checks

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

Commit Message

Gaëtan Rivet Aug. 30, 2018, 1:41 p.m. UTC 
  This iterator can be customized with a comparison function that will
trigger a stopping condition.

It can be leveraged to write several different iterators that have
similar but non-identical purposes.

It is private to librte_ethdev.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_ethdev/Makefile      |  1 +
 lib/librte_ethdev/eth_private.c | 31 +++++++++++++++++++++++++++++++
 lib/librte_ethdev/eth_private.h | 26 ++++++++++++++++++++++++++
 lib/librte_ethdev/meson.build   |  3 ++-
 4 files changed, 60 insertions(+), 1 deletion(-)
 create mode 100644 lib/librte_ethdev/eth_private.c
 create mode 100644 lib/librte_ethdev/eth_private.h

Comments

Andrew Rybchenko Aug. 31, 2018, 10:09 a.m. UTC | #1 
On 08/30/2018 04:41 PM, Gaetan Rivet wrote:
> This iterator can be customized with a comparison function that will
> trigger a stopping condition.
>
> It can be leveraged to write several different iterators that have
> similar but non-identical purposes.
>
> It is private to librte_ethdev.
>
> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> ---
>   lib/librte_ethdev/Makefile      |  1 +
>   lib/librte_ethdev/eth_private.c | 31 +++++++++++++++++++++++++++++++
>   lib/librte_ethdev/eth_private.h | 26 ++++++++++++++++++++++++++
>   lib/librte_ethdev/meson.build   |  3 ++-
>   4 files changed, 60 insertions(+), 1 deletion(-)
>   create mode 100644 lib/librte_ethdev/eth_private.c
>   create mode 100644 lib/librte_ethdev/eth_private.h
>
> diff --git a/lib/librte_ethdev/Makefile b/lib/librte_ethdev/Makefile
> index 0935a275e..3c1c92cb9 100644
> --- a/lib/librte_ethdev/Makefile
> +++ b/lib/librte_ethdev/Makefile
> @@ -18,6 +18,7 @@ EXPORT_MAP := rte_ethdev_version.map
>   
>   LIBABIVER := 10
>   
> +SRCS-y += eth_private.c
>   SRCS-y += rte_ethdev.c
>   SRCS-y += rte_flow.c
>   SRCS-y += rte_tm.c
> diff --git a/lib/librte_ethdev/eth_private.c b/lib/librte_ethdev/eth_private.c
> new file mode 100644
> index 000000000..d565568a0
> --- /dev/null
> +++ b/lib/librte_ethdev/eth_private.c

Just a nit
I think it is better to name it ethdev_private.c since we already
have ethdev_profile.[ch] and it is about ethdev, not Ethernet.

> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2018 Gaëtan Rivet
> + */
> +
> +#include "rte_ethdev.h"
> +#include "eth_private.h"
> +
> +struct rte_eth_dev *
> +eth_find_device(const struct rte_eth_dev *start, rte_eth_cmp_t cmp,
> +		const void *data)
> +{
> +	struct rte_eth_dev *edev;
> +	ptrdiff_t idx;
> +
> +	/* Avoid Undefined Behaviour */
> +	if (start != NULL &&
> +	    (start < &rte_eth_devices[0] ||
> +	     start > &rte_eth_devices[RTE_MAX_ETHPORTS]))
> +		return NULL;
> +	if (start != NULL)
> +		idx = start - &rte_eth_devices[0] + 1;
> +	else
> +		idx = 0;
> +	for (; idx < RTE_MAX_ETHPORTS; idx++) {
> +		edev = &rte_eth_devices[idx];

Shouldn't we limit it to valid ports only?
If no, I think it would be useful to highlight it in the function
description that it iterates over all devices including unused.

> +		if (cmp(edev, data) == 0)
> +			return edev;
> +	}
> +	return NULL;
> +}
> +
> diff --git a/lib/librte_ethdev/eth_private.h b/lib/librte_ethdev/eth_private.h
> new file mode 100644
> index 000000000..0f5c6d5c4
> --- /dev/null
> +++ b/lib/librte_ethdev/eth_private.h

ethdev_private.h

<...>
Gaëtan Rivet Aug. 31, 2018, 10:22 a.m. UTC | #2 
On Fri, Aug 31, 2018 at 01:09:34PM +0300, Andrew Rybchenko wrote:
> On 08/30/2018 04:41 PM, Gaetan Rivet wrote:
> > This iterator can be customized with a comparison function that will
> > trigger a stopping condition.
> > 
> > It can be leveraged to write several different iterators that have
> > similar but non-identical purposes.
> > 
> > It is private to librte_ethdev.
> > 
> > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> > ---
> >   lib/librte_ethdev/Makefile      |  1 +
> >   lib/librte_ethdev/eth_private.c | 31 +++++++++++++++++++++++++++++++
> >   lib/librte_ethdev/eth_private.h | 26 ++++++++++++++++++++++++++
> >   lib/librte_ethdev/meson.build   |  3 ++-
> >   4 files changed, 60 insertions(+), 1 deletion(-)
> >   create mode 100644 lib/librte_ethdev/eth_private.c
> >   create mode 100644 lib/librte_ethdev/eth_private.h
> > 
> > diff --git a/lib/librte_ethdev/Makefile b/lib/librte_ethdev/Makefile
> > index 0935a275e..3c1c92cb9 100644
> > --- a/lib/librte_ethdev/Makefile
> > +++ b/lib/librte_ethdev/Makefile
> > @@ -18,6 +18,7 @@ EXPORT_MAP := rte_ethdev_version.map
> >   LIBABIVER := 10
> > +SRCS-y += eth_private.c
> >   SRCS-y += rte_ethdev.c
> >   SRCS-y += rte_flow.c
> >   SRCS-y += rte_tm.c
> > diff --git a/lib/librte_ethdev/eth_private.c b/lib/librte_ethdev/eth_private.c
> > new file mode 100644
> > index 000000000..d565568a0
> > --- /dev/null
> > +++ b/lib/librte_ethdev/eth_private.c
> 
> Just a nit
> I think it is better to name it ethdev_private.c since we already
> have ethdev_profile.[ch] and it is about ethdev, not Ethernet.
> 

Agreed.

> > @@ -0,0 +1,31 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2018 Gaëtan Rivet
> > + */
> > +
> > +#include "rte_ethdev.h"
> > +#include "eth_private.h"
> > +
> > +struct rte_eth_dev *
> > +eth_find_device(const struct rte_eth_dev *start, rte_eth_cmp_t cmp,
> > +		const void *data)
> > +{
> > +	struct rte_eth_dev *edev;
> > +	ptrdiff_t idx;
> > +
> > +	/* Avoid Undefined Behaviour */
> > +	if (start != NULL &&
> > +	    (start < &rte_eth_devices[0] ||
> > +	     start > &rte_eth_devices[RTE_MAX_ETHPORTS]))
> > +		return NULL;
> > +	if (start != NULL)
> > +		idx = start - &rte_eth_devices[0] + 1;
> > +	else
> > +		idx = 0;
> > +	for (; idx < RTE_MAX_ETHPORTS; idx++) {
> > +		edev = &rte_eth_devices[idx];
> 
> Shouldn't we limit it to valid ports only?
> If no, I think it would be useful to highlight it in the function
> description that it iterates over all devices including unused.
> 

I'm undecided about it, I wanted eth_find_device to be and internal
operator in the most generic way, allowing ethdev layer to move from
manually iterating to using this, to introduce the probable future
evolution of not using an array of rte_eth_devices.

So I thought of this operator as something stateless, only looking at
currently available memory and letting the comparison select what is
useful. Maybe for example this operator would be used to find the next
unused device, etc.

My doubts is that this is kind of future-proofing the design, something
that I don't think is good practice.

So, if you prefer something that takes care of state, as far as devargs
parsing is concerned, I'm ok with that. Otherwise it can stay that way,
UNUSED devices are taken care of afterward.

> > +		if (cmp(edev, data) == 0)
> > +			return edev;
> > +	}
> > +	return NULL;
> > +}
> > +
> > diff --git a/lib/librte_ethdev/eth_private.h b/lib/librte_ethdev/eth_private.h
> > new file mode 100644
> > index 000000000..0f5c6d5c4
> > --- /dev/null
> > +++ b/lib/librte_ethdev/eth_private.h
> 
> ethdev_private.h

sure.

> 
> <...>
diff mbox series

Patch

diff --git a/lib/librte_ethdev/Makefile b/lib/librte_ethdev/Makefile
index 0935a275e..3c1c92cb9 100644
--- a/lib/librte_ethdev/Makefile
+++ b/lib/librte_ethdev/Makefile
@@ -18,6 +18,7 @@  EXPORT_MAP := rte_ethdev_version.map
 
 LIBABIVER := 10
 
+SRCS-y += eth_private.c
 SRCS-y += rte_ethdev.c
 SRCS-y += rte_flow.c
 SRCS-y += rte_tm.c
diff --git a/lib/librte_ethdev/eth_private.c b/lib/librte_ethdev/eth_private.c
new file mode 100644
index 000000000..d565568a0
--- /dev/null
+++ b/lib/librte_ethdev/eth_private.c
@@ -0,0 +1,31 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2018 Gaëtan Rivet
+ */
+
+#include "rte_ethdev.h"
+#include "eth_private.h"
+
+struct rte_eth_dev *
+eth_find_device(const struct rte_eth_dev *start, rte_eth_cmp_t cmp,
+		const void *data)
+{
+	struct rte_eth_dev *edev;
+	ptrdiff_t idx;
+
+	/* Avoid Undefined Behaviour */
+	if (start != NULL &&
+	    (start < &rte_eth_devices[0] ||
+	     start > &rte_eth_devices[RTE_MAX_ETHPORTS]))
+		return NULL;
+	if (start != NULL)
+		idx = start - &rte_eth_devices[0] + 1;
+	else
+		idx = 0;
+	for (; idx < RTE_MAX_ETHPORTS; idx++) {
+		edev = &rte_eth_devices[idx];
+		if (cmp(edev, data) == 0)
+			return edev;
+	}
+	return NULL;
+}
+
diff --git a/lib/librte_ethdev/eth_private.h b/lib/librte_ethdev/eth_private.h
new file mode 100644
index 000000000..0f5c6d5c4
--- /dev/null
+++ b/lib/librte_ethdev/eth_private.h
@@ -0,0 +1,26 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2018 Gaëtan Rivet
+ */
+
+#ifndef _RTE_ETH_PRIVATE_H_
+#define _RTE_ETH_PRIVATE_H_
+
+#include "rte_ethdev.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/* Generic rte_eth_dev comparison function. */
+typedef int (*rte_eth_cmp_t)(const struct rte_eth_dev *, const void *);
+
+/* Generic rte_eth_dev iterator. */
+struct rte_eth_dev *
+eth_find_device(const struct rte_eth_dev *_start, rte_eth_cmp_t cmp,
+		const void *data);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* _RTE_ETH_PRIVATE_H_ */
diff --git a/lib/librte_ethdev/meson.build b/lib/librte_ethdev/meson.build
index 596cd0f39..372d3ca06 100644
--- a/lib/librte_ethdev/meson.build
+++ b/lib/librte_ethdev/meson.build
@@ -4,7 +4,8 @@ 
 name = 'ethdev'
 version = 10
 allow_experimental_apis = true
-sources = files('ethdev_profile.c',
+sources = files('eth_private.c',
+	'ethdev_profile.c',
 	'rte_ethdev.c',
 	'rte_flow.c',
 	'rte_mtr.c',