Re: [dpdk-dev] [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. 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 2/2] ethdev: fix the race condition for fp ops reset
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)
>0x00a58317 <+35>: mov%rsi,%rdi
>0x00a5831a <+38>: mov%rdx,%rcx
> => 0x00a5831d <+41>: rep stos %rax,%es:(%rdi)
>0x00a58320 <+44>: mov-0x38(%rsp),%rax
>0x00a58325 <+49>: lea-0xe0(%rip),%rdx
> // # 0xa5824c
>
> 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.anan...@intel.com
>
> Signed-off-by: Bing Zhao
> ---
> --- 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] dma/idxd: fix build on Windows
2021-10-23 08:55 (UTC+0200), David Marchand:
> Windows compilation gives us a splat:
> In file included from ../drivers/dma/idxd/idxd_pci.c:10:
> In file included from ..\drivers\dma\idxd/idxd_internal.h:11:
> ..\drivers\dma\idxd/idxd_hw_defs.h:46:21: error: expected member name or
> ';' after declaration specifiers
> uint16_t __reserved[13];
> ^
> 1 error generated.
>
> Ironically, __reserved is probably a reserved token.
Yes, and it's used in system headers for static analyzer annotations:
https://docs.microsoft.com/en-us/windows/win32/winprog/header-annotations#advanced-annotations
> Some drivers that build fine on Windows have structs with a "reserved"
> field, let's go with this.
>
> Fixes: 82147042d062 ("dma/idxd: add datapath structures")
>
> Signed-off-by: David Marchand
> ---
> drivers/dma/idxd/idxd_hw_defs.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/dma/idxd/idxd_hw_defs.h b/drivers/dma/idxd/idxd_hw_defs.h
> index 55ca9f7f52..2a219c1312 100644
> --- a/drivers/dma/idxd/idxd_hw_defs.h
> +++ b/drivers/dma/idxd/idxd_hw_defs.h
> @@ -43,7 +43,7 @@ struct idxd_hw_desc {
> uint16_t intr_handle; /* completion interrupt handle */
>
> /* remaining 26 bytes are reserved */
> - uint16_t __reserved[13];
> + uint16_t reserved[13];
> } __rte_aligned(64);
>
> #define IDXD_COMP_STATUS_INCOMPLETE0
Re: [dpdk-dev] [PATCH] dma/idxd: fix build on Windows
On Sat, Oct 23, 2021 at 8:56 AM David Marchand
wrote:
>
> Windows compilation gives us a splat:
> In file included from ../drivers/dma/idxd/idxd_pci.c:10:
> In file included from ..\drivers\dma\idxd/idxd_internal.h:11:
> ..\drivers\dma\idxd/idxd_hw_defs.h:46:21: error: expected member name or
> ';' after declaration specifiers
> uint16_t __reserved[13];
> ^
> 1 error generated.
>
> Ironically, __reserved is probably a reserved token.
> Some drivers that build fine on Windows have structs with a "reserved"
> field, let's go with this.
>
> Fixes: 82147042d062 ("dma/idxd: add datapath structures")
>
> Signed-off-by: David Marchand
Tested via CI@UNH.
Applied.
--
David Marchand
Re: [dpdk-dev] [PATCH] dma/idxd: fix build on Windows
On Sat, Oct 23, 2021 at 10:38 AM Dmitry Kozlyuk wrote: > > 2021-10-23 08:55 (UTC+0200), David Marchand: > > Windows compilation gives us a splat: > > In file included from ../drivers/dma/idxd/idxd_pci.c:10: > > In file included from ..\drivers\dma\idxd/idxd_internal.h:11: > > ..\drivers\dma\idxd/idxd_hw_defs.h:46:21: error: expected member name or > > ';' after declaration specifiers > > uint16_t __reserved[13]; > > ^ > > 1 error generated. > > > > Ironically, __reserved is probably a reserved token. > > Yes, and it's used in system headers for static analyzer annotations: > https://docs.microsoft.com/en-us/windows/win32/winprog/header-annotations#advanced-annotations Thanks for confirming. -- David Marchand
[dpdk-dev] [PATCH] devtools: forbid additions of __reserved
__reserved is a reserved keyword in Windows system headers.
Signed-off-by: David Marchand
---
devtools/checkpatches.sh | 9 +
1 file changed, 9 insertions(+)
diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
index c314d83a29..25f60a4a27 100755
--- a/devtools/checkpatches.sh
+++ b/devtools/checkpatches.sh
@@ -134,6 +134,15 @@ check_forbidden_additions() { #
-f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
"$1" || res=1
+ # refrain from using __reserved which is a reserved keyword in Windows
+ # system headers
+ awk -v FOLDERS="lib drivers app examples" \
+ -v EXPRESSIONS='\\<__reserved\\>' \
+ -v RET_ON_FAIL=1 \
+ -v MESSAGE='Using __reserved' \
+ -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
+ "$1" || res=1
+
# SVG must be included with wildcard extension to allow conversion
awk -v FOLDERS='doc' \
-v EXPRESSIONS='::[[:space:]]*[^[:space:]]*\\.svg' \
--
2.23.0
Re: [dpdk-dev] [PATCH 2/2] ethdev: fix the race condition for fp ops reset
> 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)
> >0x00a58317 <+35>:mov%rsi,%rdi
> >0x00a5831a <+38>:mov%rdx,%rcx
> > => 0x00a5831d <+41>:rep stos %rax,%es:(%rdi)
> >0x00a58320 <+44>:mov-0x38(%rsp),%rax
> >0x00a58325 <+49>:lea-0xe0(%rip),%rdx
> > // # 0xa5824c
> >
> > 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.anan...@intel.com
> >
> > Signed-off-by: Bing Zhao
> > ---
> > --- 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: [dpdk-dev] [PATCH 1/2] ethdev: fix log level of Tx and Rx dummy functions
> -Original Message- > From: Thomas Monjalon > Sent: Saturday, October 23, 2021 9:33 AM > To: Bing Zhao > Cc: Yigit, Ferruh ; andrew.rybche...@oktetlabs.ru; > dev@dpdk.org; Ananyev, Konstantin > > 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 v1] test: fix devargs test case memory leak
On Sat, Oct 23, 2021 at 2:18 PM Xueming Li wrote:
>
> In layer argument test function, kvargs are parsed and checked without
> free. This patch calls rte_kvargs_free() function to avoid memory leak.
>
Coverity issue: 373631
> Fixes: a4975cd20dca ("test: add devargs test cases")
>
> Signed-off-by: Xueming Li
Reviewed-by: David Marchand
--
David Marchand
Re: [dpdk-dev] [PATCH 1/2] ethdev: fix log level of Tx and Rx dummy functions
Hi Thomas, > -Original Message- > From: Thomas Monjalon > Sent: Saturday, October 23, 2021 4:33 PM > To: Bing Zhao > Cc: ferruh.yi...@intel.com; andrew.rybche...@oktetlabs.ru; > dev@dpdk.org; konstantin.anan...@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. > >
Re: [dpdk-dev] [PATCH 2/2] ethdev: fix the race condition for fp ops reset
On Sat, 23 Oct 2021 00:14:07 +0300
Bing Zhao 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.
[dpdk-dev] [PATCH v1] test: fix devargs test case memory leak
In layer argument test function, kvargs are parsed and checked without
free. This patch calls rte_kvargs_free() function to avoid memory leak.
Fixes: a4975cd20dca ("test: add devargs test cases")
Signed-off-by: Xueming Li
---
app/test/test_devargs.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/app/test/test_devargs.c b/app/test/test_devargs.c
index 19036716bf7..16621285d2d 100644
--- a/app/test/test_devargs.c
+++ b/app/test/test_devargs.c
@@ -43,8 +43,10 @@ test_args(const char *devargs, const char *layer, const char
*args, const int n)
if ((int)kvlist->count != n) {
printf("rte_devargs_parse(%s) %s_str: %s kv number %u, not
%d\n",
devargs, layer, args, kvlist->count, n);
+ rte_kvargs_free(kvlist);
return -1;
}
+ rte_kvargs_free(kvlist);
return 0;
}
--
2.33.0
Re: [dpdk-dev] [PATCH v5 3/3] test/devargs: add devargs test cases
On Sat, 2021-10-23 at 08:17 +0200, David Marchand wrote:
> On Wed, Oct 20, 2021 at 5:48 PM Xueming Li wrote:
> > + kvlist = rte_kvargs_parse(args, NULL);
> > + if (kvlist == NULL) {
> > + printf("rte_devargs_parse(%s) %s_str: %s not parsed\n",
> > + devargs, layer, args);
> > + return -1;
> > + }
> > + if ((int)kvlist->count != n) {
> > + printf("rte_devargs_parse(%s) %s_str: %s kv number %u, not
> > %d\n",
> > + devargs, layer, args, kvlist->count, n);
> > + return -1;
> > + }
> > + return 0;
> > +}
>
> Missing some rte_kvargs_free().
> Can you send a fix?
Thanks for checking this out, fix posted:
https://patches.dpdk.org/project/dpdk/patch/20211023121755.169290-1-xuemi...@nvidia.com/
>
> Thanks.
>
>
Re: [dpdk-dev] [PATCH 1/2] ethdev: fix log level of Tx and Rx dummy functions
Hi Ananyev, > -Original Message- > From: Ananyev, Konstantin > Sent: Saturday, October 23, 2021 7:47 PM > To: NBU-Contact-Thomas Monjalon ; Bing Zhao > > Cc: Yigit, Ferruh ; > andrew.rybche...@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 > > Sent: Saturday, October 23, 2021 9:33 AM > > To: Bing Zhao > > Cc: Yigit, Ferruh ; > > andrew.rybche...@oktetlabs.ru; dev@dpdk.org; Ananyev, Konstantin > > > > 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
