[dpdk-dev] [PATCH 1/2] ethdev: fix log level of Tx and Rx dummy functions

[dpdk-dev] [PATCH 1/2] ethdev: fix log level of Tx and Rx dummy functions

[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.

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(-)

diff --git a/lib/ethdev/ethdev_private.c b/lib/ethdev/ethdev_private.c
index c905c2df6f..fbc3df91ad 100644
--- a/lib/ethdev/ethdev_private.c
+++ b/lib/ethdev/ethdev_private.c
@@ -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;
 }
-- 
2.27.0

[dpdk-dev] [PATCH 2/2] ethdev: fix the race condition for fp ops reset

[Bing Zhao]

In the function "eth_dev_fp_ops_reset", a structure assignment
operation is used to reset one queue's callback functions, etc., but
it is not thread safe.

The structure assignment is not atomic, a lot of instructions will
be generated. Right now, since not all the fields are needed, the
fields in the "dummy_ops" which is not set explicitly will be 0s
based on the specification and compiler behavior. In order to make
"fpo" has the same content with "dummy_ops", some clearing to 0
operation is needed.

By checking the object instructions (e.g. with GCC 4.8.5)
   0x0000000000a58317 <+35>:	mov    %rsi,%rdi
   0x0000000000a5831a <+38>:	mov    %rdx,%rcx
=> 0x0000000000a5831d <+41>:	rep stos %rax,%es:(%rdi)
   0x0000000000a58320 <+44>:	mov    -0x38(%rsp),%rax
   0x0000000000a58325 <+49>:	lea    -0xe0(%rip),%rdx
        // # 0xa5824c <dummy_eth_rx_burst>

It shows that "rep stos" will clear the "fpo" structure before
assigning new values.

In the other thread, if some data path Tx / Rx functions are still
running, there is a risk to get 0 instead of the correct dummy
content.
  1. qd = p->rxq.data[queue_id]
  2. (void **)&p->rxq.clbk[queue_id]
"data" and "clbk" may be observed with NULL (0) in other threads.
Even it is temporary, the accessing to a NULL pointer will cause a
crash. Using "memcpy" could get rid of this.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/ethdev/ethdev_private.c b/lib/ethdev/ethdev_private.c
index fbc3df91ad..cda9a6e228 100644
--- a/lib/ethdev/ethdev_private.c
+++ b/lib/ethdev/ethdev_private.c
@@ -206,7 +206,7 @@ eth_dev_fp_ops_reset(struct rte_eth_fp_ops *fpo)
 		.txq = {.data = dummy_data, .clbk = dummy_data,},
 	};
 
-	*fpo = dummy_ops;
+	rte_memcpy(fpo, &dummy_ops, sizeof(struct rte_eth_fp_ops));
 }
 
 void
-- 
2.27.0

Re: [dpdk-dev] [PATCH 2/2] ethdev: fix the race condition for fp ops reset

[Thomas Monjalon]

22/10/2021 23:14, Bing Zhao:
> In the function "eth_dev_fp_ops_reset", a structure assignment
> operation is used to reset one queue's callback functions, etc., but
> it is not thread safe.
> 
> The structure assignment is not atomic, a lot of instructions will
> be generated. Right now, since not all the fields are needed, the
> fields in the "dummy_ops" which is not set explicitly will be 0s
> based on the specification and compiler behavior. In order to make
> "fpo" has the same content with "dummy_ops", some clearing to 0
> operation is needed.
> 
> By checking the object instructions (e.g. with GCC 4.8.5)
>    0x0000000000a58317 <+35>:	mov    %rsi,%rdi
>    0x0000000000a5831a <+38>:	mov    %rdx,%rcx
> => 0x0000000000a5831d <+41>:	rep stos %rax,%es:(%rdi)
>    0x0000000000a58320 <+44>:	mov    -0x38(%rsp),%rax
>    0x0000000000a58325 <+49>:	lea    -0xe0(%rip),%rdx
>         // # 0xa5824c <dummy_eth_rx_burst>
> 
> It shows that "rep stos" will clear the "fpo" structure before
> assigning new values.
> 
> In the other thread, if some data path Tx / Rx functions are still
> running, there is a risk to get 0 instead of the correct dummy
> content.
>   1. qd = p->rxq.data[queue_id]
>   2. (void **)&p->rxq.clbk[queue_id]
> "data" and "clbk" may be observed with NULL (0) in other threads.
> Even it is temporary, the accessing to a NULL pointer will cause a
> crash. Using "memcpy" could get rid of this.
> 
> Fixes: c87d435a4d79 ("ethdev: copy fast-path API into separate structure")
> Cc: konstantin.ananyev@intel.com
> 
> Signed-off-by: Bing Zhao <bingz@nvidia.com>
> ---
> --- a/lib/ethdev/ethdev_private.c
> +++ b/lib/ethdev/ethdev_private.c
> @@ -206,7 +206,7 @@ eth_dev_fp_ops_reset(struct rte_eth_fp_ops *fpo)
>  		.txq = {.data = dummy_data, .clbk = dummy_data,},
>  	};
>  
> -	*fpo = dummy_ops;
> +	rte_memcpy(fpo, &dummy_ops, sizeof(struct rte_eth_fp_ops));

