ethdev: fix log level of Tx and Rx dummy functions

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 [email protected]

Signed-off-by Bing Zhao <[email protected]>

---

22/10/2021 23:14, Bing Zhao:

Quoted 42 lines

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 [email protected]

Signed-off-by Bing Zhao <[email protected]>

---

lib/ethdev/ethdev_private.c +0 -0

@@ -206,7 +206,7 @@ eth_dev_fp_ops_reset(struct rte_eth_fp_ops *fpo)
 		.txq = {.data = dummy_data, .clbk = dummy_data,},
 	};
 

lib/ethdev/ethdev_private.c eth_dev_fp_ops_reset(struct rte_eth_fp_ops *fpo) · line 209

Quoted 2 lines

lib/ethdev/ethdev_private.c +1 -1

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

Quoted 45 lines

22/10/2021 23:14, Bing Zhao:

Quoted 42 lines

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 [email protected]

Signed-off-by Bing Zhao <[email protected]>

---

lib/ethdev/ethdev_private.c +0 -0

@@ -206,7 +206,7 @@ eth_dev_fp_ops_reset(struct rte_eth_fp_ops *fpo)
  		.txq = {.data = dummy_data, .clbk = dummy_data,},
  	};
 

Quoted 6 lines

lib/ethdev/ethdev_private.c eth_dev_fp_ops_reset(struct rte_eth_fp_ops *fpo) · line 209

Quoted 2 lines

lib/ethdev/ethdev_private.c +1 -1

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

Quoted 69 lines

Quoted 43 lines

22/10/2021 23:14, Bing Zhao:

Quoted 42 lines

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 [email protected]

Signed-off-by Bing Zhao <[email protected]>

---

lib/ethdev/ethdev_private.c +0 -0

@@ -206,7 +206,7 @@ eth_dev_fp_ops_reset(struct rte_eth_fp_ops *fpo)
  		.txq = {.data = dummy_data, .clbk = dummy_data,},
  	};
 

Quoted 6 lines

lib/ethdev/ethdev_private.c eth_dev_fp_ops_reset(struct rte_eth_fp_ops *fpo) · line 209

Quoted 2 lines

lib/ethdev/ethdev_private.c +1 -1

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

Quoted 2 lines

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.

Hi Ferruh,

Quoted 69 lines

Quoted 69 lines

Quoted 43 lines

22/10/2021 23:14, Bing Zhao:

Quoted 42 lines

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 [email protected]

Signed-off-by Bing Zhao <[email protected]>

---

lib/ethdev/ethdev_private.c +0 -0

@@ -206,7 +206,7 @@ eth_dev_fp_ops_reset(struct rte_eth_fp_ops *fpo)
  		.txq = {.data = dummy_data, .clbk = dummy_data,},
  	};
 

Quoted 6 lines

lib/ethdev/ethdev_private.c eth_dev_fp_ops_reset(struct rte_eth_fp_ops *fpo) · line 209

Quoted 2 lines

lib/ethdev/ethdev_private.c +1 -1

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

Quoted 4 lines

Quoted 2 lines

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.

Quoted 2 lines

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

-----Original Message-----
From: Thomas Monjalon <[email protected]>
Sent: Wednesday, November 10, 2021 10:58 PM
To: Yigit, Ferruh <[email protected]>; Bing Zhao <[email protected]>; Ananyev, Konstantin <[email protected]>
Cc: [email protected]; [email protected]
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:

Quoted 5 lines

Hi Ferruh,

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

22/10/2021 23:14, Bing Zhao:

Quoted 1 line

In the function "eth_dev_fp_ops_reset", a structure

assignment

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

operation is used to reset one queue's callback functions,

etc.,

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

but it is not thread safe.

The structure assignment is not atomic, a lot of

instructions

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

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

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

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>

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

It shows that "rep stos" will clear the "fpo" structure

before

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

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

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

dummy content.
1. qd = p->rxq.data[queue_id]
2. (void **)&p->rxq.clbk[queue_id] "data" and "clbk" may

be

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

observed with NULL (0) in other threads.
Even it is temporary, the accessing to a NULL pointer will

cause

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

a crash. Using "memcpy" could get rid of this.

