[4/5] app/testpmd: move ethdev events registration
Checks
Commit Message
The callback for ethdev events was registered on port start,
so it was missing some events.
It is now registered at the beginning of the main function.
Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
app/test-pmd/testpmd.c | 72 ++++++++++++++++++++++++------------------
1 file changed, 42 insertions(+), 30 deletions(-)
Comments
Hi Thomas,
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, October 24, 2018 2:41 PM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Cc: dev@dpdk.org; ophirmu@mellanox.com; wisamm@mellanox.com; Yigit,
> Ferruh <ferruh.yigit@intel.com>; arybchenko@solarflare.com
> Subject: [PATCH 4/5] app/testpmd: move ethdev events registration
>
> The callback for ethdev events was registered on port start, so it was missing
> some events.
>
> It is now registered at the beginning of the main function.
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
> app/test-pmd/testpmd.c | 72 ++++++++++++++++++++++++------------------
> 1 file changed, 42 insertions(+), 30 deletions(-)
>
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> 72b91adf5..6eb416e7a 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -345,6 +345,21 @@ uint8_t rmv_interrupt = 1; /* enabled by default */
>
> uint8_t hot_plug = 0; /**< hotplug disabled by default. */
>
> +/* Pretty printing of ethdev events */
> +static const char * const eth_event_desc[] = {
> + [RTE_ETH_EVENT_UNKNOWN] = "unknown",
> + [RTE_ETH_EVENT_INTR_LSC] = "LSC",
How about replacing "LSC" with "interrupt link status change"
> + [RTE_ETH_EVENT_QUEUE_STATE] = "queue state",
> + [RTE_ETH_EVENT_INTR_RESET] = "interrupt reset",
> + [RTE_ETH_EVENT_VF_MBOX] = "VF mbox",
> + [RTE_ETH_EVENT_IPSEC] = "IPsec",
> + [RTE_ETH_EVENT_MACSEC] = "MACsec",
> + [RTE_ETH_EVENT_INTR_RMV] = "device removal",
How about replacing "device removal" with "interrupt device removal"
> + [RTE_ETH_EVENT_NEW] = "device probed",
> + [RTE_ETH_EVENT_DESTROY] = "device released",
> + [RTE_ETH_EVENT_MAX] = NULL,
> +};
> +
> /*
> * Display or mask ether events
> * Default to all events except VF_MBOX @@ -1934,7 +1949,6 @@
> start_port(portid_t pid)
> queueid_t qi;
> struct rte_port *port;
> struct ether_addr mac_addr;
> - enum rte_eth_event_type event_type;
>
> if (port_id_is_invalid(pid, ENABLED_WARN))
> return 0;
> @@ -2090,20 +2104,6 @@ start_port(portid_t pid)
> need_check_link_status = 1;
> }
>
> - for (event_type = RTE_ETH_EVENT_UNKNOWN;
> - event_type < RTE_ETH_EVENT_MAX;
> - event_type++) {
> - diag = rte_eth_dev_callback_register(RTE_ETH_ALL,
> - event_type,
> - eth_event_callback,
> - NULL);
> - if (diag) {
> - printf("Failed to setup even callback for event %d\n",
> - event_type);
> - return -1;
> - }
> - }
> -
> if (need_check_link_status == 1 && !no_link_check)
> check_all_ports_link_status(RTE_PORT_ALL);
> else if (need_check_link_status == 0)
> @@ -2522,20 +2522,6 @@ static int
> eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void
> *param,
> void *ret_param)
> {
> - static const char * const event_desc[] = {
> - [RTE_ETH_EVENT_UNKNOWN] = "Unknown",
> - [RTE_ETH_EVENT_INTR_LSC] = "LSC",
> - [RTE_ETH_EVENT_QUEUE_STATE] = "Queue state",
> - [RTE_ETH_EVENT_INTR_RESET] = "Interrupt reset",
> - [RTE_ETH_EVENT_VF_MBOX] = "VF Mbox",
> - [RTE_ETH_EVENT_IPSEC] = "IPsec",
> - [RTE_ETH_EVENT_MACSEC] = "MACsec",
> - [RTE_ETH_EVENT_INTR_RMV] = "device removal",
> - [RTE_ETH_EVENT_NEW] = "device probed",
> - [RTE_ETH_EVENT_DESTROY] = "device released",
> - [RTE_ETH_EVENT_MAX] = NULL,
> - };
> -
> RTE_SET_USED(param);
> RTE_SET_USED(ret_param);
>
> @@ -2545,7 +2531,7 @@ eth_event_callback(portid_t port_id, enum
> rte_eth_event_type type, void *param,
> fflush(stderr);
> } else if (event_print_mask & (UINT32_C(1) << type)) {
> printf("\nPort %" PRIu16 ": %s event\n", port_id,
> - event_desc[type]);
> + eth_event_desc[type]);
> fflush(stdout);
> }
>
> @@ -2564,6 +2550,28 @@ eth_event_callback(portid_t port_id, enum
> rte_eth_event_type type, void *param,
> return 0;
> }
>
> +static int
> +register_eth_event_callback(void)
> +{
> + int ret;
> + enum rte_eth_event_type event;
> +
> + for (event = RTE_ETH_EVENT_UNKNOWN;
> + event < RTE_ETH_EVENT_MAX; event++) {
> + ret = rte_eth_dev_callback_register(RTE_ETH_ALL,
> + event,
> + eth_event_callback,
> + NULL);
> + if (ret != 0) {
> + TESTPMD_LOG(ERR, "Failed to register callback for "
> + "%s event\n", eth_event_desc[event]);
> + return -1;
> + }
> + }
> +
> + return 0;
> +}
> +
> /* This function is used by the interrupt thread */ static void
> eth_dev_event_callback(const char *device_name, enum rte_dev_event_type
> type, @@ -3048,6 +3056,10 @@ main(int argc, char** argv)
> rte_panic("Cannot register log type");
> rte_log_set_level(testpmd_logtype, RTE_LOG_DEBUG);
>
> + ret = register_eth_event_callback();
> + if (ret != 0)
> + rte_panic("Cannot register for ethdev events");
> +
> #ifdef RTE_LIBRTE_PDUMP
> /* initialize packet capture framework */
> rte_pdump_init(NULL);
> --
> 2.19.0
Regards,
Bernard
24/10/2018 17:55, Iremonger, Bernard:
> Hi Thomas,
>
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > +/* Pretty printing of ethdev events */
> > +static const char * const eth_event_desc[] = {
> > + [RTE_ETH_EVENT_UNKNOWN] = "unknown",
> > + [RTE_ETH_EVENT_INTR_LSC] = "LSC",
>
> How about replacing "LSC" with "interrupt link status change"
When it is printed, "event" is appended.
So I think "interrupt" is a bit too much.
OK for "link state change"?
> > + [RTE_ETH_EVENT_QUEUE_STATE] = "queue state",
> > + [RTE_ETH_EVENT_INTR_RESET] = "interrupt reset",
> > + [RTE_ETH_EVENT_VF_MBOX] = "VF mbox",
> > + [RTE_ETH_EVENT_IPSEC] = "IPsec",
> > + [RTE_ETH_EVENT_MACSEC] = "MACsec",
> > + [RTE_ETH_EVENT_INTR_RMV] = "device removal",
>
> How about replacing "device removal" with "interrupt device removal"
For same reason, I think "device removal" is enough.
It will be printed as "device removal event".
Hi Thomas,
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, October 24, 2018 8:55 PM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>
> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; dev@dpdk.org; ophirmu@mellanox.com;
> wisamm@mellanox.com; Yigit, Ferruh <ferruh.yigit@intel.com>;
> arybchenko@solarflare.com
> Subject: Re: [PATCH 4/5] app/testpmd: move ethdev events registration
>
> 24/10/2018 17:55, Iremonger, Bernard:
> > Hi Thomas,
> >
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > +/* Pretty printing of ethdev events */ static const char * const
> > > +eth_event_desc[] = {
> > > + [RTE_ETH_EVENT_UNKNOWN] = "unknown",
> > > + [RTE_ETH_EVENT_INTR_LSC] = "LSC",
> >
> > How about replacing "LSC" with "interrupt link status change"
>
> When it is printed, "event" is appended.
> So I think "interrupt" is a bit too much.
> OK for "link state change"?
Yes,
> > > + [RTE_ETH_EVENT_QUEUE_STATE] = "queue state",
> > > + [RTE_ETH_EVENT_INTR_RESET] = "interrupt reset",
Should "interrupt" be dropped from "interrupt reset" too for consistency?
> > > + [RTE_ETH_EVENT_VF_MBOX] = "VF mbox",
> > > + [RTE_ETH_EVENT_IPSEC] = "IPsec",
> > > + [RTE_ETH_EVENT_MACSEC] = "MACsec",
> > > + [RTE_ETH_EVENT_INTR_RMV] = "device removal",
> >
> > How about replacing "device removal" with "interrupt device removal"
>
> For same reason, I think "device removal" is enough.
> It will be printed as "device removal event".
>
>
Regards,
Bernard.
25/10/2018 10:54, Iremonger, Bernard:
> Hi Thomas,
>
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 24/10/2018 17:55, Iremonger, Bernard:
> > > Hi Thomas,
> > >
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > +/* Pretty printing of ethdev events */ static const char * const
> > > > +eth_event_desc[] = {
> > > > + [RTE_ETH_EVENT_UNKNOWN] = "unknown",
> > > > + [RTE_ETH_EVENT_INTR_LSC] = "LSC",
> > >
> > > How about replacing "LSC" with "interrupt link status change"
> >
> > When it is printed, "event" is appended.
> > So I think "interrupt" is a bit too much.
> > OK for "link state change"?
>
> Yes,
>
> > > > + [RTE_ETH_EVENT_QUEUE_STATE] = "queue state",
> > > > + [RTE_ETH_EVENT_INTR_RESET] = "interrupt reset",
>
> Should "interrupt" be dropped from "interrupt reset" too for consistency?
Yes, you're right.
> > > > + [RTE_ETH_EVENT_VF_MBOX] = "VF mbox",
> > > > + [RTE_ETH_EVENT_IPSEC] = "IPsec",
> > > > + [RTE_ETH_EVENT_MACSEC] = "MACsec",
> > > > + [RTE_ETH_EVENT_INTR_RMV] = "device removal",
> > >
> > > How about replacing "device removal" with "interrupt device removal"
> >
> > For same reason, I think "device removal" is enough.
> > It will be printed as "device removal event".
I will send a v2 today.
Thanks for the review.
@@ -345,6 +345,21 @@ uint8_t rmv_interrupt = 1; /* enabled by default */
uint8_t hot_plug = 0; /**< hotplug disabled by default. */
+/* Pretty printing of ethdev events */
+static const char * const eth_event_desc[] = {
+ [RTE_ETH_EVENT_UNKNOWN] = "unknown",
+ [RTE_ETH_EVENT_INTR_LSC] = "LSC",
+ [RTE_ETH_EVENT_QUEUE_STATE] = "queue state",
+ [RTE_ETH_EVENT_INTR_RESET] = "interrupt reset",
+ [RTE_ETH_EVENT_VF_MBOX] = "VF mbox",
+ [RTE_ETH_EVENT_IPSEC] = "IPsec",
+ [RTE_ETH_EVENT_MACSEC] = "MACsec",
+ [RTE_ETH_EVENT_INTR_RMV] = "device removal",
+ [RTE_ETH_EVENT_NEW] = "device probed",
+ [RTE_ETH_EVENT_DESTROY] = "device released",
+ [RTE_ETH_EVENT_MAX] = NULL,
+};
+
/*
* Display or mask ether events
* Default to all events except VF_MBOX
@@ -1934,7 +1949,6 @@ start_port(portid_t pid)
queueid_t qi;
struct rte_port *port;
struct ether_addr mac_addr;
- enum rte_eth_event_type event_type;
if (port_id_is_invalid(pid, ENABLED_WARN))
return 0;
@@ -2090,20 +2104,6 @@ start_port(portid_t pid)
need_check_link_status = 1;
}
- for (event_type = RTE_ETH_EVENT_UNKNOWN;
- event_type < RTE_ETH_EVENT_MAX;
- event_type++) {
- diag = rte_eth_dev_callback_register(RTE_ETH_ALL,
- event_type,
- eth_event_callback,
- NULL);
- if (diag) {
- printf("Failed to setup even callback for event %d\n",
- event_type);
- return -1;
- }
- }
-
if (need_check_link_status == 1 && !no_link_check)
check_all_ports_link_status(RTE_PORT_ALL);
else if (need_check_link_status == 0)
@@ -2522,20 +2522,6 @@ static int
eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
void *ret_param)
{
- static const char * const event_desc[] = {
- [RTE_ETH_EVENT_UNKNOWN] = "Unknown",
- [RTE_ETH_EVENT_INTR_LSC] = "LSC",
- [RTE_ETH_EVENT_QUEUE_STATE] = "Queue state",
- [RTE_ETH_EVENT_INTR_RESET] = "Interrupt reset",
- [RTE_ETH_EVENT_VF_MBOX] = "VF Mbox",
- [RTE_ETH_EVENT_IPSEC] = "IPsec",
- [RTE_ETH_EVENT_MACSEC] = "MACsec",
- [RTE_ETH_EVENT_INTR_RMV] = "device removal",
- [RTE_ETH_EVENT_NEW] = "device probed",
- [RTE_ETH_EVENT_DESTROY] = "device released",
- [RTE_ETH_EVENT_MAX] = NULL,
- };
-
RTE_SET_USED(param);
RTE_SET_USED(ret_param);
@@ -2545,7 +2531,7 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
fflush(stderr);
} else if (event_print_mask & (UINT32_C(1) << type)) {
printf("\nPort %" PRIu16 ": %s event\n", port_id,
- event_desc[type]);
+ eth_event_desc[type]);
fflush(stdout);
}
@@ -2564,6 +2550,28 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
return 0;
}
+static int
+register_eth_event_callback(void)
+{
+ int ret;
+ enum rte_eth_event_type event;
+
+ for (event = RTE_ETH_EVENT_UNKNOWN;
+ event < RTE_ETH_EVENT_MAX; event++) {
+ ret = rte_eth_dev_callback_register(RTE_ETH_ALL,
+ event,
+ eth_event_callback,
+ NULL);
+ if (ret != 0) {
+ TESTPMD_LOG(ERR, "Failed to register callback for "
+ "%s event\n", eth_event_desc[event]);
+ return -1;
+ }
+ }
+
+ return 0;
+}
+
/* This function is used by the interrupt thread */
static void
eth_dev_event_callback(const char *device_name, enum rte_dev_event_type type,
@@ -3048,6 +3056,10 @@ main(int argc, char** argv)
rte_panic("Cannot register log type");
rte_log_set_level(testpmd_logtype, RTE_LOG_DEBUG);
+ ret = register_eth_event_callback();
+ if (ret != 0)
+ rte_panic("Cannot register for ethdev events");
+
#ifdef RTE_LIBRTE_PDUMP
/* initialize packet capture framework */
rte_pdump_init(NULL);