That's not trivial.
Please add a comment to briefly explain that memcpy avoids zeroing of a simple assignment.

Re: [dpdk-dev] [PATCH 2/2] ethdev: fix the race condition for fp ops reset

[Ananyev, Konstantin]

> 22/10/2021 23:14, Bing Zhao:
> > In the function "eth_dev_fp_ops_reset", a structure assignment
> > operation is used to reset one queue's callback functions, etc., but
> > it is not thread safe.
> >
> > The structure assignment is not atomic, a lot of instructions will
> > be generated. Right now, since not all the fields are needed, the
> > fields in the "dummy_ops" which is not set explicitly will be 0s
> > based on the specification and compiler behavior. In order to make
> > "fpo" has the same content with "dummy_ops", some clearing to 0
> > operation is needed.
> >
> > By checking the object instructions (e.g. with GCC 4.8.5)
> >    0x0000000000a58317 <+35>:	mov    %rsi,%rdi
> >    0x0000000000a5831a <+38>:	mov    %rdx,%rcx
> > => 0x0000000000a5831d <+41>:	rep stos %rax,%es:(%rdi)
> >    0x0000000000a58320 <+44>:	mov    -0x38(%rsp),%rax
> >    0x0000000000a58325 <+49>:	lea    -0xe0(%rip),%rdx
> >         // # 0xa5824c <dummy_eth_rx_burst>
> >
> > It shows that "rep stos" will clear the "fpo" structure before
> > assigning new values.
> >
> > In the other thread, if some data path Tx / Rx functions are still
> > running, there is a risk to get 0 instead of the correct dummy
> > content.
> >   1. qd = p->rxq.data[queue_id]
> >   2. (void **)&p->rxq.clbk[queue_id]
> > "data" and "clbk" may be observed with NULL (0) in other threads.
> > Even it is temporary, the accessing to a NULL pointer will cause a
> > crash. Using "memcpy" could get rid of this.
> >
> > Fixes: c87d435a4d79 ("ethdev: copy fast-path API into separate structure")
> > Cc: konstantin.ananyev@intel.com
> >
> > Signed-off-by: Bing Zhao <bingz@nvidia.com>
> > ---
> > --- a/lib/ethdev/ethdev_private.c
> > +++ b/lib/ethdev/ethdev_private.c
> > @@ -206,7 +206,7 @@ eth_dev_fp_ops_reset(struct rte_eth_fp_ops *fpo)
> >  		.txq = {.data = dummy_data, .clbk = dummy_data,},
> >  	};
> >
> > -	*fpo = dummy_ops;
> > +	rte_memcpy(fpo, &dummy_ops, sizeof(struct rte_eth_fp_ops));
> 
> That's not trivial.
> Please add a comment to briefly explain that memcpy avoids zeroing of a simple assignment.
> 

I think that patch is based on two totally wrong assumptions:
1) ethdev data-path and control-path API is MT-safe.
    With current design it is not.
    When calling rx/tx_burst it is caller responsibility to make sure that given port is
    already properly configured and started. Also it is user responsibility to guarantee
    that none other thread doing dev_stop for the same port simultaneously.
    And visa-versa when calling dev_stop(), it is user responsibility to ensure that
    none other thread doing rx/tx_burst for given port simultaneously.
    If your app doesn't follow these principles, then it is a bug that needs to be fixed.
