[dpdk-dev,v2] ethdev: allow all ports event registration
Checks
Commit Message
Add option to register event callback for all ports by one call to
rte_eth_dev_callback_register using port_id=RTE_ETH_ALL.
In this case the callback is also registered to invalid ports.
Signed-off-by: Matan Azrad <matan@mellanox.com>
---
lib/librte_ether/rte_ethdev.c | 124 +++++++++++++++++++++++++++++-------------
lib/librte_ether/rte_ethdev.h | 8 ++-
2 files changed, 91 insertions(+), 41 deletions(-)
V2:
Move the ports cb lists initialization to dpdk init time (before the main calling).
Comments
Hi Matan,
Please find some review details below.
As this patch is needed for the notification of new ports,
I will re-send them in a patchset, with the minor modifications
described below.
04/12/2017 16:43, Matan Azrad:
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> +RTE_INIT(eth_dev_init_cb_lists);
[...]
> +static void
> +eth_dev_init_cb_lists(void)
RTE_INIT macro can be used in function definition
without prior declaration.
This function should be moved just before the callback
register/unregister functions.
> @@ -2827,37 +2837,59 @@
> + uint32_t next_port;
> + uint32_t last_port;
A port id should be uint16_t.
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> /**
> - * Register a callback function for specific port id.
> + * Register a callback function for port id event.
[...]
> - * Unregister a callback function for specific port id.
> + * Unregister a callback function for port id event.
"port event" would be more appropriate than "port id event".
Hi Thomas
From: Thomas Monjalon, December 29, 2017
> Hi Matan,
>
> Please find some review details below.
>
> As this patch is needed for the notification of new ports, I will re-send them
> in a patchset, with the minor modifications described below.
>
> 04/12/2017 16:43, Matan Azrad:
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > +RTE_INIT(eth_dev_init_cb_lists);
> [...]
> > +static void
> > +eth_dev_init_cb_lists(void)
>
> RTE_INIT macro can be used in function definition without prior declaration.
>
> This function should be moved just before the callback register/unregister
> functions.
>
OK, nice.
> > @@ -2827,37 +2837,59 @@
> > + uint32_t next_port;
> > + uint32_t last_port;
>
> A port id should be uint16_t.
>
Yes, I know but please note that we use next_port variable in the while statement and it can be rolled in case the max value of port id is the max value of uint16_t type.
This is the reason I defined it as uint32_t type.
> > --- a/lib/librte_ether/rte_ethdev.h
> > +++ b/lib/librte_ether/rte_ethdev.h
> > /**
> > - * Register a callback function for specific port id.
> > + * Register a callback function for port id event.
> [...]
> > - * Unregister a callback function for specific port id.
> > + * Unregister a callback function for port id event.
>
> "port event" would be more appropriate than "port id event".
While this change is relevant even before this patch, it is fine to add it here.
Thanks.
30/12/2017 20:04, Matan Azrad:
> From: Thomas Monjalon, December 29, 2017
> > 04/12/2017 16:43, Matan Azrad:
> > > @@ -2827,37 +2837,59 @@
> > > + uint32_t next_port;
> > > + uint32_t last_port;
> >
> > A port id should be uint16_t.
> >
> Yes, I know but please note that we use next_port variable in the while statement and it can be rolled in case the max value of port id is the max value of uint16_t type.
> This is the reason I defined it as uint32_t type.
OK, so I suggest 2 possible fixes:
- either keep uint32_t with a comment in the code.
- or use a for loop
What do you prefer?
From: Thomas Monjalon, December 30, 2017 10:51 PM
> 30/12/2017 20:04, Matan Azrad:
> > From: Thomas Monjalon, December 29, 2017
> > > 04/12/2017 16:43, Matan Azrad:
> > > > @@ -2827,37 +2837,59 @@
> > > > + uint32_t next_port;
> > > > + uint32_t last_port;
> > >
> > > A port id should be uint16_t.
> > >
> > Yes, I know but please note that we use next_port variable in the while
> statement and it can be rolled in case the max value of port id is the max
> value of uint16_t type.
> > This is the reason I defined it as uint32_t type.
>
> OK, so I suggest 2 possible fixes:
> - either keep uint32_t with a comment in the code.
> - or use a for loop
>
> What do you prefer?
The comment :)
@@ -73,6 +73,8 @@
static struct rte_eth_dev_data *rte_eth_dev_data;
static uint8_t eth_dev_last_created_port;
+RTE_INIT(eth_dev_init_cb_lists);
+
/* spinlock for eth device callbacks */
static rte_spinlock_t rte_eth_dev_cb_lock = RTE_SPINLOCK_INITIALIZER;
@@ -204,13 +206,21 @@ struct rte_eth_dev *
eth_dev->data = &rte_eth_dev_data[port_id];
eth_dev->state = RTE_ETH_DEV_ATTACHED;
- TAILQ_INIT(&(eth_dev->link_intr_cbs));
eth_dev_last_created_port = port_id;
return eth_dev;
}
+static void
+eth_dev_init_cb_lists(void)
+{
+ int i;
+
+ for (i = 0; i < RTE_MAX_ETHPORTS; i++)
+ TAILQ_INIT(&rte_eth_devices[i].link_intr_cbs);
+}
+
struct rte_eth_dev *
rte_eth_dev_allocate(const char *name)
{
@@ -2827,37 +2837,59 @@ int rte_eth_set_queue_rate_limit(uint16_t port_id, uint16_t queue_idx,
{
struct rte_eth_dev *dev;
struct rte_eth_dev_callback *user_cb;
+ uint32_t next_port;
+ uint32_t last_port;
if (!cb_fn)
return -EINVAL;
- RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
+ if (!rte_eth_dev_is_valid_port(port_id) && port_id != RTE_ETH_ALL) {
+ RTE_LOG(ERR, EAL, "Invalid port_id=%d\n", port_id);
+ return -EINVAL;
+ }
+
+ if (port_id == RTE_ETH_ALL) {
+ next_port = 0;
+ last_port = RTE_MAX_ETHPORTS - 1;
+ } else {
+ next_port = last_port = port_id;
+ }
- dev = &rte_eth_devices[port_id];
rte_spinlock_lock(&rte_eth_dev_cb_lock);
- TAILQ_FOREACH(user_cb, &(dev->link_intr_cbs), next) {
- if (user_cb->cb_fn == cb_fn &&
- user_cb->cb_arg == cb_arg &&
- user_cb->event == event) {
- break;
+ do {
+ dev = &rte_eth_devices[next_port];
+
+ TAILQ_FOREACH(user_cb, &(dev->link_intr_cbs), next) {
+ if (user_cb->cb_fn == cb_fn &&
+ user_cb->cb_arg == cb_arg &&
+ user_cb->event == event) {
+ break;
+ }
}
- }
- /* create a new callback. */
- if (user_cb == NULL) {
- user_cb = rte_zmalloc("INTR_USER_CALLBACK",
- sizeof(struct rte_eth_dev_callback), 0);
- if (user_cb != NULL) {
- user_cb->cb_fn = cb_fn;
- user_cb->cb_arg = cb_arg;
- user_cb->event = event;
- TAILQ_INSERT_TAIL(&(dev->link_intr_cbs), user_cb, next);
+ /* create a new callback. */
+ if (user_cb == NULL) {
+ user_cb = rte_zmalloc("INTR_USER_CALLBACK",
+ sizeof(struct rte_eth_dev_callback), 0);
+ if (user_cb != NULL) {
+ user_cb->cb_fn = cb_fn;
+ user_cb->cb_arg = cb_arg;
+ user_cb->event = event;
+ TAILQ_INSERT_TAIL(&(dev->link_intr_cbs),
+ user_cb, next);
+ } else {
+ rte_spinlock_unlock(&rte_eth_dev_cb_lock);
+ rte_eth_dev_callback_unregister(port_id, event,
+ cb_fn, cb_arg);
+ return -ENOMEM;
+ }
+
}
- }
+ } while (++next_port <= last_port);
rte_spinlock_unlock(&rte_eth_dev_cb_lock);
- return (user_cb == NULL) ? -ENOMEM : 0;
+ return 0;
}
int
@@ -2868,36 +2900,50 @@ int rte_eth_set_queue_rate_limit(uint16_t port_id, uint16_t queue_idx,
int ret;
struct rte_eth_dev *dev;
struct rte_eth_dev_callback *cb, *next;
+ uint32_t next_port;
+ uint32_t last_port;
if (!cb_fn)
return -EINVAL;
- RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
+ if (!rte_eth_dev_is_valid_port(port_id) && port_id != RTE_ETH_ALL) {
+ RTE_LOG(ERR, EAL, "Invalid port_id=%d\n", port_id);
+ return -EINVAL;
+ }
+
+ if (port_id == RTE_ETH_ALL) {
+ next_port = 0;
+ last_port = RTE_MAX_ETHPORTS - 1;
+ } else {
+ next_port = last_port = port_id;
+ }
- dev = &rte_eth_devices[port_id];
rte_spinlock_lock(&rte_eth_dev_cb_lock);
- ret = 0;
- for (cb = TAILQ_FIRST(&dev->link_intr_cbs); cb != NULL; cb = next) {
+ do {
+ dev = &rte_eth_devices[next_port];
+ ret = 0;
+ for (cb = TAILQ_FIRST(&dev->link_intr_cbs); cb != NULL;
+ cb = next) {
- next = TAILQ_NEXT(cb, next);
+ next = TAILQ_NEXT(cb, next);
- if (cb->cb_fn != cb_fn || cb->event != event ||
- (cb->cb_arg != (void *)-1 &&
- cb->cb_arg != cb_arg))
- continue;
+ if (cb->cb_fn != cb_fn || cb->event != event ||
+ (cb->cb_arg != (void *)-1 && cb->cb_arg != cb_arg))
+ continue;
- /*
- * if this callback is not executing right now,
- * then remove it.
- */
- if (cb->active == 0) {
- TAILQ_REMOVE(&(dev->link_intr_cbs), cb, next);
- rte_free(cb);
- } else {
- ret = -EAGAIN;
+ /*
+ * if this callback is not executing right now,
+ * then remove it.
+ */
+ if (cb->active == 0) {
+ TAILQ_REMOVE(&(dev->link_intr_cbs), cb, next);
+ rte_free(cb);
+ } else {
+ ret = -EAGAIN;
+ }
}
- }
+ } while (++next_port <= last_port);
rte_spinlock_unlock(&rte_eth_dev_cb_lock);
return ret;
@@ -1137,6 +1137,8 @@ struct rte_eth_dcb_info {
struct rte_eth_dev;
+#define RTE_ETH_ALL RTE_MAX_ETHPORTS
+
struct rte_eth_dev_callback;
/** @internal Structure to keep track of registered callbacks */
TAILQ_HEAD(rte_eth_dev_cb_list, rte_eth_dev_callback);
@@ -3536,10 +3538,11 @@ typedef int (*rte_eth_dev_cb_fn)(uint16_t port_id,
/**
- * Register a callback function for specific port id.
+ * Register a callback function for port id event.
*
* @param port_id
* Port id.
+ * RTE_ETH_ALL means register the event for all port ids.
* @param event
* Event interested.
* @param cb_fn
@@ -3556,10 +3559,11 @@ int rte_eth_dev_callback_register(uint16_t port_id,
rte_eth_dev_cb_fn cb_fn, void *cb_arg);
/**
- * Unregister a callback function for specific port id.
+ * Unregister a callback function for port id event.
*
* @param port_id
* Port id.
+ * RTE_ETH_ALL means unregister the event for all port ids.
* @param event
* Event interested.
* @param cb_fn