* Re: [dpdk-dev] [PATCH] ethdev: warn only once for badly behaving applications
2021-10-26 14:58 [dpdk-dev] [PATCH] ethdev: warn only once for badly behaving applications David Marchand
@ 2021-10-26 15:56 ` Thomas Monjalon
2021-10-27 7:20 ` David Marchand
2021-10-26 17:10 ` Ananyev, Konstantin
2021-10-27 12:01 ` [dpdk-dev] [PATCH v2] ethdev: warn once for buggy applications David Marchand
2 siblings, 1 reply; 11+ messages in thread
From: Thomas Monjalon @ 2021-10-26 15:56 UTC (permalink / raw)
To: David Marchand
Cc: dev, konstantin.ananyev, ferruh.yigit, andrew.rybchenko, bingz,
Xiaoyun Li
26/10/2021 16:58, David Marchand:
> Warning continuously is a pain when developping or if a unit test
> is/gets broken.
>
> It could also be a problem if application behaves badly only in some
> corner cases and a DoS results of those logs being continuously displayed.
>
> Let's warn once per port and per rx/tx.
>
> Getting such a log is scary, but let's make it more eye catching by
> dumping a backtrace with it.
[...]
> Fixes: c87d435a4d79 ("ethdev: copy fast-path API into separate structure")
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
[...]
> +static struct dummy_queue *dummy_queues_ref[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
> +static struct dummy_queue dummy_queues[RTE_MAX_ETHPORTS];
I feel we could better name those arrays, maybe adding a comment.
First one is really queues array while the second one is to share
the same value with all queues of a port. Right?
> +RTE_INIT(dummy_queue_init)
> +{
> + uint16_t port_id;
> +
> + for (port_id = 0; port_id < RTE_DIM(dummy_queues); port_id++) {
> + unsigned int i;
q would be a better name than i
> +
> + for (i = 0; i < RTE_DIM(dummy_queues_ref[port_id]); i++)
> + dummy_queues_ref[port_id][i] = &dummy_queues[port_id];
> + }
> +}
> +
> static uint16_t
> -dummy_eth_rx_burst(__rte_unused void *rxq,
> +dummy_eth_rx_burst(void *rxq,
> __rte_unused struct rte_mbuf **rx_pkts,
> __rte_unused uint16_t nb_pkts)
> {
> - RTE_ETHDEV_LOG(ERR, "rx_pkt_burst for not ready port\n");
> + struct dummy_queue *q = rxq;
> +
> + if (!q->rx_warn_once) {
> + uint16_t port_id = q - dummy_queues;
> +
> + RTE_ETHDEV_LOG(ERR, "lcore %u called rx_pkt_burst for not ready port %"PRIu16"\n",
> + rte_lcore_id(), port_id);
> + rte_dump_stack();
> + q->rx_warn_once = true;
> + }
> rte_errno = ENOTSUP;
> return 0;
> }
OK with this log.
[...]
> eth_dev_fp_ops_reset(struct rte_eth_fp_ops *fpo)
> {
> static void *dummy_data[RTE_MAX_QUEUES_PER_PORT];
> - static const struct rte_eth_fp_ops dummy_ops = {
> + uint16_t port_id = fpo - rte_eth_fp_ops;
> +
> + dummy_queues[port_id].rx_warn_once = false;
> + dummy_queues[port_id].tx_warn_once = false;
> + *fpo = (struct rte_eth_fp_ops) {
> .rx_pkt_burst = dummy_eth_rx_burst,
> .tx_pkt_burst = dummy_eth_tx_burst,
> - .rxq = {.data = dummy_data, .clbk = dummy_data,},
> - .txq = {.data = dummy_data, .clbk = dummy_data,},
> + .rxq = (struct rte_ethdev_qdata) {
Why this cast? rte_eth_fp_ops.rxq is of type rte_ethdev_qdata.
> + .data = (void **)&dummy_queues_ref[port_id],
> + .clbk = dummy_data,
> + },
> + .txq = (struct rte_ethdev_qdata) {
> + .data = (void **)&dummy_queues_ref[port_id],
> + .clbk = dummy_data,
> + },
> };
> -
> - *fpo = dummy_ops;
> }
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [dpdk-dev] [PATCH] ethdev: warn only once for badly behaving applications
2021-10-26 15:56 ` Thomas Monjalon
@ 2021-10-27 7:20 ` David Marchand
2021-10-27 8:16 ` Olivier Matz
0 siblings, 1 reply; 11+ messages in thread
From: David Marchand @ 2021-10-27 7:20 UTC (permalink / raw)
To: Thomas Monjalon
Cc: dev, Ananyev, Konstantin, Yigit, Ferruh, Andrew Rybchenko,
Bing Zhao, Xiaoyun Li
On Tue, Oct 26, 2021 at 5:57 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 26/10/2021 16:58, David Marchand:
> > Warning continuously is a pain when developping or if a unit test
> > is/gets broken.
> >
> > It could also be a problem if application behaves badly only in some
> > corner cases and a DoS results of those logs being continuously displayed.
> >
> > Let's warn once per port and per rx/tx.
> >
> > Getting such a log is scary, but let's make it more eye catching by
> > dumping a backtrace with it.
> [...]
> > Fixes: c87d435a4d79 ("ethdev: copy fast-path API into separate structure")
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> [...]
> > +static struct dummy_queue *dummy_queues_ref[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
> > +static struct dummy_queue dummy_queues[RTE_MAX_ETHPORTS];
>
> I feel we could better name those arrays, maybe adding a comment.
> First one is really queues array while the second one is to share
> the same value with all queues of a port. Right?
Yes, look fwd to v2 for better names.
>
> > +RTE_INIT(dummy_queue_init)
> > +{
> > + uint16_t port_id;
> > +
> > + for (port_id = 0; port_id < RTE_DIM(dummy_queues); port_id++) {
> > + unsigned int i;
>
> q would be a better name than i
Ok, and I'll rename other variable q for actual queue objects later in
the patch.
> > eth_dev_fp_ops_reset(struct rte_eth_fp_ops *fpo)
> > {
> > static void *dummy_data[RTE_MAX_QUEUES_PER_PORT];
> > - static const struct rte_eth_fp_ops dummy_ops = {
> > + uint16_t port_id = fpo - rte_eth_fp_ops;
> > +
> > + dummy_queues[port_id].rx_warn_once = false;
> > + dummy_queues[port_id].tx_warn_once = false;
> > + *fpo = (struct rte_eth_fp_ops) {
> > .rx_pkt_burst = dummy_eth_rx_burst,
> > .tx_pkt_burst = dummy_eth_tx_burst,
> > - .rxq = {.data = dummy_data, .clbk = dummy_data,},
> > - .txq = {.data = dummy_data, .clbk = dummy_data,},
> > + .rxq = (struct rte_ethdev_qdata) {
>
> Why this cast? rte_eth_fp_ops.rxq is of type rte_ethdev_qdata.
Funny how the compiler complains about:
../lib/ethdev/ethdev_private.c: In function ‘eth_dev_fp_ops_reset’:
../lib/ethdev/ethdev_private.c:243:9: error: expected expression
before ‘{’ token
*fpo = {
^
if we don't explicitely tell this anonymous struct is of type struct
rte_eth_fp_ops (note that *fpo is of type struct rte_eth_fp_ops).
But otoh, compiler silently understands that, in .rxq case, the
anonymous struct is of type rte_ethdev_qdata.
So indeed, it works without the cast on .rxq and .txq.
I applied the cast on all anonymous struct in my patch once I hit the
first compiler complaint.
Do you have the explanation or can you point me at some standard
explaining the difference in treatment?
>
> > + .data = (void **)&dummy_queues_ref[port_id],
> > + .clbk = dummy_data,
> > + },
> > + .txq = (struct rte_ethdev_qdata) {
> > + .data = (void **)&dummy_queues_ref[port_id],
> > + .clbk = dummy_data,
> > + },
> > };
> > -
> > - *fpo = dummy_ops;
> > }
--
David Marchand
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [dpdk-dev] [PATCH] ethdev: warn only once for badly behaving applications
2021-10-27 7:20 ` David Marchand
@ 2021-10-27 8:16 ` Olivier Matz
2021-10-27 8:42 ` David Marchand
0 siblings, 1 reply; 11+ messages in thread
From: Olivier Matz @ 2021-10-27 8:16 UTC (permalink / raw)
To: David Marchand
Cc: Thomas Monjalon, dev, Ananyev, Konstantin, Yigit, Ferruh,
Andrew Rybchenko, Bing Zhao, Xiaoyun Li
Hi,
On Wed, Oct 27, 2021 at 09:20:52AM +0200, David Marchand wrote:
> On Tue, Oct 26, 2021 at 5:57 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> >
> > 26/10/2021 16:58, David Marchand:
> > > Warning continuously is a pain when developping or if a unit test
> > > is/gets broken.
> > >
> > > It could also be a problem if application behaves badly only in some
> > > corner cases and a DoS results of those logs being continuously displayed.
> > >
> > > Let's warn once per port and per rx/tx.
> > >
> > > Getting such a log is scary, but let's make it more eye catching by
> > > dumping a backtrace with it.
> > [...]
> > > Fixes: c87d435a4d79 ("ethdev: copy fast-path API into separate structure")
> > >
> > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > [...]
> > > +static struct dummy_queue *dummy_queues_ref[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
> > > +static struct dummy_queue dummy_queues[RTE_MAX_ETHPORTS];
> >
> > I feel we could better name those arrays, maybe adding a comment.
> > First one is really queues array while the second one is to share
> > the same value with all queues of a port. Right?
>
> Yes, look fwd to v2 for better names.
>
>
> >
> > > +RTE_INIT(dummy_queue_init)
> > > +{
> > > + uint16_t port_id;
> > > +
> > > + for (port_id = 0; port_id < RTE_DIM(dummy_queues); port_id++) {
> > > + unsigned int i;
> >
> > q would be a better name than i
>
> Ok, and I'll rename other variable q for actual queue objects later in
> the patch.
>
>
> > > eth_dev_fp_ops_reset(struct rte_eth_fp_ops *fpo)
> > > {
> > > static void *dummy_data[RTE_MAX_QUEUES_PER_PORT];
> > > - static const struct rte_eth_fp_ops dummy_ops = {
> > > + uint16_t port_id = fpo - rte_eth_fp_ops;
> > > +
> > > + dummy_queues[port_id].rx_warn_once = false;
> > > + dummy_queues[port_id].tx_warn_once = false;
> > > + *fpo = (struct rte_eth_fp_ops) {
> > > .rx_pkt_burst = dummy_eth_rx_burst,
> > > .tx_pkt_burst = dummy_eth_tx_burst,
> > > - .rxq = {.data = dummy_data, .clbk = dummy_data,},
> > > - .txq = {.data = dummy_data, .clbk = dummy_data,},
> > > + .rxq = (struct rte_ethdev_qdata) {
> >
> > Why this cast? rte_eth_fp_ops.rxq is of type rte_ethdev_qdata.
>
> Funny how the compiler complains about:
>
> ../lib/ethdev/ethdev_private.c: In function ‘eth_dev_fp_ops_reset’:
> ../lib/ethdev/ethdev_private.c:243:9: error: expected expression
> before ‘{’ token
> *fpo = {
> ^
> if we don't explicitely tell this anonymous struct is of type struct
> rte_eth_fp_ops (note that *fpo is of type struct rte_eth_fp_ops).
> But otoh, compiler silently understands that, in .rxq case, the
> anonymous struct is of type rte_ethdev_qdata.
>
> So indeed, it works without the cast on .rxq and .txq.
> I applied the cast on all anonymous struct in my patch once I hit the
> first compiler complaint.
>
> Do you have the explanation or can you point me at some standard
> explaining the difference in treatment?
Let me try an explanation, hope it is the correct one.
In the first case, this is an assignment as described in 6.5.16 of
the standard [1]:
*fpo = (struct rte_eth_fp_ops) { .rx_pkt_burst = dummy_eth_rx_burst, ... };
The compiler expects the right side to be an expression. The expression
is a "compound literal", as described in 6.5.2.5:
1. The type name shall specify a complete object type or an array of
unknown size, but not avariable length array type.
2. All the constraints for initializer lists in 6.7.9 also apply to
compound literals
The second cast { ..., .rxq = (struct rte_ethdev_qdata) { ... } } is
inside a construction that behaves like an initialization (according to
the second point above). The compiler already knows the type of the
struct (and therefore the types of the fields), so the cast is not
required.
[1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf
> >
> > > + .data = (void **)&dummy_queues_ref[port_id],
> > > + .clbk = dummy_data,
> > > + },
> > > + .txq = (struct rte_ethdev_qdata) {
> > > + .data = (void **)&dummy_queues_ref[port_id],
> > > + .clbk = dummy_data,
> > > + },
> > > };
> > > -
> > > - *fpo = dummy_ops;
> > > }
>
>
> --
> David Marchand
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [dpdk-dev] [PATCH] ethdev: warn only once for badly behaving applications
2021-10-27 8:16 ` Olivier Matz
@ 2021-10-27 8:42 ` David Marchand
0 siblings, 0 replies; 11+ messages in thread
From: David Marchand @ 2021-10-27 8:42 UTC (permalink / raw)
To: Olivier Matz
Cc: Thomas Monjalon, dev, Ananyev, Konstantin, Yigit, Ferruh,
Andrew Rybchenko, Bing Zhao, Xiaoyun Li
On Wed, Oct 27, 2021 at 10:16 AM Olivier Matz <olivier.matz@6wind.com> wrote:
> > Do you have the explanation or can you point me at some standard
> > explaining the difference in treatment?
>
> Let me try an explanation, hope it is the correct one.
>
> In the first case, this is an assignment as described in 6.5.16 of
> the standard [1]:
>
> *fpo = (struct rte_eth_fp_ops) { .rx_pkt_burst = dummy_eth_rx_burst, ... };
>
> The compiler expects the right side to be an expression. The expression
> is a "compound literal", as described in 6.5.2.5:
>
> 1. The type name shall specify a complete object type or an array of
> unknown size, but not avariable length array type.
> 2. All the constraints for initializer lists in 6.7.9 also apply to
> compound literals
>
> The second cast { ..., .rxq = (struct rte_ethdev_qdata) { ... } } is
> inside a construction that behaves like an initialization (according to
> the second point above). The compiler already knows the type of the
> struct (and therefore the types of the fields), so the cast is not
> required.
>
> [1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf
I read/understand it like this too.
Thanks a lot, reading standards is always illuminating.
--
David Marchand
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH] ethdev: warn only once for badly behaving applications
2021-10-26 14:58 [dpdk-dev] [PATCH] ethdev: warn only once for badly behaving applications David Marchand
2021-10-26 15:56 ` Thomas Monjalon
@ 2021-10-26 17:10 ` Ananyev, Konstantin
2021-10-27 7:23 ` David Marchand
2021-10-27 12:01 ` [dpdk-dev] [PATCH v2] ethdev: warn once for buggy applications David Marchand
2 siblings, 1 reply; 11+ messages in thread
From: Ananyev, Konstantin @ 2021-10-26 17:10 UTC (permalink / raw)
To: David Marchand, dev
Cc: Yigit, Ferruh, andrew.rybchenko, thomas, bingz, Li, Xiaoyun
>
> Warning continuously is a pain when developping or if a unit test
> is/gets broken.
>
> It could also be a problem if application behaves badly only in some
> corner cases and a DoS results of those logs being continuously displayed.
>
> Let's warn once per port and per rx/tx.
>
> Getting such a log is scary, but let's make it more eye catching by
> dumping a backtrace with it.
>
> Tested by introducing a bug in testpmd:
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -610,7 +610,7 @@ eth_dev_configure_mp(uint16_t port_id, uint16_t
> nb_rx_q, uint16_t nb_tx_q,
> static int
> eth_dev_start_mp(uint16_t port_id)
> {
> - if (is_proc_primary())
> + if (!is_proc_primary())
> return rte_eth_dev_start(port_id);
>
> return 0;
>
> Then, running a basic null test:
> $ ./devtools/test-null.sh
> ...
> Start automatic packet forwarding
> io packet forwarding - ports=2 - cores=1 - streams=2 - NUMA support
> enabled, MP allocation mode: native
> Logical Core 1 (socket 0) forwards packets on 2 streams:
> RX P=0/Q=0 (socket 0) -> TX P=1/Q=0 (socket 0) peer=02:00:00:00:00:01
> RX P=1/Q=0 (socket 0) -> TX P=0/Q=0 (socket 0) peer=02:00:00:00:00:00
>
> lcore 0 called rx_pkt_burst for not ready port 0
> 8: [build/app/dpdk-testpmd() [0x59e839]]
> 7: [/lib64/libc.so.6(__libc_start_main+0xf5) [0x7ff481b69555]]
> 6: [build/app/dpdk-testpmd(main+0x54b) [0x662d24]]
> 5: [build/app/dpdk-testpmd(start_packet_forwarding+0x263) [0x65e795]]
> 4: [build/app/dpdk-testpmd() [0x65e1be]]
> 3: [build/app/dpdk-testpmd() [0x65a996]]
> 2: [build/app/dpdk-testpmd() [0xa6cbc7]]
> 1: [build/app/dpdk-testpmd(rte_dump_stack+0x27) [0xaee796]]
> lcore 0 called rx_pkt_burst for not ready port 1
> 8: [build/app/dpdk-testpmd() [0x59e839]]
> 7: [/lib64/libc.so.6(__libc_start_main+0xf5) [0x7ff481b69555]]
> 6: [build/app/dpdk-testpmd(main+0x54b) [0x662d24]]
> 5: [build/app/dpdk-testpmd(start_packet_forwarding+0x263) [0x65e795]]
> 4: [build/app/dpdk-testpmd() [0x65e1be]]
> 3: [build/app/dpdk-testpmd() [0x65a996]]
> 2: [build/app/dpdk-testpmd() [0xa6cbc7]]
> 1: [build/app/dpdk-testpmd(rte_dump_stack+0x27) [0xaee796]]
> io packet forwarding packets/burst=32
> nb forwarding cores=1 - nb forwarding ports=2
> port 0: RX queue number: 1 Tx queue number: 1
> Rx offloads=0x0 Tx offloads=0x0
>
> Fixes: c87d435a4d79 ("ethdev: copy fast-path API into separate structure")
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> lib/ethdev/ethdev_private.c | 63 +++++++++++++++++++++++++++++++------
> 1 file changed, 54 insertions(+), 9 deletions(-)
>
> diff --git a/lib/ethdev/ethdev_private.c b/lib/ethdev/ethdev_private.c
> index c905c2df6f..7a5d05ff43 100644
> --- a/lib/ethdev/ethdev_private.c
> +++ b/lib/ethdev/ethdev_private.c
> @@ -2,6 +2,7 @@
> * Copyright(c) 2018 Gaëtan Rivet
> */
>
> +#include <rte_debug.h>
> #include "rte_ethdev.h"
> #include "ethdev_driver.h"
> #include "ethdev_private.h"
> @@ -175,22 +176,58 @@ rte_eth_devargs_parse_representor_ports(char *str, void *data)
> return str == NULL ? -1 : 0;
> }
>
> +struct dummy_queue {
> + bool rx_warn_once;
> + bool tx_warn_once;
> +};
> +static struct dummy_queue *dummy_queues_ref[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
> +static struct dummy_queue dummy_queues[RTE_MAX_ETHPORTS];
> +RTE_INIT(dummy_queue_init)
> +{
> + uint16_t port_id;
> +
> + for (port_id = 0; port_id < RTE_DIM(dummy_queues); port_id++) {
> + unsigned int i;
> +
> + for (i = 0; i < RTE_DIM(dummy_queues_ref[port_id]); i++)
> + dummy_queues_ref[port_id][i] = &dummy_queues[port_id];
> + }
> +}
> +
> static uint16_t
> -dummy_eth_rx_burst(__rte_unused void *rxq,
> +dummy_eth_rx_burst(void *rxq,
> __rte_unused struct rte_mbuf **rx_pkts,
> __rte_unused uint16_t nb_pkts)
> {
> - RTE_ETHDEV_LOG(ERR, "rx_pkt_burst for not ready port\n");
> + struct dummy_queue *q = rxq;
> +
LGTM in general, just one thing:
I think we'd better add extra check that rxq really points to dummy queues
before de-referencing it.
Something like:
uintptr_t port_id;
....
port_id = q - dummy_queues;
if (port_id < RTE_DIM(dummy_queues) && !q->rx_warn_once) {
....
}
Same for tx.
> + if (!q->rx_warn_once) {
> + uint16_t port_id = q - dummy_queues;
> +
> + RTE_ETHDEV_LOG(ERR, "lcore %u called rx_pkt_burst for not ready port %"PRIu16"\n",
> + rte_lcore_id(), port_id);
> + rte_dump_stack();
> + q->rx_warn_once = true;
> + }
> rte_errno = ENOTSUP;
> return 0;
> }
>
> static uint16_t
> -dummy_eth_tx_burst(__rte_unused void *txq,
> +dummy_eth_tx_burst(void *txq,
> __rte_unused struct rte_mbuf **tx_pkts,
> __rte_unused uint16_t nb_pkts)
> {
> - RTE_ETHDEV_LOG(ERR, "tx_pkt_burst for not ready port\n");
> + struct dummy_queue *q = txq;
> +
> + if (!q->tx_warn_once) {
> + uint16_t port_id = q - dummy_queues;
> +
> + RTE_ETHDEV_LOG(ERR, "lcore %u called tx_pkt_burst for not ready port %"PRIu16"\n",
> + rte_lcore_id(), port_id);
> + rte_dump_stack();
> + q->tx_warn_once = true;
> + }
> rte_errno = ENOTSUP;
> return 0;
> }
> @@ -199,14 +236,22 @@ void
> eth_dev_fp_ops_reset(struct rte_eth_fp_ops *fpo)
> {
> static void *dummy_data[RTE_MAX_QUEUES_PER_PORT];
> - static const struct rte_eth_fp_ops dummy_ops = {
> + uint16_t port_id = fpo - rte_eth_fp_ops;
> +
> + dummy_queues[port_id].rx_warn_once = false;
> + dummy_queues[port_id].tx_warn_once = false;
> + *fpo = (struct rte_eth_fp_ops) {
> .rx_pkt_burst = dummy_eth_rx_burst,
> .tx_pkt_burst = dummy_eth_tx_burst,
> - .rxq = {.data = dummy_data, .clbk = dummy_data,},
> - .txq = {.data = dummy_data, .clbk = dummy_data,},
> + .rxq = (struct rte_ethdev_qdata) {
Here and for txq, do we need to explicitly specify type?
Wouldn't:
.rxq = {.data=..., .clbk=...,},
be enough here?
> + .data = (void **)&dummy_queues_ref[port_id],
> + .clbk = dummy_data,
> + },
> + .txq = (struct rte_ethdev_qdata) {
> + .data = (void **)&dummy_queues_ref[port_id],
> + .clbk = dummy_data,
> + },
> };
> -
> - *fpo = dummy_ops;
> }
>
> void
> --
> 2.23.0
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [dpdk-dev] [PATCH] ethdev: warn only once for badly behaving applications
2021-10-26 17:10 ` Ananyev, Konstantin
@ 2021-10-27 7:23 ` David Marchand
0 siblings, 0 replies; 11+ messages in thread
From: David Marchand @ 2021-10-27 7:23 UTC (permalink / raw)
To: Ananyev, Konstantin
Cc: dev, Yigit, Ferruh, andrew.rybchenko, thomas, bingz, Li, Xiaoyun
On Tue, Oct 26, 2021 at 7:10 PM Ananyev, Konstantin
<konstantin.ananyev@intel.com> wrote:
> > static uint16_t
> > -dummy_eth_rx_burst(__rte_unused void *rxq,
> > +dummy_eth_rx_burst(void *rxq,
> > __rte_unused struct rte_mbuf **rx_pkts,
> > __rte_unused uint16_t nb_pkts)
> > {
> > - RTE_ETHDEV_LOG(ERR, "rx_pkt_burst for not ready port\n");
> > + struct dummy_queue *q = rxq;
> > +
>
> LGTM in general, just one thing:
> I think we'd better add extra check that rxq really points to dummy queues
> before de-referencing it.
> Something like:
>
> uintptr_t port_id;
> ....
> port_id = q - dummy_queues;
> if (port_id < RTE_DIM(dummy_queues) && !q->rx_warn_once) {
> ....
> }
>
> Same for tx.
Yep, will add.
>
> > + if (!q->rx_warn_once) {
> > + uint16_t port_id = q - dummy_queues;
> > +
> > + RTE_ETHDEV_LOG(ERR, "lcore %u called rx_pkt_burst for not ready port %"PRIu16"\n",
> > + rte_lcore_id(), port_id);
> > + rte_dump_stack();
> > + q->rx_warn_once = true;
> > + }
> > rte_errno = ENOTSUP;
> > return 0;
> > }
> >
> > static uint16_t
> > -dummy_eth_tx_burst(__rte_unused void *txq,
> > +dummy_eth_tx_burst(void *txq,
> > __rte_unused struct rte_mbuf **tx_pkts,
> > __rte_unused uint16_t nb_pkts)
> > {
> > - RTE_ETHDEV_LOG(ERR, "tx_pkt_burst for not ready port\n");
> > + struct dummy_queue *q = txq;
> > +
> > + if (!q->tx_warn_once) {
> > + uint16_t port_id = q - dummy_queues;
> > +
> > + RTE_ETHDEV_LOG(ERR, "lcore %u called tx_pkt_burst for not ready port %"PRIu16"\n",
> > + rte_lcore_id(), port_id);
> > + rte_dump_stack();
> > + q->tx_warn_once = true;
> > + }
> > rte_errno = ENOTSUP;
> > return 0;
> > }
> > @@ -199,14 +236,22 @@ void
> > eth_dev_fp_ops_reset(struct rte_eth_fp_ops *fpo)
> > {
> > static void *dummy_data[RTE_MAX_QUEUES_PER_PORT];
> > - static const struct rte_eth_fp_ops dummy_ops = {
> > + uint16_t port_id = fpo - rte_eth_fp_ops;
> > +
> > + dummy_queues[port_id].rx_warn_once = false;
> > + dummy_queues[port_id].tx_warn_once = false;
> > + *fpo = (struct rte_eth_fp_ops) {
> > .rx_pkt_burst = dummy_eth_rx_burst,
> > .tx_pkt_burst = dummy_eth_tx_burst,
> > - .rxq = {.data = dummy_data, .clbk = dummy_data,},
> > - .txq = {.data = dummy_data, .clbk = dummy_data,},
> > + .rxq = (struct rte_ethdev_qdata) {
>
> Here and for txq, do we need to explicitly specify type?
> Wouldn't:
> .rxq = {.data=..., .clbk=...,},
> be enough here?
Well, same question from Thomas.
It seems to work without it.
--
David Marchand
^ permalink raw reply [flat|nested] 11+ messages in thread
* [dpdk-dev] [PATCH v2] ethdev: warn once for buggy applications
2021-10-26 14:58 [dpdk-dev] [PATCH] ethdev: warn only once for badly behaving applications David Marchand
2021-10-26 15:56 ` Thomas Monjalon
2021-10-26 17:10 ` Ananyev, Konstantin
@ 2021-10-27 12:01 ` David Marchand
2021-10-27 12:15 ` Ananyev, Konstantin
2 siblings, 1 reply; 11+ messages in thread
From: David Marchand @ 2021-10-27 12:01 UTC (permalink / raw)
To: dev
Cc: konstantin.ananyev, ferruh.yigit, andrew.rybchenko, thomas,
bingz, olivier.matz
Warning continuously is a pain when developping or if a unit test
is/gets broken.
It could also be a problem if application behaves badly only in some
corner cases and a DoS results of those logs being continuously displayed.
Let's warn once per port and per rx/tx.
Getting such a log is scary, but let's make it more eye catching by
dumping a backtrace with it.
Tested by introducing a bug in testpmd:
static int
eth_dev_start_mp(uint16_t port_id)
{
- if (is_proc_primary())
+ if (!is_proc_primary())
return rte_eth_dev_start(port_id);
return 0;
Then, running a basic null test:
$ ./devtools/test-null.sh
...
Start automatic packet forwarding
io packet forwarding - ports=2 - cores=1 - streams=2 - NUMA support
enabled, MP allocation mode: native
Logical Core 1 (socket 0) forwards packets on 2 streams:
RX P=0/Q=0 (socket 0) -> TX P=1/Q=0 (socket 0) peer=02:00:00:00:00:01
RX P=1/Q=0 (socket 0) -> TX P=0/Q=0 (socket 0) peer=02:00:00:00:00:00
lcore 0 called rx_pkt_burst for not ready port 0
8: [build/app/dpdk-testpmd() [0x59e839]]
7: [/lib64/libc.so.6(__libc_start_main+0xf5) [0x7ff481b69555]]
6: [build/app/dpdk-testpmd(main+0x54b) [0x662d24]]
5: [build/app/dpdk-testpmd(start_packet_forwarding+0x263) [0x65e795]]
4: [build/app/dpdk-testpmd() [0x65e1be]]
3: [build/app/dpdk-testpmd() [0x65a996]]
2: [build/app/dpdk-testpmd() [0xa6cbc7]]
1: [build/app/dpdk-testpmd(rte_dump_stack+0x27) [0xaee796]]
lcore 0 called rx_pkt_burst for not ready port 1
8: [build/app/dpdk-testpmd() [0x59e839]]
7: [/lib64/libc.so.6(__libc_start_main+0xf5) [0x7ff481b69555]]
6: [build/app/dpdk-testpmd(main+0x54b) [0x662d24]]
5: [build/app/dpdk-testpmd(start_packet_forwarding+0x263) [0x65e795]]
4: [build/app/dpdk-testpmd() [0x65e1be]]
3: [build/app/dpdk-testpmd() [0x65a996]]
2: [build/app/dpdk-testpmd() [0xa6cbc7]]
1: [build/app/dpdk-testpmd(rte_dump_stack+0x27) [0xaee796]]
io packet forwarding packets/burst=32
nb forwarding cores=1 - nb forwarding ports=2
port 0: RX queue number: 1 Tx queue number: 1
Rx offloads=0x0 Tx offloads=0x0
Fixes: c87d435a4d79 ("ethdev: copy fast-path API into separate structure")
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v1:
- removed diff banner in commitlog,
- renamed vars,
- removed unneeded cast on anonymous struct in initialisation construct,
- added check on queue pointer,
---
lib/ethdev/ethdev_private.c | 63 +++++++++++++++++++++++++++++++------
1 file changed, 54 insertions(+), 9 deletions(-)
diff --git a/lib/ethdev/ethdev_private.c b/lib/ethdev/ethdev_private.c
index c905c2df6f..8fca20c7d4 100644
--- a/lib/ethdev/ethdev_private.c
+++ b/lib/ethdev/ethdev_private.c
@@ -2,6 +2,7 @@
* Copyright(c) 2018 Gaëtan Rivet
*/
+#include <rte_debug.h>
#include "rte_ethdev.h"
#include "ethdev_driver.h"
#include "ethdev_private.h"
@@ -175,22 +176,58 @@ rte_eth_devargs_parse_representor_ports(char *str, void *data)
return str == NULL ? -1 : 0;
}
+struct dummy_queue {
+ bool rx_warn_once;
+ bool tx_warn_once;
+};
+static struct dummy_queue *dummy_queues_array[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
+static struct dummy_queue per_port_queues[RTE_MAX_ETHPORTS];
+RTE_INIT(dummy_queue_init)
+{
+ uint16_t port_id;
+
+ for (port_id = 0; port_id < RTE_DIM(per_port_queues); port_id++) {
+ unsigned int q;
+
+ for (q = 0; q < RTE_DIM(dummy_queues_array[port_id]); q++)
+ dummy_queues_array[port_id][q] = &per_port_queues[port_id];
+ }
+}
+
static uint16_t
-dummy_eth_rx_burst(__rte_unused void *rxq,
+dummy_eth_rx_burst(void *rxq,
__rte_unused struct rte_mbuf **rx_pkts,
__rte_unused uint16_t nb_pkts)
{
- RTE_ETHDEV_LOG(ERR, "rx_pkt_burst for not ready port\n");
+ struct dummy_queue *queue = rxq;
+ uintptr_t port_id;
+
+ port_id = queue - per_port_queues;
+ if (port_id < RTE_DIM(per_port_queues) && !queue->rx_warn_once) {
+ RTE_ETHDEV_LOG(ERR, "lcore %u called rx_pkt_burst for not ready port %"PRIuPTR"\n",
+ rte_lcore_id(), port_id);
+ rte_dump_stack();
+ queue->rx_warn_once = true;
+ }
rte_errno = ENOTSUP;
return 0;
}
static uint16_t
-dummy_eth_tx_burst(__rte_unused void *txq,
+dummy_eth_tx_burst(void *txq,
__rte_unused struct rte_mbuf **tx_pkts,
__rte_unused uint16_t nb_pkts)
{
- RTE_ETHDEV_LOG(ERR, "tx_pkt_burst for not ready port\n");
+ struct dummy_queue *queue = txq;
+ uintptr_t port_id;
+
+ port_id = queue - per_port_queues;
+ if (port_id < RTE_DIM(per_port_queues) && !queue->tx_warn_once) {
+ RTE_ETHDEV_LOG(ERR, "lcore %u called tx_pkt_burst for not ready port %"PRIuPTR"\n",
+ rte_lcore_id(), port_id);
+ rte_dump_stack();
+ queue->tx_warn_once = true;
+ }
rte_errno = ENOTSUP;
return 0;
}
@@ -199,14 +236,22 @@ void
eth_dev_fp_ops_reset(struct rte_eth_fp_ops *fpo)
{
static void *dummy_data[RTE_MAX_QUEUES_PER_PORT];
- static const struct rte_eth_fp_ops dummy_ops = {
+ uintptr_t port_id = fpo - rte_eth_fp_ops;
+
+ per_port_queues[port_id].rx_warn_once = false;
+ per_port_queues[port_id].tx_warn_once = false;
+ *fpo = (struct rte_eth_fp_ops) {
.rx_pkt_burst = dummy_eth_rx_burst,
.tx_pkt_burst = dummy_eth_tx_burst,
- .rxq = {.data = dummy_data, .clbk = dummy_data,},
- .txq = {.data = dummy_data, .clbk = dummy_data,},
+ .rxq = {
+ .data = (void **)&dummy_queues_array[port_id],
+ .clbk = dummy_data,
+ },
+ .txq = {
+ .data = (void **)&dummy_queues_array[port_id],
+ .clbk = dummy_data,
+ },
};
-
- *fpo = dummy_ops;
}
void
--
2.23.0
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [dpdk-dev] [PATCH v2] ethdev: warn once for buggy applications
2021-10-27 12:01 ` [dpdk-dev] [PATCH v2] ethdev: warn once for buggy applications David Marchand
@ 2021-10-27 12:15 ` Ananyev, Konstantin
2021-10-27 12:46 ` Thomas Monjalon
0 siblings, 1 reply; 11+ messages in thread
From: Ananyev, Konstantin @ 2021-10-27 12:15 UTC (permalink / raw)
To: David Marchand, dev
Cc: Yigit, Ferruh, andrew.rybchenko, thomas, bingz, olivier.matz
> Warning continuously is a pain when developping or if a unit test
> is/gets broken.
>
> It could also be a problem if application behaves badly only in some
> corner cases and a DoS results of those logs being continuously displayed.
>
> Let's warn once per port and per rx/tx.
>
> Getting such a log is scary, but let's make it more eye catching by
> dumping a backtrace with it.
>
> Tested by introducing a bug in testpmd:
> static int
> eth_dev_start_mp(uint16_t port_id)
> {
> - if (is_proc_primary())
> + if (!is_proc_primary())
> return rte_eth_dev_start(port_id);
>
> return 0;
>
> Then, running a basic null test:
> $ ./devtools/test-null.sh
> ...
> Start automatic packet forwarding
> io packet forwarding - ports=2 - cores=1 - streams=2 - NUMA support
> enabled, MP allocation mode: native
> Logical Core 1 (socket 0) forwards packets on 2 streams:
> RX P=0/Q=0 (socket 0) -> TX P=1/Q=0 (socket 0) peer=02:00:00:00:00:01
> RX P=1/Q=0 (socket 0) -> TX P=0/Q=0 (socket 0) peer=02:00:00:00:00:00
>
> lcore 0 called rx_pkt_burst for not ready port 0
> 8: [build/app/dpdk-testpmd() [0x59e839]]
> 7: [/lib64/libc.so.6(__libc_start_main+0xf5) [0x7ff481b69555]]
> 6: [build/app/dpdk-testpmd(main+0x54b) [0x662d24]]
> 5: [build/app/dpdk-testpmd(start_packet_forwarding+0x263) [0x65e795]]
> 4: [build/app/dpdk-testpmd() [0x65e1be]]
> 3: [build/app/dpdk-testpmd() [0x65a996]]
> 2: [build/app/dpdk-testpmd() [0xa6cbc7]]
> 1: [build/app/dpdk-testpmd(rte_dump_stack+0x27) [0xaee796]]
> lcore 0 called rx_pkt_burst for not ready port 1
> 8: [build/app/dpdk-testpmd() [0x59e839]]
> 7: [/lib64/libc.so.6(__libc_start_main+0xf5) [0x7ff481b69555]]
> 6: [build/app/dpdk-testpmd(main+0x54b) [0x662d24]]
> 5: [build/app/dpdk-testpmd(start_packet_forwarding+0x263) [0x65e795]]
> 4: [build/app/dpdk-testpmd() [0x65e1be]]
> 3: [build/app/dpdk-testpmd() [0x65a996]]
> 2: [build/app/dpdk-testpmd() [0xa6cbc7]]
> 1: [build/app/dpdk-testpmd(rte_dump_stack+0x27) [0xaee796]]
> io packet forwarding packets/burst=32
> nb forwarding cores=1 - nb forwarding ports=2
> port 0: RX queue number: 1 Tx queue number: 1
> Rx offloads=0x0 Tx offloads=0x0
>
> Fixes: c87d435a4d79 ("ethdev: copy fast-path API into separate structure")
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> Changes since v1:
> - removed diff banner in commitlog,
> - renamed vars,
> - removed unneeded cast on anonymous struct in initialisation construct,
> - added check on queue pointer,
>
> ---
> lib/ethdev/ethdev_private.c | 63 +++++++++++++++++++++++++++++++------
> 1 file changed, 54 insertions(+), 9 deletions(-)
>
> diff --git a/lib/ethdev/ethdev_private.c b/lib/ethdev/ethdev_private.c
> index c905c2df6f..8fca20c7d4 100644
> --- a/lib/ethdev/ethdev_private.c
> +++ b/lib/ethdev/ethdev_private.c
> @@ -2,6 +2,7 @@
> * Copyright(c) 2018 Gaëtan Rivet
> */
>
> +#include <rte_debug.h>
> #include "rte_ethdev.h"
> #include "ethdev_driver.h"
> #include "ethdev_private.h"
> @@ -175,22 +176,58 @@ rte_eth_devargs_parse_representor_ports(char *str, void *data)
> return str == NULL ? -1 : 0;
> }
>
> +struct dummy_queue {
> + bool rx_warn_once;
> + bool tx_warn_once;
> +};
> +static struct dummy_queue *dummy_queues_array[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
> +static struct dummy_queue per_port_queues[RTE_MAX_ETHPORTS];
> +RTE_INIT(dummy_queue_init)
> +{
> + uint16_t port_id;
> +
> + for (port_id = 0; port_id < RTE_DIM(per_port_queues); port_id++) {
> + unsigned int q;
> +
> + for (q = 0; q < RTE_DIM(dummy_queues_array[port_id]); q++)
> + dummy_queues_array[port_id][q] = &per_port_queues[port_id];
> + }
> +}
> +
> static uint16_t
> -dummy_eth_rx_burst(__rte_unused void *rxq,
> +dummy_eth_rx_burst(void *rxq,
> __rte_unused struct rte_mbuf **rx_pkts,
> __rte_unused uint16_t nb_pkts)
> {
> - RTE_ETHDEV_LOG(ERR, "rx_pkt_burst for not ready port\n");
> + struct dummy_queue *queue = rxq;
> + uintptr_t port_id;
> +
> + port_id = queue - per_port_queues;
> + if (port_id < RTE_DIM(per_port_queues) && !queue->rx_warn_once) {
> + RTE_ETHDEV_LOG(ERR, "lcore %u called rx_pkt_burst for not ready port %"PRIuPTR"\n",
> + rte_lcore_id(), port_id);
> + rte_dump_stack();
> + queue->rx_warn_once = true;
> + }
> rte_errno = ENOTSUP;
> return 0;
> }
>
> static uint16_t
> -dummy_eth_tx_burst(__rte_unused void *txq,
> +dummy_eth_tx_burst(void *txq,
> __rte_unused struct rte_mbuf **tx_pkts,
> __rte_unused uint16_t nb_pkts)
> {
> - RTE_ETHDEV_LOG(ERR, "tx_pkt_burst for not ready port\n");
> + struct dummy_queue *queue = txq;
> + uintptr_t port_id;
> +
> + port_id = queue - per_port_queues;
> + if (port_id < RTE_DIM(per_port_queues) && !queue->tx_warn_once) {
> + RTE_ETHDEV_LOG(ERR, "lcore %u called tx_pkt_burst for not ready port %"PRIuPTR"\n",
> + rte_lcore_id(), port_id);
> + rte_dump_stack();
> + queue->tx_warn_once = true;
> + }
> rte_errno = ENOTSUP;
> return 0;
> }
> @@ -199,14 +236,22 @@ void
> eth_dev_fp_ops_reset(struct rte_eth_fp_ops *fpo)
> {
> static void *dummy_data[RTE_MAX_QUEUES_PER_PORT];
> - static const struct rte_eth_fp_ops dummy_ops = {
> + uintptr_t port_id = fpo - rte_eth_fp_ops;
> +
> + per_port_queues[port_id].rx_warn_once = false;
> + per_port_queues[port_id].tx_warn_once = false;
> + *fpo = (struct rte_eth_fp_ops) {
> .rx_pkt_burst = dummy_eth_rx_burst,
> .tx_pkt_burst = dummy_eth_tx_burst,
> - .rxq = {.data = dummy_data, .clbk = dummy_data,},
> - .txq = {.data = dummy_data, .clbk = dummy_data,},
> + .rxq = {
> + .data = (void **)&dummy_queues_array[port_id],
> + .clbk = dummy_data,
> + },
> + .txq = {
> + .data = (void **)&dummy_queues_array[port_id],
> + .clbk = dummy_data,
> + },
> };
> -
> - *fpo = dummy_ops;
> }
>
> void
> --
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
Tested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> 2.23.0
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [dpdk-dev] [PATCH v2] ethdev: warn once for buggy applications
2021-10-27 12:15 ` Ananyev, Konstantin
@ 2021-10-27 12:46 ` Thomas Monjalon
2021-10-27 17:31 ` Ferruh Yigit
0 siblings, 1 reply; 11+ messages in thread
From: Thomas Monjalon @ 2021-10-27 12:46 UTC (permalink / raw)
To: David Marchand
Cc: dev, Ananyev, Konstantin, Yigit, Ferruh, andrew.rybchenko, bingz,
olivier.matz
> > Warning continuously is a pain when developping or if a unit test
> > is/gets broken.
> >
> > It could also be a problem if application behaves badly only in some
> > corner cases and a DoS results of those logs being continuously displayed.
> >
> > Let's warn once per port and per rx/tx.
> >
> > Getting such a log is scary, but let's make it more eye catching by
> > dumping a backtrace with it.
> >
> > Tested by introducing a bug in testpmd:
> > static int
> > eth_dev_start_mp(uint16_t port_id)
> > {
> > - if (is_proc_primary())
> > + if (!is_proc_primary())
> > return rte_eth_dev_start(port_id);
> >
> > return 0;
> >
> > Then, running a basic null test:
> > $ ./devtools/test-null.sh
> > ...
> > Start automatic packet forwarding
> > io packet forwarding - ports=2 - cores=1 - streams=2 - NUMA support
> > enabled, MP allocation mode: native
> > Logical Core 1 (socket 0) forwards packets on 2 streams:
> > RX P=0/Q=0 (socket 0) -> TX P=1/Q=0 (socket 0) peer=02:00:00:00:00:01
> > RX P=1/Q=0 (socket 0) -> TX P=0/Q=0 (socket 0) peer=02:00:00:00:00:00
> >
> > lcore 0 called rx_pkt_burst for not ready port 0
> > 8: [build/app/dpdk-testpmd() [0x59e839]]
> > 7: [/lib64/libc.so.6(__libc_start_main+0xf5) [0x7ff481b69555]]
> > 6: [build/app/dpdk-testpmd(main+0x54b) [0x662d24]]
> > 5: [build/app/dpdk-testpmd(start_packet_forwarding+0x263) [0x65e795]]
> > 4: [build/app/dpdk-testpmd() [0x65e1be]]
> > 3: [build/app/dpdk-testpmd() [0x65a996]]
> > 2: [build/app/dpdk-testpmd() [0xa6cbc7]]
> > 1: [build/app/dpdk-testpmd(rte_dump_stack+0x27) [0xaee796]]
> > lcore 0 called rx_pkt_burst for not ready port 1
> > 8: [build/app/dpdk-testpmd() [0x59e839]]
> > 7: [/lib64/libc.so.6(__libc_start_main+0xf5) [0x7ff481b69555]]
> > 6: [build/app/dpdk-testpmd(main+0x54b) [0x662d24]]
> > 5: [build/app/dpdk-testpmd(start_packet_forwarding+0x263) [0x65e795]]
> > 4: [build/app/dpdk-testpmd() [0x65e1be]]
> > 3: [build/app/dpdk-testpmd() [0x65a996]]
> > 2: [build/app/dpdk-testpmd() [0xa6cbc7]]
> > 1: [build/app/dpdk-testpmd(rte_dump_stack+0x27) [0xaee796]]
> > io packet forwarding packets/burst=32
> > nb forwarding cores=1 - nb forwarding ports=2
> > port 0: RX queue number: 1 Tx queue number: 1
> > Rx offloads=0x0 Tx offloads=0x0
> >
> > Fixes: c87d435a4d79 ("ethdev: copy fast-path API into separate structure")
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> > Changes since v1:
> > - removed diff banner in commitlog,
> > - renamed vars,
> > - removed unneeded cast on anonymous struct in initialisation construct,
> > - added check on queue pointer,
> >
> > ---
>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> Tested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
Acked-by: Thomas Monjalon <thomas@monjalon.net>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [dpdk-dev] [PATCH v2] ethdev: warn once for buggy applications
2021-10-27 12:46 ` Thomas Monjalon
@ 2021-10-27 17:31 ` Ferruh Yigit
0 siblings, 0 replies; 11+ messages in thread
From: Ferruh Yigit @ 2021-10-27 17:31 UTC (permalink / raw)
To: Thomas Monjalon, David Marchand
Cc: dev, Ananyev, Konstantin, andrew.rybchenko, bingz, olivier.matz
On 10/27/2021 1:46 PM, Thomas Monjalon wrote:
>>> Warning continuously is a pain when developping or if a unit test
>>> is/gets broken.
>>>
>>> It could also be a problem if application behaves badly only in some
>>> corner cases and a DoS results of those logs being continuously displayed.
>>>
>>> Let's warn once per port and per rx/tx.
>>>
>>> Getting such a log is scary, but let's make it more eye catching by
>>> dumping a backtrace with it.
>>>
>>> Tested by introducing a bug in testpmd:
>>> static int
>>> eth_dev_start_mp(uint16_t port_id)
>>> {
>>> - if (is_proc_primary())
>>> + if (!is_proc_primary())
>>> return rte_eth_dev_start(port_id);
>>>
>>> return 0;
>>>
>>> Then, running a basic null test:
>>> $ ./devtools/test-null.sh
>>> ...
>>> Start automatic packet forwarding
>>> io packet forwarding - ports=2 - cores=1 - streams=2 - NUMA support
>>> enabled, MP allocation mode: native
>>> Logical Core 1 (socket 0) forwards packets on 2 streams:
>>> RX P=0/Q=0 (socket 0) -> TX P=1/Q=0 (socket 0) peer=02:00:00:00:00:01
>>> RX P=1/Q=0 (socket 0) -> TX P=0/Q=0 (socket 0) peer=02:00:00:00:00:00
>>>
>>> lcore 0 called rx_pkt_burst for not ready port 0
>>> 8: [build/app/dpdk-testpmd() [0x59e839]]
>>> 7: [/lib64/libc.so.6(__libc_start_main+0xf5) [0x7ff481b69555]]
>>> 6: [build/app/dpdk-testpmd(main+0x54b) [0x662d24]]
>>> 5: [build/app/dpdk-testpmd(start_packet_forwarding+0x263) [0x65e795]]
>>> 4: [build/app/dpdk-testpmd() [0x65e1be]]
>>> 3: [build/app/dpdk-testpmd() [0x65a996]]
>>> 2: [build/app/dpdk-testpmd() [0xa6cbc7]]
>>> 1: [build/app/dpdk-testpmd(rte_dump_stack+0x27) [0xaee796]]
>>> lcore 0 called rx_pkt_burst for not ready port 1
>>> 8: [build/app/dpdk-testpmd() [0x59e839]]
>>> 7: [/lib64/libc.so.6(__libc_start_main+0xf5) [0x7ff481b69555]]
>>> 6: [build/app/dpdk-testpmd(main+0x54b) [0x662d24]]
>>> 5: [build/app/dpdk-testpmd(start_packet_forwarding+0x263) [0x65e795]]
>>> 4: [build/app/dpdk-testpmd() [0x65e1be]]
>>> 3: [build/app/dpdk-testpmd() [0x65a996]]
>>> 2: [build/app/dpdk-testpmd() [0xa6cbc7]]
>>> 1: [build/app/dpdk-testpmd(rte_dump_stack+0x27) [0xaee796]]
>>> io packet forwarding packets/burst=32
>>> nb forwarding cores=1 - nb forwarding ports=2
>>> port 0: RX queue number: 1 Tx queue number: 1
>>> Rx offloads=0x0 Tx offloads=0x0
>>>
>>> Fixes: c87d435a4d79 ("ethdev: copy fast-path API into separate structure")
>>>
>>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>>> ---
>>> Changes since v1:
>>> - removed diff banner in commitlog,
>>> - renamed vars,
>>> - removed unneeded cast on anonymous struct in initialisation construct,
>>> - added check on queue pointer,
>>>
>>> ---
>>
>> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
>> Tested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
>
> Acked-by: Thomas Monjalon <thomas@monjalon.net>
>
>
Applied to dpdk-next-net/main, thanks.
^ permalink raw reply [flat|nested] 11+ messages in thread