2) rte_memcpy() provides some sort of atomicity and it is safe to use it on its own 
    in MT environment. That's totally wrong.
    In both cases compiler has total freedom to perform copy in any order it likes
    (let say it can first read whole source data in some temporary buffer (SIMD register),
    and then right it in one go, or it can do the same trick with 'rep stos' as above).   
    Moreover CPU itself can reorder instructions.
    So if you need this copy to be atomic you need to use some sort of
    sync primitives along with it (mutex, rwlock, rcu, etc.). 
    But as I said above right now ethdev API is not MT-safe, so it is not required.
 
To summarise - there is no point to mae these changes,
and patch comment is wrong and misleading.
Konstantin

Re: [PATCH 2/2] ethdev: fix the race condition for fp ops reset

[Ferruh Yigit]

On 10/23/2021 12:39 PM, Ananyev, Konstantin wrote:
> 
> 
>> 22/10/2021 23:14, Bing Zhao:
>>> In the function "eth_dev_fp_ops_reset", a structure assignment
>>> operation is used to reset one queue's callback functions, etc., but
>>> it is not thread safe.
>>>
>>> The structure assignment is not atomic, a lot of instructions will
>>> be generated. Right now, since not all the fields are needed, the
>>> fields in the "dummy_ops" which is not set explicitly will be 0s
>>> based on the specification and compiler behavior. In order to make
>>> "fpo" has the same content with "dummy_ops", some clearing to 0
>>> operation is needed.
>>>
>>> By checking the object instructions (e.g. with GCC 4.8.5)
>>>     0x0000000000a58317 <+35>:	mov    %rsi,%rdi
>>>     0x0000000000a5831a <+38>:	mov    %rdx,%rcx
>>> => 0x0000000000a5831d <+41>:	rep stos %rax,%es:(%rdi)
>>>     0x0000000000a58320 <+44>:	mov    -0x38(%rsp),%rax
>>>     0x0000000000a58325 <+49>:	lea    -0xe0(%rip),%rdx
>>>          // # 0xa5824c <dummy_eth_rx_burst>
>>>
>>> It shows that "rep stos" will clear the "fpo" structure before
>>> assigning new values.
>>>
>>> In the other thread, if some data path Tx / Rx functions are still
>>> running, there is a risk to get 0 instead of the correct dummy
>>> content.
>>>    1. qd = p->rxq.data[queue_id]
>>>    2. (void **)&p->rxq.clbk[queue_id]
>>> "data" and "clbk" may be observed with NULL (0) in other threads.
>>> Even it is temporary, the accessing to a NULL pointer will cause a
>>> crash. Using "memcpy" could get rid of this.
>>>
>>> Fixes: c87d435a4d79 ("ethdev: copy fast-path API into separate structure")
>>> Cc: konstantin.ananyev@intel.com
>>>
>>> Signed-off-by: Bing Zhao <bingz@nvidia.com>
>>> ---
>>> --- a/lib/ethdev/ethdev_private.c
>>> +++ b/lib/ethdev/ethdev_private.c
>>> @@ -206,7 +206,7 @@ eth_dev_fp_ops_reset(struct rte_eth_fp_ops *fpo)
>>>   		.txq = {.data = dummy_data, .clbk = dummy_data,},
>>>   	};
>>>
>>> -	*fpo = dummy_ops;
>>> +	rte_memcpy(fpo, &dummy_ops, sizeof(struct rte_eth_fp_ops));
>>
>> That's not trivial.
>> Please add a comment to briefly explain that memcpy avoids zeroing of a simple assignment.
>>
> 
> I think that patch is based on two totally wrong assumptions:
> 1) ethdev data-path and control-path API is MT-safe.
>      With current design it is not.
>      When calling rx/tx_burst it is caller responsibility to make sure that given port is
>      already properly configured and started. Also it is user responsibility to guarantee
>      that none other thread doing dev_stop for the same port simultaneously.
>      And visa-versa when calling dev_stop(), it is user responsibility to ensure that
>      none other thread doing rx/tx_burst for given port simultaneously.
>      If your app doesn't follow these principles, then it is a bug that needs to be fixed.
> 2) rte_memcpy() provides some sort of atomicity and it is safe to use it on its own
>      in MT environment. That's totally wrong.
>      In both cases compiler has total freedom to perform copy in any order it likes
>      (let say it can first read whole source data in some temporary buffer (SIMD register),
>      and then right it in one go, or it can do the same trick with 'rep stos' as above).
>      Moreover CPU itself can reorder instructions.
>      So if you need this copy to be atomic you need to use some sort of
>      sync primitives along with it (mutex, rwlock, rcu, etc.).
>      But as I said above right now ethdev API is not MT-safe, so it is not required.
>   
> To summarise - there is no point to mae these changes,
> and patch comment is wrong and misleading.