Fixes c87d435a4d79 ("ethdev: copy fast-path API into

separate

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

lib/ethdev/ethdev_private.c eth_dev_fp_ops_reset(struct

Quoted 8 lines

structure")
Cc: [email protected]

Signed-off-by Bing Zhao <[email protected]>

---

lib/ethdev/ethdev_private.c +0 -0

@@ -206,7 +206,7 @@ eth_dev_fp_ops_reset(struct

rte_eth_fp_ops *fpo)

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

lib/ethdev/ethdev_private.c eth_dev_fp_ops_reset(struct · line 206

Quoted 1 line

lib/ethdev/ethdev_private.c +0 -0

                .txq = {.data = dummy_data, .clbk =

dummy_data,},

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

lib/ethdev/ethdev_private.c eth_dev_fp_ops_reset(struct rte_eth_fp_ops *fpo)

Quoted 5 lines

lib/ethdev/ethdev_private.c +2 -1

        };
 
-       *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.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

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

Quoted 1 line

Quoted 1 line

Quoted 1 line

already properly configured and started. Also it is user

responsibility to guarantee

Quoted 1 line

Quoted 1 line

Quoted 1 line

that none other thread doing dev_stop for the same port

simultaneously.

Quoted 1 line

Quoted 1 line

Quoted 1 line

And visa-versa when calling dev_stop(), it is user

responsibility to ensure that

Quoted 1 line

Quoted 1 line

Quoted 1 line

none other thread doing rx/tx_burst for given port

simultaneously.

Quoted 1 line

Quoted 1 line

Quoted 1 line

If your app doesn't follow these principles, then it is a

bug that needs to be fixed.

Quoted 1 line

Quoted 1 line

Quoted 1 line

2) rte_memcpy() provides some sort of atomicity and it is safe

to use it on its own

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

in MT environment. That's totally wrong.
In both cases compiler has total freedom to perform copy

in any order it likes

Quoted 1 line

Quoted 1 line

Quoted 1 line

(let say it can first read whole source data in some

temporary buffer (SIMD register),

Quoted 1 line

Quoted 1 line

Quoted 1 line

and then right it in one go, or it can do the same trick

with 'rep stos' as above).

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Moreover CPU itself can reorder instructions. So if you need this copy to be atomic you need to use

some sort of

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

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.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

To summarise - there is no point to mae these changes, and

patch

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

comment is wrong and misleading.

Can we mark this patch as rejected now?

I believe so.

Quoted 1 line

Patch seems trying to cover a wrong application usage, and it

should

Quoted 1 line

Quoted 1 line

be addressed in the application level.

Yes

-----Original Message-----
From: Stephen Hemminger <[email protected]>
Sent: Sunday, October 24, 2021 12:13 AM
To: Bing Zhao <[email protected]>
Cc: NBU-Contact-Thomas Monjalon <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [dpdk-dev] [PATCH 2/2] ethdev: fix the race condition for fp ops reset

External email: Use caution opening links or attachments

Review

Quoted 1 line · On Sat, 23 Oct 2021 00:14:07 +0300 Bing Zhao <[email protected]> wrote:

(quoted diff) +0 -0

b/lib/ethdev/ethdev_private.c

lib/ethdev/ethdev_private.c eth_dev_fp_ops_reset(struct rte_eth_fp_ops

Quoted 4 lines

index fbc3df91ad..cda9a6e228 100644

lib/ethdev/ethdev_private.c +0 -0

@@ -206,7 +206,7 @@ eth_dev_fp_ops_reset(struct rte_eth_fp_ops

*fpo)

lib/ethdev/ethdev_private.c eth_dev_fp_ops_reset(struct rte_eth_fp_ops *fpo) · line 209

Quoted 5 lines

lib/ethdev/ethdev_private.c +1 -1

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

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.

-----Original Message-----
From: Thomas Monjalon <[email protected]>
Sent: Saturday, October 23, 2021 9:33 AM
To: Bing Zhao <[email protected]>
Cc: Yigit, Ferruh <[email protected]>; [email protected]; [email protected]; Ananyev, Konstantin <[email protected]>
Subject: Re: [PATCH 1/2] ethdev: fix log level of Tx and Rx dummy functions

22/10/2021 23:14, Bing Zhao:

Quoted 6 lines

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

Quoted 1 line

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?

Quoted 1 line

Debug level log should be enough instead of the error level.

-----Original Message-----
From: Ananyev, Konstantin <[email protected]>
Sent: Saturday, October 23, 2021 7:47 PM
To: NBU-Contact-Thomas Monjalon <[email protected]>; Bing Zhao <[email protected]>
Cc: Yigit, Ferruh <[email protected]>; [email protected]; [email protected]
Subject: RE: [PATCH 1/2] ethdev: fix log level of Tx and Rx dummy functions

External email: Use caution opening links or attachments

Quoted 10 lines

-----Original Message-----
From: Thomas Monjalon <[email protected]>
Sent: Saturday, October 23, 2021 9:33 AM
To: Bing Zhao <[email protected]>
Cc: Yigit, Ferruh <[email protected]>; [email protected]; [email protected]; Ananyev, Konstantin <[email protected]>
Subject: Re: [PATCH 1/2] ethdev: fix log level of Tx and Rx dummy functions

Quoted 2 lines

22/10/2021 23:14, Bing Zhao:

Quoted 1 line

When stopping a port, the data path Tx and Rx burst functions

should

Quoted 1 line

Quoted 1 line

be stopped firstly conventionally. Then the dummy functions are

used

Quoted 4 lines

Quoted 4 lines

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.

Quoted 10 lines

The dummy functions may be invoked heavily and a lot

Quoted 1 line

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?

Quoted 1 line

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.

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

When stopping a port, the data path Tx and Rx burst functions

should

Quoted 1 line

Quoted 1 line

be stopped firstly conventionally. Then the dummy functions are

Quoted 8 lines

used

Quoted 4 lines

Quoted 4 lines

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.

Quoted 11 lines

Quoted 10 lines

The dummy functions may be invoked heavily and a lot

Quoted 1 line

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?

Quoted 1 line

Debug level log should be enough instead of the error level.

Quoted 6 lines

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?

Quoted 33 lines

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

When stopping a port, the data path Tx and Rx burst functions

should

Quoted 1 line

Quoted 1 line

be stopped firstly conventionally. Then the dummy functions are

Quoted 8 lines

used

Quoted 4 lines

Quoted 4 lines

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.

Quoted 11 lines

Quoted 10 lines

The dummy functions may be invoked heavily and a lot

Quoted 1 line

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?

Quoted 1 line

Debug level log should be enough instead of the error level.

Quoted 6 lines

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.

Quoted 5 lines

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.

Quoted 8 lines

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.

Quoted 9 lines

Quoted 9 lines

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.

Quoted 5 lines

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.

Quoted 10 lines

Quoted 9 lines

Quoted 9 lines

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.

Quoted 3 lines

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.

Quoted 5 lines

Quoted 2 lines

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?

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

Quoted 4 lines

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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 1 line

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

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

Quoted 3 lines

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.

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

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?

Quoted 2 lines

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.

Quoted 1 line

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.

Quoted 14 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

Quoted 3 lines

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.

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

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.

Quoted 12 lines

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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 4 lines

Quoted 1 line

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

Quoted 37 lines

Quoted 14 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

Quoted 3 lines

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.

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

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.

Quoted 12 lines

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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

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

-----Original Message-----
From: David Marchand <[email protected]>
Sent: Monday, October 25, 2021 9:32 PM
To: NBU-Contact-Thomas Monjalon <[email protected]>
Cc: Ananyev, Konstantin <[email protected]>; Bing Zhao <[email protected]>; Yigit, Ferruh <[email protected]>; [email protected]; [email protected]
Subject: Re: [PATCH 1/2] ethdev: fix log level of Tx and Rx dummy functions

External email: Use caution opening links or attachments

Quoted 6 lines · Thomas Monjalon

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

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.

Quoted 7 lines

Quoted 3 lines

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

-----Original Message-----
From: Thomas Monjalon <[email protected]>
Sent: Saturday, October 23, 2021 4:33 PM
To: Bing Zhao <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]
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:

Quoted 1 line

When stopping a port, the data path Tx and Rx burst functions

should

Quoted 1 line

be stopped firstly conventionally. Then the dummy functions are

used

Quoted 2 lines

to replace the callback functions provided by the PMD.

Quoted 1 line

When the application stops a port without or before stopping the

data

Quoted 1 line

path handling. The dummy functions may be invoked heavily and a

lot of

Quoted 1 line

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?

Quoted 2 lines

Debug level log should be enough instead of the error level.