From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
To: Joyce Kong <Joyce.Kong@arm.com>,
"thomas@monjalon.net" <thomas@monjalon.net>,
"david.marchand@redhat.com" <david.marchand@redhat.com>,
Ruifeng Wang <Ruifeng.Wang@arm.com>,
"konstantin.ananyev@intel.com" <konstantin.ananyev@intel.com>,
"rsanford@akamai.com" <rsanford@akamai.com>,
"erik.g.carrillo@intel.com" <erik.g.carrillo@intel.com>,
"olivier.matz@6wind.com" <olivier.matz@6wind.com>,
"yipeng1.wang@intel.com" <yipeng1.wang@intel.com>,
"sameh.gobriel@intel.com" <sameh.gobriel@intel.com>,
"bruce.richardson@intel.com" <bruce.richardson@intel.com>,
"vladimir.medvedkin@intel.com" <vladimir.medvedkin@intel.com>,
"anatoly.burakov@intel.com" <anatoly.burakov@intel.com>,
"andrew.rybchenko@oktetlabs.ru" <andrew.rybchenko@oktetlabs.ru>,
"jerinj@marvell.com" <jerinj@marvell.com>,
"declan.doherty@intel.com" <declan.doherty@intel.com>,
"ciara.power@intel.com" <ciara.power@intel.com>,
"xiaoyun.li@intel.com" <xiaoyun.li@intel.com>,
"nicolas.chautru@intel.com" <nicolas.chautru@intel.com>,
"maryam.tahhan@intel.com" <maryam.tahhan@intel.com>,
"reshma.pattan@intel.com" <reshma.pattan@intel.com>,
"cristian.dumitrescu@intel.com" <cristian.dumitrescu@intel.com>,
"Yigit, Ferruh" <ferruh.yigit@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, nd <nd@arm.com>,
Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>,
nd <nd@arm.com>
Subject: Re: [dpdk-dev] [PATCH v1 10/12] app/testpmd: use compiler atomic builtins for port sync
Date: Tue, 9 Nov 2021 23:14:24 +0000 [thread overview]
Message-ID: <DBAPR08MB58145C2243EAC108689D424998929@DBAPR08MB5814.eurprd08.prod.outlook.com> (raw)
In-Reply-To: <20210802101847.3462-11-joyce.kong@arm.com>
+ Ferruh
<snip>
Hi Joyce/Ferruh,
I do not think the port_status changes need to be handled atomically. The changes to port_status do not seem to be happening from multiple threads. It seems to be getting modified during initialization or through the test pmd prompt.
Do we really need atomic operations on port_status?
>
> Replace rte_atomic16_cmpset operation with compiler atomic CAS operation
> for port status sync in testpmd case.
>
> Signed-off-by: Joyce Kong <joyce.kong@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
> app/test-pmd/testpmd.c | 75 +++++++++++++++++++++++++++---------------
> 1 file changed, 48 insertions(+), 27 deletions(-)
>
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> 6cbe9ba3c8..22579dd438 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -36,7 +36,6 @@
> #include <rte_alarm.h>
> #include <rte_per_lcore.h>
> #include <rte_lcore.h>
> -#include <rte_atomic.h>
> #include <rte_branch_prediction.h>
> #include <rte_mempool.h>
> #include <rte_malloc.h>
> @@ -2302,6 +2301,7 @@ setup_hairpin_queues(portid_t pi, portid_t p_pi,
> uint16_t cnt_pi)
> uint16_t peer_tx_port = pi;
> uint32_t manual = 1;
> uint32_t tx_exp = hairpin_mode & 0x10;
> + uint16_t port_exp;
>
> if (!(hairpin_mode & 0xf)) {
> peer_rx_port = pi;
> @@ -2347,10 +2347,11 @@ setup_hairpin_queues(portid_t pi, portid_t p_pi,
> uint16_t cnt_pi)
> if (diag == 0)
> continue;
>
> + port_exp = RTE_PORT_HANDLING;
> /* Fail to setup rx queue, return */
> - if (rte_atomic16_cmpset(&(port->port_status),
> - RTE_PORT_HANDLING,
> - RTE_PORT_STOPPED) == 0)
> + if (__atomic_compare_exchange_n(&(port->port_status),
> + &port_exp, RTE_PORT_STOPPED, 0,
> + __ATOMIC_RELAXED, __ATOMIC_RELAXED)
> == 0)
> fprintf(stderr,
> "Port %d can not be set back to stopped\n",
> pi);
> fprintf(stderr, "Fail to configure port %d hairpin queues\n",
> @@ -2370,10 +2371,11 @@ setup_hairpin_queues(portid_t pi, portid_t p_pi,
> uint16_t cnt_pi)
> if (diag == 0)
> continue;
>
> + port_exp = RTE_PORT_HANDLING;
> /* Fail to setup rx queue, return */
> - if (rte_atomic16_cmpset(&(port->port_status),
> - RTE_PORT_HANDLING,
> - RTE_PORT_STOPPED) == 0)
> + if (__atomic_compare_exchange_n(&(port->port_status),
> + &port_exp, RTE_PORT_STOPPED, 0,
> + __ATOMIC_RELAXED, __ATOMIC_RELAXED)
> == 0)
> fprintf(stderr,
> "Port %d can not be set back to stopped\n",
> pi);
> fprintf(stderr, "Fail to configure port %d hairpin queues\n",
> @@ -2444,6 +2446,7 @@ start_port(portid_t pid)
> queueid_t qi;
> struct rte_port *port;
> struct rte_eth_hairpin_cap cap;
> + uint16_t port_exp;
>
> if (port_id_is_invalid(pid, ENABLED_WARN))
> return 0;
> @@ -2454,8 +2457,10 @@ start_port(portid_t pid)
>
> need_check_link_status = 0;
> port = &ports[pi];
> - if (rte_atomic16_cmpset(&(port->port_status),
> RTE_PORT_STOPPED,
> - RTE_PORT_HANDLING) == 0)
> {
> + port_exp = RTE_PORT_STOPPED;
> + if (__atomic_compare_exchange_n(&(port->port_status),
> + &port_exp, RTE_PORT_HANDLING, 0,
> + __ATOMIC_RELAXED, __ATOMIC_RELAXED)
> == 0) {
> fprintf(stderr, "Port %d is now not stopped\n", pi);
> continue;
> }
> @@ -2487,8 +2492,10 @@ start_port(portid_t pid)
> nb_txq + nb_hairpinq,
> &(port->dev_conf));
> if (diag != 0) {
> - if (rte_atomic16_cmpset(&(port-
> >port_status),
> - RTE_PORT_HANDLING, RTE_PORT_STOPPED)
> == 0)
> + port_exp = RTE_PORT_HANDLING;
> + if (__atomic_compare_exchange_n(&(port-
> >port_status),
> + &port_exp,
> RTE_PORT_STOPPED, 0,
> + __ATOMIC_RELAXED,
> __ATOMIC_RELAXED) == 0)
> fprintf(stderr,
> "Port %d can not be set back
> to stopped\n",
> pi);
> @@ -2518,10 +2525,11 @@ start_port(portid_t pid)
> if (diag == 0)
> continue;
>
> + port_exp = RTE_PORT_HANDLING;
> /* Fail to setup tx queue, return */
> - if (rte_atomic16_cmpset(&(port-
> >port_status),
> -
> RTE_PORT_HANDLING,
> - RTE_PORT_STOPPED)
> == 0)
> + if (__atomic_compare_exchange_n(&(port-
> >port_status),
> + &port_exp,
> RTE_PORT_STOPPED, 0,
> + __ATOMIC_RELAXED,
> __ATOMIC_RELAXED) == 0)
> fprintf(stderr,
> "Port %d can not be set back
> to stopped\n",
> pi);
> @@ -2570,10 +2578,11 @@ start_port(portid_t pid)
> if (diag == 0)
> continue;
>
> + port_exp = RTE_PORT_HANDLING;
> /* Fail to setup rx queue, return */
> - if (rte_atomic16_cmpset(&(port-
> >port_status),
> -
> RTE_PORT_HANDLING,
> - RTE_PORT_STOPPED)
> == 0)
> + if (__atomic_compare_exchange_n(&(port-
> >port_status),
> + &port_exp,
> RTE_PORT_STOPPED, 0,
> + __ATOMIC_RELAXED,
> __ATOMIC_RELAXED) == 0)
> fprintf(stderr,
> "Port %d can not be set back
> to stopped\n",
> pi);
> @@ -2607,17 +2616,21 @@ start_port(portid_t pid)
> fprintf(stderr, "Fail to start port %d: %s\n",
> pi, rte_strerror(-diag));
>
> + port_exp = RTE_PORT_HANDLING;
> /* Fail to setup rx queue, return */
> - if (rte_atomic16_cmpset(&(port->port_status),
> - RTE_PORT_HANDLING, RTE_PORT_STOPPED)
> == 0)
> + if (__atomic_compare_exchange_n(&(port-
> >port_status),
> + &port_exp, RTE_PORT_STOPPED, 0,
> + __ATOMIC_RELAXED,
> __ATOMIC_RELAXED) == 0)
> fprintf(stderr,
> "Port %d can not be set back to
> stopped\n",
> pi);
> continue;
> }
>
> - if (rte_atomic16_cmpset(&(port->port_status),
> - RTE_PORT_HANDLING, RTE_PORT_STARTED) == 0)
> + port_exp = RTE_PORT_HANDLING;
> + if (__atomic_compare_exchange_n(&(port->port_status),
> + &port_exp, RTE_PORT_STARTED, 0,
> + __ATOMIC_RELAXED, __ATOMIC_RELAXED)
> == 0)
> fprintf(stderr, "Port %d can not be set into started\n",
> pi);
>
> @@ -2697,6 +2710,7 @@ stop_port(portid_t pid)
> int need_check_link_status = 0;
> portid_t peer_pl[RTE_MAX_ETHPORTS];
> int peer_pi;
> + uint16_t port_exp;
>
> if (port_id_is_invalid(pid, ENABLED_WARN))
> return;
> @@ -2722,8 +2736,10 @@ stop_port(portid_t pid)
> }
>
> port = &ports[pi];
> - if (rte_atomic16_cmpset(&(port->port_status),
> RTE_PORT_STARTED,
> - RTE_PORT_HANDLING) == 0)
> + port_exp = RTE_PORT_STARTED;
> + if (__atomic_compare_exchange_n(&(port->port_status),
> + &port_exp, RTE_PORT_HANDLING, 0,
> + __ATOMIC_RELAXED, __ATOMIC_RELAXED)
> == 0)
> continue;
>
> if (hairpin_mode & 0xf) {
> @@ -2749,8 +2765,10 @@ stop_port(portid_t pid)
> RTE_LOG(ERR, EAL, "rte_eth_dev_stop failed for port
> %u\n",
> pi);
>
> - if (rte_atomic16_cmpset(&(port->port_status),
> - RTE_PORT_HANDLING, RTE_PORT_STOPPED) == 0)
> + port_exp = RTE_PORT_HANDLING;
> + if (__atomic_compare_exchange_n(&(port->port_status),
> + &port_exp, RTE_PORT_STOPPED, 0,
> + __ATOMIC_RELAXED, __ATOMIC_RELAXED)
> == 0)
> fprintf(stderr, "Port %d can not be set into
> stopped\n",
> pi);
> need_check_link_status = 1;
> @@ -2788,6 +2806,7 @@ close_port(portid_t pid) {
> portid_t pi;
> struct rte_port *port;
> + uint16_t port_exp;
>
> if (port_id_is_invalid(pid, ENABLED_WARN))
> return;
> @@ -2813,8 +2832,10 @@ close_port(portid_t pid)
> }
>
> port = &ports[pi];
> - if (rte_atomic16_cmpset(&(port->port_status),
> - RTE_PORT_CLOSED, RTE_PORT_CLOSED) == 1) {
> + port_exp = RTE_PORT_CLOSED;
> + if (__atomic_compare_exchange_n(&(port->port_status),
> + &port_exp, RTE_PORT_CLOSED, 0,
> + __ATOMIC_RELAXED, __ATOMIC_RELAXED)) {
> fprintf(stderr, "Port %d is already closed\n", pi);
> continue;
> }
> --
> 2.17.1
next prev parent reply other threads:[~2021-11-09 23:14 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-02 10:18 [dpdk-dev] [PATCH v1 00/12] use compiler atomic builtins for app Joyce Kong
2021-08-02 10:18 ` [dpdk-dev] [PATCH v1 01/12] test/pmd_perf: use compiler atomic builtins for polling sync Joyce Kong
2021-11-08 22:50 ` Honnappa Nagarahalli
2021-11-10 6:10 ` Joyce Kong
2021-08-02 10:18 ` [dpdk-dev] [PATCH v1 02/12] test/ring_perf: use compiler atomic builtins for lcores sync Joyce Kong
2021-11-09 5:43 ` Honnappa Nagarahalli
2021-08-02 10:18 ` [dpdk-dev] [PATCH v1 03/12] test/timer: use compiler atomic builtins for sync Joyce Kong
2021-11-09 20:59 ` Honnappa Nagarahalli
2021-08-02 10:18 ` [dpdk-dev] [PATCH v1 04/12] test/stack_perf: use compiler atomics for lcore sync Joyce Kong
2021-11-09 21:12 ` Honnappa Nagarahalli
2021-08-02 10:18 ` [dpdk-dev] [PATCH v1 05/12] test/bpf: use compiler atomics for calculation Joyce Kong
2021-08-02 10:18 ` [dpdk-dev] [PATCH v1 06/12] test/func_reentrancy: use compiler atomic for data sync Joyce Kong
2021-11-09 21:54 ` Honnappa Nagarahalli
2021-08-02 10:18 ` [dpdk-dev] [PATCH v1 07/12] app/eventdev: use compiler atomic builtins for packets sync Joyce Kong
2021-11-10 23:19 ` Honnappa Nagarahalli
2021-11-11 7:27 ` Joyce Kong
2021-08-02 10:18 ` [dpdk-dev] [PATCH v1 08/12] app/crypto: use compiler atomic builtins for display sync Joyce Kong
2021-11-09 22:11 ` Honnappa Nagarahalli
2021-08-02 10:18 ` [dpdk-dev] [PATCH v1 09/12] app/compress: " Joyce Kong
2021-11-09 22:59 ` Honnappa Nagarahalli
2021-11-11 8:13 ` Joyce Kong
2021-08-02 10:18 ` [dpdk-dev] [PATCH v1 10/12] app/testpmd: use compiler atomic builtins for port sync Joyce Kong
2021-11-09 23:14 ` Honnappa Nagarahalli [this message]
2021-11-11 8:51 ` Joyce Kong
2021-08-02 10:18 ` [dpdk-dev] [PATCH v1 11/12] app/bbdev: use compiler atomics for thread sync Joyce Kong
2021-11-10 21:25 ` Honnappa Nagarahalli
2021-08-02 10:18 ` [dpdk-dev] [PATCH v1 12/12] app: remove unnecessary include of atomic Joyce Kong
2021-10-21 6:35 ` [dpdk-dev] [PATCH v1 00/12] use compiler atomic builtins for app Joyce Kong
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DBAPR08MB58145C2243EAC108689D424998929@DBAPR08MB5814.eurprd08.prod.outlook.com \
--to=honnappa.nagarahalli@arm.com \
--cc=Joyce.Kong@arm.com \
--cc=Ruifeng.Wang@arm.com \
--cc=anatoly.burakov@intel.com \
--cc=andrew.rybchenko@oktetlabs.ru \
--cc=bruce.richardson@intel.com \
--cc=ciara.power@intel.com \
--cc=cristian.dumitrescu@intel.com \
--cc=david.marchand@redhat.com \
--cc=declan.doherty@intel.com \
--cc=dev@dpdk.org \
--cc=erik.g.carrillo@intel.com \
--cc=ferruh.yigit@intel.com \
--cc=jerinj@marvell.com \
--cc=konstantin.ananyev@intel.com \
--cc=maryam.tahhan@intel.com \
--cc=nd@arm.com \
--cc=nicolas.chautru@intel.com \
--cc=olivier.matz@6wind.com \
--cc=reshma.pattan@intel.com \
--cc=rsanford@akamai.com \
--cc=sameh.gobriel@intel.com \
--cc=thomas@monjalon.net \
--cc=vladimir.medvedkin@intel.com \
--cc=xiaoyun.li@intel.com \
--cc=yipeng1.wang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).