Can we mark this patch as rejected now?

Patch seems trying to cover a wrong application usage, and it should
be addressed in the application level.

RE: [PATCH 2/2] ethdev: fix the race condition for fp ops reset

[Ananyev, Konstantin]

Hi Ferruh,

> >> 22/10/2021 23:14, Bing Zhao:
> >>> In the function "eth_dev_fp_ops_reset", a structure assignment
> >>> operation is used to reset one queue's callback functions, etc., but
> >>> it is not thread safe.
> >>>
> >>> The structure assignment is not atomic, a lot of instructions will
> >>> be generated. Right now, since not all the fields are needed, the
> >>> fields in the "dummy_ops" which is not set explicitly will be 0s
> >>> based on the specification and compiler behavior. In order to make
> >>> "fpo" has the same content with "dummy_ops", some clearing to 0
> >>> operation is needed.
> >>>
> >>> By checking the object instructions (e.g. with GCC 4.8.5)
> >>>     0x0000000000a58317 <+35>:	mov    %rsi,%rdi
> >>>     0x0000000000a5831a <+38>:	mov    %rdx,%rcx
> >>> => 0x0000000000a5831d <+41>:	rep stos %rax,%es:(%rdi)
> >>>     0x0000000000a58320 <+44>:	mov    -0x38(%rsp),%rax
> >>>     0x0000000000a58325 <+49>:	lea    -0xe0(%rip),%rdx
> >>>          // # 0xa5824c <dummy_eth_rx_burst>
> >>>
> >>> It shows that "rep stos" will clear the "fpo" structure before
> >>> assigning new values.
> >>>
> >>> In the other thread, if some data path Tx / Rx functions are still
> >>> running, there is a risk to get 0 instead of the correct dummy
> >>> content.
> >>>    1. qd = p->rxq.data[queue_id]
> >>>    2. (void **)&p->rxq.clbk[queue_id]
> >>> "data" and "clbk" may be observed with NULL (0) in other threads.
> >>> Even it is temporary, the accessing to a NULL pointer will cause a
> >>> crash. Using "memcpy" could get rid of this.
> >>>
> >>> Fixes: c87d435a4d79 ("ethdev: copy fast-path API into separate structure")
> >>> Cc: konstantin.ananyev@intel.com
> >>>
> >>> Signed-off-by: Bing Zhao <bingz@nvidia.com>
> >>> ---
> >>> --- a/lib/ethdev/ethdev_private.c
> >>> +++ b/lib/ethdev/ethdev_private.c
> >>> @@ -206,7 +206,7 @@ eth_dev_fp_ops_reset(struct rte_eth_fp_ops *fpo)
> >>>   		.txq = {.data = dummy_data, .clbk = dummy_data,},
> >>>   	};
> >>>
> >>> -	*fpo = dummy_ops;
> >>> +	rte_memcpy(fpo, &dummy_ops, sizeof(struct rte_eth_fp_ops));
> >>
> >> That's not trivial.
> >> Please add a comment to briefly explain that memcpy avoids zeroing of a simple assignment.
> >>
> >
> > I think that patch is based on two totally wrong assumptions:
> > 1) ethdev data-path and control-path API is MT-safe.
> >      With current design it is not.
> >      When calling rx/tx_burst it is caller responsibility to make sure that given port is
> >      already properly configured and started. Also it is user responsibility to guarantee
> >      that none other thread doing dev_stop for the same port simultaneously.
> >      And visa-versa when calling dev_stop(), it is user responsibility to ensure that
> >      none other thread doing rx/tx_burst for given port simultaneously.
> >      If your app doesn't follow these principles, then it is a bug that needs to be fixed.
> > 2) rte_memcpy() provides some sort of atomicity and it is safe to use it on its own
> >      in MT environment. That's totally wrong.
> >      In both cases compiler has total freedom to perform copy in any order it likes
> >      (let say it can first read whole source data in some temporary buffer (SIMD register),
> >      and then right it in one go, or it can do the same trick with 'rep stos' as above).
> >      Moreover CPU itself can reorder instructions.
> >      So if you need this copy to be atomic you need to use some sort of
> >      sync primitives along with it (mutex, rwlock, rcu, etc.).
> >      But as I said above right now ethdev API is not MT-safe, so it is not required.
> >
> > To summarise - there is no point to mae these changes,
> > and patch comment is wrong and misleading.
> 
> Can we mark this patch as rejected now?

