[1/2] ethdev: fix log level of Tx and Rx dummy functions
Checks
Commit Message
When stopping a port, the data path Tx and Rx burst functions should
be stopped firstly conventionally. Then the dummy functions are used
to replace the callback functions provided by the PMD.
When the application stops a port without or before stopping the data
path handling. The dummy functions may be invoked heavily and a lot
of logs in these dummy functions will result in a flood.
Debug level log should be enough instead of the error level.
Fixes: c87d435a4d79 ("ethdev: copy fast-path API into separate structure")
Cc: konstantin.ananyev@intel.com
Signed-off-by: Bing Zhao <bingz@nvidia.com>
---
lib/ethdev/ethdev_private.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Comments
22/10/2021 23:14, Bing Zhao:
> When stopping a port, the data path Tx and Rx burst functions should
> be stopped firstly conventionally. Then the dummy functions are used
> to replace the callback functions provided by the PMD.
>
> When the application stops a port without or before stopping the data
> path handling. The dummy functions may be invoked heavily and a lot
> of logs in these dummy functions will result in a flood.
Why does it happen? We should not use a stopped port.
Is it a problem of core synchronization?
> Debug level log should be enough instead of the error level.
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Saturday, October 23, 2021 9:33 AM
> To: Bing Zhao <bingz@nvidia.com>
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; andrew.rybchenko@oktetlabs.ru; dev@dpdk.org; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>
> Subject: Re: [PATCH 1/2] ethdev: fix log level of Tx and Rx dummy functions
>
> 22/10/2021 23:14, Bing Zhao:
> > When stopping a port, the data path Tx and Rx burst functions should
> > be stopped firstly conventionally. Then the dummy functions are used
> > to replace the callback functions provided by the PMD.
> >
> > When the application stops a port without or before stopping the data
> > path handling.
If the application really does that, then it is a severe bug in the application,
then needs to be fixed ASAP.
> The dummy functions may be invoked heavily and a lot
> > of logs in these dummy functions will result in a flood.
>
> Why does it happen? We should not use a stopped port.
> Is it a problem of core synchronization?
>
> > Debug level log should be enough instead of the error level.
>
>
Dummy function is supposed to be set only when device is not able to do RX/TX properly
(not attached, or attached but not configured, or attached and configured, but not started).
Obviously if app calls rx/tx_burst for such port it is a major issue, that should be flagged immediately.
So I believe having ERR level here makes a perfect sense here.
Hi Thomas,
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Saturday, October 23, 2021 4:33 PM
> To: Bing Zhao <bingz@nvidia.com>
> Cc: ferruh.yigit@intel.com; andrew.rybchenko@oktetlabs.ru;
> dev@dpdk.org; konstantin.ananyev@intel.com
> Subject: Re: [PATCH 1/2] ethdev: fix log level of Tx and Rx dummy
> functions
>
> External email: Use caution opening links or attachments
>
>
> 22/10/2021 23:14, Bing Zhao:
> > When stopping a port, the data path Tx and Rx burst functions
> should
> > be stopped firstly conventionally. Then the dummy functions are
> used
> > to replace the callback functions provided by the PMD.
> >
> > When the application stops a port without or before stopping the
> data
> > path handling. The dummy functions may be invoked heavily and a
> lot of
> > logs in these dummy functions will result in a flood.
>
> Why does it happen? We should not use a stopped port.
> Is it a problem of core synchronization?
This is observed due to some "improper" application behavior.
Correct me if my understanding is wrong.
When configuring the device, usually it should be stopped and then started again, not all the features / attributes could be configured in flight.
Like in testpmd right now, when configuring a RSS Reta table of a port, the start / stop status of the port is not check properly. If the PMD needs to restart in order to make it take effect, this could be observed.
>
> > Debug level log should be enough instead of the error level.
>
>
Hi Ananyev,
> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Saturday, October 23, 2021 7:47 PM
> To: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>; Bing Zhao
> <bingz@nvidia.com>
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>;
> andrew.rybchenko@oktetlabs.ru; dev@dpdk.org
> Subject: RE: [PATCH 1/2] ethdev: fix log level of Tx and Rx dummy
> functions
>
> External email: Use caution opening links or attachments
>
>
> > -----Original Message-----
> > From: Thomas Monjalon <thomas@monjalon.net>
> > Sent: Saturday, October 23, 2021 9:33 AM
> > To: Bing Zhao <bingz@nvidia.com>
> > Cc: Yigit, Ferruh <ferruh.yigit@intel.com>;
> > andrew.rybchenko@oktetlabs.ru; dev@dpdk.org; Ananyev, Konstantin
> > <konstantin.ananyev@intel.com>
> > Subject: Re: [PATCH 1/2] ethdev: fix log level of Tx and Rx dummy
> > functions
> >
> > 22/10/2021 23:14, Bing Zhao:
> > > When stopping a port, the data path Tx and Rx burst functions
> should
> > > be stopped firstly conventionally. Then the dummy functions are
> used
> > > to replace the callback functions provided by the PMD.
> > >
> > > When the application stops a port without or before stopping the
> > > data path handling.
>
> If the application really does that, then it is a severe bug in the
> application, then needs to be fixed ASAP.
I agree, this should be some improper / wrong behavior in the application.
>
> > The dummy functions may be invoked heavily and a lot
> > > of logs in these dummy functions will result in a flood.
> >
> > Why does it happen? We should not use a stopped port.
> > Is it a problem of core synchronization?
> >
> > > Debug level log should be enough instead of the error level.
> >
> >
>
> Dummy function is supposed to be set only when device is not able to
> do RX/TX properly (not attached, or attached but not configured, or
> attached and configured, but not started).
> Obviously if app calls rx/tx_burst for such port it is a major issue,
> that should be flagged immediately.
> So I believe having ERR level here makes a perfect sense here.
I do not insist on this. Some notification to the application may be needed. While to my understanding, the log flood should be prevented, or the logs may slow down the application, the IO, and would also have impact on other logs and some information may get lost (but that is the users' decision).
Since the rx/tx burst are usually in the data path and invoked heavily, if the log is needed, how about print it only once? WDYT?
BR. Bing
> > > > When stopping a port, the data path Tx and Rx burst functions
> > should
> > > > be stopped firstly conventionally. Then the dummy functions are
> > used
> > > > to replace the callback functions provided by the PMD.
> > > >
> > > > When the application stops a port without or before stopping the
> > > > data path handling.
> >
> > If the application really does that, then it is a severe bug in the
> > application, then needs to be fixed ASAP.
>
> I agree, this should be some improper / wrong behavior in the application.
>
> >
> > > The dummy functions may be invoked heavily and a lot
> > > > of logs in these dummy functions will result in a flood.
> > >
> > > Why does it happen? We should not use a stopped port.
> > > Is it a problem of core synchronization?
> > >
> > > > Debug level log should be enough instead of the error level.
> > >
> > >
> >
> > Dummy function is supposed to be set only when device is not able to
> > do RX/TX properly (not attached, or attached but not configured, or
> > attached and configured, but not started).
> > Obviously if app calls rx/tx_burst for such port it is a major issue,
> > that should be flagged immediately.
> > So I believe having ERR level here makes a perfect sense here.
>
> I do not insist on this. Some notification to the application may be needed. While to my understanding, the log flood should be prevented,
> or the logs may slow down the application, the IO, and would also have impact on other logs and some information may get lost (but that is
> the users' decision).
> Since the rx/tx burst are usually in the data path and invoked heavily, if the log is needed, how about print it only once? WDYT?
>
Correctly behaving app should never call these stub functions and should never see these messages.
If your app ended up inside this function, then there something really wrong is going on,
that can cause app crash, silent memory corruption, NIC HW hang, or many other nasty things.
The aim of this stubs mechanism:
1) minimize (but not completely avoid) risk of such damage to happen in case of
programming error within user app.
2) flag to the user that something very wrong is going on within his app.
In such situation, possible slowdown of misbehaving program is out of my concern.
24/10/2021 13:48, Ananyev, Konstantin:
>
> > > > > When stopping a port, the data path Tx and Rx burst functions
> > > should
> > > > > be stopped firstly conventionally. Then the dummy functions are
> > > used
> > > > > to replace the callback functions provided by the PMD.
> > > > >
> > > > > When the application stops a port without or before stopping the
> > > > > data path handling.
> > >
> > > If the application really does that, then it is a severe bug in the
> > > application, then needs to be fixed ASAP.
> >
> > I agree, this should be some improper / wrong behavior in the application.
> >
> > >
> > > > The dummy functions may be invoked heavily and a lot
> > > > > of logs in these dummy functions will result in a flood.
> > > >
> > > > Why does it happen? We should not use a stopped port.
> > > > Is it a problem of core synchronization?
> > > >
> > > > > Debug level log should be enough instead of the error level.
> > > >
> > > >
> > >
> > > Dummy function is supposed to be set only when device is not able to
> > > do RX/TX properly (not attached, or attached but not configured, or
> > > attached and configured, but not started).
> > > Obviously if app calls rx/tx_burst for such port it is a major issue,
> > > that should be flagged immediately.
> > > So I believe having ERR level here makes a perfect sense here.
> >
> > I do not insist on this. Some notification to the application may be needed. While to my understanding, the log flood should be prevented,
> > or the logs may slow down the application, the IO, and would also have impact on other logs and some information may get lost (but that is
> > the users' decision).
> > Since the rx/tx burst are usually in the data path and invoked heavily, if the log is needed, how about print it only once? WDYT?
> >
>
> Correctly behaving app should never call these stub functions and should never see these messages.
> If your app ended up inside this function, then there something really wrong is going on,
> that can cause app crash, silent memory corruption, NIC HW hang, or many other nasty things.
> The aim of this stubs mechanism:
> 1) minimize (but not completely avoid) risk of such damage to happen in case of
> programming error within user app.
> 2) flag to the user that something very wrong is going on within his app.
> In such situation, possible slowdown of misbehaving program is out of my concern.
There is a concern about getting efficient log report,
especially when looking at CI issues.
On Mon, Oct 25, 2021 at 11:43 AM Thomas Monjalon <thomas@monjalon.net> wrote:
> > Correctly behaving app should never call these stub functions and should never see these messages.
> > If your app ended up inside this function, then there something really wrong is going on,
> > that can cause app crash, silent memory corruption, NIC HW hang, or many other nasty things.
> > The aim of this stubs mechanism:
> > 1) minimize (but not completely avoid) risk of such damage to happen in case of
> > programming error within user app.
> > 2) flag to the user that something very wrong is going on within his app.
> > In such situation, possible slowdown of misbehaving program is out of my concern.
If correctly behaving app should not do this, why not put an assert()
or a rte_panic?
This way, the users will definitely catch it.
>
> There is a concern about getting efficient log report,
> especially when looking at CI issues.
+1.
The current solution with logs is a real pain.
> > > Correctly behaving app should never call these stub functions and should never see these messages.
> > > If your app ended up inside this function, then there something really wrong is going on,
> > > that can cause app crash, silent memory corruption, NIC HW hang, or many other nasty things.
> > > The aim of this stubs mechanism:
> > > 1) minimize (but not completely avoid) risk of such damage to happen in case of
> > > programming error within user app.
> > > 2) flag to the user that something very wrong is going on within his app.
> > > In such situation, possible slowdown of misbehaving program is out of my concern.
>
> If correctly behaving app should not do this, why not put an assert()
> or a rte_panic?
> This way, the users will definitely catch it.
That was my first intention, though generic DPDK policy is
to avoid panics inside library functions.
But if everyone think it would be ok here, then I am fine with it too.
>
>
> >
> > There is a concern about getting efficient log report,
> > especially when looking at CI issues.
>
> +1.
> The current solution with logs is a real pain.
Are you guys talking about problems with
app/test/sample_packet_forward.* David reported?
Or some extra problems arise?
25/10/2021 14:55, Ananyev, Konstantin:
>
> > > > Correctly behaving app should never call these stub functions and should never see these messages.
> > > > If your app ended up inside this function, then there something really wrong is going on,
> > > > that can cause app crash, silent memory corruption, NIC HW hang, or many other nasty things.
> > > > The aim of this stubs mechanism:
> > > > 1) minimize (but not completely avoid) risk of such damage to happen in case of
> > > > programming error within user app.
> > > > 2) flag to the user that something very wrong is going on within his app.
> > > > In such situation, possible slowdown of misbehaving program is out of my concern.
> >
> > If correctly behaving app should not do this, why not put an assert()
> > or a rte_panic?
> > This way, the users will definitely catch it.
>
> That was my first intention, though generic DPDK policy is
> to avoid panics inside library functions.
> But if everyone think it would be ok here, then I am fine with it too.
I would prefer not having panic/assert in the lib.
> > > There is a concern about getting efficient log report,
> > > especially when looking at CI issues.
> >
> > +1.
> > The current solution with logs is a real pain.
>
> Are you guys talking about problems with
> app/test/sample_packet_forward.* David reported?
> Or some extra problems arise?
The problem will arise each time an app is misbehaving.
That's going to be a recurring problem in the CI.
On Mon, Oct 25, 2021 at 3:27 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > > There is a concern about getting efficient log report,
> > > > especially when looking at CI issues.
> > >
> > > +1.
> > > The current solution with logs is a real pain.
> >
> > Are you guys talking about problems with
> > app/test/sample_packet_forward.* David reported?
> > Or some extra problems arise?
>
> The problem will arise each time an app is misbehaving.
> That's going to be a recurring problem in the CI.
>
One thing that could be done is compiling with asserts in CI, and let
default build not have those asserts.
Otherwise, logging once should be enough (I have a patch for this latter idea).
> > > > > There is a concern about getting efficient log report,
> > > > > especially when looking at CI issues.
> > > >
> > > > +1.
> > > > The current solution with logs is a real pain.
> > >
> > > Are you guys talking about problems with
> > > app/test/sample_packet_forward.* David reported?
> > > Or some extra problems arise?
> >
> > The problem will arise each time an app is misbehaving.
> > That's going to be a recurring problem in the CI.
It is still not clear to me why it is going to be a recurring one?
Ok, right now we have some test-cases that are misbehaving unintentionally.
So we need to fix them.
I admit that it might be a pain, but it still looks like a one time job to me.
With new test-cases we should be able to catch such misbehaving at patch
submission stage (by checking then logs).
I guess there might be some test-cases that misbehave intentionally -
some negative test-cases for error-condition checking etc.
But for them error message in the log and error return value seems like a
right thing, no? Again I expect such test-cases do erroneous rx/tx_burst
just few times (not dozens or hundreds) so they shouldn't pollute log too much.
So, what I am missing here?
> >
>
> One thing that could be done is compiling with asserts in CI, and let
> default build not have those asserts.
Agree, log+assert seems like a good alternative to panic() for me.
> Otherwise, logging once should be enough (I have a patch for this latter idea).
I understand the intention, but I am a bit sceptical about that one:
it is quite often people don’t pay much attention to single log message.
25/10/2021 22:29, Ananyev, Konstantin:
>
> > > > > > There is a concern about getting efficient log report,
> > > > > > especially when looking at CI issues.
> > > > >
> > > > > +1.
> > > > > The current solution with logs is a real pain.
> > > >
> > > > Are you guys talking about problems with
> > > > app/test/sample_packet_forward.* David reported?
> > > > Or some extra problems arise?
> > >
> > > The problem will arise each time an app is misbehaving.
> > > That's going to be a recurring problem in the CI.
>
> It is still not clear to me why it is going to be a recurring one?
> Ok, right now we have some test-cases that are misbehaving unintentionally.
> So we need to fix them.
> I admit that it might be a pain, but it still looks like a one time job to me.
> With new test-cases we should be able to catch such misbehaving at patch
> submission stage (by checking then logs).
> I guess there might be some test-cases that misbehave intentionally -
> some negative test-cases for error-condition checking etc.
> But for them error message in the log and error return value seems like a
> right thing, no? Again I expect such test-cases do erroneous rx/tx_burst
> just few times (not dozens or hundreds) so they shouldn't pollute log too much.
> So, what I am missing here?
You don't miss anything, but as you said above, we are going to catch
some issues at patch submission stage.
And we want this stage to be easy to catch.
Having megabytes of log does not help to check in the CI.
> > One thing that could be done is compiling with asserts in CI, and let
> > default build not have those asserts.
>
> Agree, log+assert seems like a good alternative to panic() for me.
>
> > Otherwise, logging once should be enough (I have a patch for this latter idea).
>
> I understand the intention, but I am a bit sceptical about that one:
> it is quite often people don’t pay much attention to single log message.
Not a good argument in my opinion.
One error == one log.
We are not going to flood all error logs to make sure devs pay attention :)
Hi David,
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Monday, October 25, 2021 9:32 PM
> To: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Bing Zhao
> <bingz@nvidia.com>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> andrew.rybchenko@oktetlabs.ru; dev@dpdk.org
> Subject: Re: [PATCH 1/2] ethdev: fix log level of Tx and Rx dummy
> functions
>
> External email: Use caution opening links or attachments
>
>
> On Mon, Oct 25, 2021 at 3:27 PM Thomas Monjalon <thomas@monjalon.net>
> wrote:
> > > > > There is a concern about getting efficient log report,
> > > > > especially when looking at CI issues.
> > > >
> > > > +1.
> > > > The current solution with logs is a real pain.
> > >
> > > Are you guys talking about problems with
> > > app/test/sample_packet_forward.* David reported?
> > > Or some extra problems arise?
> >
> > The problem will arise each time an app is misbehaving.
> > That's going to be a recurring problem in the CI.
> >
>
> One thing that could be done is compiling with asserts in CI, and
> let default build not have those asserts.
>
> Otherwise, logging once should be enough (I have a patch for this
> latter idea).
If you already have a patch to log once, I will suppress this patch. Thanks
>
>
> --
> David Marchand
BR. Bing
> > > > > > > There is a concern about getting efficient log report,
> > > > > > > especially when looking at CI issues.
> > > > > >
> > > > > > +1.
> > > > > > The current solution with logs is a real pain.
> > > > >
> > > > > Are you guys talking about problems with
> > > > > app/test/sample_packet_forward.* David reported?
> > > > > Or some extra problems arise?
> > > >
> > > > The problem will arise each time an app is misbehaving.
> > > > That's going to be a recurring problem in the CI.
> >
> > It is still not clear to me why it is going to be a recurring one?
> > Ok, right now we have some test-cases that are misbehaving unintentionally.
> > So we need to fix them.
> > I admit that it might be a pain, but it still looks like a one time job to me.
> > With new test-cases we should be able to catch such misbehaving at patch
> > submission stage (by checking then logs).
> > I guess there might be some test-cases that misbehave intentionally -
> > some negative test-cases for error-condition checking etc.
> > But for them error message in the log and error return value seems like a
> > right thing, no? Again I expect such test-cases do erroneous rx/tx_burst
> > just few times (not dozens or hundreds) so they shouldn't pollute log too much.
> > So, what I am missing here?
>
> You don't miss anything, but as you said above, we are going to catch
> some issues at patch submission stage.
> And we want this stage to be easy to catch.
> Having megabytes of log does not help to check in the CI.
>
> > > One thing that could be done is compiling with asserts in CI, and let
> > > default build not have those asserts.
> >
> > Agree, log+assert seems like a good alternative to panic() for me.
> >
> > > Otherwise, logging once should be enough (I have a patch for this latter idea).
> >
> > I understand the intention, but I am a bit sceptical about that one:
> > it is quite often people don’t pay much attention to single log message.
>
> Not a good argument in my opinion.
> One error == one log.
> We are not going to flood all error logs to make sure devs pay attention :)
Well, that error could come from different sources (rx/tx, different ports, etc.).
But yes, healthy CI is important.
So if suppressing subsequent messages will help it anyhow, I wouldn't object.
Few thoughts though:
we probably need to make it more informative (and scary :)) then now:
bump log-level, print current lcore id and dump current call-stack.
Another thought - might be worth to make it logging once per lcore
(instead of global logging once).
26/10/2021 14:38, Ananyev, Konstantin:
>
> > > > > > > > There is a concern about getting efficient log report,
> > > > > > > > especially when looking at CI issues.
> > > > > > >
> > > > > > > +1.
> > > > > > > The current solution with logs is a real pain.
> > > > > >
> > > > > > Are you guys talking about problems with
> > > > > > app/test/sample_packet_forward.* David reported?
> > > > > > Or some extra problems arise?
> > > > >
> > > > > The problem will arise each time an app is misbehaving.
> > > > > That's going to be a recurring problem in the CI.
> > >
> > > It is still not clear to me why it is going to be a recurring one?
> > > Ok, right now we have some test-cases that are misbehaving unintentionally.
> > > So we need to fix them.
> > > I admit that it might be a pain, but it still looks like a one time job to me.
> > > With new test-cases we should be able to catch such misbehaving at patch
> > > submission stage (by checking then logs).
> > > I guess there might be some test-cases that misbehave intentionally -
> > > some negative test-cases for error-condition checking etc.
> > > But for them error message in the log and error return value seems like a
> > > right thing, no? Again I expect such test-cases do erroneous rx/tx_burst
> > > just few times (not dozens or hundreds) so they shouldn't pollute log too much.
> > > So, what I am missing here?
> >
> > You don't miss anything, but as you said above, we are going to catch
> > some issues at patch submission stage.
> > And we want this stage to be easy to catch.
> > Having megabytes of log does not help to check in the CI.
> >
> > > > One thing that could be done is compiling with asserts in CI, and let
> > > > default build not have those asserts.
> > >
> > > Agree, log+assert seems like a good alternative to panic() for me.
> > >
> > > > Otherwise, logging once should be enough (I have a patch for this latter idea).
> > >
> > > I understand the intention, but I am a bit sceptical about that one:
> > > it is quite often people don’t pay much attention to single log message.
> >
> > Not a good argument in my opinion.
> > One error == one log.
> > We are not going to flood all error logs to make sure devs pay attention :)
>
> Well, that error could come from different sources (rx/tx, different ports, etc.).
> But yes, healthy CI is important.
> So if suppressing subsequent messages will help it anyhow, I wouldn't object.
> Few thoughts though:
> we probably need to make it more informative (and scary :)) then now:
> bump log-level, print current lcore id and dump current call-stack.
> Another thought - might be worth to make it logging once per lcore
> (instead of global logging once).
David tried to print once per queue.
I don't know whether it can work. David?
@@ -180,7 +180,7 @@ dummy_eth_rx_burst(__rte_unused 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");
+ RTE_ETHDEV_LOG(DEBUG, "rx_pkt_burst for not ready port\n");
rte_errno = ENOTSUP;
return 0;
}
@@ -190,7 +190,7 @@ dummy_eth_tx_burst(__rte_unused 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");
+ RTE_ETHDEV_LOG(DEBUG, "tx_pkt_burst for not ready port\n");
rte_errno = ENOTSUP;
return 0;
}