I believe so.

> Patch seems trying to cover a wrong application usage, and it should
> be addressed in the application level.

Re: [PATCH 2/2] ethdev: fix the race condition for fp ops reset

[Thomas Monjalon]

10/11/2021 15:37, Ananyev, Konstantin:
> 
> Hi Ferruh,
> 
> > >> 22/10/2021 23:14, Bing Zhao:
> > >>> In the function "eth_dev_fp_ops_reset", a structure assignment
> > >>> operation is used to reset one queue's callback functions, etc., but
> > >>> it is not thread safe.
> > >>>
> > >>> The structure assignment is not atomic, a lot of instructions will
> > >>> be generated. Right now, since not all the fields are needed, the
> > >>> fields in the "dummy_ops" which is not set explicitly will be 0s
> > >>> based on the specification and compiler behavior. In order to make
> > >>> "fpo" has the same content with "dummy_ops", some clearing to 0
> > >>> operation is needed.
> > >>>
> > >>> By checking the object instructions (e.g. with GCC 4.8.5)
> > >>>     0x0000000000a58317 <+35>:	mov    %rsi,%rdi
> > >>>     0x0000000000a5831a <+38>:	mov    %rdx,%rcx
> > >>> => 0x0000000000a5831d <+41>:	rep stos %rax,%es:(%rdi)
> > >>>     0x0000000000a58320 <+44>:	mov    -0x38(%rsp),%rax
> > >>>     0x0000000000a58325 <+49>:	lea    -0xe0(%rip),%rdx
> > >>>          // # 0xa5824c <dummy_eth_rx_burst>
> > >>>
> > >>> It shows that "rep stos" will clear the "fpo" structure before
> > >>> assigning new values.
> > >>>
> > >>> In the other thread, if some data path Tx / Rx functions are still
> > >>> running, there is a risk to get 0 instead of the correct dummy
> > >>> content.
> > >>>    1. qd = p->rxq.data[queue_id]
> > >>>    2. (void **)&p->rxq.clbk[queue_id]
> > >>> "data" and "clbk" may be observed with NULL (0) in other threads.
> > >>> Even it is temporary, the accessing to a NULL pointer will cause a
> > >>> crash. Using "memcpy" could get rid of this.
> > >>>
> > >>> Fixes: c87d435a4d79 ("ethdev: copy fast-path API into separate structure")
> > >>> Cc: konstantin.ananyev@intel.com
> > >>>
> > >>> Signed-off-by: Bing Zhao <bingz@nvidia.com>
> > >>> ---
> > >>> --- a/lib/ethdev/ethdev_private.c
> > >>> +++ b/lib/ethdev/ethdev_private.c
> > >>> @@ -206,7 +206,7 @@ eth_dev_fp_ops_reset(struct rte_eth_fp_ops *fpo)
> > >>>   		.txq = {.data = dummy_data, .clbk = dummy_data,},
> > >>>   	};
> > >>>
> > >>> -	*fpo = dummy_ops;
> > >>> +	rte_memcpy(fpo, &dummy_ops, sizeof(struct rte_eth_fp_ops));
> > >>
> > >> That's not trivial.
> > >> Please add a comment to briefly explain that memcpy avoids zeroing of a simple assignment.
> > >>
> > >
> > > I think that patch is based on two totally wrong assumptions:
> > > 1) ethdev data-path and control-path API is MT-safe.
> > >      With current design it is not.
> > >      When calling rx/tx_burst it is caller responsibility to make sure that given port is
> > >      already properly configured and started. Also it is user responsibility to guarantee
> > >      that none other thread doing dev_stop for the same port simultaneously.
> > >      And visa-versa when calling dev_stop(), it is user responsibility to ensure that
> > >      none other thread doing rx/tx_burst for given port simultaneously.
> > >      If your app doesn't follow these principles, then it is a bug that needs to be fixed.
> > > 2) rte_memcpy() provides some sort of atomicity and it is safe to use it on its own
> > >      in MT environment. That's totally wrong.
> > >      In both cases compiler has total freedom to perform copy in any order it likes
> > >      (let say it can first read whole source data in some temporary buffer (SIMD register),
> > >      and then right it in one go, or it can do the same trick with 'rep stos' as above).
> > >      Moreover CPU itself can reorder instructions.
> > >      So if you need this copy to be atomic you need to use some sort of
> > >      sync primitives along with it (mutex, rwlock, rcu, etc.).
> > >      But as I said above right now ethdev API is not MT-safe, so it is not required.
> > >
> > > To summarise - there is no point to mae these changes,
> > > and patch comment is wrong and misleading.
> > 
> > Can we mark this patch as rejected now?
> 
> I believe so.
> 
> > Patch seems trying to cover a wrong application usage, and it should
> > be addressed in the application level.

Yes

RE: [PATCH 2/2] ethdev: fix the race condition for fp ops reset

[Bing Zhao]

Yes +1

Let the application handle it once there is an issue.

BR. Bing

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, November 10, 2021 10:58 PM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Bing Zhao
> <bingz@nvidia.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>
> Cc: andrew.rybchenko@oktetlabs.ru; dev@dpdk.org
> Subject: Re: [PATCH 2/2] ethdev: fix the race condition for fp ops
> reset
> 
> External email: Use caution opening links or attachments
> 
> 
> 10/11/2021 15:37, Ananyev, Konstantin:
> >
> > Hi Ferruh,
> >
> > > >> 22/10/2021 23:14, Bing Zhao:
> > > >>> In the function "eth_dev_fp_ops_reset", a structure
> assignment
> > > >>> operation is used to reset one queue's callback functions,
> etc.,
> > > >>> but it is not thread safe.
> > > >>>
> > > >>> The structure assignment is not atomic, a lot of
> instructions
> > > >>> will be generated. Right now, since not all the fields are
> > > >>> needed, the fields in the "dummy_ops" which is not set
> > > >>> explicitly will be 0s based on the specification and
> compiler
> > > >>> behavior. In order to make "fpo" has the same content with
> > > >>> "dummy_ops", some clearing to 0 operation is needed.
> > > >>>
> > > >>> By checking the object instructions (e.g. with GCC 4.8.5)
> > > >>>     0x0000000000a58317 <+35>:   mov    %rsi,%rdi
> > > >>>     0x0000000000a5831a <+38>:   mov    %rdx,%rcx
> > > >>> => 0x0000000000a5831d <+41>:    rep stos %rax,%es:(%rdi)
> > > >>>     0x0000000000a58320 <+44>:   mov    -0x38(%rsp),%rax
> > > >>>     0x0000000000a58325 <+49>:   lea    -0xe0(%rip),%rdx
> > > >>>          // # 0xa5824c <dummy_eth_rx_burst>
> > > >>>
> > > >>> It shows that "rep stos" will clear the "fpo" structure
> before
> > > >>> assigning new values.
> > > >>>
> > > >>> In the other thread, if some data path Tx / Rx functions are
> > > >>> still running, there is a risk to get 0 instead of the
> correct
> > > >>> dummy content.
> > > >>>    1. qd = p->rxq.data[queue_id]
> > > >>>    2. (void **)&p->rxq.clbk[queue_id] "data" and "clbk" may
> be
> > > >>> observed with NULL (0) in other threads.
> > > >>> Even it is temporary, the accessing to a NULL pointer will
> cause
> > > >>> a crash. Using "memcpy" could get rid of this.
> > > >>>
> > > >>> Fixes: c87d435a4d79 ("ethdev: copy fast-path API into
> separate
> > > >>> structure")
> > > >>> Cc: konstantin.ananyev@intel.com
> > > >>>
> > > >>> Signed-off-by: Bing Zhao <bingz@nvidia.com>
> > > >>> ---
> > > >>> --- a/lib/ethdev/ethdev_private.c
> > > >>> +++ b/lib/ethdev/ethdev_private.c
> > > >>> @@ -206,7 +206,7 @@ eth_dev_fp_ops_reset(struct
> rte_eth_fp_ops *fpo)
> > > >>>                 .txq = {.data = dummy_data, .clbk =
> dummy_data,},
> > > >>>         };
> > > >>>
> > > >>> -       *fpo = dummy_ops;
> > > >>> +       rte_memcpy(fpo, &dummy_ops, sizeof(struct
> > > >>> + rte_eth_fp_ops));
> > > >>
> > > >> That's not trivial.
> > > >> Please add a comment to briefly explain that memcpy avoids
> zeroing of a simple assignment.
> > > >>
> > > >
> > > > I think that patch is based on two totally wrong assumptions:
> > > > 1) ethdev data-path and control-path API is MT-safe.
> > > >      With current design it is not.
> > > >      When calling rx/tx_burst it is caller responsibility to
> make sure that given port is
> > > >      already properly configured and started. Also it is user
> responsibility to guarantee
> > > >      that none other thread doing dev_stop for the same port
> simultaneously.
> > > >      And visa-versa when calling dev_stop(), it is user
> responsibility to ensure that
> > > >      none other thread doing rx/tx_burst for given port
> simultaneously.
> > > >      If your app doesn't follow these principles, then it is a
> bug that needs to be fixed.
> > > > 2) rte_memcpy() provides some sort of atomicity and it is safe
> to use it on its own
> > > >      in MT environment. That's totally wrong.
> > > >      In both cases compiler has total freedom to perform copy
> in any order it likes
> > > >      (let say it can first read whole source data in some
> temporary buffer (SIMD register),
> > > >      and then right it in one go, or it can do the same trick
> with 'rep stos' as above).
> > > >      Moreover CPU itself can reorder instructions.
> > > >      So if you need this copy to be atomic you need to use
> some sort of
> > > >      sync primitives along with it (mutex, rwlock, rcu, etc.).
> > > >      But as I said above right now ethdev API is not MT-safe,
> so it is not required.
> > > >
> > > > To summarise - there is no point to mae these changes, and
> patch
> > > > comment is wrong and misleading.
> > >
> > > Can we mark this patch as rejected now?
> >
> > I believe so.
> >
> > > Patch seems trying to cover a wrong application usage, and it
> should
> > > be addressed in the application level.
> 
> Yes
> 

Re: [dpdk-dev] [PATCH 2/2] ethdev: fix the race condition for fp ops reset

[Stephen Hemminger]

On Sat, 23 Oct 2021 00:14:07 +0300
Bing Zhao <bingz@nvidia.com> wrote:

> diff --git a/lib/ethdev/ethdev_private.c b/lib/ethdev/ethdev_private.c
> index fbc3df91ad..cda9a6e228 100644
> --- a/lib/ethdev/ethdev_private.c
> +++ b/lib/ethdev/ethdev_private.c
> @@ -206,7 +206,7 @@ eth_dev_fp_ops_reset(struct rte_eth_fp_ops *fpo)
>  		.txq = {.data = dummy_data, .clbk = dummy_data,},
>  	};
>  
> -	*fpo = dummy_ops;
> +	rte_memcpy(fpo, &dummy_ops, sizeof(struct rte_eth_fp_ops));

memcpy is not thread safe either.

Re: [dpdk-dev] [PATCH 2/2] ethdev: fix the race condition for fp ops reset

[Bing Zhao]

Hi Stephen,

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Sunday, October 24, 2021 12:13 AM
> To: Bing Zhao <bingz@nvidia.com>
> Cc: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>;
> ferruh.yigit@intel.com; andrew.rybchenko@oktetlabs.ru; dev@dpdk.org;
> konstantin.ananyev@intel.com
> Subject: Re: [dpdk-dev] [PATCH 2/2] ethdev: fix the race condition
> for fp ops reset
> 
> External email: Use caution opening links or attachments
> 
> 
> On Sat, 23 Oct 2021 00:14:07 +0300
> Bing Zhao <bingz@nvidia.com> wrote:
> 
> > diff --git a/lib/ethdev/ethdev_private.c
> b/lib/ethdev/ethdev_private.c
> > index fbc3df91ad..cda9a6e228 100644
> > --- a/lib/ethdev/ethdev_private.c
> > +++ b/lib/ethdev/ethdev_private.c
> > @@ -206,7 +206,7 @@ eth_dev_fp_ops_reset(struct rte_eth_fp_ops
> *fpo)
> >               .txq = {.data = dummy_data, .clbk = dummy_data,},
> >       };
> >
> > -     *fpo = dummy_ops;
> > +     rte_memcpy(fpo, &dummy_ops, sizeof(struct rte_eth_fp_ops));
> 
> memcpy is not thread safe either.

Sorry for my commit log and it may not be that clear. The reason is the thread-safe, it is because the structure assignment and initialization. Then the pointer will be set to 0 and then set to the new value again.
I am not quite sure if it is OK so use part of the old values and part of the new values in the same function (rx/tx burst). However, changing a pointer to NULL (0) is risk and would cause a crash of the whole application. Using memcpy will keep the old value still value and change is into the new value immediately w/o setting it to 0 fist.

But YES again, the application should avoid such case.

BR. Bing

Re: [dpdk-dev] [PATCH 1/2] ethdev: fix log level of Tx and Rx dummy functions

[Thomas Monjalon]

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.

Re: [dpdk-dev] [PATCH 1/2] ethdev: fix log level of Tx and Rx dummy functions

[Ananyev, Konstantin]

> -----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.

Re: [dpdk-dev] [PATCH 1/2] ethdev: fix log level of Tx and Rx dummy functions

[Bing Zhao]

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

Re: [dpdk-dev] [PATCH 1/2] ethdev: fix log level of Tx and Rx dummy functions

[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.  

Re: [dpdk-dev] [PATCH 1/2] ethdev: fix log level of Tx and Rx dummy functions

[Thomas Monjalon]

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.

Re: [dpdk-dev] [PATCH 1/2] ethdev: fix log level of Tx and Rx dummy functions

[David Marchand]

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.


-- 
David Marchand

Re: [dpdk-dev] [PATCH 1/2] ethdev: fix log level of Tx and Rx dummy functions

[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.   

> 
> 
> >
> > 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?
  

Re: [dpdk-dev] [PATCH 1/2] ethdev: fix log level of Tx and Rx dummy functions

[Thomas Monjalon]

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.

Re: [dpdk-dev] [PATCH 1/2] ethdev: fix log level of Tx and Rx dummy functions

[David Marchand]

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).


-- 
David Marchand

Re: [dpdk-dev] [PATCH 1/2] ethdev: fix log level of Tx and Rx dummy functions

[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?

> >
> 
> 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.

Re: [dpdk-dev] [PATCH 1/2] ethdev: fix log level of Tx and Rx dummy functions

[Thomas Monjalon]

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 :)

Re: [dpdk-dev] [PATCH 1/2] ethdev: fix log level of Tx and Rx dummy functions

[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).
 

Re: [dpdk-dev] [PATCH 1/2] ethdev: fix log level of Tx and Rx dummy functions

[Thomas Monjalon]

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?

Re: [dpdk-dev] [PATCH 1/2] ethdev: fix log level of Tx and Rx dummy functions

[Bing Zhao]

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

Re: [dpdk-dev] [PATCH 1/2] ethdev: fix log level of Tx and Rx dummy functions

[Bing Zhao]

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.
> 
>